[msan] Implement -msan-disable-checks.

[glider]

Summary

[msan] Implement -msan-disable-checks.

To ease the deployment of KMSAN, we need a way to apply
attribute((no_sanitize("kernel-memory"))) to the whole source file.

Passing -msan-disable-checks=1 to the compiler will make it
treat every function in the file as if it was lacking the
sanitize_memory attribute.

[glider]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

312

This edit was suggested by clang-format, I am not particularly fond of it.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

312

I guess we need this because simple -fno-sanitize=memory will also remove shadow propagation?
What about SanitizerSpecialCaseList?

[melver]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

310

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.

311

__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.

llvm/test/Instrumentation/MemorySanitizer/msan_no_sanitize_whole_file.ll

4 ↗

(On Diff #392351)

This is not fully "noinstr", right? I.e. "noinstr" is slightly misleading since we still want instrumentation, just no checks.

27 ↗

(On Diff #392351)

This can be a "CHECK" because both are supposed to have this, right?

48 ↗

(On Diff #392351)

Aren't INSTR and NOINSTR supposed to be identical in the NoSanitizeFn case? I.e. you could just use CHECK throughout?

[glider]

Addressed review comments

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

310

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).

311

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.

312

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.

llvm/test/Instrumentation/MemorySanitizer/msan_no_sanitize_whole_file.ll

4 ↗

(On Diff #392351)

Changed to NOSANITIZE.

27 ↗

(On Diff #392351)

No. This instruction unpoisons the return value, which only happens in the "non-instrumented" case.

48 ↗

(On Diff #392351)

Right, thank you!

[melver]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

311

s/ClNoSanitizeWholeFile/ClNoSanitizeAll/ ?

[glider]

Fixed flag name

[glider]

Rebase

[vitalybuka]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

311

I don't see value in -all suffix on the flag applied to compilation unit. It's obvious that it's apply to "all"
"msan-no-sanitize-all" -> "msan-no-sanitize"

Maybe, but I am not sure: it's also a boolean flag why not just

static cl::opt<bool>
ClSanitize("msan-sanitize",
...
cl::init(true));

Are you going to add fronted flag? Usually they have -fsanitize-memory- prefix:
-fsanitize-memory-???? Also not sure what this could be.

-fsanitize-memory-ignore?

[glider]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

311

I don't see value in -all suffix on the flag applied to compilation unit. It's obvious that it's apply to "all"
"msan-no-sanitize-all" -> "msan-no-sanitize"

I imagine this being read as "apply no_sanitize to all functions", and "msan-no-sanitize" actually sounds very much like -fnosanitize=memory. Perhaps we need a better name...

Maybe, but I am not sure: it's also a boolean flag why not just

static cl::opt<bool>
ClSanitize("msan-sanitize",
...
cl::init(true));

Are you going to add fronted flag? Usually they have -fsanitize-memory- prefix:
-fsanitize-memory-???? Also not sure what this could be.

Good question, it didn't come to my mind. Not necessarily needed for KMSAN, but as long as we have the backend flag, might be a good idea.

-fsanitize-memory-ignore?

-fsanitize-memory-ignore-all-checks, maybe?

(and -mllvm -msan-ignore-all-checks ?)

[melver]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

311

Probably, like Vitaly pointed out, the "all" might be redundant.

The other question is, are checks "ignored", or "disabled".

Perhaps one of the following:

-fsanitize-memory-ignore-checks / -mllvm -msan-ignore-checks
-fsanitize-memory-disable-checks / -mllvm -msan-disable-checks

whichever you find to be clearest.

[glider]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

311

Probably, like Vitaly pointed out, the "all" might be redundant.

The other question is, are checks "ignored", or "disabled".

Perhaps one of the following:

-fsanitize-memory-ignore-checks / -mllvm -msan-ignore-checks
-fsanitize-memory-disable-checks / -mllvm -msan-disable-checks

whichever you find to be clearest.

I think "disable" should be better.
I'll go ahead and rename the -mllvm flag then.

[glider]

Rename the flag

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

cl::desc(
         "inline checks (-1 means never use callbacks)."),
     cl::Hidden, cl::init(3500));
 
 static cl::opt<bool>
     ClEnableKmsan("msan-kernel",
                   cl::desc("Enable KernelMemorySanitizer instrumentation"),
                   cl::Hidden, cl::init(false));
 
+static cl::opt<bool>
+    ClDisableChecks("msan-disable-checks",
+                    cl::desc("Apply no_sanitize to the whole file"), cl::Hidden,
+                    cl::init(false));
+
 // This is an experiment to enable handling of cases where shadow is a non-zero
 // compile-time constant. For some unexplainable reason they were silently
 // ignored in the instrumentation.
 static cl::opt<bool> ClCheckConstantShadow("msan-check-constant-shadow",
        cl::desc("Insert checks for constant shadow values"),
        cl::Hidden, cl::init(false));
 
 // This is off by default because of a bug in gold:
struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
   bool InstrumentLifetimeStart = ClHandleLifetimeIntrinsics;
   SmallSet<AllocaInst *, 16> AllocaSet;
   SmallVector<std::pair<IntrinsicInst *, AllocaInst *>, 16> LifetimeStartList;
   SmallVector<StoreInst *, 16> StoreList;
 
   MemorySanitizerVisitor(Function &F, MemorySanitizer &MS,
                          const TargetLibraryInfo &TLI)
       : F(F), MS(MS), VAHelper(CreateVarArgHelper(F, MS, *this)), TLI(&TLI) {
-    bool SanitizeFunction = F.hasFnAttribute(Attribute::SanitizeMemory);
+    bool SanitizeFunction =
+        F.hasFnAttribute(Attribute::SanitizeMemory) && !ClDisableChecks;
     InsertChecks = SanitizeFunction;
     PropagateShadow = SanitizeFunction;
     PoisonStack = SanitizeFunction && ClPoisonStack;
     PoisonUndef = SanitizeFunction && ClPoisonUndef;
 
     // In the presence of unreachable blocks, we may see Phi nodes with
     // incoming nodes from such blocks. Since InstVisitor skips unreachable
     // blocks, such nodes will not have any shadow value associated with them.

llvm/test/Instrumentation/MemorySanitizer/msan-disable-checks.ll

+; Test for -msan-disable-checks, which should treat every function in the file
+; as if it didn't have the sanitize_memory attribute.
+; 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-disable-checks=1 2>&1 | FileCheck -allow-deprecated-dag-overlap -check-prefixes=CHECK,NOSANITIZE %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @bar()
+
+define i32 @SanitizeFn(i32 %x) uwtable sanitize_memory {
+entry:
+  %tobool = icmp eq i32 %x, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  tail call void @bar()
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  ret i32 %x
+}
+
+; CHECK-LABEL: @SanitizeFn
+; INSTR: @__msan_warning
+; NOSANITIZE-NOT: @__msan_warning
+; NOSANITIZE: store i32 0, {{.*}} @__msan_retval_tls
+; CHECK: ret i32
+
+
+define i32 @NoSanitizeFn(i32 %x) uwtable {
+entry:
+  %tobool = icmp eq i32 %x, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  tail call void @bar()
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  ret i32 %x
+}
+
+
+; CHECK-LABEL: @NoSanitizeFn
+; CHECK-NOT: @__msan_warning
+; CHECK: store i32 0, {{.*}} @__msan_retval_tls
+; CHECK: ret i32
+