-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[msan] Enable msan-handle-asm-conservative for userspace by default #79251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[msan] Enable msan-handle-asm-conservative for userspace by default #79251
Conversation
Created using spr 1.3.4
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Fangrui Song (MaskRay) Changesmsan-handle-asm-conservative is enabled by KMSAN by default. Full diff: https://github.com/llvm/llvm-project/pull/79251.diff 2 Files Affected:
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 \
|
@llvm/pr-subscribers-llvm-transforms Author: Fangrui Song (MaskRay) Changesmsan-handle-asm-conservative is enabled by KMSAN by default. Full diff: https://github.com/llvm/llvm-project/pull/79251.diff 2 Files Affected:
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 \
|
LGTM, we shouldn't enable something with potential false-positives by default. |
The false negative only occurs when the inline asm doesn't actually initialize the indirect output ( 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. |
msan-handle-asm-conservative is enabled by KMSAN by default.
Enable the userspace by default as well after #77393.