[PATCH] D73294: [GWP-ASan] enable/disable and fork support.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:121
     uintptr_t P = reinterpret_cast<uintptr_t>(Ptr);
     return GuardedPagePool <= P && P < GuardedPagePoolEnd;
   }
----------------
hctim wrote:
> Now that `GuardedPagePool` is zero-initialised, the fast path has an additional branch. Can be fixed via:
> `return P < GuardedPagePoolEnd && GuardedPagePool <= P;`
Sure.


================
Comment at: compiler-rt/lib/gwp_asan/tests/enable_disable.cpp:46
+  auto &GPA = *(gwp_asan::GuardedPoolAllocator *)arg;
+  // Initialize the allocator for this thread.
+  void *P = GPA.allocate(Size);
----------------
hctim wrote:
> What do mean by "initialize" here? GPA should already be inited by the parent.
> 
> If you're trying to init the sample counters to some nonzero value here, calling `shouldSample()` is the only way, not `allocate()`.
That's left over from the scudo tests. Removed the initialization and cleaned up synchronization a bit. We can not get rid of the sleep, unfortunately, because the thread needs to act while the other one is blocked inside fork(), and there is no clear way to detect that condition.



================
Comment at: compiler-rt/lib/gwp_asan/tests/harness.h:28
+
+extern gwp_asan::GuardedPoolAllocator GPA;
+
----------------
hctim wrote:
> Why a single global instance?
Yeah, I don't think it is required. We do need SetUp/TearDown logic though to properly set and remove the signal handlers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73294