From 6424edaeaad2c3e327b367aaafdadf3fe8800400 Mon Sep 17 00:00:00 2001 From: Nathaniel Wesley Filardo Date: Thu, 24 Mar 2022 13:47:41 +0000 Subject: [PATCH] Stop playing OO games with MetaEntry David points out that the downcasts I had introduced were UB. Instead, go back to passing MetaEntry-s around and make MetaslabMetaEntry just a namespace of static methods. This partially reverts 7940fee00cf0ce43c3c40584a3b3e4fb8d77cfd8 --- src/backend/backend.h | 10 +++----- src/backend/backend_concept.h | 4 ++-- src/mem/corealloc.h | 44 ++++++++++++++++++----------------- src/mem/localalloc.h | 44 ++++++++++++++++++----------------- src/mem/metaslab.h | 37 ++++++++++++++--------------- src/mem/remotecache.h | 20 +++++++--------- 6 files changed, 78 insertions(+), 81 deletions(-) diff --git a/src/backend/backend.h b/src/backend/backend.h index 5bf8015..d1022a3 100644 --- a/src/backend/backend.h +++ b/src/backend/backend.h @@ -58,14 +58,10 @@ namespace snmalloc * Set template parameter to true if it not an error * to access a location that is not backed by a chunk. */ - template - SNMALLOC_FAST_PATH static const Ret& get_metaentry(address_t p) + template + SNMALLOC_FAST_PATH static const MetaEntry& get_metaentry(address_t p) { - static_assert( - std::is_base_of_v && sizeof(MetaEntry) == sizeof(Ret), - "Backend Pagemap get_metaentry return must look like MetaEntry"); - return static_cast( - concretePagemap.template get(p)); + return concretePagemap.template get(p); } /** diff --git a/src/backend/backend_concept.h b/src/backend/backend_concept.h index c0d8f8e..895e19a 100644 --- a/src/backend/backend_concept.h +++ b/src/backend/backend_concept.h @@ -24,10 +24,10 @@ namespace snmalloc { { Meta::set_metaentry(addr, sz, t) } -> ConceptSame; - { Meta::template get_metaentry(addr) } + { Meta::template get_metaentry(addr) } -> ConceptSame; - { Meta::template get_metaentry(addr) } + { Meta::template get_metaentry(addr) } -> ConceptSame; }; diff --git a/src/mem/corealloc.h b/src/mem/corealloc.h index ad6e81a..40e08a3 100644 --- a/src/mem/corealloc.h +++ b/src/mem/corealloc.h @@ -400,17 +400,17 @@ namespace snmalloc * by this thread, or handling the final deallocation onto a slab, * so it can be reused by other threads. */ - SNMALLOC_SLOW_PATH void - dealloc_local_object_slow(const MetaslabMetaEntry& entry) + SNMALLOC_SLOW_PATH void dealloc_local_object_slow(const MetaEntry& entry) { // TODO: Handle message queue on this path? - Metaslab* meta = entry.get_metaslab(); + Metaslab* meta = FrontendMetaEntry::get_metaslab(entry); if (meta->is_large()) { // Handle large deallocation here. - size_t entry_sizeclass = entry.get_sizeclass().as_large(); + size_t entry_sizeclass = + FrontendMetaEntry::get_sizeclass(entry).as_large(); size_t size = bits::one_at_bit(entry_sizeclass); #ifdef SNMALLOC_TRACING @@ -425,7 +425,8 @@ namespace snmalloc return; } - smallsizeclass_t sizeclass = entry.get_sizeclass().as_small(); + smallsizeclass_t sizeclass = + FrontendMetaEntry::get_sizeclass(entry).as_small(); UNUSED(entropy); if (meta->is_sleeping()) @@ -488,9 +489,8 @@ namespace snmalloc message<1024>("Handling remote"); #endif - auto& entry = - SharedStateHandle::Pagemap::template get_metaentry( - snmalloc::address_cast(msg)); + auto& entry = SharedStateHandle::Pagemap::template get_metaentry( + snmalloc::address_cast(msg)); handle_dealloc_remote(entry, msg.as_void(), need_post); @@ -529,7 +529,7 @@ namespace snmalloc * need_post will be set to true, if capacity is exceeded. */ void handle_dealloc_remote( - const MetaslabMetaEntry& entry, + const MetaEntry& entry, CapPtr p, bool& need_post) { @@ -537,7 +537,8 @@ namespace snmalloc // TODO this needs to not double revoke if using MTE // TODO thread capabilities? - if (SNMALLOC_LIKELY(entry.get_remote() == public_state())) + if (SNMALLOC_LIKELY( + FrontendMetaEntry::get_remote(entry) == public_state())) { if (SNMALLOC_LIKELY( dealloc_local_object_fast(entry, p.as_void(), entropy))) @@ -553,7 +554,9 @@ namespace snmalloc need_post = true; attached_cache->remote_dealloc_cache .template dealloc( - entry.get_remote()->trunc_id(), p.as_void(), key_global); + FrontendMetaEntry::get_remote(entry)->trunc_id(), + p.as_void(), + key_global); } } @@ -670,9 +673,8 @@ namespace snmalloc dealloc_local_object(CapPtr p) { // MetaEntry-s seen here are expected to have meaningful Remote pointers - auto& entry = - SharedStateHandle::Pagemap::template get_metaentry( - snmalloc::address_cast(p)); + auto& entry = SharedStateHandle::Pagemap::template get_metaentry( + snmalloc::address_cast(p)); if (SNMALLOC_LIKELY(dealloc_local_object_fast(entry, p, entropy))) return; @@ -680,16 +682,17 @@ namespace snmalloc } SNMALLOC_FAST_PATH static bool dealloc_local_object_fast( - const MetaslabMetaEntry& entry, + const MetaEntry& entry, CapPtr p, LocalEntropy& entropy) { - auto meta = entry.get_metaslab(); + auto meta = FrontendMetaEntry::get_metaslab(entry); SNMALLOC_ASSERT(!meta->is_unused()); snmalloc_check_client( - is_start_of_object(entry.get_sizeclass(), address_cast(p)), + is_start_of_object( + FrontendMetaEntry::get_sizeclass(entry), address_cast(p)), "Not deallocating start of an object"); auto cp = p.as_static>(); @@ -784,7 +787,7 @@ namespace snmalloc auto [slab, meta] = SharedStateHandle::alloc_chunk( get_backend_local_state(), slab_size, - MetaslabMetaEntry::encode( + FrontendMetaEntry::encode( public_state(), sizeclass_t::from_small_class(sizeclass))); if (slab == nullptr) @@ -838,9 +841,8 @@ namespace snmalloc { bool need_post = true; // Always going to post, so ignore. auto n_tame = p_tame->atomic_read_next(key_global, domesticate); - const MetaslabMetaEntry& entry = - SharedStateHandle::Pagemap::template get_metaentry< - MetaslabMetaEntry>(snmalloc::address_cast(p_tame)); + const MetaEntry& entry = SharedStateHandle::Pagemap::get_metaentry( + snmalloc::address_cast(p_tame)); handle_dealloc_remote(entry, p_tame.as_void(), need_post); p_tame = n_tame; } diff --git a/src/mem/localalloc.h b/src/mem/localalloc.h index 50a1cd2..2760488 100644 --- a/src/mem/localalloc.h +++ b/src/mem/localalloc.h @@ -183,7 +183,7 @@ namespace snmalloc auto [chunk, meta] = SharedStateHandle::alloc_chunk( core_alloc->get_backend_local_state(), large_size_to_chunk_size(size), - MetaslabMetaEntry::encode( + FrontendMetaEntry::encode( core_alloc->public_state(), size_to_sizeclass_full(size))); // set up meta data so sizeclass is correct, and hence alloc size, and // external pointer. @@ -267,11 +267,10 @@ namespace snmalloc p.unsafe_ptr(), alloc_size(p.unsafe_ptr())); #endif - const MetaslabMetaEntry& entry = - SharedStateHandle::Pagemap::template get_metaentry( - address_cast(p)); + const MetaEntry& entry = + SharedStateHandle::Pagemap::get_metaentry(address_cast(p)); local_cache.remote_dealloc_cache.template dealloc( - entry.get_remote()->trunc_id(), p, key_global); + FrontendMetaEntry::get_remote(entry)->trunc_id(), p, key_global); post_remote_cache(); return; } @@ -625,10 +624,11 @@ namespace snmalloc capptr::Alloc p_tame = capptr_domesticate( core_alloc->backend_state_ptr(), p_wild); - const MetaslabMetaEntry& entry = - SharedStateHandle::Pagemap::template get_metaentry( - address_cast(p_tame)); - if (SNMALLOC_LIKELY(local_cache.remote_allocator == entry.get_remote())) + const MetaEntry& entry = + SharedStateHandle::Pagemap::get_metaentry(address_cast(p_tame)); + if (SNMALLOC_LIKELY( + local_cache.remote_allocator == + FrontendMetaEntry::get_remote(entry))) { # if defined(__CHERI_PURE_CAPABILITY__) && defined(SNMALLOC_CHECK_CLIENT) dealloc_cheri_checks(p_tame.unsafe_ptr()); @@ -640,7 +640,8 @@ namespace snmalloc return; } - if (SNMALLOC_LIKELY(entry.get_remote() != nullptr)) + RemoteAllocator* remote = FrontendMetaEntry::get_remote(entry); + if (SNMALLOC_LIKELY(remote != nullptr)) { # if defined(__CHERI_PURE_CAPABILITY__) && defined(SNMALLOC_CHECK_CLIENT) dealloc_cheri_checks(p_tame.unsafe_ptr()); @@ -649,7 +650,7 @@ namespace snmalloc if (local_cache.remote_dealloc_cache.reserve_space(entry)) { local_cache.remote_dealloc_cache.template dealloc( - entry.get_remote()->trunc_id(), p_tame, key_global); + remote->trunc_id(), p_tame, key_global); # ifdef SNMALLOC_TRACING message<1024>( "Remote dealloc fast {} ({})", p_raw, alloc_size(p_raw)); @@ -715,11 +716,10 @@ namespace snmalloc // To handle this case we require the uninitialised pagemap contain an // entry for the first chunk of memory, that states it represents a // large object, so we can pull the check for null off the fast path. - const MetaslabMetaEntry& entry = - SharedStateHandle::Pagemap::template get_metaentry( - address_cast(p_raw)); + const MetaEntry& entry = + SharedStateHandle::Pagemap::get_metaentry(address_cast(p_raw)); - return sizeclass_full_to_size(entry.get_sizeclass()); + return sizeclass_full_to_size(FrontendMetaEntry::get_sizeclass(entry)); #endif } @@ -761,10 +761,11 @@ namespace snmalloc size_t remaining_bytes(const void* p) { #ifndef SNMALLOC_PASS_THROUGH - const MetaslabMetaEntry& entry = SharedStateHandle::Pagemap:: - template get_metaentry(address_cast(p)); + const MetaEntry& entry = + SharedStateHandle::Pagemap::template get_metaentry( + address_cast(p)); - auto sizeclass = entry.get_sizeclass(); + auto sizeclass = FrontendMetaEntry::get_sizeclass(entry); return snmalloc::remaining_bytes(sizeclass, address_cast(p)); #else return pointer_diff(p, reinterpret_cast(UINTPTR_MAX)); @@ -789,10 +790,11 @@ namespace snmalloc size_t index_in_object(const void* p) { #ifndef SNMALLOC_PASS_THROUGH - const MetaslabMetaEntry& entry = SharedStateHandle::Pagemap:: - template get_metaentry(address_cast(p)); + const MetaEntry& entry = + SharedStateHandle::Pagemap::template get_metaentry( + address_cast(p)); - auto sizeclass = entry.get_sizeclass(); + auto sizeclass = FrontendMetaEntry::get_sizeclass(entry); return snmalloc::index_in_object(sizeclass, address_cast(p)); #else return reinterpret_cast(p); diff --git a/src/mem/metaslab.h b/src/mem/metaslab.h index 2695114..997784d 100644 --- a/src/mem/metaslab.h +++ b/src/mem/metaslab.h @@ -226,18 +226,17 @@ namespace snmalloc }; /* - * A convenience wrapper aroun MetaEntry with a meaningful RemoteAllocator - * pointer. This encodes a RemoteAllocator* and a sizeclass_t into a the - * uintptr_t remote_and_sizeclass field. + * Define the encoding of a RemoteAllocator* and a sizeclass_t into a + * MetaEntry's uintptr_t remote_and_sizeclass field. * * There's a little bit of an asymmetry here. Since the backend actually sets - * the entry (when associating a metadata structure), MetaslabMetaEntry-s are - * not constructed directly; please use ::encode(). On the other hand, the - * backend's Pagemap::get_metaentry() method is templated on its return type, - * so it is relatively straightforward to view a pagemap entry as a - * MetaslabMetaEntry and then use the accessors here for decoding. + * the entry (when associating a metadata structure), we don't construct a + * full MetaEntry here, but rather use ::encode() to compute its + * remote_and_sizeclass value. On the decode side, we are given read-only + * access to MetaEntry-s so can directly read therefrom rather than having to + * speak in terms of uintptr_t-s. */ - struct MetaslabMetaEntry : public MetaEntry + struct FrontendMetaEntry { /// Perform the encoding. static SNMALLOC_FAST_PATH uintptr_t @@ -248,19 +247,21 @@ namespace snmalloc reinterpret_cast(remote), sizeclass.raw()); } - [[nodiscard]] SNMALLOC_FAST_PATH RemoteAllocator* get_remote() const + [[nodiscard]] static SNMALLOC_FAST_PATH RemoteAllocator* + get_remote(const MetaEntry& me) { return reinterpret_cast( pointer_align_down( - get_remote_and_sizeclass())); + me.get_remote_and_sizeclass())); } - [[nodiscard]] SNMALLOC_FAST_PATH sizeclass_t get_sizeclass() const + [[nodiscard]] static SNMALLOC_FAST_PATH sizeclass_t + get_sizeclass(const MetaEntry& me) { // TODO: perhaps remove static_cast with resolution of // https://github.com/CTSRD-CHERI/llvm-project/issues/588 return sizeclass_t::from_raw( - static_cast(get_remote_and_sizeclass()) & + static_cast(me.get_remote_and_sizeclass()) & (REMOTE_WITH_BACKEND_MARKER_ALIGN - 1)); } @@ -269,13 +270,11 @@ namespace snmalloc * assert that this chunk is being used as a slab (i.e., has an associated * owning allocator). */ - [[nodiscard]] SNMALLOC_FAST_PATH Metaslab* get_metaslab() const + [[nodiscard]] static SNMALLOC_FAST_PATH Metaslab* + get_metaslab(const MetaEntry& me) { - SNMALLOC_ASSERT(get_remote() != nullptr); - return reinterpret_cast(get_meta()); + SNMALLOC_ASSERT(get_remote(me) != nullptr); + return reinterpret_cast(me.get_meta()); } }; - - static_assert(sizeof(MetaslabMetaEntry) == sizeof(MetaEntry)); - } // namespace snmalloc diff --git a/src/mem/remotecache.h b/src/mem/remotecache.h index e97df1a..a258a38 100644 --- a/src/mem/remotecache.h +++ b/src/mem/remotecache.h @@ -52,10 +52,10 @@ namespace snmalloc * * This does not require initialisation to be safely called. */ - SNMALLOC_FAST_PATH bool reserve_space(const MetaslabMetaEntry& entry) + SNMALLOC_FAST_PATH bool reserve_space(const MetaEntry& entry) { - auto size = - static_cast(sizeclass_full_to_size(entry.get_sizeclass())); + auto size = static_cast( + sizeclass_full_to_size(FrontendMetaEntry::get_sizeclass(entry))); bool result = capacity > size; if (result) @@ -101,10 +101,9 @@ namespace snmalloc if (!list[i].empty()) { auto [first, last] = list[i].extract_segment(key); - const MetaslabMetaEntry& entry = - SharedStateHandle::Pagemap::template get_metaentry< - MetaslabMetaEntry>(address_cast(first)); - auto remote = entry.get_remote(); + const MetaEntry& entry = + SharedStateHandle::Pagemap::get_metaentry(address_cast(first)); + auto remote = FrontendMetaEntry::get_remote(entry); // If the allocator is not correctly aligned, then the bit that is // set implies this is used by the backend, and we should not be // deallocating memory here. @@ -142,10 +141,9 @@ namespace snmalloc // Use the next N bits to spread out remote deallocs in our own // slot. auto r = resend.take(key, domesticate); - const MetaslabMetaEntry& entry = - SharedStateHandle::Pagemap::template get_metaentry< - MetaslabMetaEntry>(address_cast(r)); - auto i = entry.get_remote()->trunc_id(); + const MetaEntry& entry = + SharedStateHandle::Pagemap::get_metaentry(address_cast(r)); + auto i = FrontendMetaEntry::get_remote(entry)->trunc_id(); size_t slot = get_slot(i, post_round); list[slot].add(r, key); }