[PATCH] D62872: [GWP-ASan] Core Guarded Pool Allocator [4].

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 10 inline comments as done.
hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:162
+
+  if (Meta->IsDeallocated) {
+    reportError(UPtr, Error::DOUBLE_FREE);
----------------
vlad.tsyrklevich wrote:
> There's a race checking this and having it set it later in RecordDeallocation.
Darn, I tried to be clever here and not hold the mutex during the expensive call to `markInaccessible`.  Instead, I clearly need to scope this, unlock for the `mprotect` and then lock the critical path again.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:186
+AllocationMetadata *GuardedPoolAllocator::addrToMetadata(uintptr_t Ptr) const {
+  assert(pointerIsMine(reinterpret_cast<void *>(Ptr)));
+  return &Metadata[addrToSlot(Ptr)];
----------------
vlad.tsyrklevich wrote:
> Put this assert in addrToSlot and then this function is covered as well.
Done. Also added them to `getPageAddr()` and `isGuardPage()`.


================
Comment at: compiler-rt/lib/gwp_asan/tests/thread_contention.cpp:82
+
+  runThreadContentionTest(NumThreads, NumIterations, &GPA);
+}
----------------
vlad.tsyrklevich wrote:
> Given that NumIterations >> NumThreads (e.g. NumSlots), isn't this test redundant?
Yep. Removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62872/new/

https://reviews.llvm.org/D62872