[MSan] Don't emit __msan_instrument_asm_load() calls in conservative assembly handling mode

[glider]

Summary

LLVM treats void* pointers passed to assembly routines as pointers to sized types.
We used to emit calls to __msan_instrument_asm_load() for every such void*, which sometimes led to false positives.
A less error-prone (and truly "conservative") approach is to unpoison only assembly output arguments.

[eugenis]

LGTM

[glider]

Landed r349734, thank you!

lib/Transforms/Instrumentation/MemorySanitizer.cpp

 ///
 /// For inline assembly code LLVM has little idea about which memory locations
 /// become initialized depending on the arguments. It can be possible to figure
 /// out which arguments are meant to point to inputs and outputs, but the
 /// actual semantics can be only visible at runtime. In the Linux kernel it's
 /// also possible that the arguments only indicate the offset for a base taken
 /// from a segment register, so it's dangerous to treat any asm() arguments as
 /// pointers. We take a conservative approach generating calls to
-///   __msan_instrument_asm_load(ptr, size) and
 ///   __msan_instrument_asm_store(ptr, size)
-/// , which defer the memory checking/unpoisoning to the runtime library.
+/// , which defer the memory unpoisoning to the runtime library.
 /// The latter can perform more complex address checks to figure out whether
 /// it's safe to touch the shadow memory.
 /// Like with atomic operations, we call __msan_instrument_asm_store() before
 /// the assembly call, so that changes to the shadow memory will be seen by
 /// other threads together with main memory initialization.
 ///
 ///                  KernelMemorySanitizer (KMSAN) implementation.
 ///
private:
   /// Functions for poisoning/unpoisoning local variables
   Value *MsanPoisonAllocaFn, *MsanUnpoisonAllocaFn;
 
   /// Each of the MsanMetadataPtrXxx functions returns a pair of shadow/origin
   /// pointers.
   Value *MsanMetadataPtrForLoadN, *MsanMetadataPtrForStoreN;
   Value *MsanMetadataPtrForLoad_1_8[4];
   Value *MsanMetadataPtrForStore_1_8[4];
-  Value *MsanInstrumentAsmStoreFn, *MsanInstrumentAsmLoadFn;
+  Value *MsanInstrumentAsmStoreFn;
 
   /// Helper to choose between different MsanMetadataPtrXxx().
   Value *getKmsanShadowOriginAccessFn(bool isStore, int size);
 
   /// Memory map parameters used in application-to-shadow calculation.
   const MemoryMapParams *MapParams;
 
   /// Custom memory map parameters used when -msan-shadow-base or
void MemorySanitizer::initializeCallbacks(Module &M) {
   MemsetFn = M.getOrInsertFunction(
     "__msan_memset", IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IRB.getInt32Ty(),
     IntptrTy);
   // We insert an empty inline asm after __msan_report* to avoid callback merge.
   EmptyAsm = InlineAsm::get(FunctionType::get(IRB.getVoidTy(), false),
                             StringRef(""), StringRef(""),
                             /*hasSideEffects=*/true);
 
-  MsanInstrumentAsmLoadFn =
-      M.getOrInsertFunction("__msan_instrument_asm_load", IRB.getVoidTy(),
-                            PointerType::get(IRB.getInt8Ty(), 0), IntptrTy);
   MsanInstrumentAsmStoreFn =
       M.getOrInsertFunction("__msan_instrument_asm_store", IRB.getVoidTy(),
                             PointerType::get(IRB.getInt8Ty(), 0), IntptrTy);
 
   if (CompileKernel) {
     createKernelApi(M);
   } else {
     createUserspaceApi(M);
void instrumentAsmArgument(Value *Operand, Instruction &I, IRBuilder<> &IRB,
                              const DataLayout &DL, bool isOutput) {
     // For each assembly argument, we check its value for being initialized.
     // If the argument is a pointer, we assume it points to a single element
     // of the corresponding type (or to a 8-byte word, if the type is unsized).
     // Each such pointer is instrumented with a call to the runtime library.
     Type *OpType = Operand->getType();
     // Check the operand value itself.
     insertShadowCheck(Operand, &I);
-    if (!OpType->isPointerTy()) {
+    if (!OpType->isPointerTy() || !isOutput) {
       assert(!isOutput);
       return;
     }
-    Value *Hook =
-        isOutput ? MS.MsanInstrumentAsmStoreFn : MS.MsanInstrumentAsmLoadFn;
     Type *ElType = OpType->getPointerElementType();
     if (!ElType->isSized())
       return;
     int Size = DL.getTypeStoreSize(ElType);
     Value *Ptr = IRB.CreatePointerCast(Operand, IRB.getInt8PtrTy());
     Value *SizeVal = ConstantInt::get(MS.IntptrTy, Size);
-    IRB.CreateCall(Hook, {Ptr, SizeVal});
+    IRB.CreateCall(MS.MsanInstrumentAsmStoreFn, {Ptr, SizeVal});
   }
 
   /// Get the number of output arguments returned by pointers.
   int getNumOutputArgs(InlineAsm *IA, CallInst *CI) {
     int NumRetOutputs = 0;
     int NumOutputs = 0;
     Type *RetTy = dyn_cast<Value>(CI)->getType();
     if (!RetTy->isVoidTy()) {

test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll

 ;  asm("" : "=m" (id1), "=m" (id2) : "m" (is1), "m"(is2))
 define dso_local void @f_2i_2o_mem() sanitize_memory {
 entry:
   call void asm "", "=*m,=*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(i32* @id1, i32* @id2, i32* @is1, i32* @is2)
   ret void
 }
 
 ; CHECK-LABEL: @f_2i_2o_mem
-; CHECK-CONS: call void @__msan_instrument_asm_load({{.*}}@is1{{.*}}, i64 4)
-; CHECK-CONS: call void @__msan_instrument_asm_load({{.*}}@is2{{.*}}, i64 4)
 ; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@id1{{.*}}, i64 4)
 ; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@id2{{.*}}, i64 4)
 ; CHECK: call void asm "", "=*m,=*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(i32* @id1, i32* @id2, i32* @is1, i32* @is2)
 
 
 ; Same input and output passed as both memory and register:
 ;  asm("" : "=r" (id1), "=m"(id1) : "r"(is1), "m"(is1));
 define dso_local void @f_1i_1o_memreg() sanitize_memory {
 entry:
   %0 = load i32, i32* @is1, align 4
   %1 = call i32 asm "", "=r,=*m,r,*m,~{dirflag},~{fpsr},~{flags}"(i32* @id1, i32 %0, i32* @is1)
   store i32 %1, i32* @id1, align 4
   ret void
 }
 
 ; CHECK-LABEL: @f_1i_1o_memreg
 ; CHECK: [[IS1_F7:%.*]] = load i32, i32* @is1, align 4
-; CHECK-CONS: call void @__msan_instrument_asm_load({{.*}}@is1{{.*}}, i64 4)
 ; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@id1{{.*}}, i64 4)
 ; CHECK: call void @__msan_warning
 ; CHECK: call i32 asm "", "=r,=*m,r,*m,~{dirflag},~{fpsr},~{flags}"(i32* @id1, i32 [[IS1_F7]], i32* @is1)
 
 
 ; Three outputs, first and last returned via regs, second via mem:
 ;  asm("" : "=r" (id1), "=m"(id2), "=r" (id3):);
 define dso_local void @f_3o_reg_mem_reg() sanitize_memory {

 ;  asm("" : "=m" (pair2), "=m" (c2), "=m" (memcpy_d1) : "m"(pair1), "m"(c1), "m"(memcpy_s1));
 define dso_local void @f_3i_3o_complex_mem() sanitize_memory {
 entry:
   call void asm "", "=*m,=*m,=*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(%struct.pair* @pair2, i8* @c2, i8* (i8*, i8*, i32)** @memcpy_d1, %struct.pair* @pair1, i8* @c1, i8* (i8*, i8*, i32)** @memcpy_s1)
   ret void
 }
 
 ; CHECK-LABEL: @f_3i_3o_complex_mem
-; CHECK-CONS: call void @__msan_instrument_asm_load({{.*}}@pair1{{.*}}, i64 8)
-; CHECK-CONS: call void @__msan_instrument_asm_load(i8* @c1, i64 1)
-; CHECK-CONS: call void @__msan_instrument_asm_load({{.*}}@memcpy_s1{{.*}}, i64 8)
 ; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@pair2{{.*}}, i64 8)
 ; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@c2{{.*}}, i64 1)
 ; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@memcpy_d1{{.*}}, i64 8)
 ; CHECK: call void asm "", "=*m,=*m,=*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(%struct.pair* @pair2, i8* @c2, i8* (i8*, i8*, i32)** @memcpy_d1, %struct.pair* @pair1, i8* @c1, i8* (i8*, i8*, i32)** @memcpy_s1)