[msan] Fix PR32842

[glider]

Summary

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

[eugenis]

Please add an IR test, and, optionally, a runtime test in compiler-rt.

lib/Transforms/Instrumentation/MemorySanitizer.cpp

1581

Inner braces are unnecessary.

[glider]

Fixed the comment.
The test is in https://reviews.llvm.org/D33002

[eugenis]

Still missing an IR test, which should look something like test/Instrumentation/MemorySanitizer/msan_basic.ll - pass a single-instruction function through opt -msan and see what comes out.

Also, if you have a program that fails to find a bug, add it as a test under projects/compiler-rt/test/msan/.

[glider]

Added an IR test suggested by Evgenii.

[glider]

Thanks for the suggestion, works for me!
I've also added a compiler-rt regtest, see https://reviews.llvm.org/D33043

[eugenis]

LGTM with a note

test/Instrumentation/MemorySanitizer/pr32842.ll

14

%1 and %0 could be unreliable.
Just test the entire instruction sequence, something like this:
%[[A]] = load.*msan_param_tls
%[[B]] = load.*msan_param_tls
%[[C]] = or i32 %[[A], %[[B]]
%[[D]] = icmp ne i32%[[C]], 0
store i1 %[[D]], .*__msan_retval_tls

[glider]

Fixed the test.

[glider]

Committed in r302787.

[dvyukov]

Nice!

lib/Transforms/Instrumentation/MemorySanitizer.cpp

return Ty->isVectorTy() ?
       Ty->getPrimitiveSizeInBits();
   }
 
   /// \brief Cast between two shadow types, extending or truncating as
   /// necessary.
   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);
     return IRB.CreateBitCast(V2, dstTy);
     // TODO: handle struct types.
   }
 
   /// \brief Cast an application value to the type of its own shadow.

test/Instrumentation/MemorySanitizer/pr32842.ll

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