[llvm] r302787 - [msan] Fix PR32842

[Alexander Potapenko via llvm-commits]

How come this doesn't show up when I'm running check-msan locally?

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.

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