[llvm] r302787 - [msan] Fix PR32842

Fri May 12 02:52:55 PDT 2017

Fixed in r302887.

On Thu, May 11, 2017 at 11:22 PM, Evgenii Stepanov
<eugeni.stepanov at gmail.com> wrote:
> 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


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