[ASan][MSan] Remove EmptyAsm and set the CallInst to nomerge to avoid from merging.

[zequanwu]

Summary

nomerge attribute was added at D78659. So, we can remove the EmptyAsm workaround in ASan the MSan and use this attribute.

[zequanwu]

Update with SanitizerCoverage.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

1082

HasNonEmptyInlineAsm -> HasInlineAsm

[zequanwu]

address comments.

[uweigand]

It looks like this broke the s390x build bots, since then I'm seeing a lot of ASan and MSan errors:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33485

[zequanwu]

In D82322#2119815, @uweigand wrote:

It looks like this broke the s390x build bots, since then I'm seeing a lot of ASan and MSan errors:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33485

It seems like all those tests failed because the second CHECK should match output line 4 but missing the line number at the end.
Since the patch passed tests in all other build bots, I wonder if it's because the build bot config is flaky.

[uweigand]

In D82322#2120497, @zequanwu wrote:

In D82322#2119815, @uweigand wrote:

It looks like this broke the s390x build bots, since then I'm seeing a lot of ASan and MSan errors:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33485

It seems like all those tests failed because the second CHECK should match output line 4 but missing the line number at the end.
Since the patch passed tests in all other build bots, I wonder if it's because the build bot config is flaky.

@iii had a look and it appears the problem is that this patch https://reviews.llvm.org/D79537 didn't add support on SystemZ, so we get wrong line numbers when merging happens. Ilya is working on a patch.

[zequanwu]

In D82322#2120525, @uweigand wrote:

In D82322#2120497, @zequanwu wrote:

In D82322#2119815, @uweigand wrote:

It looks like this broke the s390x build bots, since then I'm seeing a lot of ASan and MSan errors:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33485

It seems like all those tests failed because the second CHECK should match output line 4 but missing the line number at the end.
Since the patch passed tests in all other build bots, I wonder if it's because the build bot config is flaky.

@iii had a look and it appears the problem is that this patch https://reviews.llvm.org/D79537 didn't add support on SystemZ, so we get wrong line numbers when merging happens. Ilya is working on a patch.

Thanks for the fix

llvm/include/llvm/IR/InstrTypes.h

public:
   /// Determine if the invoke cannot be duplicated.
   bool cannotDuplicate() const { return hasFnAttr(Attribute::NoDuplicate); }
   void setCannotDuplicate() {
     addAttribute(AttributeList::FunctionIndex, Attribute::NoDuplicate);
   }
 
   /// Determine if the call cannot be tail merged.
   bool cannotMerge() const { return hasFnAttr(Attribute::NoMerge); }
+  void setCannotMerge() {
+    addAttribute(AttributeList::FunctionIndex, Attribute::NoMerge);
+  }
 
   /// Determine if the invoke is convergent
   bool isConvergent() const { return hasFnAttr(Attribute::Convergent); }
   void setConvergent() {
     addAttribute(AttributeList::FunctionIndex, Attribute::Convergent);
   }
   void setNotConvergent() {
     removeAttribute(AttributeList::FunctionIndex, Attribute::Convergent);

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

private:
   FunctionCallee AsanErrorCallback[2][2][kNumberOfAccessSizes];
   FunctionCallee AsanMemoryAccessCallback[2][2][kNumberOfAccessSizes];
 
   // These arrays is indexed by AccessIsWrite and Experiment.
   FunctionCallee AsanErrorCallbackSized[2][2];
   FunctionCallee AsanMemoryAccessCallbackSized[2][2];
 
   FunctionCallee AsanMemmove, AsanMemcpy, AsanMemset;
-  InlineAsm *EmptyAsm;
   Value *LocalDynamicShadow = nullptr;
   const GlobalsMetadata &GlobalsMD;
   DenseMap<const AllocaInst *, bool> ProcessedAllocas;
 };
 
 class AddressSanitizerLegacyPass : public FunctionPass {
 public:
   static char ID;
struct FunctionStackPoisoner : public InstVisitor<FunctionStackPoisoner> {
   SmallVector<IntrinsicInst *, 1> StackRestoreVec;
   AllocaInst *DynamicAllocaLayout = nullptr;
   IntrinsicInst *LocalEscapeCall = nullptr;
 
   // Maps Value to an AllocaInst from which the Value is originated.
   using AllocaForValueMapTy = DenseMap<Value *, AllocaInst *>;
   AllocaForValueMapTy AllocaForValue;
 
-  bool HasNonEmptyInlineAsm = false;
+  bool HasInlineAsm = false;
   bool HasReturnsTwiceCall = false;
-  std::unique_ptr<CallInst> EmptyInlineAsm;
 
   FunctionStackPoisoner(Function &F, AddressSanitizer &ASan)
       : F(F), ASan(ASan), DIB(*F.getParent(), /*AllowUnresolved*/ false),
         C(ASan.C), IntptrTy(ASan.IntptrTy),
         IntptrPtrTy(PointerType::get(IntptrTy, 0)), Mapping(ASan.Mapping),
-        StackAlignment(1 << Mapping.Scale),
-        EmptyInlineAsm(CallInst::Create(ASan.EmptyAsm)) {}
+        StackAlignment(1 << Mapping.Scale) {}
 
   bool runOnFunction() {
     if (!ClStack) return false;
 
     if (ClRedzoneByvalArgs)
       copyArgsPassedByValToAllocas();
 
     // Collect alloca, ret, lifetime instructions etc.
void visitIntrinsicInst(IntrinsicInst &II) {
     if (AI->isStaticAlloca())
       StaticAllocaPoisonCallVec.push_back(APC);
     else if (ClInstrumentDynamicAllocas)
       DynamicAllocaPoisonCallVec.push_back(APC);
   }
 
   void visitCallBase(CallBase &CB) {
     if (CallInst *CI = dyn_cast<CallInst>(&CB)) {
-      HasNonEmptyInlineAsm |= CI->isInlineAsm() &&
-                              !CI->isIdenticalTo(EmptyInlineAsm.get()) &&
-                              &CB != ASan.LocalDynamicShadow;
+      HasInlineAsm |= CI->isInlineAsm() && &CB != ASan.LocalDynamicShadow;
       HasReturnsTwiceCall |= CI->canReturnTwice();
     }
   }
 
   // ---------------------- Helpers.
   void initializeCallbacks(Module &M);
 
   // Copies bytes from ShadowBytes into shadow memory for indexes where
if (SizeArgument) {
     if (Exp == 0)
       Call =
           IRB.CreateCall(AsanErrorCallback[IsWrite][0][AccessSizeIndex], Addr);
     else
       Call = IRB.CreateCall(AsanErrorCallback[IsWrite][1][AccessSizeIndex],
                             {Addr, ExpVal});
   }
 
-  // We don't do Call->setDoesNotReturn() because the BB already has
-  // UnreachableInst at the end.
-  // This EmptyAsm is required to avoid callback merge.
-  IRB.CreateCall(EmptyAsm, {});
+  Call->setCannotMerge();
   return Call;
 }
 
 Value *AddressSanitizer::createSlowPathCmp(IRBuilder<> &IRB, Value *AddrLong,
                                            Value *ShadowValue,
                                            uint32_t TypeSize) {
   size_t Granularity = static_cast<size_t>(1) << Mapping.Scale;
   // Addr & (Granularity - 1)
void AddressSanitizer::initializeCallbacks(Module &M) {
 
   AsanHandleNoReturnFunc =
       M.getOrInsertFunction(kAsanHandleNoReturnName, IRB.getVoidTy());
 
   AsanPtrCmpFunction =
       M.getOrInsertFunction(kAsanPtrCmp, IRB.getVoidTy(), IntptrTy, IntptrTy);
   AsanPtrSubFunction =
       M.getOrInsertFunction(kAsanPtrSub, IRB.getVoidTy(), IntptrTy, IntptrTy);
-  // We insert an empty inline asm after __asan_report* to avoid callback merge.
-  EmptyAsm = InlineAsm::get(FunctionType::get(IRB.getVoidTy(), false),
-                            StringRef(""), StringRef(""),
-                            /*hasSideEffects=*/true);
   if (Mapping.InGlobal)
     AsanShadowGlobal = M.getOrInsertGlobal("__asan_shadow",
                                            ArrayType::get(IRB.getInt8Ty(), 0));
 }
 
 bool AddressSanitizer::maybeInsertAsanInitAtFunctionEntry(Function &F) {
   // For each NSObject descendant having a +load method, this method is invoked
   // by the ObjC runtime before any of the static constructors is called.
bool DoStackMalloc = ClUseAfterReturn && !ASan.CompileKernel &&
                        LocalStackSize <= kMaxStackMallocSize;
   bool DoDynamicAlloca = ClDynamicAllocaStack;
   // Don't do dynamic alloca or stack malloc if:
   // 1) There is inline asm: too often it makes assumptions on which registers
   //    are available.
   // 2) There is a returns_twice call (typically setjmp), which is
   //    optimization-hostile, and doesn't play well with introduced indirect
   //    register-relative calculation of local variable addresses.
-  DoDynamicAlloca &= !HasNonEmptyInlineAsm && !HasReturnsTwiceCall;
-  DoStackMalloc &= !HasNonEmptyInlineAsm && !HasReturnsTwiceCall;
+  DoDynamicAlloca &= !HasInlineAsm && !HasReturnsTwiceCall;
+  DoStackMalloc &= !HasInlineAsm && !HasReturnsTwiceCall;
 
   Value *StaticAlloca =
       DoDynamicAlloca ? nullptr : createAllocaForLayout(IRB, L, false);
 
   Value *FakeStack;
   Value *LocalStackBase;
   Value *LocalStackBaseAlloca;
   uint8_t DIExprFlags = DIExpression::ApplyOffset;

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp

private:
   /// Custom memory map parameters used when -msan-shadow-base or
   // -msan-origin-base is provided.
   MemoryMapParams CustomMapParams;
 
   MDNode *ColdCallWeights;
 
   /// Branch weights for origin store.
   MDNode *OriginStoreWeights;
-
-  /// An empty volatile inline asm that prevents callback merge.
-  InlineAsm *EmptyAsm;
 };
 
 void insertModuleCtor(Module &M) {
   getOrCreateSanitizerCtorAndInitFunctions(
       M, kMsanModuleCtorName, kMsanInitName,
       /*InitArgTypes=*/{},
       /*InitArgs=*/{},
       // This callback is invoked when the functions are created the first
MemmoveFn = M.getOrInsertFunction(
     "__msan_memmove", IRB.getInt8PtrTy(), IRB.getInt8PtrTy(),
     IRB.getInt8PtrTy(), IntptrTy);
   MemcpyFn = M.getOrInsertFunction(
     "__msan_memcpy", IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IRB.getInt8PtrTy(),
     IntptrTy);
   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);
 
   MsanInstrumentAsmStoreFn =
       M.getOrInsertFunction("__msan_instrument_asm_store", IRB.getVoidTy(),
                             PointerType::get(IRB.getInt8Ty(), 0), IntptrTy);
 
   if (CompileKernel) {
     createKernelApi(M);
   } else {
void materializeStores(bool InstrumentWithCalls) {
     }
   }
 
   /// Helper function to insert a warning at IRB's current insert point.
   void insertWarningFn(IRBuilder<> &IRB, Value *Origin) {
     if (!Origin)
       Origin = (Value *)IRB.getInt32(0);
     assert(Origin->getType()->isIntegerTy());
-    IRB.CreateCall(MS.WarningFn, Origin);
-    IRB.CreateCall(MS.EmptyAsm, {});
+    IRB.CreateCall(MS.WarningFn, Origin)->setCannotMerge();
     // FIXME: Insert UnreachableInst if !MS.Recover?
     // This may invalidate some of the following checks and needs to be done
     // at the very end.
   }
 
   void materializeOneCheck(Instruction *OrigIns, Value *Shadow, Value *Origin,
                            bool AsCall) {
     IRBuilder<> IRB(OrigIns);

llvm/lib/Transforms/Instrumentation/SanitizerCoverage.cpp

private:
   FunctionCallee SanCovTracePCIndir;
   FunctionCallee SanCovTracePC, SanCovTracePCGuard;
   FunctionCallee SanCovTraceCmpFunction[4];
   FunctionCallee SanCovTraceConstCmpFunction[4];
   FunctionCallee SanCovTraceDivFunction[2];
   FunctionCallee SanCovTraceGepFunction;
   FunctionCallee SanCovTraceSwitchFunction;
   GlobalVariable *SanCovLowestStack;
-  InlineAsm *EmptyAsm;
   Type *IntptrTy, *IntptrPtrTy, *Int64Ty, *Int64PtrTy, *Int32Ty, *Int32PtrTy,
       *Int16Ty, *Int8Ty, *Int8PtrTy, *Int1Ty, *Int1PtrTy;
   Module *CurModule;
   std::string CurModuleUniqueId;
   Triple TargetTriple;
   LLVMContext *C;
   const DataLayout *DL;
 
C->emitError(StringRef("'") + SanCovLowestStackName +
                  "' should not be declared by the user");
     return true;
   }
   SanCovLowestStack->setThreadLocalMode(
       GlobalValue::ThreadLocalMode::InitialExecTLSModel);
   if (Options.StackDepth && !SanCovLowestStack->isDeclaration())
     SanCovLowestStack->setInitializer(Constant::getAllOnesValue(IntptrTy));
 
-  // We insert an empty inline asm after cov callbacks to avoid callback merge.
-  EmptyAsm = InlineAsm::get(FunctionType::get(IRB.getVoidTy(), false),
-                            StringRef(""), StringRef(""),
-                            /*hasSideEffects=*/true);
-
   SanCovTracePC = M.getOrInsertFunction(SanCovTracePCName, VoidTy);
   SanCovTracePCGuard =
       M.getOrInsertFunction(SanCovTracePCGuardName, VoidTy, Int32PtrTy);
 
   for (auto &F : M)
     instrumentFunction(F, DTCallback, PDTCallback);
 
   Function *Ctor = nullptr;
if (IsEntryBB) {
     IP = PrepareToSplitEntryBlock(BB, IP);
   } else {
     EntryLoc = IP->getDebugLoc();
   }
 
   IRBuilder<> IRB(&*IP);
   IRB.SetCurrentDebugLocation(EntryLoc);
   if (Options.TracePC) {
-    IRB.CreateCall(SanCovTracePC); // gets the PC using GET_CALLER_PC.
-    IRB.CreateCall(EmptyAsm, {}); // Avoids callback merge.
+    IRB.CreateCall(SanCovTracePC)
+        ->setCannotMerge(); // gets the PC using GET_CALLER_PC.
   }
   if (Options.TracePCGuard) {
     auto GuardPtr = IRB.CreateIntToPtr(
         IRB.CreateAdd(IRB.CreatePointerCast(FunctionGuardArray, IntptrTy),
                       ConstantInt::get(IntptrTy, Idx * 4)),
         Int32PtrTy);
-    IRB.CreateCall(SanCovTracePCGuard, GuardPtr);
-    IRB.CreateCall(EmptyAsm, {}); // Avoids callback merge.
+    IRB.CreateCall(SanCovTracePCGuard, GuardPtr)->setCannotMerge();
   }
   if (Options.Inline8bitCounters) {
     auto CounterPtr = IRB.CreateGEP(
         Function8bitCounterArray->getValueType(), Function8bitCounterArray,
         {ConstantInt::get(IntptrTy, 0), ConstantInt::get(IntptrTy, Idx)});
     auto Load = IRB.CreateLoad(Int8Ty, CounterPtr);
     auto Inc = IRB.CreateAdd(Load, ConstantInt::get(Int8Ty, 1));
     auto Store = IRB.CreateStore(Inc, CounterPtr);

llvm/test/Instrumentation/MemorySanitizer/msan_basic.ll

 
 ; CHECK-LABEL: @LoadAndCmp
 ; CHECK: %0 = load i32,
 ; CHECK: = load
 ; CHECK-ORIGINS: %[[ORIGIN:.*]] = load
 ; CHECK: call void @__msan_warning_with_origin_noreturn(i32
 ; CHECK-ORIGINS-SAME %[[ORIGIN]])
 ; CHECK-CONT: 
-; CHECK-NEXT: call void asm sideeffect
 ; CHECK-NEXT: unreachable
 ; CHECK: br i1 %tobool
 ; CHECK: ret void
 
 ; Check that we store the shadow for the retval.
 define i32 @ReturnInt() nounwind uwtable readnone sanitize_memory {
 entry:
   ret i32 123

llvm/test/Instrumentation/SanitizerCoverage/coverage-dbg.ll

 ; 5:
 ; 6: int A::f() {
 ; 7:    return x;
 ; 8: }
 ; clang++ ../1.cc -O3 -g -S -emit-llvm  -fno-strict-aliasing
 ; and add sanitize_address to @_ZN1A1fEv
 
 ; Test that __sanitizer_cov_trace_pc_guard call has !dbg pointing to the opening { of A::f().
-; CHECK: call void @__sanitizer_cov_trace_pc_guard(i32*{{.*}}), !dbg [[A:!.*]]
+; CHECK: call void @__sanitizer_cov_trace_pc_guard(i32*{{.*}}) #{{.*}}, !dbg [[A:!.*]]
 ; CHECK: [[A]] = !DILocation(line: 6, scope: !{{.*}})
 
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 %struct.A = type { i32 }
 

llvm/test/Instrumentation/SanitizerCoverage/coverage.ll

entry:
   tail call void %1(%struct.StructWithVptr* %foo)
   tail call void %1(%struct.StructWithVptr* %foo)
   tail call void asm sideeffect "", ""()
   ret void
 }
 
 ; CHECK_TRACE_PC-LABEL: define void @foo
 ; CHECK_TRACE_PC: call void @__sanitizer_cov_trace_pc
-; CHECK_TRACE_PC: call void asm sideeffect "", ""()
 ; CHECK_TRACE_PC: ret void
 
 ; CHECK_TRACE_PC-LABEL: define void @CallViaVptr
 ; CHECK_TRACE_PC: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC: ret void
 
 define void @call_unreachable() uwtable sanitize_address {

llvm/test/Instrumentation/SanitizerCoverage/coverage2-dbg.ll

 
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Check that __sanitizer_cov call has !dgb pointing to the beginning
 ; of appropriate basic blocks.
 ; CHECK-LABEL:_Z3fooPi
-; CHECK: call void @__sanitizer_cov{{.*}}(i32*{{.*}}), !dbg [[A:!.*]]
-; CHECK: call void @__sanitizer_cov{{.*}}(i32*{{.*}}), !dbg [[B:!.*]]
+; CHECK: call void @__sanitizer_cov{{.*}}(i32*{{.*}}) #{{.*}}, !dbg [[A:!.*]]
+; CHECK: call void @__sanitizer_cov{{.*}}(i32*{{.*}}) #{{.*}}, !dbg [[B:!.*]]
 ; CHECK: ret void
 ; CHECK: [[A]] = !DILocation(line: 1, scope: !{{.*}})
 ; CHECK: [[B]] = !DILocation(line: 3, column: 5, scope: !{{.*}})
 
 define void @_Z3fooPi(i32* %a) #0 !dbg !4 {
 entry:
   tail call void @llvm.dbg.value(metadata i32* %a, i64 0, metadata !11, metadata !DIExpression()), !dbg !15
   %tobool = icmp eq i32* %a, null, !dbg !16

llvm/test/Instrumentation/SanitizerCoverage/trace-pc-guard-comdat.ll

entry:
   tail call void %1(%struct.StructWithVptr* %foo)
   tail call void %1(%struct.StructWithVptr* %foo)
   tail call void asm sideeffect "", ""()
   ret void
 }
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define void @foo
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc
-; CHECK_TRACE_PC_GUARD: call void asm sideeffect "", ""()
 ; CHECK_TRACE_PC_GUARD: ret void
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define void @CallViaVptr
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: ret void
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define internal void @sancov.module_ctor_trace_pc_guard() comdat

llvm/test/Instrumentation/SanitizerCoverage/trace-pc-guard-nocomdat.ll

entry:
   tail call void %1(%struct.StructWithVptr* %foo)
   tail call void %1(%struct.StructWithVptr* %foo)
   tail call void asm sideeffect "", ""()
   ret void
 }
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define void @foo
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc
-; CHECK_TRACE_PC_GUARD: call void asm sideeffect "", ""()
 ; CHECK_TRACE_PC_GUARD: ret void
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define void @CallViaVptr
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: call void @__sanitizer_cov_trace_pc_indir
 ; CHECK_TRACE_PC_GUARD: ret void
 
 ; CHECK_TRACE_PC_GUARD-LABEL: define internal void @sancov.module_ctor_trace_pc_guard() {