[llvm] r302787 - [msan] Fix PR32842

Thu May 11 14:22:15 PDT 2017

On Thu, May 11, 2017 at 11:28 AM, Alexander Potapenko <glider at google.com> wrote:
> How come this doesn't show up when I'm running check-msan locally?

It does when I run it. Do you have libcxx and libcxxabi checked out?
The "unit" tests would not run without it.

> Anyway, looking at this test I think it is simply invalid.
>
> In this particular case:
>   TestForNotPoisoned((poisoned(-1, 0x80000000U) >= poisoned(-1, 0U)))
> there are no constants in the IR, so the case is handled by handleShadowOr().
> The result of ORing the shadow for the two values in this test case is
> 0x80000000U, which is a false positive we've agreed to bear with by
> approximating >= with OR.
> But previously this result was discarded because of shadow value
> truncation, and now we don't truncate it.
>
> My patch made MSan report a false positive in this case, but had also
> fixed other cases not reflected here, e.g.:
>   TestForNotPoisoned((poisoned(-1, 0x80000000U) >= poisoned(-1, 2U)))
>
> I suggest we remove the offending line from the test and add a comment
> that some cases aren't handled because, well, this is an
> approximation.

Sounds good.

>
> On Thu, May 11, 2017 at 7:48 PM, Evgenii Stepanov
> <eugeni.stepanov at gmail.com> wrote:
>> Looks like you broke MemorySanitizer.ICmpRelational test:
>>
>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/1864/steps/64-bit%20check-msan/logs/stdio
>>
>> On Thu, May 11, 2017 at 4:07 AM, Alexander Potapenko via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>> Author: glider
>>> Date: Thu May 11 06:07:48 2017
>>> New Revision: 302787
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=302787&view=rev
>>> Log:
>>> [msan] Fix PR32842
>>>
>>> It turned out that MSan was incorrectly calculating the shadow for int comparisons: it was done by truncating the result of (Shadow1 OR Shadow2) to i1, effectively rendering all bits except LSB useless.
>>> This approach doesn't work e.g. in the case where the values being compared are even (i.e. have the LSB of the shadow equal to zero).
>>> Instead, if CreateShadowCast() has to cast a bigger int to i1, we replace the truncation with an ICMP to 0.
>>>
>>> This patch doesn't affect the code generated for SPEC 2006 binaries, i.e. there's no performance impact.
>>>
>>> For the test case reported in PR32842 MSan with the patch generates a slightly more efficient code:
>>>
>>>   orq     %rcx, %rax
>>>   jne     .LBB0_6
>>> , instead of:
>>>
>>>   orl     %ecx, %eax
>>>   testb   $1, %al
>>>   jne     .LBB0_6
>>>
>>>
>>>
>>> Added:
>>>     llvm/trunk/test/Instrumentation/MemorySanitizer/pr32842.ll
>>> Modified:
>>>     llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp
>>>
>>> Modified: llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp?rev=302787&r1=302786&r2=302787&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/Instrumentation/MemorySanitizer.cpp Thu May 11 06:07:48 2017
>>> @@ -1576,13 +1576,16 @@ struct MemorySanitizerVisitor : public I
>>>    Value *CreateShadowCast(IRBuilder<> &IRB, Value *V, Type *dstTy,
>>>                            bool Signed = false) {
>>>      Type *srcTy = V->getType();
>>> +    size_t srcSizeInBits = VectorOrPrimitiveTypeSizeInBits(srcTy);
>>> +    size_t dstSizeInBits = VectorOrPrimitiveTypeSizeInBits(dstTy);
>>> +    if (srcSizeInBits > 1 && dstSizeInBits == 1)
>>> +      return IRB.CreateICmpNE(V, getCleanShadow(V));
>>> +
>>>      if (dstTy->isIntegerTy() && srcTy->isIntegerTy())
>>>        return IRB.CreateIntCast(V, dstTy, Signed);
>>>      if (dstTy->isVectorTy() && srcTy->isVectorTy() &&
>>>          dstTy->getVectorNumElements() == srcTy->getVectorNumElements())
>>>        return IRB.CreateIntCast(V, dstTy, Signed);
>>> -    size_t srcSizeInBits = VectorOrPrimitiveTypeSizeInBits(srcTy);
>>> -    size_t dstSizeInBits = VectorOrPrimitiveTypeSizeInBits(dstTy);
>>>      Value *V1 = IRB.CreateBitCast(V, Type::getIntNTy(*MS.C, srcSizeInBits));
>>>      Value *V2 =
>>>        IRB.CreateIntCast(V1, Type::getIntNTy(*MS.C, dstSizeInBits), Signed);
>>>
>>> Added: llvm/trunk/test/Instrumentation/MemorySanitizer/pr32842.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Instrumentation/MemorySanitizer/pr32842.ll?rev=302787&view=auto
>>> ==============================================================================
>>> --- llvm/trunk/test/Instrumentation/MemorySanitizer/pr32842.ll (added)
>>> +++ llvm/trunk/test/Instrumentation/MemorySanitizer/pr32842.ll Thu May 11 06:07:48 2017
>>> @@ -0,0 +1,20 @@
>>> +; Regression test for https://bugs.llvm.org/show_bug.cgi?id=32842
>>> +;
>>> +; RUN: opt < %s -msan -S | FileCheck %s
>>> +;target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
>>> +target triple = "x86_64-unknown-linux-gnu"
>>> +
>>> +define zeroext i1 @_Z1fii(i32 %x, i32 %y) sanitize_memory {
>>> +entry:
>>> +  %cmp = icmp slt i32 %x, %y
>>> +  ret i1 %cmp
>>> +}
>>> +
>>> +; CHECK:      [[X:[^ ]+]] = load{{.*}}__msan_param_tls{{.*}}
>>> +; CHECK:      [[Y:[^ ]+]] = load{{.*}}__msan_param_tls{{.*}}
>>> +; CHECK:      [[OR:[^ ]+]] = or i32 [[Y]], [[X]]
>>> +
>>> +; Make sure the shadow of the (x < y) comparison isn't truncated to i1.
>>> +; CHECK-NOT:  trunc i32 [[OR]] to i1
>>> +; CHECK:      [[CMP:[^ ]+]] = icmp ne i32 [[OR]], 0
>>> +; CHECK:      store i1 [[CMP]],{{.*}}__msan_retval_tls
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg