Enable a seconary allocator support (e.g. GWP-Asan) #737

SchrodingerZhu · 2025-01-27T03:54:39Z

No description provided.

mjp41

This looks great to me. Thanks for doing this.

I think you are missing the malloc_usable_size path, which if called on a GWP Asan allocation will not be correct.

I am keen to explore using this for two other things:

Sampling profiler - We could pass allocations to a sampling allocator as the secondary allocator. This secondary allocator would keep track of stack traces, etc.
Windows malloc replacement using Detours. This is tricky to do, and if you don't get in quick enough, then deallocations can be passed in that were from the Windows Heap. With this, we could take the undetours functions as the secondary free path, and _msize path. We would ignore the allocation path.

I think you design mostly works for this, and certainly meets the #697. I think tracking enough information to unbias the sampling profiler would need more work, but shouldn't be part of this PR.

mjp41 · 2025-01-27T13:50:07Z

src/snmalloc/mem/localalloc.h

@@ -767,6 +768,9 @@ namespace snmalloc
 #ifdef SNMALLOC_PASS_THROUGH
      return external_alloc::malloc_usable_size(const_cast<void*>(p_raw));
 #else
+
+      if (SecondaryAllocator::has_secondary_ownership(p_raw))


I would prefer this to check that the allocation is not owned by snmalloc, rather than rely on the secondary allocator having this. For GWP Asan, it is fine.

But I would also been keen to use this to build an approach on Windows to replace the allocator, and pass any original frees back to the Windows Heap.

SNMALLOC_FAST_PATH size_t alloc_size(const void* p_raw) { #ifdef SNMALLOC_PASS_THROUGH return external_alloc::malloc_usable_size(const_cast<void*>(p_raw)); #else if (!check_domestication(p_raw)) return SecondaryAllocator::alloc_size(p_raw); // ...... }

Could you illustrate a better way to do this? If do it the way above, we will always have the overhead for domestication test.

const PagemapEntry& entry = Config::Backend::get_metaentry(address_cast(p_raw)); // Check is managed by snmalloc if (SNMALLOC_LIKELY(entry.get_remote() != nullptr) return sizeclass_full_to_size(entry.get_sizeclass()); // If not managed by snmalloc, pass to secondary allocator. return SecondaryAllocator::alloc_size(p_raw);

mjp41 · 2025-01-27T15:24:24Z

src/snmalloc/mem/secondary/gwp_asan.h

+
+    SNMALLOC_FAST_PATH static void* allocate(size_t size)
+    {
+      if (SNMALLOC_UNLIKELY(singleton.shouldSample()))


As this is only getting called when a free list is exhausted should we be doing something else here. I.e. should we be passing how many requests of this size have been handled since we last called this allocator?

shouldSample is implemented as

// NextSampleCounter == 0 means we "should regenerate the counter". // == 1 means we "should sample this allocation". // AdjustedSampleRatePlusOne is designed to intentionally underflow. This // class must be valid when zero-initialised, and we wish to sample as // infrequently as possible when this is the case, hence we underflow to // UINT32_MAX. if (GWP_ASAN_UNLIKELY(getThreadLocals()->NextSampleCounter == 0)) getThreadLocals()->NextSampleCounter = ((getRandomUnsigned32() % (AdjustedSampleRatePlusOne - 1)) + 1) & ThreadLocalPackedVariables::NextSampleCounterMask; return GWP_ASAN_UNLIKELY(--getThreadLocals()->NextSampleCounter == 0);

Sorry, I meant. Normally usage of GWP Asan, every call to malloc will call shouldSample. But this usage has moved it from the fast path, so we will call it every new free list for that size. This in the case of 16 byte allocations worst case could be 1024 allocations between calls. This will alter the likelihood of it being called, and might bias away from small allocations. I think for GWP Asan on we would want randomisation too, so that the free list that gets exhausted.

I think this is probably the same information that would be required to unbias profiling, so should probably be left to a subsequent PR, but we should perhaps add either an issue or a comment about this.

devnexen · 2025-01-28T07:17:58Z

src/snmalloc/mem/secondary/gwp_asan.h

+    }
+
+    SNMALLOC_FAST_PATH
+    static bool has_secondary_ownership([[maybe_unused]] const void* pointer)


nit: do not think [maybe_unused] is necessary here ?

mjp41 · 2025-01-28T10:31:41Z

src/snmalloc/mem/secondary/default.h

+    static void initialize() {}
+
+    SNMALLOC_FAST_PATH
+    static void* allocate([[maybe_unused]] size_t size)


Suggested change

static void* allocate([[maybe_unused]] size_t size)

static void* allocate(size_t)

mjp41 · 2025-01-28T10:31:56Z

src/snmalloc/mem/secondary/default.h

+    }
+
+    SNMALLOC_FAST_PATH
+    static bool has_secondary_ownership([[maybe_unused]] const void* pointer)


Suggested change

static bool has_secondary_ownership([[maybe_unused]] const void* pointer)

static bool has_secondary_ownership(const void*)

mjp41 · 2025-01-28T10:32:12Z

src/snmalloc/mem/secondary/default.h

+    }
+
+    SNMALLOC_FAST_PATH
+    static size_t alloc_size([[maybe_unused]] const void* pointer)


Suggested change

static size_t alloc_size([[maybe_unused]] const void* pointer)

static size_t alloc_size(const void*)

mjp41 · 2025-01-28T10:39:59Z

src/snmalloc/mem/secondary/gwp_asan.h

+
+    SNMALLOC_FAST_PATH static void* allocate(size_t size)
+    {
+      if (SNMALLOC_UNLIKELY(singleton.shouldSample()))


Sorry, I meant. Normally usage of GWP Asan, every call to malloc will call shouldSample. But this usage has moved it from the fast path, so we will call it every new free list for that size. This in the case of 16 byte allocations worst case could be 1024 allocations between calls. This will alter the likelihood of it being called, and might bias away from small allocations. I think for GWP Asan on we would want randomisation too, so that the free list that gets exhausted.

I think this is probably the same information that would be required to unbias profiling, so should probably be left to a subsequent PR, but we should perhaps add either an issue or a comment about this.

mjp41 · 2025-01-29T09:40:00Z

src/snmalloc/mem/localalloc.h

+      void* result = SecondaryAllocator::allocate(size);
+      if (result != nullptr)
+        return capptr::Alloc<void>::unsafe_from(result);
+


I think we should probably move this inside the check_init call. I.e. just before // Grab slab of correct size, I think we might have initialisation issues if we haven't fully initialised snmalloc as the pagemap won't exist when we look up a GWP Asan allocation.

mjp41 · 2025-01-29T09:44:47Z

src/snmalloc/mem/corealloc.h

+      void* result = SecondaryAllocator::allocate(rsize);
+      if (result != nullptr)
+        return capptr::Alloc<void>::unsafe_from(result);
+


Thinking about this a bit more. I wonder if this should be at the start of small_alloc. I think there are allocation patterns that would never hit this path. It is possible to work completely without having to grab a new chunk from the backend.

Sorry, I think I had this wrong in my original issue.

SchrodingerZhu · 2025-01-29T14:06:07Z

@mjp41 could you help rerun the failed test in the latest action submission

SchrodingerZhu · 2025-01-29T14:27:55Z

seems working :D

mjp41

Thanks for all the changes. Just one small bit left.

@nwf would you be able to take a look at the comment below?

src/snmalloc/mem/localalloc.h

mjp41

Awesome work. Thanks

SchrodingerZhu added 5 commits January 27, 2025 11:53

[WIP] support secondary allocation

c2eaee2

almost work

573c221

clean up

9a98bc1

override alloc_size

6d0b985

fix

f6709db

SchrodingerZhu force-pushed the secondary branch from 427e852 to f6709db Compare January 27, 2025 07:25

avoid repeated leaking from gwp_asan

d7acd32

SchrodingerZhu force-pushed the secondary branch 7 times, most recently from 998c17f to 239a366 Compare January 27, 2025 09:21

add related workflows

121102c

SchrodingerZhu force-pushed the secondary branch from 239a366 to 121102c Compare January 27, 2025 09:35

SchrodingerZhu changed the title ~~[WIP] support secondary allocation~~ support secondary allocation Jan 27, 2025

SchrodingerZhu requested review from mjp41 and devnexen January 27, 2025 12:02

mjp41 requested changes Jan 27, 2025

View reviewed changes

mjp41 reviewed Jan 27, 2025

View reviewed changes

devnexen reviewed Jan 28, 2025

View reviewed changes

mjp41 requested changes Jan 28, 2025

View reviewed changes

mjp41 reviewed Jan 29, 2025

View reviewed changes

SchrodingerZhu force-pushed the secondary branch 3 times, most recently from d53aa2e to 87dcba3 Compare January 29, 2025 13:20

address code reviews

dbaae40

SchrodingerZhu force-pushed the secondary branch from 87dcba3 to dbaae40 Compare January 29, 2025 13:21

minor fix

7bc335c

add special check for nullptr

3492df6

mjp41 reviewed Jan 29, 2025

View reviewed changes

src/snmalloc/mem/localalloc.h Outdated Show resolved Hide resolved

rename according to CRs

0081383

mjp41 approved these changes Feb 5, 2025

View reviewed changes

mjp41 changed the title ~~support secondary allocation~~ Enable a seconary allocator support (e.g. GWP-Asan) Feb 5, 2025

mjp41 merged commit 16b9624 into microsoft:main Feb 5, 2025
66 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable a seconary allocator support (e.g. GWP-Asan) #737

Enable a seconary allocator support (e.g. GWP-Asan) #737

SchrodingerZhu commented Jan 27, 2025

mjp41 left a comment

mjp41 Jan 27, 2025

SchrodingerZhu Jan 29, 2025 •

edited

Loading

mjp41 Jan 29, 2025

mjp41 Jan 27, 2025

SchrodingerZhu Jan 28, 2025 •

edited

Loading

mjp41 Jan 28, 2025

devnexen Jan 28, 2025

mjp41 Jan 28, 2025

mjp41 Jan 28, 2025

mjp41 Jan 28, 2025

mjp41 Jan 28, 2025

mjp41 Jan 29, 2025

mjp41 Jan 29, 2025

SchrodingerZhu commented Jan 29, 2025

SchrodingerZhu commented Jan 29, 2025

mjp41 left a comment

mjp41 left a comment

	static void* allocate([[maybe_unused]] size_t size)
	static void* allocate(size_t)

	static bool has_secondary_ownership([[maybe_unused]] const void* pointer)
	static bool has_secondary_ownership(const void*)

	static size_t alloc_size([[maybe_unused]] const void* pointer)
	static size_t alloc_size(const void*)

Enable a seconary allocator support (e.g. GWP-Asan) #737

Enable a seconary allocator support (e.g. GWP-Asan) #737

Conversation

SchrodingerZhu commented Jan 27, 2025

mjp41 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchrodingerZhu Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchrodingerZhu Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchrodingerZhu commented Jan 29, 2025

SchrodingerZhu commented Jan 29, 2025

mjp41 left a comment

Choose a reason for hiding this comment

mjp41 left a comment

Choose a reason for hiding this comment

SchrodingerZhu Jan 29, 2025 •

edited

Loading

SchrodingerZhu Jan 28, 2025 •

edited

Loading