[ASan] Do not instrument other runtime functions with `__asan_handle_no_return`

[yln]

Summary

Currently, ASan inserts a call to __asan_handle_no_return before every
noreturn function call/invoke. This is unnecessary for calls to other
runtime funtions. This patch changes ASan to skip instrumentation for
functions calls marked with !nosanitize metadata.

[vsk]

Looks reasonable to me.

llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll

3

Could you make this more specific? E.g. 'noreturn calls that aren't inserted by sanitizers'.

[eugenis]

Do we have a test that at most one __asan_handle_no_return call is inserted before a [[noreturn]] call?

[yln]

Improve comments.
Use CHECK-NEXT in test to ensure we insert exactly one handler call.

[eugenis]

LGTM

[yln]

@vsk Any more comments?

[vsk]

Thanks, lgtm!

clang/lib/CodeGen/CGCall.cpp

#endif
   // insertion point; this allows the rest of IRGen to discard
   // unreachable code.
   if (CI->doesNotReturn()) {
     if (UnusedReturnSizePtr)
       PopCleanupBlock();
 
     // Strip away the noreturn attribute to better diagnose unreachable UB.
     if (SanOpts.has(SanitizerKind::Unreachable)) {
-      // Also remove from function since CI->hasFnAttr(..) also checks attributes
-      // of the called function.
+      // Also remove from function since CallBase::hasFnAttr additionally checks
+      // attributes of the called function.
       if (auto *F = CI->getCalledFunction())
         F->removeFnAttr(llvm::Attribute::NoReturn);
       CI->removeAttribute(llvm::AttributeList::FunctionIndex,
                           llvm::Attribute::NoReturn);
 
       // Avoid incompatibility with ASan which relies on the `noreturn`
       // attribute to insert handler calls.
       if (SanOpts.has(SanitizerKind::Address)) {

clang/test/CodeGen/ubsan-asan-noreturn.c

 // Ensure compatiblity of UBSan unreachable with ASan in the presence of
 // noreturn functions.
 // RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
 
 void my_longjmp(void) __attribute__((noreturn));
 
 // CHECK-LABEL: define void @calls_noreturn()
 void calls_noreturn() {
   my_longjmp();
   // CHECK:      @__asan_handle_no_return{{.*}} !nosanitize
   // CHECK-NEXT: @my_longjmp(){{[^#]*}}
-  // CHECK:      @__asan_handle_no_return()
-  // CHECK-NEXT: @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
+  // CHECK:      @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
   // CHECK-NEXT: unreachable
 }
 
 // CHECK: declare void @my_longjmp() [[FN_ATTR:#[0-9]+]]
 // CHECK: declare void @__asan_handle_no_return()
 
 // CHECK-LABEL: attributes
 // CHECK-NOT: [[FN_ATTR]] = { {{.*noreturn.*}} }

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

for (auto &Inst : BB) {
       } else if (isa<MemIntrinsic>(Inst)) {
         // ok, take it.
       } else {
         if (isa<AllocaInst>(Inst)) NumAllocas++;
         CallSite CS(&Inst);
         if (CS) {
           // A call inside BB.
           TempsToInstrument.clear();
-          if (CS.doesNotReturn()) NoReturnCalls.push_back(CS.getInstruction());
+          if (CS.doesNotReturn() && !CS->getMetadata("nosanitize"))
+            NoReturnCalls.push_back(CS.getInstruction());
         }
         if (CallInst *CI = dyn_cast<CallInst>(&Inst))
           maybeMarkSanitizerLibraryCallNoBuiltin(CI, TLI);
         continue;
       }
       ToInstrument.push_back(&Inst);
       NumInsnsPerBB++;
       if (NumInsnsPerBB >= ClMaxInsnsToInstrumentPerBB) break;
if (ClDebugMin < 0 || ClDebugMax < 0 ||
         instrumentMemIntrinsic(cast<MemIntrinsic>(Inst));
     }
     NumInstrumented++;
   }
 
   FunctionStackPoisoner FSP(F, *this);
   bool ChangedStack = FSP.runOnFunction();
 
-  // We must unpoison the stack before every NoReturn call (throw, _exit, etc).
+  // We must unpoison the stack before NoReturn calls (throw, _exit, etc).
   // See e.g. https://github.com/google/sanitizers/issues/37
   for (auto CI : NoReturnCalls) {
     IRBuilder<> IRB(CI);
     IRB.CreateCall(AsanHandleNoReturnFunc, {});
   }
 
   for (auto Inst : PointerComparisonsOrSubtracts) {
     instrumentPointerComparisonOrSubtraction(Inst);

llvm/test/Instrumentation/AddressSanitizer/instrument-no-return.ll

-; RUN: opt < %s -asan -asan-module -S | FileCheck %s
+; RUN: opt < %s -asan -S | FileCheck %s
 ; AddressSanitizer must insert __asan_handle_no_return
-; before every noreturn call or invoke.
+; before noreturn calls that aren't inserted by sanitizers.
 
 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"
 target triple = "x86_64-unknown-linux-gnu"
 
-declare void @MyNoReturnFunc(i32) noreturn
+declare void @NormalFunc()
+declare void @NoReturnFunc() noreturn
 
-define i32 @Call1(i8* nocapture %arg) uwtable sanitize_address {
-entry:
-  call void @MyNoReturnFunc(i32 1) noreturn  ; The call insn has noreturn attr.
-; CHECK:        @Call1
-; CHECK:        call void @__asan_handle_no_return
-; CHECK-NEXT:   call void @MyNoReturnFunc
-; CHECK-NEXT: unreachable
+; Instrument noreturn callsites (regardless of function)
+define i32 @Call1() sanitize_address {
+  call void @NormalFunc() noreturn
   unreachable
 }
-
-define i32 @Call2(i8* nocapture %arg) uwtable sanitize_address {
-entry:
-  call void @MyNoReturnFunc(i32 1)  ; No noreturn attribure on the call.
-; CHECK:        @Call2
-; CHECK:        call void @__asan_handle_no_return
-; CHECK-NEXT:   call void @MyNoReturnFunc
-; CHECK-NEXT: unreachable
+; CHECK-LABEL:  @Call1
+; CHECK-NEXT:   call void @__asan_handle_no_return
+; CHECK-NEXT:   call void @NormalFunc
+
+; Instrument calls to noreturn functions (regardless of callsite)
+define i32 @Call2() sanitize_address {
+  call void @NoReturnFunc()
+  unreachable
+}
+; CHECK-LABEL:  @Call2
+; CHECK-NEXT:   call void @__asan_handle_no_return
+; CHECK-NEXT:   call void @NoReturnFunc
+
+; Do *not* instrument callsites marked !nosanitize
+define i32 @Call3() sanitize_address {
+  call void @NoReturnFunc() noreturn, !nosanitize !{}
   unreachable
 }
+; CHECK-LABEL:  @Call3
+; CHECK-NOT:    call void @__asan_handle_no_return
+; CHECK:        call void @NoReturnFunc
 
 declare i32 @__gxx_personality_v0(...)
 
-define i64 @Invoke1(i8** %esc) nounwind uwtable ssp sanitize_address personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+define i64 @Invoke1() nounwind uwtable ssp sanitize_address personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
-  invoke void @MyNoReturnFunc(i32 1)
+  invoke void @NoReturnFunc()
           to label %invoke.cont unwind label %lpad
 
 invoke.cont:
   ret i64 0
 
 lpad:
   %0 = landingpad { i8*, i32 }
           filter [0 x i8*] zeroinitializer
   ret i64 1
 }
-; CHECK: @Invoke1
+; CHECK-LABEL:  @Invoke1
 ; CHECK:        call void @__asan_handle_no_return
-; CHECK-NEXT:   invoke void @MyNoReturnFunc
+; CHECK-NEXT:   invoke void @NoReturnFunc
 ; CHECK: ret i64 0
 ; CHECK: ret i64 1