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

[Mitch Phillips via Phabricator via llvm-commits]

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

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

> I'm struggling to understand why adding an additional branch on the fast path (e.g. init check in ret) would be faster. With the init check in the NextSampleCounter==0 branch we should be performing one fewer load/compare on the fast path in the enabled case. Could you show the code you used for the init check in ret/branch cases? I'm surprised by the results.


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`) ...

Changing `shouldSample` and `pointerIsMine` to use thread-safe atomics, and changing the RT-disabled check to be inside the `NextSampleCounter == 0` branch:

| Option                                  | Median Time (with 20 runs) | Runtime increase relative to CT-Disabled |
| Compile-time disabled                   | 3.5715s                    | 0.0000%                                  |
| Runtime disabled (init check in ret)    | 3.6270s                    | 1.5302%                                  |
| Runtime disabled (init check in branch) | 3.6330s                    | 1.6928%                                  |
| Runtime enabled (init check in ret)     | 3.6290s                    | 1.5845%                                  |
| Runtime enabled (init check in branch)  | 3.6365s                    | 1.7874%                                  |
|

Init check in ret:

  ALWAYS_INLINE bool shouldSample() {
      if (UNLIKELY(NextSampleCounter == 0))
        NextSampleCounter =
            (Random::getRandomUnsigned64() %
             __atomic_load_n(&AdjustedSampleRate, __ATOMIC_RELAXED)) +
            1;
  
      return NextSampleCounter-- == 1 && __atomic_load_n(&IsInitialised, __ATOMIC_RELAXED);
    }

Init check in branch:

  ALWAYS_INLINE bool shouldSample() {
      if (UNLIKELY(NextSampleCounter == 0)) {
        if (!__atomic_load_n(&IsInitialised, __ATOMIC_RELAXED))
           return false;
        NextSampleCounter =
            (Random::getRandomUnsigned64() %
             __atomic_load_n(&AdjustedSampleRate, __ATOMIC_RELAXED)) +
            1;
      }
  
      return NextSampleCounter-- == 1;
    }

I thought the above results were counterintuitive at first, but stepping through the disassembly...
For disabled:

1. init-ret is a TLS load, a non taken branch (with probability `1 / 2**63`), the same TLS load (is reused), a TLS decrement, and a branch (with probability `1 / 2**64`). Summary: 1 TLS load, 1 TLS store, two branches.
2. init-branch is a TLS load, a relaxed atomic load, and two branches. Summary: 1 TLS load, 1 atomic load, two branches.

... this would explain the performance increase as we generally don't have to make the atomic load, which will likely not be in L1D.

With runtime-enabled, we can generally short-circuit the atomic load as `NextSampleCounter ∉ {0, 1}`.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:182
+
+  bool ShouldRightAlign = (*Size % 2 == 1);
+
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593