[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

> I believe the short circuiting of the IsInitialised load/branch in the general case basically leads to the performance being the same. The values are so close that I'm willing to believe that it's within margin-of-error, but some minor microarchitectural feature could also be leading to the ret version being faster.

Ah, this is what I didn't consider. The IsInitialized is after the NextSampleCounter-- == 0 so it's actually not on the fast path. I glazed over that part and assumed it was being reached on every call to shouldSample. My mistake.

I'll take a look through the tests and docs after this next set of changes as I don't have much more feedback left.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:34
+
+  if (Opts.InstallSignalHandlers)
+    installSignalHandlers();
----------------
Any reason to do this here and not until just before IsInitialised is set? If we're very unlucky and hit an exception  on the next instruction the signal handler will fault on calling Printf, perhaps other such conditions will be introduced in the future as well.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:125
+
+  freeSlot(addrToSlot(UPtr));
+  markInaccessible(reinterpret_cast<void *>(Page), maximumAllocationSize());
----------------
Call freeSlot after markInaccessible, otherwise you introduce a very unlikely race where the slot is freed, then another threads takes it and marks the page RW before it's marked RO below.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:184
+
+void GuardedPoolAllocator::applyAllocationStrategy(std::size_t *Size,
+                                                   uintptr_t *AllocationPtr) {
----------------
Size isn't read again by the caller so I'd change it to pass-by-value.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:74
+    uintptr_t P = reinterpret_cast<uintptr_t>(Ptr);
+    return GetGuardedPagePool() <= P && P < GetGuardedPagePoolEnd();
+  }
----------------
We've given GuardedPagePool/GuardedPagePoolEnd the atomic treatment but since we access them sequentially it's possible that we get the sequence:
T1 Load GuardedPagePool
T2 Lazy Init
T1 Load GuardedPagePoolEnd

and give a bad value if the pointer is between 0 and GuardedPagePoolEnd. It seems like these values should not be atomic but that instead we are forced to perform an IsInitialised check first instead.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:234
+  // recalcualting the sample rate.
+  ALIGNED(8) uint64_t AdjustedSampleRate = UINT64_MAX - 1;
+  // Thread-local decrementing counter that indicates that a given allocation
----------------
ALIGNED(8) seems unnecessary for a uint64_t?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593