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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

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

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

I think you also need to enable CONFIG_FTRACE=y or patch the kernel to have the config KASAN_KUNIT_TEST select TRACING. See https://lore.kernel.org/all/CAMn1gO7Ve4-d6vP4jvASQsTZ2maHsMF6gKHL3RXSuD9N3tAOfQ@mail.gmail.com/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144057