[msan] Precise instrumentation for icmp sgt %x, -1.

Extend signed relational comparison instrumentation with a special
case for comparisons with -1. This fixes an MSan false positive when
such comparison is used as a sign bit test.

https://llvm.org/bugs/show_bug.cgi?id=24561

llvm-svn: 245980

2 files changed, 96 insertions, 25 deletions

diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index e36637a4fd8..d296474abe1 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -1736,25 +1736,30 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
 
   /// \brief Instrument signed relational comparisons.
   ///
-  /// Handle (x<0) and (x>=0) comparisons (essentially, sign bit tests) by
-  /// propagating the highest bit of the shadow. Everything else is delegated
-  /// to handleShadowOr().
+  /// Handle sign bit tests: x<0, x>=0, x<=-1, x>-1 by propagating the highest
+  /// bit of the shadow. Everything else is delegated to handleShadowOr().
   void handleSignedRelationalComparison(ICmpInst &I) {
-    Constant *constOp0 = dyn_cast<Constant>(I.getOperand(0));
-    Constant *constOp1 = dyn_cast<Constant>(I.getOperand(1));
-    Value* op = nullptr;
-    CmpInst::Predicate pre = I.getPredicate();
-    if (constOp0 && constOp0->isNullValue() &&
-        (pre == CmpInst::ICMP_SGT || pre == CmpInst::ICMP_SLE)) {
-      op = I.getOperand(1);
-    } else if (constOp1 && constOp1->isNullValue() &&
-               (pre == CmpInst::ICMP_SLT || pre == CmpInst::ICMP_SGE)) {
+    Constant *constOp;
+    Value *op = nullptr;
+    CmpInst::Predicate pre;
+    if ((constOp = dyn_cast<Constant>(I.getOperand(1)))) {
       op = I.getOperand(0);
+      pre = I.getPredicate();
+    } else if ((constOp = dyn_cast<Constant>(I.getOperand(0)))) {
+      op = I.getOperand(1);
+      pre = I.getSwappedPredicate();
+    } else {
+      handleShadowOr(I);
+      return;
     }
-    if (op) {
+
+    if ((constOp->isNullValue() &&
+         (pre == CmpInst::ICMP_SLT || pre == CmpInst::ICMP_SGE)) ||
+        (constOp->isAllOnesValue() &&
+         (pre == CmpInst::ICMP_SGT || pre == CmpInst::ICMP_SLE))) {
       IRBuilder<> IRB(&I);
-      Value* Shadow =
-        IRB.CreateICmpSLT(getShadow(op), getCleanShadow(op), "_msprop_icmpslt");
+      Value *Shadow = IRB.CreateICmpSLT(getShadow(op), getCleanShadow(op),
+                                        "_msprop_icmp_s");
       setShadow(&I, Shadow);
       setOrigin(&I, getOrigin(op));
     } else {
diff --git a/llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll b/llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
index 6ae7c1d4427..cacc9b749dd 100644
--- a/llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
+++ b/llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll
@@ -385,48 +385,99 @@ entry:
 
 ; Check that we propagate shadow for x<0, x>=0, etc (i.e. sign bit tests)
 
-define zeroext i1 @ICmpSLT(i32 %x) nounwind uwtable readnone sanitize_memory {
+define zeroext i1 @ICmpSLTZero(i32 %x) nounwind uwtable readnone sanitize_memory {
   %1 = icmp slt i32 %x, 0
   ret i1 %1
 }
 
-; CHECK-LABEL: @ICmpSLT
+; CHECK-LABEL: @ICmpSLTZero
 ; CHECK: icmp slt
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: icmp slt
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: ret i1
 
-define zeroext i1 @ICmpSGE(i32 %x) nounwind uwtable readnone sanitize_memory {
+define zeroext i1 @ICmpSGEZero(i32 %x) nounwind uwtable readnone sanitize_memory {
   %1 = icmp sge i32 %x, 0
   ret i1 %1
 }
 
-; CHECK-LABEL: @ICmpSGE
+; CHECK-LABEL: @ICmpSGEZero
 ; CHECK: icmp slt
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: icmp sge
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: ret i1
 
-define zeroext i1 @ICmpSGT(i32 %x) nounwind uwtable readnone sanitize_memory {
+define zeroext i1 @ICmpSGTZero(i32 %x) nounwind uwtable readnone sanitize_memory {
   %1 = icmp sgt i32 0, %x
   ret i1 %1
 }
 
-; CHECK-LABEL: @ICmpSGT
+; CHECK-LABEL: @ICmpSGTZero
 ; CHECK: icmp slt
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: icmp sgt
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: ret i1
 
-define zeroext i1 @ICmpSLE(i32 %x) nounwind uwtable readnone sanitize_memory {
+define zeroext i1 @ICmpSLEZero(i32 %x) nounwind uwtable readnone sanitize_memory {
   %1 = icmp sle i32 0, %x
   ret i1 %1
 }
 
-; CHECK-LABEL: @ICmpSLE
+; CHECK-LABEL: @ICmpSLEZero
+; CHECK: icmp slt
+; CHECK-NOT: call void @__msan_warning
+; CHECK: icmp sle
+; CHECK-NOT: call void @__msan_warning
+; CHECK: ret i1
+
+
+; Check that we propagate shadow for x<=-1, x>-1, etc (i.e. sign bit tests)
+
+define zeroext i1 @ICmpSLTAllOnes(i32 %x) nounwind uwtable readnone sanitize_memory {
+  %1 = icmp slt i32 -1, %x
+  ret i1 %1
+}
+
+; CHECK-LABEL: @ICmpSLTAllOnes
+; CHECK: icmp slt
+; CHECK-NOT: call void @__msan_warning
+; CHECK: icmp slt
+; CHECK-NOT: call void @__msan_warning
+; CHECK: ret i1
+
+define zeroext i1 @ICmpSGEAllOnes(i32 %x) nounwind uwtable readnone sanitize_memory {
+  %1 = icmp sge i32 -1, %x
+  ret i1 %1
+}
+
+; CHECK-LABEL: @ICmpSGEAllOnes
+; CHECK: icmp slt
+; CHECK-NOT: call void @__msan_warning
+; CHECK: icmp sge
+; CHECK-NOT: call void @__msan_warning
+; CHECK: ret i1
+
+define zeroext i1 @ICmpSGTAllOnes(i32 %x) nounwind uwtable readnone sanitize_memory {
+  %1 = icmp sgt i32 %x, -1
+  ret i1 %1
+}
+
+; CHECK-LABEL: @ICmpSGTAllOnes
+; CHECK: icmp slt
+; CHECK-NOT: call void @__msan_warning
+; CHECK: icmp sgt
+; CHECK-NOT: call void @__msan_warning
+; CHECK: ret i1
+
+define zeroext i1 @ICmpSLEAllOnes(i32 %x) nounwind uwtable readnone sanitize_memory {
+  %1 = icmp sle i32 %x, -1
+  ret i1 %1
+}
+
+; CHECK-LABEL: @ICmpSLEAllOnes
 ; CHECK: icmp slt
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: icmp sle
@@ -437,18 +488,33 @@ define zeroext i1 @ICmpSLE(i32 %x) nounwind uwtable readnone sanitize_memory {
 ; Check that we propagate shadow for x<0, x>=0, etc (i.e. sign bit tests)
 ; of the vector arguments.
 
-define <2 x i1> @ICmpSLT_vector(<2 x i32*> %x) nounwind uwtable readnone sanitize_memory {
+define <2 x i1> @ICmpSLT_vector_Zero(<2 x i32*> %x) nounwind uwtable readnone sanitize_memory {
   %1 = icmp slt <2 x i32*> %x, zeroinitializer
   ret <2 x i1> %1
 }
 
-; CHECK-LABEL: @ICmpSLT_vector
+; CHECK-LABEL: @ICmpSLT_vector_Zero
 ; CHECK: icmp slt <2 x i64>
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: icmp slt <2 x i32*>
 ; CHECK-NOT: call void @__msan_warning
 ; CHECK: ret <2 x i1>
 
+; Check that we propagate shadow for x<=-1, x>0, etc (i.e. sign bit tests)
+; of the vector arguments.
+
+define <2 x i1> @ICmpSLT_vector_AllOnes(<2 x i32> %x) nounwind uwtable readnone sanitize_memory {
+  %1 = icmp slt <2 x i32> <i32 -1, i32 -1>, %x
+  ret <2 x i1> %1
+}
+
+; CHECK-LABEL: @ICmpSLT_vector_AllOnes
+; CHECK: icmp slt <2 x i32>
+; CHECK-NOT: call void @__msan_warning
+; CHECK: icmp slt <2 x i32>
+; CHECK-NOT: call void @__msan_warning
+; CHECK: ret <2 x i1>
+
 
 ; Check that we propagate shadow for unsigned relational comparisons with
 ; constants