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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

> For the branch case, I had the conditional branch outside of `NextSampleCounter == 0`, so that the RT-disabled would incur minimal overhead (single branch instead of two branches), e.g.
> 
>   shouldSample() {
>     if (!IsInitialised) return false;
>     if (NextSampleCounter == 0`) ...

Ah, earlier you mentioned you were optimizing for the RT-enabled case so I assumed this compare was placed in the slow-path. This seems like it would be slower for the RT-enabled case.

> | Runtime enabled (init check in ret)    | 3.6290s | 1.5845% |
> | Runtime enabled (init check in branch) | 3.6365s | 1.7874% |
> |

This is what I'm trying to understand. Why is adding an IsInitialized load and branch on the fast path (e.g. the ret case) more performant? You explain the logic for the RT-disabled case, but I'm trying to figure out why ret is faster than branch for the enabled case. It seems like that IsInitialized comparison is pure overhead. With the new init check on branch code you included I would not expect that to be the case. Perhaps given the artificiality of the test the caches/branch predictors are primed to just ignore that ret?

Also is there a reason why you use atomics now and not before? I don't think that the new code requires the use of atomics compared to the old code (or is it just that we should have been using atomics previously?)



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:182
+
+  bool ShouldRightAlign = (*Size % 2 == 1);
+
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > In Chrome we left- or right-align based on whether the (randomly chosen) slot is even/odd, that means an allocation will test for both under- and over-flows across different runs. However in this case we always left-/right-align based on the size which means a given type will only ever be tested one way (and I'd expect we have a lot more even allocations than odd allocations.)
> Gotcha. In this case, we need a runtime option (like the previous `AlignmentStrategy`) to guarantee left && right alignment for testing, as we don't want flaky tests.
> 
> How would you prefer to go about this? Two runtime options, `NoRandomAlignment` and `AlignRight`?
Why not just have the tests either 1) Use full-page allocations so that we know exactly where the bounds are or 2) a helper routine to get a left-/right-aligned allocation? Those are the two things I end up using in Chrome and they've been sufficient.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593