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

Fri Apr 19 14:28:09 PDT 2019

hctim marked 2 inline comments as done.
hctim added a comment.

In D60593#1472458 <https://reviews.llvm.org/D60593#1472458>, @vlad.tsyrklevich wrote:

> 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?

New numbers (w/ 100 runs * 100m allocs per run):

| test env          | mean time (increase%) | median time (increase%) | std dev   |
| rt-enabled branch | 3.6266s (1.5193%)     | 3.6250s (1.4759%)       | 0.030507s |
| rt-enabled ret    | 3.6209s (1.3643%)     | 3.6230s (1.4215%)       | 0.021637s |
|

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.

In D60593#1472458 <https://reviews.llvm.org/D60593#1472458>, @vlad.tsyrklevich wrote:

> 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?)

New code does lazy-init, rather than c'tor-init of GwpAsan (i.e. we now call `GPA::init()` in Scudo's lazy init instead of Scudo's c'tor). Thus the non-thread local values in `shouldSample()` and `pointerIsMine()` require atomic load/store to be thread-safe.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593