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

[Mitch Phillips via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:97
+  // Return whether the allocation should be randomly chosen for sampling.
+  ALWAYS_INLINE bool shouldSample() {
+    // NextSampleCounter == 0 means we "should regenerate the counter".
----------------
morehouse wrote:
> The assembly you posted for this function looks way too slow.  Is it actually optimized with `-O2`?
> 
> - The fast path (counter > 1) has *two* branches that check if `NextSampleCounter`'s TLS init function has run and one actual call to the TLS init function.  Ouch!  We shouldn't need these.
>   - Maybe we need to make the counter function-local or use `__thread` instead of `thread_local`.
> - The counter decrement of `NextSampleCounter` requires 4 instructions?  Load TLS offset from global, read TLS, increment, write TLS.  Yikes.  For reference, TCMalloc does this operation in 1 instruction.
> - The `+ 1` operation on the `NextSampleCounter == 0` branch is done with a mov-add, rather than an lea.
> 
> Again, please triple-check that the binary you compiled is actually optimized.  Make sure it is compiled with `clang++ -O2 ...`.
> 
> 
As per offline discussion, declaring `NextSampleCounter` with `__thread` instead of `thread_local` fixed this. Also added a note in `definitions.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593