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

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added a comment.

This diff is helpful to get an overall idea of how things fit together, but it is very difficult to review thoroughly.  Let's start splicing off pieces for individual review.

I suggest:

- Individual reviews for each prereq (mutex, random, etc.)
- Review for base GPA + unit tests
- Review for "optional" options parsing, etc.
- Review for documentation
- Review for Scudo integration + e2e tests

Submitting this in pieces (especially the Scudo integration) will also make things simpler if we need to revert something.



================
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".
----------------
hctim wrote:
> 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`.
What is the assembly now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593