[PATCH] D115236: [msan] Implement -msan-no-sanitize-whole-file.

[Marco Elver via Phabricator via llvm-commits]

melver added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:310
 
+static cl::opt<bool> ClNoSanitizeWholeFile("msan-no-sanitize-whole-file",
+                                           cl::desc("Apply __no_sanitize(("
----------------
When does something compiled with LTO apply sanitizer passes?

Perhaps, to make it more general, I'd call it "msan-no-sanitize-all" or "msan-no-sanitize-all-functions", whichever you think is clearer.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:311
+static cl::opt<bool> ClNoSanitizeWholeFile("msan-no-sanitize-whole-file",
+                                           cl::desc("Apply __no_sanitize(("
+                                                    "memory"
----------------
`__no_sanitize` is the kernel spelling, should this be just `no_sanitize`?

Also, regarding formatting, I think I'd solve it by rewording the description slightly.

I think in this case, since this is about MSan, simply saying "Apply no_sanitize to the whole file" is probably unambiguous enough, and will make for nicer formatting.


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_no_sanitize_whole_file.ll:4
+; RUN: opt < %s -msan-check-access-address=0 -S -passes='module(msan-module),function(msan)' 2>&1 | FileCheck -allow-deprecated-dag-overlap -check-prefixes=CHECK,INSTR %s
+; RUN: opt < %s -msan-check-access-address=0 -S -passes='module(msan-module),function(msan)' -msan-no-sanitize-whole-file=1 2>&1 | FileCheck -allow-deprecated-dag-overlap -check-prefixes=CHECK,NOINSTR %s
+
----------------
This is not fully "noinstr", right? I.e. "noinstr" is slightly misleading since we still want instrumentation, just no checks.


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_no_sanitize_whole_file.ll:27
+; NOINSTR-NOT: @__msan_warning
+; NOINSTR: store i32 0, {{.*}} @__msan_retval_tls
+; CHECK: ret i32
----------------
This can be a "CHECK" because both are supposed to have this, right?


================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_no_sanitize_whole_file.ll:48
+; NOINSTR-NOT: @__msan_warning
+; NOINSTR: store i32 0, {{.*}} @__msan_retval_tls
+; CHECK: ret i32
----------------
Aren't INSTR and NOINSTR supposed to be identical in the NoSanitizeFn case? I.e. you could just use CHECK throughout?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115236/new/

https://reviews.llvm.org/D115236