[msan] Enable msan-handle-asm-conservative for userspace by default

[MaskRay]

msan-handle-asm-conservative is enabled by KMSAN by default.
Enable the userspace by default as well after #77393.

[llvmbot]

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

Author: Fangrui Song (MaskRay)

Changes

msan-handle-asm-conservative is enabled by KMSAN by default.
Enable the userspace by default as well after #77393.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+1-8)
• (modified) llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll (+1-1)

diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 15bca538860d96..2b697557d8a92c 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -284,9 +284,6 @@ static cl::opt<bool> ClHandleLifetimeIntrinsics(
 // passed into an assembly call. Note that this may cause false positives.
 // Because it's impossible to figure out the array sizes, we can only unpoison
 // the first sizeof(type) bytes for each type* pointer.
-// The instrumentation is only enabled in KMSAN builds, and only if
-// -msan-handle-asm-conservative is on. This is done because we may want to
-// quickly disable assembly instrumentation when it breaks.
 static cl::opt<bool> ClHandleAsmConservative(
     "msan-handle-asm-conservative",
     cl::desc("conservative handling of inline assembly"), cl::Hidden,
@@ -4103,11 +4100,7 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
       // do the usual thing: check argument shadow and mark all outputs as
       // clean. Note that any side effects of the inline asm that are not
       // immediately visible in its constraints are not handled.
-      // For now, handle inline asm by default for KMSAN.
-      bool HandleAsm = ClHandleAsmConservative.getNumOccurrences()
-                           ? ClHandleAsmConservative
-                           : MS.CompileKernel;
-      if (HandleAsm)
+      if (ClHandleAsmConservative)
         visitAsmInstruction(CB);
       else
         visitInstruction(CB);
diff --git a/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll b/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
index 9a501ee6954c9c..894f76b9b8d32a 100644
--- a/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
+++ b/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
@@ -1,7 +1,7 @@
 ; Test for handling of asm constraints in MSan instrumentation.
 ; RUN: opt < %s -msan-check-access-address=0 -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | \
 ; RUN:   FileCheck %s
-; RUN: opt < %s -msan-check-access-address=0 -msan-handle-asm-conservative=1 -S -passes=msan 2>&1 | \
+; RUN: opt < %s -msan-check-access-address=0 -S -passes=msan 2>&1 | \
 ; RUN:   FileCheck --check-prefixes=CHECK,USER-CONS %s
 ; RUN: opt < %s -msan-kernel=1 -msan-check-access-address=0                    \
 ; RUN:   -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | FileCheck      \

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Fangrui Song (MaskRay)

Changes

msan-handle-asm-conservative is enabled by KMSAN by default.
Enable the userspace by default as well after #77393.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+1-8)
• (modified) llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll (+1-1)

diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 15bca538860d96d..2b697557d8a92cf 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -284,9 +284,6 @@ static cl::opt<bool> ClHandleLifetimeIntrinsics(
 // passed into an assembly call. Note that this may cause false positives.
 // Because it's impossible to figure out the array sizes, we can only unpoison
 // the first sizeof(type) bytes for each type* pointer.
-// The instrumentation is only enabled in KMSAN builds, and only if
-// -msan-handle-asm-conservative is on. This is done because we may want to
-// quickly disable assembly instrumentation when it breaks.
 static cl::opt<bool> ClHandleAsmConservative(
     "msan-handle-asm-conservative",
     cl::desc("conservative handling of inline assembly"), cl::Hidden,
@@ -4103,11 +4100,7 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
       // do the usual thing: check argument shadow and mark all outputs as
       // clean. Note that any side effects of the inline asm that are not
       // immediately visible in its constraints are not handled.
-      // For now, handle inline asm by default for KMSAN.
-      bool HandleAsm = ClHandleAsmConservative.getNumOccurrences()
-                           ? ClHandleAsmConservative
-                           : MS.CompileKernel;
-      if (HandleAsm)
+      if (ClHandleAsmConservative)
         visitAsmInstruction(CB);
       else
         visitInstruction(CB);
diff --git a/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll b/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
index 9a501ee6954c9c6..894f76b9b8d32aa 100644
--- a/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
+++ b/llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
@@ -1,7 +1,7 @@
 ; Test for handling of asm constraints in MSan instrumentation.
 ; RUN: opt < %s -msan-check-access-address=0 -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | \
 ; RUN:   FileCheck %s
-; RUN: opt < %s -msan-check-access-address=0 -msan-handle-asm-conservative=1 -S -passes=msan 2>&1 | \
+; RUN: opt < %s -msan-check-access-address=0 -S -passes=msan 2>&1 | \
 ; RUN:   FileCheck --check-prefixes=CHECK,USER-CONS %s
 ; RUN: opt < %s -msan-kernel=1 -msan-check-access-address=0                    \
 ; RUN:   -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | FileCheck      \

[fmayer]

LGTM, we shouldn't enable something with potential false-positives by default.

[MaskRay]

LGTM, we shouldn't enable something with potential false-negatives by default.

The false negative only occurs when the inline asm doesn't actually initialize the indirect output (=m). I think it is an obvious user fault :)

The opposite (false positives), which we current have for userspace msan, is worse:)

[fmayer]

LGTM, we shouldn't enable something with potential false-negatives by default.

The false negative only occurs when the inline asm doesn't actually initialize the indirect output (=m). I think it is an obvious user fault :)

The opposite (false positives), which we current have for userspace msan, is worse:)

Sorry, I did mean "false positive". I am agreeing with this change.