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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:112
       ThreadLocals.NextSampleCounter =
-          (getRandomUnsigned32() % AdjustedSampleRate) + 1;
+          (getRandomUnsigned32() % (AdjustedSampleRatePlusOne - 1)) + 1;
 
----------------
`// 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.`


================
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;
   }
----------------
Now that `GuardedPagePool` is zero-initialised, the fast path has an additional branch. Can be fixed via:
`return P < GuardedPagePoolEnd && GuardedPagePool <= P;`


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:95
+  auto Disable = []() {
+    if (auto *S = getSingleton())
+      S->disable();
----------------
Why does this need to disable the singleton? Why not just disable this class (which should be equivalent).


================
Comment at: compiler-rt/lib/gwp_asan/tests/enable_disable.cpp:45
+static void *enableMalloc(void *arg) {
+  auto &GPA = *(gwp_asan::GuardedPoolAllocator *)arg;
+  // Initialize the allocator for this thread.
----------------
Nit: prefer c++-style casting


================
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);
----------------
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()`.


================
Comment at: compiler-rt/lib/gwp_asan/tests/harness.cpp:7
+
+bool FirstTrueThenFalse() {
+  static int x = 0;
----------------
`OnlyOnce`?


================
Comment at: compiler-rt/lib/gwp_asan/tests/harness.h:28
+
+extern gwp_asan::GuardedPoolAllocator GPA;
+
----------------
Why a single global instance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73294