[PATCH] D144057: [GVN] permit GVN of non-local loads for ASAN unless undef or alloca is produced

Tue Mar 7 06:22:45 PST 2023

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

In D144057#4130597 <https://reviews.llvm.org/D144057#4130597>, @nickdesaulniers wrote:

> In D144057#4128142 <https://reviews.llvm.org/D144057#4128142>, @nikic wrote:
>
>> I don't think this is right as implemented. Imagine you have `if (...) { store v, p } x = load p`. GVN would convert this to something like `x = phi(v, undef)`. With asan, we want this to report if the if is not taken. However, I believe that your checks will permit the transform, because the load folding result is not literally undef (but is still undef on one path).
>
> Is this what you're imagining? https://godbolt.org/z/Pc3YezE44
>
> Seems like ASAN already misses that? In this case, we go to replace the load with alloca, so adding a check remedies that case. But it doesn't change the fact that asan misses that C test case.

In this case SROA should drop the alloca and it will never get to GVN. You want something more like this: https://godbolt.org/z/8z66K7xbh

I still don't think that this makes sense as-implemented. You are only checking the final result after SSA construction. I believe this needs to at least check all AvailableValues for isUndefValue().

================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1773
+    // sanitizers to help report this case.
+    if (isa<UndefValue>(V) || isa<AllocaInst>(V)) {
+      Function *F = Load->getParent()->getParent();
----------------
I don't get the alloca check here. `V` is the result of the load, not the loaded pointer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144057