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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D144057#4175101 <https://reviews.llvm.org/D144057#4175101>, @nikic wrote:

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

Something like this?

  diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
  index f36fa0bd0979..ca80c0b40557 100644
  --- a/llvm/lib/Transforms/Scalar/GVN.cpp
  +++ b/llvm/lib/Transforms/Scalar/GVN.cpp
  @@ -1781,11 +1781,14 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
       // If a non-local load would result in producing undef or an alloca (which
       // is not initialized), don't fold away control flow. We want the
       // sanitizers to help report this case.
  -    if (isa<UndefValue>(V) || isa<AllocaInst>(V)) {
  -      Function *F = Load->getParent()->getParent();
  -      if (F->hasFnAttribute(Attribute::SanitizeAddress) ||
  -          F->hasFnAttribute(Attribute::SanitizeHWAddress))
  -        return false;
  +    Function *F = Load->getParent()->getParent();
  +    if (F->hasFnAttribute(Attribute::SanitizeAddress) ||
  +        F->hasFnAttribute(Attribute::SanitizeHWAddress)) {
  +      for (AvailableValueInBlock &AV : ValuesPerBlock) {
  +        if (AV.AV.isUndefValue()) {
  +          return false;
  +        }
  +      }
       }
   
       Load->replaceAllUsesWith(V);

That doesn't seem to work, though perhaps you envisioned something else?

In D144057#4180735 <https://reviews.llvm.org/D144057#4180735>, @melver wrote:

> In D144057#4179643 <https://reviews.llvm.org/D144057#4179643>, @nickdesaulniers wrote:
>
>> In D144057#4166558 <https://reviews.llvm.org/D144057#4166558>, @melver wrote:
>>
>>> Do the KASAN tests in the kernel pass (need to use -next, mainline is currently broken)? Wondering how we can double check there are no new false positives nor false negatives.
>>
>> How do I run those?
>
> Just CONFIG_KASAN_KUNIT_TEST=y should do and then boot kernel.

I've done so and the system boots.  Was there supposed to be anything printed to the console? I just enabled KASAN=y, KUNIT=y, KASAN_KUNIT_TEST=y.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144057