address code reviews

.github/workflows/main.yml

@@ -586,7 +586,7 @@ jobs:
     - name: Test
       run: |
         cd ${{github.workspace}}/build
-        ctest --parallel
+        ctest --parallel --output-on-failure
 
   all-checks:
     # Currently FreeBSD and NetBSD CI are not working, so we do not require them to pass.

src/snmalloc/ds_core/ptrwrap.h

@@ -4,6 +4,8 @@
 #include "defines.h"
 #include "snmalloc/stl/atomic.h"
 
+#include <stdint.h>
+
 namespace snmalloc
 {
   /*

src/snmalloc/mem/corealloc.h

@@ -818,6 +818,14 @@ namespace snmalloc
     SNMALLOC_SLOW_PATH capptr::Alloc<void>
     small_alloc(smallsizeclass_t sizeclass, freelist::Iter<>& fast_free_list)
     {
+      void* result = SecondaryAllocator::allocate(
+        [sizeclass]() -> stl::Pair<size_t, size_t> {
+          auto size = sizeclass_to_size(sizeclass);
+          return {size, natural_alignment(size)};
+        });
+
+      if (result != nullptr)
+        return capptr::Alloc<void>::unsafe_from(result);
       // Look to see if we can grab a free list.
       auto& sl = alloc_classes[sizeclass].available;
       if (SNMALLOC_LIKELY(alloc_classes[sizeclass].length > 0))
@@ -886,10 +894,6 @@ namespace snmalloc
     {
       size_t rsize = sizeclass_to_size(sizeclass);
 
-      void* result = SecondaryAllocator::allocate(rsize);
-      if (result != nullptr)
-        return capptr::Alloc<void>::unsafe_from(result);
-
       // No existing free list get a new slab.
       size_t slab_size = sizeclass_to_slab_size(sizeclass);
 

src/snmalloc/mem/localalloc.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "snmalloc/aal/address.h"
+#include "snmalloc/mem/remoteallocator.h"
 #include "snmalloc/mem/secondary.h"
 #if defined(_MSC_VER)
 #  define ALLOCATOR __declspec(allocator) __declspec(restrict)
@@ -187,10 +188,6 @@ namespace snmalloc
         return small_alloc<NoZero>(1);
       }
 
-      void* result = SecondaryAllocator::allocate(size);
-      if (result != nullptr)
-        return capptr::Alloc<void>::unsafe_from(result);
-
       return check_init([&](CoreAlloc* core_alloc) {
         if (size > bits::one_at_bit(bits::BITS - 1))
         {
@@ -199,6 +196,15 @@ namespace snmalloc
           errno = ENOMEM;
           return capptr::Alloc<void>{nullptr};
         }
+
+        // Check if secondary allocator wants to offer the memory
+        void* result =
+          SecondaryAllocator::allocate([size]() -> stl::Pair<size_t, size_t> {
+            return {size, natural_alignment(size)};
+          });
+        if (result != nullptr)
+          return capptr::Alloc<void>::unsafe_from(result);
+
         // Grab slab of correct size
         // Set remote as large allocator remote.
         auto [chunk, meta] = Config::Backend::alloc_chunk(
@@ -611,17 +617,13 @@ namespace snmalloc
 #endif
     }
 
-    SNMALLOC_FAST_PATH void dealloc(void* p_raw)
-    {
-#ifdef SNMALLOC_PASS_THROUGH
-      external_alloc::free(p_raw);
-#else
-      // Care is needed so that dealloc(nullptr) works before init
-      //  The backend allocator must ensure that a minimal page map exists
-      //  before init, that maps null to a remote_deallocator that will never
-      //  be in thread local state.
+    // The domestic pointer with its origin allocator
+    using DomesticInfo = stl::Pair<capptr::Alloc<void>, const PagemapEntry&>;
 
-#  ifdef __CHERI_PURE_CAPABILITY__
+    // Check whether the raw pointer is owned by snmalloc
+    SNMALLOC_FAST_PATH_INLINE DomesticInfo get_domestic_info(const void* p_raw)
+    {
+#ifdef __CHERI_PURE_CAPABILITY__
       /*
        * On CHERI platforms, snap the provided pointer to its base, ignoring
        * any client-provided offset, which may have taken the pointer out of
@@ -637,10 +639,29 @@ namespace snmalloc
        * start of the allocation and so the offset is zero.
        */
       p_raw = __builtin_cheri_offset_set(p_raw, 0);
-#  endif
+#endif
+      capptr::AllocWild<void> p_wild =
+        capptr_from_client(const_cast<void*>(p_raw));
+      auto p_tame =
+        capptr_domesticate<Config>(core_alloc->backend_state_ptr(), p_wild);
+      const PagemapEntry& entry =
+        Config::Backend::get_metaentry(address_cast(p_tame));
+      return {p_tame, entry};
+    }
 
-      capptr::AllocWild<void> p_wild = capptr_from_client(p_raw);
+    // Check if a pointer is domestic to SnMalloc
+    SNMALLOC_FAST_PATH bool check_domestication(const void* p_raw)
+    {
+      auto [_, entry] = get_domestic_info(p_raw);
+      RemoteAllocator* remote = entry.get_remote();
+      return remote != nullptr;
+    }
 
+    SNMALLOC_FAST_PATH void dealloc(void* p_raw)
+    {
+#ifdef SNMALLOC_PASS_THROUGH
+      external_alloc::free(p_raw);
+#else
       /*
        * p_tame may be nullptr, even if p_raw/p_wild are not, in the case
        * where domestication fails.  We exclusively use p_tame below so that
@@ -653,11 +674,7 @@ namespace snmalloc
        * well-formedness) of this pointer.  The remainder of the logic will
        * deal with the object's extent.
        */
-      capptr::Alloc<void> p_tame =
-        capptr_domesticate<Config>(core_alloc->backend_state_ptr(), p_wild);
-
-      const PagemapEntry& entry =
-        Config::Backend::get_metaentry(address_cast(p_tame));
+      auto [p_tame, entry] = get_domestic_info(p_raw);
 
       if (SNMALLOC_LIKELY(local_cache.remote_allocator == entry.get_remote()))
       {
@@ -702,12 +719,15 @@ namespace snmalloc
         return;
       }
 
-      SecondaryAllocator::deallocate(p_tame.unsafe_ptr());
-
+      if (SNMALLOC_LIKELY(p_tame == nullptr))
+      {
 #  ifdef SNMALLOC_TRACING
-      message<1024>("nullptr deallocation");
+        message<1024>("nullptr deallocation");
 #  endif
-      return;
+        return;
+      }
+
+      SecondaryAllocator::deallocate(p_tame.unsafe_ptr());
 #endif
     }
 
@@ -718,7 +738,7 @@ namespace snmalloc
 #else
       if constexpr (mitigations(sanity_checks))
       {
-        if (SecondaryAllocator::has_secondary_ownership(p))
+        if (!check_domestication(p))
           return;
         size = size == 0 ? 1 : size;
         auto sc = size_to_sizeclass_full(size);
@@ -769,7 +789,7 @@ namespace snmalloc
       return external_alloc::malloc_usable_size(const_cast<void*>(p_raw));
 #else
 
-      if (SecondaryAllocator::has_secondary_ownership(p_raw))
+      if (!SecondaryAllocator::pass_through && !check_domestication(p_raw))
         return SecondaryAllocator::alloc_size(p_raw);
       // TODO What's the domestication policy here?  At the moment we just
       // probe the pagemap with the raw address, without checks.  There could

src/snmalloc/mem/secondary.h

@@ -1,4 +1,8 @@
 #pragma once
+
+#include "snmalloc/ds_core/defines.h"
+#include "snmalloc/ds_core/ptrwrap.h"
+
 #ifdef SNMALLOC_ENABLE_GWP_ASAN_INTEGRATION
 #  include "snmalloc/mem/secondary/gwp_asan.h"
 

src/snmalloc/mem/secondary/default.h

@@ -10,11 +10,15 @@ namespace snmalloc
   class DefaultSecondaryAllocator
   {
   public:
+    // This flag is used to turn off checks on fast paths if the secondary
+    // allocator does not own the memory at all.
+    static constexpr inline bool pass_through = true;
+
     SNMALLOC_FAST_PATH
     static void initialize() {}
 
-    SNMALLOC_FAST_PATH
-    static void* allocate([[maybe_unused]] size_t size)
+    template<class SizeAlign>
+    SNMALLOC_FAST_PATH static void* allocate(SizeAlign&&)
     {
       return nullptr;
     }
@@ -32,13 +36,7 @@ namespace snmalloc
     }
 
     SNMALLOC_FAST_PATH
-    static bool has_secondary_ownership([[maybe_unused]] const void* pointer)
-    {
-      return false;
-    }
-
-    SNMALLOC_FAST_PATH
-    static size_t alloc_size([[maybe_unused]] const void* pointer)
+    static size_t alloc_size(const void*)
     {
       SNMALLOC_ASSERT(
         false &&

src/snmalloc/mem/secondary/gwp_asan.h

@@ -15,6 +15,8 @@ namespace snmalloc
     static inline size_t max_allocation_size;
 
   public:
+    static constexpr inline bool pass_through = false;
+
     static void initialize() noexcept
     {
       // for now, we use default options
@@ -31,10 +33,17 @@ namespace snmalloc
         singleton.getAllocatorState()->maximumAllocationSize();
     }
 
-    SNMALLOC_FAST_PATH static void* allocate(size_t size)
+    // Use thunk to avoid extra computation when allocation decision can be made
+    // before size and alignment are computed.
+    template<class SizeAlign>
+    SNMALLOC_FAST_PATH static void* allocate(SizeAlign&& getter)
     {
+      // TODO: this `shouldSample` is only triggered on snmalloc's slowpath,
+      // which may reduce the chance of error detection. We may reconsider
+      // the logic to improve the precision in future commits.
       if (SNMALLOC_UNLIKELY(singleton.shouldSample()))
       {
+        auto [size, align] = getter();
         if (size > max_allocation_size)
           return nullptr;
         auto alignment = natural_alignment(size);
@@ -46,9 +55,6 @@ namespace snmalloc
     SNMALLOC_FAST_PATH
     static void deallocate(void* pointer)
     {
-      if (SNMALLOC_LIKELY(pointer == nullptr))
-        return;
-
       snmalloc_check_client(
         mitigations(sanity_checks),
         singleton.pointerIsMine(pointer),
@@ -57,12 +63,6 @@ namespace snmalloc
       singleton.deallocate(pointer);
     }
 
-    SNMALLOC_FAST_PATH
-    static bool has_secondary_ownership([[maybe_unused]] const void* pointer)
-    {
-      return singleton.pointerIsMine(pointer);
-    }
-
     SNMALLOC_FAST_PATH
     static size_t alloc_size(const void* pointer)
     {

src/test/func/fixed_region/fixed_region.cc

@@ -43,7 +43,12 @@ int main()
     count += object_size;
     i++;
 
-    if (SecondaryAllocator::has_secondary_ownership(r1))
+    // Run until we exhaust the fixed region.
+    // This should return null.
+    if (r1 == nullptr)
+      break;
+
+    if (!a.check_domestication(r1))
     {
       a.dealloc(r1);
       continue;
@@ -54,10 +59,6 @@ int main()
       i = 0;
       std::cout << ".";
     }
-    // Run until we exhaust the fixed region.
-    // This should return null.
-    if (r1 == nullptr)
-      break;
 
     if (oe_base > r1)
     {

src/test/perf/external_pointer/externalpointer.cc

@@ -76,14 +76,16 @@ namespace test
         size_t rand = (size_t)r.next();
         size_t oid = rand & (((size_t)1 << count_log) - 1);
         size_t* external_ptr = objects[oid];
-        if (SecondaryAllocator::has_secondary_ownership(external_ptr))
+        if (!alloc.check_domestication(external_ptr))
           continue;
         size_t size = *external_ptr;
         size_t offset = (size >> 4) * (rand & 15);
         void* interior_ptr = pointer_offset(external_ptr, offset);
         void* calced_external = alloc.external_pointer(interior_ptr);
         if (calced_external != external_ptr)
+        {
           abort();
+        }
       }
     }