[msan] Don't sanitize "nosanitize" instructions

[eugenis]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

1344

what's the point of this using statement?

I think it would be more readable to do this in runOnFunction (see the depth_first loop over basic blocks).

[vitalybuka]

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

1344

InstVisitor has other overloads of visit()
c++ stops looking for overloads if it found something in the current class
if I add mine here "visit(basic_block)" will try to call mine and fail.
"using" inherit all overloads from parent class, so for basic block still will be able to call the same overload as before.

seems like this visitor exists for things exactly like this, and I see similar usage in other places. moving check into runOnFunction will make us to copy loop over instructions which does not look more readable to me. I'd prefer to keep as much as possible logic in InstVisitor. I guess we have own look for bblocks in runOnFunction only to have depth_first.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
 
   /// \brief Get the shadow value for a given Value.
   ///
   /// This function either returns the value set earlier with setShadow,
   /// or extracts if from ParamTLS (for function arguments).
   Value *getShadow(Value *V) {
     if (!PropagateShadow) return getCleanShadow(V);
     if (Instruction *I = dyn_cast<Instruction>(V)) {
+      if (I->getMetadata("nosanitize"))
+        return getCleanShadow(V);
       // For instructions the shadow is already stored in the map.
       Value *Shadow = ShadowMap[V];
       if (!Shadow) {
         DEBUG(dbgs() << "No shadow: " << *V << "\n" << *(I->getParent()));
         (void)I;
         assert(Shadow && "No shadow for a value");
       }
       return Shadow;
struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
 
   /// \brief Get the origin for a value.
   Value *getOrigin(Value *V) {
     if (!MS.TrackOrigins) return nullptr;
     if (!PropagateShadow) return getCleanOrigin();
     if (isa<Constant>(V)) return getCleanOrigin();
     assert((isa<Instruction>(V) || isa<Argument>(V)) &&
            "Unexpected value type in getOrigin()");
+    if (Instruction *I = dyn_cast<Instruction>(V)) {
+      if (I->getMetadata("nosanitize"))
+        return getCleanOrigin();
+    }
     Value *Origin = OriginMap[V];
     assert(Origin && "Missing origin");
     return Origin;
   }
 
   /// \brief Get the origin for i-th argument of the instruction I.
   Value *getOrigin(Instruction *I, int i) {
     return getOrigin(I->getOperand(i));
switch (a) {
         return AtomicOrdering::AcquireRelease;
       case AtomicOrdering::SequentiallyConsistent:
         return AtomicOrdering::SequentiallyConsistent;
     }
     llvm_unreachable("Unknown ordering");
   }
 
   // ------------------- Visitors.
+  using InstVisitor<MemorySanitizerVisitor>::visit;
+  void visit(Instruction &I) {
+    if (!I.getMetadata("nosanitize"))
+      InstVisitor<MemorySanitizerVisitor>::visit(I);
+  }
 
   /// \brief Instrument LoadInst
   ///
   /// Loads the corresponding shadow and (optionally) origin.
   /// Optionally, checks that the load address is fully defined.
   void visitLoadInst(LoadInst &I) {
     assert(I.getType()->isSized() && "Load type must have size");
+    assert(!I.getMetadata("nosanitize"));
     IRBuilder<> IRB(I.getNextNode());
     Type *ShadowTy = getShadowTy(&I);
     Value *Addr = I.getPointerOperand();
-    if (PropagateShadow && !I.getMetadata("nosanitize")) {
+    if (PropagateShadow) {
       Value *ShadowPtr = getShadowPtr(Addr, ShadowTy, IRB);
       setShadow(&I,
                 IRB.CreateAlignedLoad(ShadowPtr, I.getAlignment(), "_msld"));
     } else {
       setShadow(&I, getCleanShadow(&I));
     }
 
     if (ClCheckAccessAddress)
default:
       if (!handleUnknownIntrinsic(I))
         visitInstruction(I);
       break;
     }
   }
 
   void visitCallSite(CallSite CS) {
     Instruction &I = *CS.getInstruction();
-    if (I.getMetadata("nosanitize")) return;
+    assert(!I.getMetadata("nosanitize"));
     assert((CS.isCall() || CS.isInvoke()) && "Unknown type of CallSite");
     if (CS.isCall()) {
       CallInst *Call = cast<CallInst>(&I);
 
       // For inline asm, 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.
       if (Call->isInlineAsm()) {

llvm/test/Instrumentation/MemorySanitizer/call-nosanitize.ll

-; Verify that calls with !nosanitize are not instrumented by MSan.
-; RUN: opt < %s -msan -S | FileCheck %s
-target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-unknown-linux-gnu"
-
-declare void @bar(i32 %x)
-
-define void @foo() {
-  call void @bar(i32 7), !nosanitize !{}
-  ret void
-}
-
-; CHECK-LABEL: define void @foo
-; CHECK-NOT: store i{{[0-9]+}} 0, {{.*}} @__msan_param_tls
-; CHECK: call void @bar
-; CHECK: ret void

llvm/test/Instrumentation/MemorySanitizer/nosanitize.ll

+; Verify that calls with !nosanitize are not instrumented by MSan.
+; RUN: opt < %s -msan -S | FileCheck %s
+; RUN: opt < %s -msan -msan-track-origins=1 -S | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @bar(i32 %x)
+
+define void @foo() {
+  call void @bar(i32 7), !nosanitize !{}
+  ret void
+}
+
+; CHECK-LABEL: define void @foo
+; CHECK-NOT: store {{.*}} @__msan_param_tls
+; CHECK: call void @bar
+; CHECK: ret void
+
+
+@__sancov_gen_ = private global [1 x i8] zeroinitializer, section "__sancov_cntrs", align 1
+define void @sancov() sanitize_memory {
+entry:
+  %0 = load i8, i8* getelementptr inbounds ([1 x i8], [1 x i8]* @__sancov_gen_, i64 0, i64 0), !nosanitize !{}
+  %1 = add i8 %0, 1
+  store i8 %1, i8* getelementptr inbounds ([1 x i8], [1 x i8]* @__sancov_gen_, i64 0, i64 0), !nosanitize !{}
+  ret void
+}
+
+; CHECK-LABEL: define void @sancov
+; CHECK-NOT: xor
+; CHECK-NOT: 87960930222080
+; CHECK: ret void
+
+
+define void @load_store() sanitize_memory {
+entry:
+  %x = alloca i32, align 4, !nosanitize !{}
+  store i32 4, i32* %x, align 4, !nosanitize !{}
+  %0 = load i32, i32* %x, align 4, !nosanitize !{}
+  %add = add nsw i32 %0, %0
+  store i32 %add, i32* %x, align 4, !nosanitize !{}
+  ret void
+}
+
+; CHECK-LABEL: define void @load_store
+; CHECK-NOT: xor
+; CHECK-NOT: 87960930222080
+; CHECK: ret void