[msan] Re-fix disjoint OR instrumentation from #145990

[thurstond]

When disjoint OR was specified and a bit position contained a 1 in both operands, #145990 would set the corresponding shadow bit to uninitialized. However, the output of the operation is (LLVM) 'poison' for the entire result, hence the entire shadow ought to be uninitialized. This patch corrects the issue.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Thurston Dang (thurstond)

Changes

When disjoint OR was specified and a bit position contained a 1 in both operands, #145990 would set the corresponding shadow bit to uninitialized. However, the output of the operation is (LLVM) 'poison' for the entire result, hence the entire shadow ought to be uninitialized. This patch corrects the issue.

---

Full diff: https://github.com/llvm/llvm-project/pull/148760.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+6-4)
• (modified) llvm/test/Instrumentation/MemorySanitizer/or.ll (+8-6)

diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 5f5200b2c9e62..fecec845f8d58 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -2509,9 +2509,9 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
     //
     //    S = (S1 & S2) | (~V1 & S2) | (S1 & ~V2)
     //
-    //  Addendum if the "Or" is "disjoint":
-    //    1|1 => p;
-    //    S = S | (V1 & V2)
+    //  If the "disjoint OR" property is violated, the result is poison, and
+    //  hence the entire shadow is uninitialized:
+    //    S = S | SignExt(V1 & V2 != 0)
     Value *S1 = getShadow(&I, 0);
     Value *S2 = getShadow(&I, 1);
     Value *V1 = I.getOperand(0);
@@ -2532,7 +2532,9 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
 
     if (ClPreciseDisjointOr && cast<PossiblyDisjointInst>(&I)->isDisjoint()) {
       Value *V1V2 = IRB.CreateAnd(V1, V2);
-      S = IRB.CreateOr(S, V1V2, "_ms_disjoint");
+      Value *DisjointOrShadow = IRB.CreateSExt(
+          IRB.CreateICmpNE(V1V2, getCleanShadow(V1V2)), V1V2->getType());
+      S = IRB.CreateOr(S, DisjointOrShadow, "_ms_disjoint");
     }
 
     setShadow(&I, S);
diff --git a/llvm/test/Instrumentation/MemorySanitizer/or.ll b/llvm/test/Instrumentation/MemorySanitizer/or.ll
index 27a1800aa495b..650c70b6a4d94 100644
--- a/llvm/test/Instrumentation/MemorySanitizer/or.ll
+++ b/llvm/test/Instrumentation/MemorySanitizer/or.ll
@@ -1,4 +1,4 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; NOTE: Assertions have mostly been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
 ; RUN: opt < %s -S -passes=msan -msan-precise-disjoint-or=false 2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-IMPRECISE
 ; RUN: opt < %s -S -passes=msan -msan-precise-disjoint-or=true  2>&1 | FileCheck %s --check-prefixes=CHECK,CHECK-PRECISE
 ;
@@ -40,15 +40,17 @@ define i8 @test_disjoint_or(i8 %a, i8 %b) sanitize_memory {
 ; CHECK-NEXT:    [[TMP6:%.*]] = and i8 [[TMP3]], [[TMP2]]
 ; CHECK-NEXT:    [[TMP7:%.*]] = and i8 [[TMP1]], [[TMP4]]
 ; CHECK-NEXT:    [[TMP8:%.*]] = or i8 [[TMP5]], [[TMP6]]
-; CHECK-NEXT:    [[TMP11:%.*]] = or i8 [[TMP8]], [[TMP7]]
+; CHECK-NEXT:    [[TMP9:%.*]] = or i8 [[TMP8]], [[TMP7]]
 ;
 ; CHECK-IMPRECISE:      [[C:%.*]] = or disjoint i8 [[A]], [[B]]
-; CHECK-IMPRECISE-NEXT: store i8 [[TMP11]], ptr @__msan_retval_tls, align 8
+; CHECK-IMPRECISE-NEXT: store i8 [[TMP9]], ptr @__msan_retval_tls, align 8
 ;
-; CHECK-PRECISE:         [[TMP10:%.*]] = and i8 [[A]], [[B]]
-; CHECK-PRECISE-NEXT:    [[TMP12:%.*]] = or i8 [[TMP11]], [[TMP10]]
+; CHECK-PRECISE-NEXT:    [[TMP10:%.*]] = and i8 [[A]], [[B]]
+; CHECK-PRECISE-NEXT:    [[TMP11:%.*]] = icmp ne i8 [[TMP10]], 0
+; CHECK-PRECISE-NEXT:    [[TMP12:%.*]] = sext i1 [[TMP11]] to i8
+; CHECK-PRECISE-NEXT:    [[_MS_DISJOINT:%.*]] = or i8 [[TMP9]], [[TMP12]]
 ; CHECK-PRECISE-NEXT:    [[C:%.*]] = or disjoint i8 [[A]], [[B]]
-; CHECK-PRECISE-NEXT:    store i8 [[TMP12]], ptr @__msan_retval_tls, align 8
+; CHECK-PRECISE-NEXT:    store i8 [[_MS_DISJOINT]], ptr @__msan_retval_tls, align 8
 ;
 ; CHECK-NEXT:    ret i8 [[C]]
 ;

[fmayer]

which part is not autogenerated?

[thurstond]

The auto-generator will duplicate lines between CHECK-PRECISE and CHECK-IMPRECISE, instead of putting common lines into CHECK.

[fmayer]

Let it do that maybe? Then whoever wants to autogenerate this in the future isn't confused by the stray diff

[thurstond]

Done: a8bfd6b

We need to upgrade AutoGen to AGI.