[PATCH] D143565: [Asan] Ensure unpoisonning doesn't get inlined unnecessarily due to small holes in the mask

[Sylvain Audi via Phabricator via llvm-commits]

saudi added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2964
+
     if (j - i >= ClMaxInlinePoisoningSize) {
       copyToShadowInline(ShadowMask, ShadowBytes, Done, i, IRB, ShadowBase);
----------------
rsundahl wrote:
> This function is probably not the place to guess that it's ok to ignore the shadow mask because it's assumed that it's being called for un-poisoning. While this may be the case, the shadow mask and shadow data provide high fidelity control over behavior and assuming the intent at this low level is problematic. I realize that at a line 2888 the same assumption is made during inlining, but moving this up to the caller and cleaning up these two functions would be far superior. 
I can take a look and try to move the logic to a higher level.
The `copyToShadow` declaration has a comment attached:
```
  // Copies bytes from ShadowBytes into shadow memory for indexes where
  // ShadowMask is not zero. If ShadowMask[i] is zero, we assume that
  // ShadowBytes[i] is constantly zero and doesn't need to be overwritten.
```

Would the solution be to:
 1) remove from `copyToShadow` / `copyToShadowInline` the assumption about zero (and remove the second sentence from the comment I mentionned above)
 2) Add a method `bridgeUnpoisonningGaps` that would optimize the shadow buffer so that the generated instructions are more optimized 
?

I'm a bit stuck, as this may make `bridgeUnpoisonningGaps` assume too much about the optimization details.

What do you think?


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-optimize-inline-poisoning.ll:2
+; RUN: opt < %s  -passes=asan -asan-max-inline-poisoning-size=8 -S | FileCheck %s --check-prefix=CHECK
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
----------------
rsundahl wrote:
> Your test when run with -asan-max-inline-poisoning-size=0 still inlines but it doesn't look like it is due to your changes (unless datalayout is contrived for the test and doesn't occur otherwise.) It's more likely that my testing for [[ https://reviews.llvm.org/D136197  ]] was inadequate.
I tried this on trunk, but the output didn't seem to contain inlined poisonning. see https://llvm.godbolt.org/z/qWr3P54Ex

The generated stores don't look related with [un]poisonning, as they don't write to shadow memory.

Or did I miss something?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143565