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

[Alexander Potapenko via Phabricator via llvm-commits]

glider 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(("
----------------
melver wrote:
> 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.
IIUC, currently LTO works as follows:
 - kernel is built as usual, but with bitcode files instead of .o files;
 - these bitcode files are merged together, and additional optimization passes are ran on the resulting file.

Nobody ever tried this with KMSAN, but I would still expect it to work on a per-file basis.

But I agree not mentioning "file" is better. I'd go for "msan-no-sanitize-all" (we don't sanitize anything except functions anyway).


================
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"
----------------
melver wrote:
> `__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.
I actually failed to write "__attribute__((no_sanitize("memory")))" :)

But that's too verbose and might raise questions about the compatibility between "memory" and "kernel-memory", so perhaps just "no_sanitize" should be fine.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:312
+                                           cl::desc("Apply __no_sanitize(("
+                                                    "memory"
+                                                    ")) to the whole file"),
----------------
vitalybuka wrote:
> glider wrote:
> > This edit was suggested by clang-format, I am not particularly fond of it.
> I guess we need this because simple -fno-sanitize=memory will also remove shadow propagation?
> What about SanitizerSpecialCaseList?
Yes, that's right, -fno-sanitize=memory doesn't work.

-fsanitize-ignorelist could be an option, but we want Makefiles to be the source of truth for the ignore lists.
Currently we disable KMSAN completely using the following syntax:

```
# One file in this dir: vclock_gettime.c
KMSAN_SANITIZE_vclock_gettime.o := n
# Either head32.c or head64.c, depending on $(BITS) exported from the parent Makefile
KMSAN_SANITIZE_head$(BITS).o := n
# All files in this directory and subdirectories
KMSAN_SANITIZE := n
```

The proposed flag will enable us to have the same syntax to disable shadow checking, but with -fsanitize-ignorelist we would need to generate the list on the fly, which might be tricky.


================
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
+
----------------
melver wrote:
> This is not fully "noinstr", right? I.e. "noinstr" is slightly misleading since we still want instrumentation, just no checks.
Changed to NOSANITIZE.


================
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
----------------
melver wrote:
> This can be a "CHECK" because both are supposed to have this, right?
No. This instruction unpoisons the return value, which only happens in the "non-instrumented" case.


================
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
----------------
melver wrote:
> Aren't INSTR and NOINSTR supposed to be identical in the NoSanitizeFn case? I.e. you could just use CHECK throughout?
Right, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115236