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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
