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

[Roy Sundahl via Phabricator via llvm-commits]

rsundahl added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2964
+
     if (j - i >= ClMaxInlinePoisoningSize) {
       copyToShadowInline(ShadowMask, ShadowBytes, Done, i, IRB, ShadowBase);
----------------
saudi wrote:
> 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?
An optimization may be a much better place to capture the idea and your proposal seems good provided more senior reviewers are supportive . I have looked at this loopy/call/inline code enough to wish it simpler and if the shadow mask wasn't broken up into these smaller fragments then the code could be simpler and more readable. (This effectively would be equivalent to doing a local pre-scan of the shadowMask and setting it to 1 wherever the shadowData was 0, and then falling into the loop. Might be a good way to test it first.)


================
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"
----------------
saudi wrote:
> 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?
> 
> 
I think you'll find the stores at line 69 and line 139 are inline accesses to the shadow memory. That said, I don't think that this is a consequence of your change but more a result of the logic in the loop that's already there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143565