[Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

Summary:
UBSan wants to detect when unreachable code is actually reached, so it
adds instrumentation before every `unreachable` instruction. However,
the optimizer will remove code after calls to functions marked with
`noreturn`. To avoid this UBSan removes `noreturn` from both the call
instruction as well as from the function itself. Unfortunately, ASan
relies on this annotation to unpoison the stack by inserting calls to
`_asan_handle_no_return` before `noreturn` functions. This is important
for functions that do not return but access the the stack memory, e.g.,
unwinder functions *like* `longjmp` (`longjmp` itself is actually
"double-proofed" via its interceptor). The result is that when ASan and
UBSan are combined, the `noreturn` attributes are missing and ASan
cannot unpoison the stack, so it has false positives when stack
unwinding is used.

Changes:
  # UBSan now adds the `expect_noreturn` attribute whenever it removes
    the `noreturn` attribute from a function
  # ASan additionally checks for the presence of this attribute

Generated code:
```
call void @__asan_handle_no_return    // Additionally inserted to avoid false positives
call void @longjmp
call void @__asan_handle_no_return
call void @__ubsan_handle_builtin_unreachable
unreachable
```

The second call to `__asan_handle_no_return` is redundant. This will be
cleaned up in a follow-up patch.

rdar://problem/40723397

Reviewers: delcypher, eugenis

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D56624

llvm-svn: 352003

2 files changed, 23 insertions, 16 deletions

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index a650493c5ba..bebefe5384a 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -4401,12 +4401,16 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
     if (UnusedReturnSizePtr)
       PopCleanupBlock();
 
-    // Strip away the noreturn attribute to better diagnose unreachable UB.
+    // Replace the noreturn attribute to better diagnose unreachable UB.
     if (SanOpts.has(SanitizerKind::Unreachable)) {
+      // Also remove from function since CS.hasFnAttr(..) also checks attributes
+      // of the called function.
       if (auto *F = CS.getCalledFunction())
         F->removeFnAttr(llvm::Attribute::NoReturn);
       CS.removeAttribute(llvm::AttributeList::FunctionIndex,
                          llvm::Attribute::NoReturn);
+      CS.addAttribute(llvm::AttributeList::FunctionIndex,
+                      llvm::Attribute::ExpectNoReturn);
     }
 
     EmitUnreachable(Loc);
diff --git a/clang/test/CodeGenCXX/ubsan-unreachable.cpp b/clang/test/CodeGenCXX/ubsan-unreachable.cpp
index 32a78048cfd..9ef613e630c 100644
--- a/clang/test/CodeGenCXX/ubsan-unreachable.cpp
+++ b/clang/test/CodeGenCXX/ubsan-unreachable.cpp
@@ -2,38 +2,35 @@
 
 extern void __attribute__((noreturn)) abort();
 
-// CHECK-LABEL: define void @_Z14calls_noreturnv
+// CHECK-LABEL: define void @_Z14calls_noreturnv()
 void calls_noreturn() {
+  // CHECK: call void @_Z5abortv() [[CALL_SITE_ATTR:#[0-9]+]]
   abort();
 
-  // Check that there are no attributes on the call site.
-  // CHECK-NOT: call void @_Z5abortv{{.*}}#
-
   // CHECK: __ubsan_handle_builtin_unreachable
   // CHECK: unreachable
 }
 
 struct A {
-  // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+  // CHECK: declare void @_Z5abortv() [[EXTERN_FN_ATTR:#[0-9]+]]
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
   void call1() {
-    // CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+    // CHECK: call void @_ZN1A16does_not_return2Ev({{.*}}) [[CALL_SITE_ATTR]]
     does_not_return2();
 
     // CHECK: __ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 
-  // Test static members.
+  // Test static members. Checks are below after `struct A` scope ends.
   static void __attribute__((noreturn)) does_not_return1() {
-    // CHECK-NOT: call void @_Z5abortv{{.*}}#
     abort();
   }
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
   void call2() {
-    // CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+    // CHECK: call void @_ZN1A16does_not_return1Ev() [[CALL_SITE_ATTR]]
     does_not_return1();
 
     // CHECK: __ubsan_handle_builtin_unreachable
@@ -46,18 +43,18 @@ struct A {
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
   void call3() {
     MemFn MF = &A::does_not_return2;
+    // CHECK: call void %{{[0-9]+\(.*}}) [[CALL_SITE_ATTR]]
     (this->*MF)();
 
-    // CHECK-NOT: call void %{{.*}}#
     // CHECK: __ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 
   // Test regular members.
   // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
-  // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
+  // CHECK-SAME: [[USER_FN_ATTR:#[0-9]+]]
   void __attribute__((noreturn)) does_not_return2() {
-    // CHECK-NOT: call void @_Z5abortv(){{.*}}#
+    // CHECK: call void @_Z5abortv() [[CALL_SITE_ATTR]]
     abort();
 
     // CHECK: call void @__ubsan_handle_builtin_unreachable
@@ -68,7 +65,9 @@ struct A {
   }
 };
 
-// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() [[DOES_NOT_RETURN_ATTR]]
+// CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return1Ev()
+// CHECK-SAME: [[USER_FN_ATTR]]
+// CHECK: call void @_Z5abortv() [[CALL_SITE_ATTR]]
 
 void force_irgen() {
   A a;
@@ -77,5 +76,9 @@ void force_irgen() {
   a.call3();
 }
 
-// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
-// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn
+// 1) 'noreturn' should be removed from functions and call sites
+// 2) 'expect_noreturn' added to call sites
+// CHECK-LABEL: attributes
+// CHECK: [[USER_FN_ATTR]] = { {{.*[^noreturn].*}} }
+// CHECK: [[EXTERN_FN_ATTR]] = { {{.*[^noreturn].*}} }
+// CHECK: [[CALL_SITE_ATTR]] = { expect_noreturn }