Skip to content

[msan] Add experimental '-mllvm -msan-embed-faulting-instruction' and MSAN_OPTIONS=print_faulting_instruction #136539

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

thurstond
Copy link
Contributor

This adds an experimental flag, -mllvm -msan-embed-faulting-instruction, which changes the instrumentation to embed the LLVM instruction that resulted in an MSan UUM report. The embedded instruction can be printed using a new experimental MSAN_OPTION, 'print_faulting_instruction'.

Although MSan UUM reports will print out the line and column number (assuming symbolization is available), inlining, macros and LLVM "auto-upgraded" intrinsics can obscure the root cause.

This patch also adds a test case, compiler-rt/test/msan/print_faulting_inst.cpp, which illustrates that printing the faulting instruction can provide more information than line and column number.

Future work could extend the runtime to use the embedded instruction in other ways, such as an ignorelist for particular instructions (e.g., to avoid incorrectly instrumented intrinsics, or to skip over a particular line of LLVM IR).

@llvmbot
Copy link
Member

llvmbot commented Apr 21, 2025

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

@llvm/pr-subscribers-llvm-transforms

Author: Thurston Dang (thurstond)

Changes

This adds an experimental flag, -mllvm -msan-embed-faulting-instruction, which changes the instrumentation to embed the LLVM instruction that resulted in an MSan UUM report. The embedded instruction can be printed using a new experimental MSAN_OPTION, 'print_faulting_instruction'.

Although MSan UUM reports will print out the line and column number (assuming symbolization is available), inlining, macros and LLVM "auto-upgraded" intrinsics can obscure the root cause.

This patch also adds a test case, compiler-rt/test/msan/print_faulting_inst.cpp, which illustrates that printing the faulting instruction can provide more information than line and column number.

Future work could extend the runtime to use the embedded instruction in other ways, such as an ignorelist for particular instructions (e.g., to avoid incorrectly instrumented intrinsics, or to skip over a particular line of LLVM IR).


Patch is 23.69 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/136539.diff

5 Files Affected:

  • (modified) compiler-rt/lib/msan/msan.cpp (+89)
  • (modified) compiler-rt/lib/msan/msan_flags.inc (+4)
  • (modified) compiler-rt/lib/msan/msan_interface_internal.h (+20)
  • (added) compiler-rt/test/msan/print_faulting_inst.cpp (+83)
  • (modified) llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp (+117-29)
diff --git a/compiler-rt/lib/msan/msan.cpp b/compiler-rt/lib/msan/msan.cpp
index a3c0c2e485af3..676b9908bfd43 100644
--- a/compiler-rt/lib/msan/msan.cpp
+++ b/compiler-rt/lib/msan/msan.cpp
@@ -352,10 +352,44 @@ void __sanitizer::BufferedStackTrace::UnwindImpl(
 
 using namespace __msan;
 
+#define PRINT_FAULTING_INSTRUCTION(instname)                            \
+  if (__msan::flags()->print_faulting_instruction) {                    \
+    Printf("Instruction that failed the shadow check: %s\n", instname); \
+    Printf("\n");                                                       \
+  }
+
+#define CANNOT_PRINT_FAULTING_INSTRUCTION                               \
+  if (__msan::flags()->print_faulting_instruction) {                    \
+    Printf(                                                             \
+        "Error: print_faulting_instruction requested but code was not " \
+        "instrumented with -mllvm -embed-faulting-instruction.\n");     \
+    Printf("\n");                                                       \
+  }
+
+#define MSAN_MAYBE_WARNING_INSTNAME(type, size, instname)                    \
+  void __msan_maybe_warning_instname_##size(type s, u32 o, char *instname) { \
+    GET_CALLER_PC_BP;                                                        \
+    if (UNLIKELY(s)) {                                                       \
+      if (instname)                                                          \
+        PRINT_FAULTING_INSTRUCTION(instname);                                \
+      PrintWarningWithOrigin(pc, bp, o);                                     \
+      if (__msan::flags()->halt_on_error) {                                  \
+        Printf("Exiting\n");                                                 \
+        Die();                                                               \
+      }                                                                      \
+    }                                                                        \
+  }
+
+MSAN_MAYBE_WARNING_INSTNAME(u8, 1, instname)
+MSAN_MAYBE_WARNING_INSTNAME(u16, 2, instname)
+MSAN_MAYBE_WARNING_INSTNAME(u32, 4, instname)
+MSAN_MAYBE_WARNING_INSTNAME(u64, 8, instname)
+
 #define MSAN_MAYBE_WARNING(type, size)              \
   void __msan_maybe_warning_##size(type s, u32 o) { \
     GET_CALLER_PC_BP;                               \
     if (UNLIKELY(s)) {                              \
+      CANNOT_PRINT_FAULTING_INSTRUCTION;            \
       PrintWarningWithOrigin(pc, bp, o);            \
       if (__msan::flags()->halt_on_error) {         \
         Printf("Exiting\n");                        \
@@ -387,6 +421,7 @@ MSAN_MAYBE_STORE_ORIGIN(u32, 4)
 MSAN_MAYBE_STORE_ORIGIN(u64, 8)
 
 void __msan_warning() {
+  CANNOT_PRINT_FAULTING_INSTRUCTION;
   GET_CALLER_PC_BP;
   PrintWarningWithOrigin(pc, bp, 0);
   if (__msan::flags()->halt_on_error) {
@@ -398,6 +433,7 @@ void __msan_warning() {
 }
 
 void __msan_warning_noreturn() {
+  CANNOT_PRINT_FAULTING_INSTRUCTION;
   GET_CALLER_PC_BP;
   PrintWarningWithOrigin(pc, bp, 0);
   if (__msan::flags()->print_stats)
@@ -407,6 +443,7 @@ void __msan_warning_noreturn() {
 }
 
 void __msan_warning_with_origin(u32 origin) {
+  CANNOT_PRINT_FAULTING_INSTRUCTION;
   GET_CALLER_PC_BP;
   PrintWarningWithOrigin(pc, bp, origin);
   if (__msan::flags()->halt_on_error) {
@@ -418,6 +455,58 @@ void __msan_warning_with_origin(u32 origin) {
 }
 
 void __msan_warning_with_origin_noreturn(u32 origin) {
+  CANNOT_PRINT_FAULTING_INSTRUCTION;
+  GET_CALLER_PC_BP;
+  PrintWarningWithOrigin(pc, bp, origin);
+  if (__msan::flags()->print_stats)
+    ReportStats();
+  Printf("Exiting\n");
+  Die();
+}
+
+// We duplicate the non _instname function's body because we don't want to
+// pollute the stack traces with an additional function call.
+//
+// We can't use a simple macro wrapper, because the instrumentation pass
+// expects function symbols.
+// We don't add instname as a parameter everywhere (with a check whether the
+// value is null) to avoid polluting the fastpath.
+void __msan_warning_instname(char *instname) {
+  PRINT_FAULTING_INSTRUCTION(instname);
+  GET_CALLER_PC_BP;
+  PrintWarningWithOrigin(pc, bp, 0);
+  if (__msan::flags()->halt_on_error) {
+    if (__msan::flags()->print_stats)
+      ReportStats();
+    Printf("Exiting\n");
+    Die();
+  }
+}
+
+void __msan_warning_noreturn_instname(char *instname) {
+  PRINT_FAULTING_INSTRUCTION(instname);
+  GET_CALLER_PC_BP;
+  PrintWarningWithOrigin(pc, bp, 0);
+  if (__msan::flags()->print_stats)
+    ReportStats();
+  Printf("Exiting\n");
+  Die();
+}
+
+void __msan_warning_with_origin_instname(u32 origin, char *instname) {
+  PRINT_FAULTING_INSTRUCTION(instname);
+  GET_CALLER_PC_BP;
+  PrintWarningWithOrigin(pc, bp, origin);
+  if (__msan::flags()->halt_on_error) {
+    if (__msan::flags()->print_stats)
+      ReportStats();
+    Printf("Exiting\n");
+    Die();
+  }
+}
+
+void __msan_warning_with_origin_noreturn_instname(u32 origin, char *instname) {
+  PRINT_FAULTING_INSTRUCTION(instname);
   GET_CALLER_PC_BP;
   PrintWarningWithOrigin(pc, bp, origin);
   if (__msan::flags()->print_stats)
diff --git a/compiler-rt/lib/msan/msan_flags.inc b/compiler-rt/lib/msan/msan_flags.inc
index 16db26bd42ed9..30a17fb3db9a8 100644
--- a/compiler-rt/lib/msan/msan_flags.inc
+++ b/compiler-rt/lib/msan/msan_flags.inc
@@ -25,6 +25,10 @@ MSAN_FLAG(bool, poison_stack_with_zeroes, false, "")
 MSAN_FLAG(bool, poison_in_malloc, true, "")
 MSAN_FLAG(bool, poison_in_free, true, "")
 MSAN_FLAG(bool, poison_in_dtor, true, "")
+MSAN_FLAG(bool, print_faulting_instruction, false,
+          "When reporting a UUM, print the name of the faulting instruction. "
+          "Note: requires code to be instrumented with -mllvm "
+          "-msan-embed-faulting-instruction.")
 MSAN_FLAG(bool, report_umrs, true, "")
 MSAN_FLAG(bool, wrap_signals, true, "")
 MSAN_FLAG(bool, print_stats, false, "")
diff --git a/compiler-rt/lib/msan/msan_interface_internal.h b/compiler-rt/lib/msan/msan_interface_internal.h
index c2eead13c20cf..5607d08e66d86 100644
--- a/compiler-rt/lib/msan/msan_interface_internal.h
+++ b/compiler-rt/lib/msan/msan_interface_internal.h
@@ -30,12 +30,18 @@ void __msan_init();
 SANITIZER_INTERFACE_ATTRIBUTE
 void __msan_warning();
 
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_warning_instname(char *instname);
+
 // Print a warning and die.
 // Instrumentation inserts calls to this function when building in "fast" mode
 // (i.e. -mllvm -msan-keep-going)
 SANITIZER_INTERFACE_ATTRIBUTE __attribute__((noreturn))
 void __msan_warning_noreturn();
 
+SANITIZER_INTERFACE_ATTRIBUTE __attribute__((noreturn)) void
+__msan_warning_noreturn_instname(char *instname);
+
 using __sanitizer::uptr;
 using __sanitizer::sptr;
 using __sanitizer::uu64;
@@ -49,8 +55,13 @@ using __sanitizer::u8;
 // Versions of the above which take Origin as a parameter
 SANITIZER_INTERFACE_ATTRIBUTE
 void __msan_warning_with_origin(u32 origin);
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_warning_with_origin_instname(u32 origin, char *instname);
+
 SANITIZER_INTERFACE_ATTRIBUTE __attribute__((noreturn)) void
 __msan_warning_with_origin_noreturn(u32 origin);
+SANITIZER_INTERFACE_ATTRIBUTE __attribute__((noreturn)) void
+__msan_warning_with_origin_noreturn_instname(u32 origin, char *instname);
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __msan_maybe_warning_1(u8 s, u32 o);
@@ -61,6 +72,15 @@ void __msan_maybe_warning_4(u32 s, u32 o);
 SANITIZER_INTERFACE_ATTRIBUTE
 void __msan_maybe_warning_8(u64 s, u32 o);
 
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_maybe_warning_1_instname(u8 s, u32 o, char *instname);
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_maybe_warning_2_instname(u16 s, u32 o, char *instname);
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_maybe_warning_4_instname(u32 s, u32 o, char *instname);
+SANITIZER_INTERFACE_ATTRIBUTE
+void __msan_maybe_warning_8_instname(u64 s, u32 o, char *instname);
+
 SANITIZER_INTERFACE_ATTRIBUTE
 void __msan_maybe_store_origin_1(u8 s, void *p, u32 o);
 SANITIZER_INTERFACE_ATTRIBUTE
diff --git a/compiler-rt/test/msan/print_faulting_inst.cpp b/compiler-rt/test/msan/print_faulting_inst.cpp
new file mode 100644
index 0000000000000..1cbea9eb035e0
--- /dev/null
+++ b/compiler-rt/test/msan/print_faulting_inst.cpp
@@ -0,0 +1,83 @@
+// Try parameter '0' (program runs cleanly)
+// -------------------------------------------------------
+// RUN: env MSAN_OPTIONS=print_faulting_instruction=true %clangxx_msan -g %s -o %t && env MSAN_OPTIONS=print_faulting_instruction=true %run %t 0
+
+// Try parameter '1'
+// -------------------------------------------------------
+// RUN: %clangxx_msan -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 1 >%t.out 2>&1
+// RUN: FileCheck --check-prefix STORE-CHECK %s < %t.out
+
+// RUN: %clangxx_msan -mllvm -msan-embed-faulting-instruction=1 -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 1 >%t.out 2>&1
+// RUN: FileCheck --check-prefixes VERBOSE-STORE-CHECK,STORE-CHECK %s < %t.out
+
+// RUN: %clangxx_msan -mllvm -msan-embed-faulting-instruction=2 -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 1 >%t.out 2>&1
+// RUN: FileCheck --check-prefixes VERY-VERBOSE-STORE-CHECK,STORE-CHECK %s < %t.out
+
+// Try parameter '2', with -fsanitize-memory-param-retval
+// -------------------------------------------------------
+// RUN: %clangxx_msan -fsanitize-memory-param-retval -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 2 >%t.out 2>&1
+// RUN: FileCheck --check-prefix PARAM-CHECK %s < %t.out
+
+// RUN: %clangxx_msan -fsanitize-memory-param-retval -mllvm -msan-embed-faulting-instruction=1 -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 2 >%t.out 2>&1
+// RUN: FileCheck --check-prefixes VERBOSE-PARAM-CHECK,PARAM-CHECK %s < %t.out
+
+// RUN: %clangxx_msan -fsanitize-memory-param-retval -mllvm -msan-embed-faulting-instruction=2 -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 2 >%t.out 2>&1
+// RUN: FileCheck --check-prefixes VERY-VERBOSE-PARAM-CHECK,PARAM-CHECK %s < %t.out
+
+// Try parameter '2', with -fno-sanitize-memory-param-retval
+// -------------------------------------------------------
+// RUN: %clangxx_msan -fno-sanitize-memory-param-retval -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 2 >%t.out 2>&1
+// RUN: FileCheck --check-prefix NO-PARAM-CHECK %s < %t.out
+
+// RUN: %clangxx_msan -fno-sanitize-memory-param-retval -mllvm -msan-embed-faulting-instruction=1 -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 2 >%t.out 2>&1
+// RUN: FileCheck --check-prefixes VERBOSE-NO-PARAM-CHECK,NO-PARAM-CHECK %s < %t.out
+
+// RUN: %clangxx_msan -fno-sanitize-memory-param-retval -mllvm -msan-embed-faulting-instruction=2 -g %s -o %t && not env MSAN_OPTIONS=print_faulting_instruction=true %run %t 2 >%t.out 2>&1
+// RUN: FileCheck --check-prefixes VERY-VERBOSE-NO-PARAM-CHECK,NO-PARAM-CHECK %s < %t.out
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#define THRICE(o,t) twice(o,t)
+
+__attribute__((noinline)) extern "C" int twice(int o, int t) {
+  return o + t < 3;
+}
+
+int main(int argc, char *argv[]) {
+  int buf[100];
+  buf[0] = 42;
+  buf[1] = 43;
+
+  if (argc != 2) {
+    printf("Usage: %s index\n", argv[0]);
+    return 1;
+  }
+
+  int index = atoi(argv[1]);
+  int val = buf[index];
+
+  printf("index %d, abs(val) %d, THRICE(val,5) %d\n", index, abs(val), THRICE(val,5));
+  // VERY-VERBOSE-PARAM-CHECK: Instruction that failed the shadow check: %{{.*}} = call noundef i32 @twice(i32 noundef %{{.*}}, i32 noundef 5)
+  // VERBOSE-PARAM-CHECK: Instruction that failed the shadow check: call twice
+  // PARAM-CHECK: WARNING: MemorySanitizer: use-of-uninitialized-value
+  // PARAM-CHECK: {{#0 0x.* in main .*print_faulting_inst.cpp:}}[[@LINE-4]]
+
+  if (val)
+    // VERY-VERBOSE-NO-PARAM-CHECK: Instruction that failed the shadow check:  br i1 %{{.*}}, label %{{.*}}, label %{{.*}}
+    // VERBOSE-NO-PARAM-CHECK: Instruction that failed the shadow check: br
+    // NO-PARAM-CHECK: WARNING: MemorySanitizer: use-of-uninitialized-value
+    // NO-PARAM-CHECK: {{#0 0x.* in main .*print_faulting_inst.cpp:}}[[@LINE-4]]
+    printf("Variable is non-zero\n");
+  else
+    printf("Variable is zero\n");
+
+  int nextval = buf[index + 1];
+  buf[nextval + abs(index)] = twice(index,6);
+  // VERY-VERBOSE-STORE-CHECK: Instruction that failed the shadow check: store i32 %{{.*}}, ptr %{{.*}}
+  // VERBOSE-STORE-CHECK: Instruction that failed the shadow check: store
+  // STORE-CHECK: WARNING: MemorySanitizer: use-of-uninitialized-value
+  // STORE-CHECK: {{#0 0x.* in main .*print_faulting_inst.cpp:}}[[@LINE-4]]
+
+  return 0;
+}
diff --git a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
index 506a880e3f67d..76dc539ee6af2 100644
--- a/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
@@ -274,6 +274,14 @@ static cl::opt<bool>
                  cl::desc("propagate shadow through ICmpEQ and ICmpNE"),
                  cl::Hidden, cl::init(true));
 
+static cl::opt<uint> ClEmbedFaultingInst(
+    "msan-embed-faulting-instruction",
+    cl::desc("If set to 1, embed the name of the LLVM IR instruction that "
+             "failed the shadow check."
+             "If set to 2, embed the full LLVM IR instruction. "
+             "The runtime can print the embedded instruction."),
+    cl::Hidden, cl::init(0));
+
 static cl::opt<bool>
     ClHandleICmpExact("msan-handle-icmp-exact",
                       cl::desc("exact handling of relational integer ICmp"),
@@ -816,9 +824,14 @@ void MemorySanitizer::createKernelApi(Module &M, const TargetLibraryInfo &TLI) {
   VAArgOriginTLS = nullptr;
   VAArgOverflowSizeTLS = nullptr;
 
-  WarningFn = M.getOrInsertFunction("__msan_warning",
-                                    TLI.getAttrList(C, {0}, /*Signed=*/false),
-                                    IRB.getVoidTy(), IRB.getInt32Ty());
+  if (ClEmbedFaultingInst)
+    WarningFn = M.getOrInsertFunction(
+        "__msan_warning_instname", TLI.getAttrList(C, {0}, /*Signed=*/false),
+        IRB.getVoidTy(), IRB.getInt32Ty(), IRB.getPtrTy());
+  else
+    WarningFn = M.getOrInsertFunction("__msan_warning",
+                                      TLI.getAttrList(C, {0}, /*Signed=*/false),
+                                      IRB.getVoidTy(), IRB.getInt32Ty());
 
   // Requests the per-task context state (kmsan_context_state*) from the
   // runtime library.
@@ -870,19 +883,37 @@ void MemorySanitizer::createUserspaceApi(Module &M,
                                          const TargetLibraryInfo &TLI) {
   IRBuilder<> IRB(*C);
 
+  Type *Int8Ptr = PointerType::getUnqual(*C);
+
   // Create the callback.
   // FIXME: this function should have "Cold" calling conv,
   // which is not yet implemented.
   if (TrackOrigins) {
-    StringRef WarningFnName = Recover ? "__msan_warning_with_origin"
-                                      : "__msan_warning_with_origin_noreturn";
-    WarningFn = M.getOrInsertFunction(WarningFnName,
-                                      TLI.getAttrList(C, {0}, /*Signed=*/false),
-                                      IRB.getVoidTy(), IRB.getInt32Ty());
+    if (ClEmbedFaultingInst) {
+      StringRef WarningFnName =
+          Recover ? "__msan_warning_with_origin_instname"
+                  : "__msan_warning_with_origin_noreturn_instname";
+      WarningFn = M.getOrInsertFunction(
+          WarningFnName, TLI.getAttrList(C, {0}, /*Signed=*/false),
+          IRB.getVoidTy(), IRB.getInt32Ty(), Int8Ptr);
+    } else {
+      StringRef WarningFnName = Recover ? "__msan_warning_with_origin"
+                                        : "__msan_warning_with_origin_noreturn";
+      WarningFn = M.getOrInsertFunction(
+          WarningFnName, TLI.getAttrList(C, {0}, /*Signed=*/false),
+          IRB.getVoidTy(), IRB.getInt32Ty());
+    }
   } else {
-    StringRef WarningFnName =
-        Recover ? "__msan_warning" : "__msan_warning_noreturn";
-    WarningFn = M.getOrInsertFunction(WarningFnName, IRB.getVoidTy());
+    if (ClEmbedFaultingInst) {
+      StringRef WarningFnName = Recover ? "__msan_warning_instname"
+                                        : "__msan_warning_noreturn_instname";
+      WarningFn =
+          M.getOrInsertFunction(WarningFnName, IRB.getVoidTy(), Int8Ptr);
+    } else {
+      StringRef WarningFnName =
+          Recover ? "__msan_warning" : "__msan_warning_noreturn";
+      WarningFn = M.getOrInsertFunction(WarningFnName, IRB.getVoidTy());
+    }
   }
 
   // Create the global TLS variables.
@@ -914,10 +945,19 @@ void MemorySanitizer::createUserspaceApi(Module &M,
   for (size_t AccessSizeIndex = 0; AccessSizeIndex < kNumberOfAccessSizes;
        AccessSizeIndex++) {
     unsigned AccessSize = 1 << AccessSizeIndex;
-    std::string FunctionName = "__msan_maybe_warning_" + itostr(AccessSize);
-    MaybeWarningFn[AccessSizeIndex] = M.getOrInsertFunction(
-        FunctionName, TLI.getAttrList(C, {0, 1}, /*Signed=*/false),
-        IRB.getVoidTy(), IRB.getIntNTy(AccessSize * 8), IRB.getInt32Ty());
+    std::string FunctionName;
+    if (ClEmbedFaultingInst) {
+      FunctionName = "__msan_maybe_warning_instname_" + itostr(AccessSize);
+      MaybeWarningFn[AccessSizeIndex] = M.getOrInsertFunction(
+          FunctionName, TLI.getAttrList(C, {0, 1}, /*Signed=*/false),
+          IRB.getVoidTy(), IRB.getIntNTy(AccessSize * 8), IRB.getInt32Ty(),
+          IRB.getPtrTy());
+    } else {
+      FunctionName = "__msan_maybe_warning_" + itostr(AccessSize);
+      MaybeWarningFn[AccessSizeIndex] = M.getOrInsertFunction(
+          FunctionName, TLI.getAttrList(C, {0, 1}, /*Signed=*/false),
+          IRB.getVoidTy(), IRB.getIntNTy(AccessSize * 8), IRB.getInt32Ty());
+    }
 
     FunctionName = "__msan_maybe_store_origin_" + itostr(AccessSize);
     MaybeStoreOriginFn[AccessSizeIndex] = M.getOrInsertFunction(
@@ -1387,7 +1427,7 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
   }
 
   /// Helper function to insert a warning at IRB's current insert point.
-  void insertWarningFn(IRBuilder<> &IRB, Value *Origin) {
+  void insertWarningFn(IRBuilder<> &IRB, Value *Origin, Value *InstName) {
     if (!Origin)
       Origin = (Value *)IRB.getInt32(0);
     assert(Origin->getType()->isIntegerTy());
@@ -1410,17 +1450,24 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
       }
     }
 
-    if (MS.CompileKernel || MS.TrackOrigins)
-      IRB.CreateCall(MS.WarningFn, Origin)->setCannotMerge();
-    else
-      IRB.CreateCall(MS.WarningFn)->setCannotMerge();
+    if (ClEmbedFaultingInst) {
+      if (MS.CompileKernel || MS.TrackOrigins)
+        IRB.CreateCall(MS.WarningFn, {Origin, InstName})->setCannotMerge();
+      else
+        IRB.CreateCall(MS.WarningFn, {InstName})->setCannotMerge();
+    } else {
+      if (MS.CompileKernel || MS.TrackOrigins)
+        IRB.CreateCall(MS.WarningFn, Origin)->setCannotMerge();
+      else
+        IRB.CreateCall(MS.WarningFn)->setCannotMerge();
+    }
     // FIXME: Insert UnreachableInst if !MS.Recover?
     // This may invalidate some of the following checks and needs to be done
     // at the very end.
   }
 
   void materializeOneCheck(IRBuilder<> &IRB, Value *ConvertedShadow,
-                           Value *Origin) {
+                           Value *Origin, Value *InstName) {
     const DataLayout &DL = F.getDataLayout();
     TypeSize TypeSizeInBits = DL.getTypeSizeInBits(ConvertedShadow->getType());
     unsigned SizeIndex = TypeSizeToSizeIndex(TypeSizeInBits);
@@ -1431,9 +1478,18 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
       ConvertedShadow = convertShadowToScalar(ConvertedShadow, IRB);
       Value *ConvertedShadow2 =
           IRB.CreateZExt(ConvertedShadow, IRB.getIntNTy(8 * (1 << SizeIndex)));
-      CallBase *CB = IRB.CreateCall(
-          Fn, {ConvertedShadow2,
-               MS.TrackOrigins && Origin ? Origin : (Value *)IRB.getInt32(0)});
+      CallBase *CB;
+      if (ClEmbedFaultingInst)
+        CB = IRB.CreateCall(
+            Fn, {ConvertedShadow2,
+                 MS.TrackOrigins && Origin ? Origin : (Value *)IRB.getInt32(0),
+    ...
[truncated]

Copy link

github-actions bot commented Apr 21, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@fmayer fmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix failing CI test

@vitalybuka
Copy link
Collaborator

Why DEBUG_COUNTER DebugInsertCheck and DebugInstrumentInstruction are not enough?

@thurstond
Copy link
Contributor Author

Why DEBUG_COUNTER DebugInsertCheck and DebugInstrumentInstruction are not enough?

  • Compared to MSAN_OPTIONS=print_faulting_instruction=...: DebugInsertCheck and DebugInstrumentInstruction requires trial and error (bisection?) to find the debug counter value that would isolate the failing instruction. Doing the bisection - which requires recompiling and then rerunning the binary - is a lot of manual effort and a waste of compute resources.
  • Compared to the proposed but not implemented MSAN_OPTIONS=ignore_faulting_instruction=...: DebugInsertCheck and DebugInstrumentInstruction are configured at compile-time, while ignore_faulting_instruction is compile one-time with instrumentation and then configure at runtime which instructions to skip.

@vitalybuka
Copy link
Collaborator

Why DEBUG_COUNTER DebugInsertCheck and DebugInstrumentInstruction are not enough?

  • Compared to MSAN_OPTIONS=print_faulting_instruction=...: DebugInsertCheck and DebugInstrumentInstruction requires trial and error (bisection?) to find the debug counter value that would isolate the failing instruction. Doing the bisection - which requires recompiling and then rerunning the binary - is a lot of manual effort and a waste of compute resources.

But it's generic and well known. And can debug wider range of problems.

  • Compared to the proposed but not implemented MSAN_OPTIONS=ignore_faulting_instruction=...: DebugInsertCheck and DebugInstrumentInstruction are configured at compile-time, while ignore_faulting_instruction is compile one-time with instrumentation and then configure at runtime which instructions to skip.

How big is binary size overhead?

@thurstond
Copy link
Contributor Author

thurstond commented Apr 23, 2025

  • Compared to the proposed but not implemented MSAN_OPTIONS=ignore_faulting_instruction=...: DebugInsertCheck and DebugInstrumentInstruction are configured at compile-time, while ignore_faulting_instruction is compile one-time with instrumentation and then configure at runtime which instructions to skip.

How big is binary size overhead?

With -embed-faulting-instruction=name, the instrumented binary is around 11% of the original binary (or 5% of the MSan-instrumented binary) larger than regular MSan. This is negligible compared to the overall overhead of MSan (114% increase), hence it can be deployed in most cases where MSan is used (nonetheless, the default is off).

File size for span_server built with '-c opt':

  • No MSan: 762,042,216 bytes
  • Regular MSan (-embed-faulting-instruction=none): 1,632,739,840 bytes (114% overhead)
  • MSan with -embed-faulting-instruction=name: 1,719,331,688 (125% overhead)
  • MSan with -embed-faulting-instruction=full: [build timed out]

@thurstond
Copy link
Contributor Author

Please fix failing CI test

[insert "It's done" meme]

@fmayer
Copy link
Contributor

fmayer commented Apr 23, 2025

LGTM from my side, leaving it to Vitaly and you to figure out.

This adds an experimental flag, -mllvm -msan-print-faulting-instruction,
which will print the LLVM instruction that resulted in an MSan UUM
report.

Although MSan UUM reports will print out the line and column number
(assuming symbolization is available), inlining, macros and LLVM
"auto-upgraded" intrinsics can obscure the root cause.

This patch adds a test case,
compiler-rt/test/msan/print_faulting_inst.cpp, which illustrates that
-msan-print-faulting-instruction can provide more information than line
and column number.
Also add "[EXPERIMENTAL]" disclaimer
@thurstond thurstond force-pushed the msan_print_faulting_inst branch from fe79621 to 9907618 Compare April 23, 2025 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants