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

[yln]

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:

1. UBSan now adds the expect_noreturn attribute whenever it removes the noreturn attribute from a function
2. 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

Is the second call to __asan_handle_no_return redundant?
If we care about this, then I can provide a follow-up patch dealing with this.

rdar://problem/40723397

[delcypher]

@yln A few comments on the patch description.

ASan relies on this annotation to unpoisen the stack

s/unpoisen/unpoison

3. (unrelated change) ASan now also only instruments calls that are not marked nosanitize

I'm confused. If this change is unrelated to problem we are trying to fix then why is it in this patch?

[delcypher]

clang/lib/CodeGen/CGCall.cpp

4398

I feel like this comment needs more explanation. Where is hasFnAttr called here? It's not at all obvious to me.

4404

This really deserves a comment. Maybe something like

// Sanitizer instrumentation passes (e.g. ASan) still need to know that this function won't return.

clang/lib/CodeGen/SanitizerMetadata.cpp

95 ↗

(On Diff #181394)

Perhaps add a comment in here describing the semantics of this attribute and explaining why its necessary (i.e. llvm::Attribute::NoReturn might get stripped but we need this information to produce correct instrumentation).

clang/test/CodeGenCXX/ubsan-unreachable.cpp

7

This check assumes that sanitizer_noreturn is the first metadata attribute. Is that a safe assumption to make?

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

1

Why did you drop -asan-module here?

[delcypher]

compiler-rt/test/asan/TestCases/ubsan_noreturn_compatibility.c

3 ↗

(On Diff #181394)

This test should have a REQUIRES: check that checks for UBSan support.

[delcypher]

Is it possible to write a compiler-rt lit test with a custom noreturn function that tries to touch the stack such that with the old Clang we'd get a false positive report (when building with ASan+UBSan) when we try to execute it?

[eugenis]

Metadata seems like a wrong tool for this job because it can be discarded at will.
How about an intrinsic, ex. call @llvm.sanitizer.noreturn() before a noreturn call, which asan can replace with __asan_handle_noreturn, and other sanitizers can either handle or drop as necessary?

[yln]

In D56624#1362244, @eugenis wrote:

Metadata seems like a wrong tool for this job because it can be discarded at will.
How about an intrinsic, ex. call @llvm.sanitizer.noreturn() before a noreturn call, which asan can replace with __asan_handle_noreturn, and other sanitizers can either handle or drop as necessary?

Good point. Another option would be to replace the noreturn with another attribute. Do you have a preference between those two options?

[eugenis]

I'm leaning toward the intrinsic, mainly because I've no idea what the new attribute should be called, nor what it would even mean.
ASan uses noreturn attribute as a proxy for "may do non-local jump".

The intrinsic must be declared to have side effects to prevent memory ops from sneaking between it and the original function call.

Hmm, how about "expect_noreturn" for an attribute name?

[yln]

How about sanitizer_noreturn or noreturn_for_sanitizer. It should convey the same meaning as noreturn but has a specific audience: sanitizers.

Quick question about intrinsics (I don't know very much about them yet):

asan can replace with __asan_handle_noreturn, and other sanitizers can either handle or drop as necessary

Can this be structured in a way so that we can define in one place that the default is to drop/ignore and override the default for ASan?
If yes, I am happy to go with the intrinsic.

Otherwise, I prefer the attribute because the sanitizers that don't care can simply ignore it.
Thanks for your feedback! :)

[eugenis]

In D56624#1362458, @yln wrote:

How about sanitizer_noreturn or noreturn_for_sanitizer. It should convey the same meaning as noreturn but has a specific audience: sanitizers.

It's fine, I guess. But I'm starting to like "expect_noreturn" - it basically tells you that the code following the call is cold. That's exactly what happens with ubsan, and there may even be optimizations that could take advantage of it. It has meaning outside of sanitizers. Could also be "unlikely_return" or something like that.

Quick question about intrinsics (I don't know very much about them yet):

asan can replace with __asan_handle_noreturn, and other sanitizers can either handle or drop as necessary

Can this be structured in a way so that we can define in one place that the default is to drop/ignore and override the default for ASan?
If yes, I am happy to go with the intrinsic.

SelectionDAG could be made to ignore this intrinsic if it ever sees it.

[yln]

I like your approach with the attribute with a separate distinctive meaning outside of the sanitizers! I will update the patch accordingly.
About the name: to me, "unlikely_return" sounds more natural and "expect_noreturn" reminds me of "__builtin_expect", which is not a bad thing.

[yln]

Use a function attribute instead of metadata to mark functions which are unlikely to return. Update summary description.

Remove the unrelated change/optimization of not instrumenting calls with !nosanitize. I will provide a separate patch, if we decide that we want this.

[yln]

compiler-rt/test/asan/TestCases/ubsan_noreturn_compatibility.c

3 ↗

(On Diff #181394)

Good point! How can I make this work? llvm-lit always marks the test all unsupported for me.
I tried it with ubsan, asan-ubsan, ubsan-standalone.

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

1

ASan is split into a function and module pass. The module pass is not required for this test.

[yln]

Update language reference documentation

[yln]

@eugenis: We now use an attribute expect_noreturn instead of metadata. Does that look good to you?

[delcypher]

llvm/docs/LangRef.rst

1463

s/correctenss/correctness/

1464

Suggested wording:

This function attribute indicates that the function is unlikely to return normally, but that it still allowed to do so.
This is useful in cases where ``noreturn`` is too strong a guarantee.

[eugenis]

This patch is missing tests for the new attribute in AsmParser / BitcodeReader / BitcodeWriter.

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

Yes, the second one is redundant. Is that because the ubsan call has noreturn attribute? Does it also have !nosanitize, and would it help in this case? The clang test should probably check for the attribute(s) here:

// CHECK: call void @__ubsan_handle_builtin_unreachable

In general, this looks fine.

compiler-rt/test/asan/TestCases/ubsan_noreturn_compatibility.c

3 ↗

(On Diff #181394)

Does not asan always include ubsan?

[delcypher]

compiler-rt/test/asan/TestCases/ubsan_noreturn_compatibility.c

3 ↗

(On Diff #181394)

How can I make this work?

If you put the test in UBSan test suite and add REQUIRES: ubsan-asan it should work as we build multiple combinations of sanitizers in that test suite, including UBSan+ASan.

3 ↗

(On Diff #181394)

Does not asan always include ubsan?

Not necessarily. I believe there is support for RTEMS in the ASan code and sanitizer_common but I don't see any evidence of obvious evidence of support in the UBSan code. I'm just grepping for RTEMS so I could be wrong.

Regardless of this. I think it's better to write to clearly requires support for both sanitizers given that it's pretty easy to do.

[yln]

compiler-rt/test/asan/TestCases/ubsan_noreturn_compatibility.c

3 ↗

(On Diff #181394)

I tested ninja clean && ninja check-asan which does *not* build the ubsan runtime, but this test still passes.

llvm/docs/LangRef.rst

1464

Much clearer. Thanks!

[yln]

Refine wording in docs.
Move test to ubsan/TestCases and mark with REQUIRES: ubsan-asan.
Add test for BitcodeReader/Writer.

[yln]

In D56624#1368382, @eugenis wrote:

This patch is missing tests for the new attribute in AsmParser / BitcodeReader / BitcodeWriter.

Are the additions in llvm/test/Bitcode/attributes.ll sufficient? Please point me to the appropriate places, if additional tests are required.

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

Yes, the second one is redundant. Is that because the ubsan call has noreturn attribute? Does it also have !nosanitize, and would it help in this case?

Yes the call to ubsan_unreachable is marked both noreturn and !nosanitize. However, I am not sure if we can simply ignore calls marked with !nosanitize. My assumption is that we still have to unpoison the stack so that __ubsan_handle_builtin_unreachable can unwind and print a stack trace without a false positive from ASan. Do you know if my assumption holds or is it unnecessary?
My first idea would be to insert calls to __asan_handle_no_return only once per basic block.

I will address this in a separate patch since doing this optimization is an orthogonal pre-existing issue.

[eugenis]

That should not be necessary.
__asan_handle_noreturn is needed for functions that move SP without going through ASan epilogue, in order to maintain the requirement that stack below SP has clean shadow.
Ubsan-rt does nothing of the sort.

[lebedev.ri]

Please revert this.
First, this wasn't reviewed.
Second, the lists weren't subscribed.

[yln]

In D56624#1368966, @lebedev.ri wrote:

Please revert this.
First, this wasn't reviewed.
Second, the lists weren't subscribed.

I apologize for this. It was not my intention to land the revision without formal acceptance.

commit-lists:
I prepared this patch via the monorepo and did not select a repository in Phabricator because the changes span multiple repos. This means that I have to manually ensure that the correct lists are subscribed in the Phabricator web interface, correct?

[lebedev.ri]

In D56624#1369626, @yln wrote:

In D56624#1368966, @lebedev.ri wrote:

Please revert this.
First, this wasn't reviewed.
Second, the lists weren't subscribed.

I apologize for this. It was not my intention to land the revision without formal acceptance.

commit-lists:
I prepared this patch via the monorepo and did not select a repository in Phabricator because the changes span multiple repos.

I guess rL would be a safe default to make.

This means that I have to manually ensure that the correct lists are subscribed in the Phabricator web interface, correct?

Looks like the rules to subscribe the lists based on a content of the patch are still not added, so i'd say yes.

That being said, the normal practice in such situations is to open a new review.

Also, i suspect this should be split up into at least two parts - the new LLVM IR expect_noreturn attribute, and the rest.

[vsk]

What are the advantages of a generalized expect_noreturn attribute, vs. a narrower attribute or intrinsic? The expect_noreturn semantics do not provide strong guarantees, and are not really orthogonal from the pre-existing cold attribute. In particular, expect_noreturn doesn't even seem strong enough to allow ASan to unpoison its stack.

Apologies if discussion has shifted elsewhere -- I'd be happy to chime in on the new review.

[yln]

@lebedev.ri
Thanks for the clarifications!
I will split this up into multiple patches once we settled on a design.

In D56624#1369635, @vsk wrote:

What are the advantages of a generalized expect_noreturn attribute, vs. a narrower attribute or intrinsic? The expect_noreturn semantics do not provide strong guarantees, and are not really orthogonal from the pre-existing cold attribute.

@eugenis Do you want to chime in here?
I think they convey different meanings even if their treatment by the optimizer is similar. The cold attribute says nothing about whether or not a function is expected to return.

In particular, expect_noreturn doesn't even seem strong enough to allow ASan to unpoison its stack.

I am not sure I understand this part. Can you elaborate?

For context:

``cold``
  This attribute indicates that this function is rarely called. When
  computing edge weights, basic blocks post-dominated by a cold
  function call are also considered to be cold; and, thus, given low
  weight.
``noreturn``
  This function attribute indicates that the function never returns
  normally. This produces undefined behavior at runtime if the
  function ever does dynamically return.
``expect_noreturn``
  This function attribute indicates that the function is unlikely to return
  normally, but that it is still allowed to do so. This is useful in cases
  for which ``noreturn`` is too strong a guarantee.

[vsk]

In D56624#1369767, @yln wrote:

In D56624#1369635, @vsk wrote:

What are the advantages of a generalized expect_noreturn attribute, vs. a narrower attribute or intrinsic? The expect_noreturn semantics do not provide strong guarantees, and are not really orthogonal from the pre-existing cold attribute.

@eugenis Do you want to chime in here?
I think they convey different meanings even if their treatment by the optimizer is similar. The cold attribute says nothing about whether or not a function is expected to return.

That's my point: it doesn't need to, because it's orthogonal. It's just a hint that a call is cold and could be profitable to split/reorder. Features of llvm IR generally try to be orthogonal to reduce complexity in the optimizer.

In particular, expect_noreturn doesn't even seem strong enough to allow ASan to unpoison its stack.

I am not sure I understand this part. Can you elaborate?

Because "expect_noreturn" calls are allowed to return, the compiler must behave as they could. In particular, this means that unpoisoning the stack before expect_noreturn calls (given the current semantics) is premature.

Put another way, a frontend author may (understandably, but mistakenly!) attach expect_noreturn to calls which they expect to be cold. That would regress ASan coverage.

[yln]

Note that all of this currently only matters when compiling with -fsanitize=unreachable. The following discussion is within the context of the current implementation: UBSan removes the noreturn so it can instrument unreachable without the added instrumentation being optimized away. Maybe we should take a step back and ask if that is the right approach at all?

In D56624#1369795, @vsk wrote:

Because "expect_noreturn" calls are allowed to return, the compiler must behave as they could. In particular, this means that unpoisoning the stack before expect_noreturn calls (given the current semantics) is premature.

Put another way, a frontend author may (understandably, but mistakenly!) attach expect_noreturn to calls which they expect to be cold.

I think about this differently. Yes, most noreturn functions are also cold, e.g., abort, but not necessarily, e.g., calls to longjmp do not necessarily have to be. Why would it be okay to attach expect_noreturn instead of cold? Why do we think that this is an easy-to-make mistake? Have people accidentally put noreturn on cold functions before?
Can we agree on the following?
"It is orthogonal on the language level, but seems to be redundant in terms of the optimizer. Since LLVM IR's main purpose it support the optimizer, this is a strong argument against the general purpose attribute."

That would regress ASan coverage.

You talk specifically about cases of misuses of the attribute, right?
In the context of the current issue with UBSan the possibility for false negative is not too much of a regression: it only occurs when UBSan is going to diagnose an "unreachable error" anyways.

So the main point is whether or not to use a "general purpose" attribute or a "narrow purpose" attribute/intrinsic. My understanding is that you list the following points as arguments against general purpose. Is my understanding accurate?

1. Potential misuse can regress ASan coverage
2. Complicates optimizer

Narrow purpose: No potential misuses, and optimizer can simply ignore it.

Initially I proposed a narrow purpose attribute, but while iterating on this revision changed it to be general purpose. @eugenis
Now, I have a slight preference for general purpose: I don't think 1. is a big issue (but then again, I have no experience here), and 2. it is always correct for the optimizer to continue ignoring the attribute (current status).
Actually, 2. also encompasses the potential upside: a more complicated optimizer that takes advantage of the attribute to do additional optimizations.

[vsk]

In D56624#1369940, @yln wrote:

Note that all of this currently only matters when compiling with -fsanitize=unreachable. The following discussion is within the context of the current implementation: UBSan removes the noreturn so it can instrument unreachable without the added instrumentation being optimized away. Maybe we should take a step back and ask if that is the right approach at all?

In D56624#1369795, @vsk wrote:

Because "expect_noreturn" calls are allowed to return, the compiler must behave as they could. In particular, this means that unpoisoning the stack before expect_noreturn calls (given the current semantics) is premature.

Put another way, a frontend author may (understandably, but mistakenly!) attach expect_noreturn to calls which they expect to be cold.

I think about this differently. Yes, most noreturn functions are also cold, e.g., abort, but not necessarily, e.g., calls to longjmp do not necessarily have to be. Why would it be okay to attach expect_noreturn instead of cold?

It would be okay by definition, because it would be allowed by the proposed IR semantics.

Why do we think that this is an easy-to-make mistake?

I don't think that's the right question. Rather, we should ask: why is it acceptable to define semantics in a way that makes the mistake possible?

My thinking on this is: it's not acceptable, because a narrower change (say, introducing a sanitizer_noreturn attribute) would address the issue without as much potential for abuse.

Can we agree on the following?
"It is orthogonal on the language level, but seems to be redundant in terms of the optimizer. Since LLVM IR's main purpose it support the optimizer, this is a strong argument against the general purpose attribute."

I'm making a more neutral point: that expect_noreturn conflates different concerns -- optimization and sanitizer correctness. I'm not making a claim about what the main purpose of IR is.

That would regress ASan coverage.

You talk specifically about cases of misuses of the attribute, right?
In the context of the current issue with UBSan the possibility for false negative is not too much of a regression: it only occurs when UBSan is going to diagnose an "unreachable error" anyways.

So the main point is whether or not to use a "general purpose" attribute or a "narrow purpose" attribute/intrinsic. My understanding is that you list the following points as arguments against general purpose. Is my understanding accurate?

1. Potential misuse can regress ASan coverage
2. Complicates optimizer

Narrow purpose: No potential misuses, and optimizer can simply ignore it.

Yes, I think this is a fair summary, thanks :).

Initially I proposed a narrow purpose attribute, but while iterating on this revision changed it to be general purpose. @eugenis
Now, I have a slight preference for general purpose: I don't think 1. is a big issue (but then again, I have no experience here),

Changes to the IR semantics have hard-to-predict ripple effects on many, many other projects. It pays to be conservative in this area.

and 2. it is always correct for the optimizer to continue ignoring the attribute (current status).
Actually, 2. also encompasses the potential upside: a more complicated optimizer that takes advantage of the attribute to do additional optimizations.

I'm having a hard time thinking of any optimizations based on expect_noreturn which aren't already enabled by the cold attribute. What do you have in mind?

[eugenis]

Because "expect_noreturn" calls are allowed to return, the compiler must behave as they could. In particular, this means that unpoisoning the stack before expect_noreturn calls (given the current semantics) is premature.

I don't think that's true. A hypothetical function

maybe_longjmp(jmp_buf env)

that checks an opaque condition needs stack unpoisoning before the call, in the absense of a better solution.

One possible optimization that I can think of is splitting code after the call into a separate basic block and marking it as cold.
Admittedly, that's unlikely to have big impact in practice. I'd guess that [[expect_noreturn]] calls are typically not very hot in the first place.

[vsk]

In D56624#1370243, @eugenis wrote:

Because "expect_noreturn" calls are allowed to return, the compiler must behave as they could. In particular, this means that unpoisoning the stack before expect_noreturn calls (given the current semantics) is premature.

I don't think that's true. A hypothetical function

maybe_longjmp(jmp_buf env)

that checks an opaque condition needs stack unpoisoning before the call, in the absense of a better solution.

Wouldn’t it be preferable to unpoison the stack inside of maybe_longjmp, once the opaque condition can be checked? Even if not, a narrower sanitizer_noreturn attribute is still perfectly fine, here.

One possible optimization that I can think of is splitting code after the call into a separate basic block and marking it as cold.
Admittedly, that's unlikely to have big impact in practice. I'd guess that [[expect_noreturn]] calls are typically not very hot in the first place.

The cold attribute is already used for this kind of splitting/reordering. I don't yet see how expect_noreturn creates new opportunities for the optimizer.

[eugenis]

Wouldn’t it be preferable to unpoison the stack inside of maybe_longjmp, once the opaque condition can be checked?

Sure, but that's not always possible. That's why we have interceptors.

One possible optimization that I can think of is splitting code after the call into a separate basic block and marking it as cold.
Admittedly, that's unlikely to have big impact in practice. I'd guess that [[expect_noreturn]] calls are typically not very hot in the first place.

The cold attribute is already used for this kind of splitting/reordering. I don't yet see how expect_noreturn creates new opportunities for the optimizer.

Strictly speaking, cold attribute on a function means that it is rarely called. It does not say anything about the code after the call being colder than the code before the call (within the same BB), which makes splitting the BB pointless.

Anyway, I agree that the arguments [[expect_noreturn]] are not that strong and perhaps don't make the bar for the addition of a new IR attribute.
Should we go back to the intrinsic idea?

[vsk]

In D56624#1370280, @eugenis wrote:

Wouldn’t it be preferable to unpoison the stack inside of maybe_longjmp, once the opaque condition can be checked?

Sure, but that's not always possible. That's why we have interceptors.

Fair enough!

One possible optimization that I can think of is splitting code after the call into a separate basic block and marking it as cold.
Admittedly, that's unlikely to have big impact in practice. I'd guess that [[expect_noreturn]] calls are typically not very hot in the first place.

The cold attribute is already used for this kind of splitting/reordering. I don't yet see how expect_noreturn creates new opportunities for the optimizer.

Strictly speaking, cold attribute on a function means that it is rarely called. It does not say anything about the code after the call being colder than the code before the call (within the same BB), which makes splitting the BB pointless.

That's true, but it's safe to assume that code which dominates the cold call (or is post-dominated by it) is at least as cold as the call.

Anyway, I agree that the arguments [[expect_noreturn]] are not that strong and perhaps don't make the bar for the addition of a new IR attribute.
Should we go back to the intrinsic idea?

Sgtm, I think that'd be the simplest solution (something like inserting llvm.asan.stack_unpoison() where needed).

[yln]

Seems as if we reached consensus! :) I will change the revision to use an intrinsic.

Before I start doing that, just one more quick idea:
Would it work if UBsan directly inserts calls to __asan_handle_no_return (of course only when ASan is requested). Similar to how it inserts calls to it's own runtime functions (e.g., __ubsan_handle_builtin_unreachable).
If we strive for the "simplest" solution... but maybe I am missing something in this is too simple?

[eugenis]

Maybe the frontend should insert __asan_handle_noreturn whenever ASan is enabled, and then ASan would not care about the attribute? I'd like to avoid having this logic in two places.

[yln]

In D56624#1370579, @eugenis wrote:

Maybe the frontend should insert __asan_handle_noreturn whenever ASan is enabled, and then ASan would not care about the attribute? I'd like to avoid having this logic in two places.

+1 for this. @vsk Can you sign off on this design?

[vsk]

In D56624#1370607, @yln wrote:

In D56624#1370579, @eugenis wrote:

Maybe the frontend should insert __asan_handle_noreturn whenever ASan is enabled, and then ASan would not care about the attribute? I'd like to avoid having this logic in two places.

+1 for this. @vsk Can you sign off on this design?

Sounds good to me.

[yln]

Created new revision for this change: https://reviews.llvm.org/D57278

clang/lib/CodeGen/CGCall.cpp

#endif
 
   // If the call doesn't return, finish the basic block and clear the
   // insertion point; this allows the rest of IRGen to discard
   // unreachable code.
   if (CS.doesNotReturn()) {
     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);
     Builder.ClearInsertionPoint();
 
     // FIXME: For now, emit a dummy basic block because expr emitters in
     // generally are not ready to handle emitting expressions at unreachable
     // points.

clang/test/CodeGenCXX/ubsan-unreachable.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
 
 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
     // CHECK: unreachable
   }
 
   // Test calls through pointers to non-static member functions.
   typedef void __attribute__((noreturn)) (A::*MemFn)();
 
   // 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
     // CHECK: unreachable
 
     // CHECK: call void @__ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 };
 
-// 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;
   a.call1();
   a.call2();
   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 }

compiler-rt/test/ubsan/TestCases/Misc/unreachable_asan-compatibility.c

+// Ensure compatiblity of UBSan unreachable with ASan in the presence of
+// noreturn functions
+// RUN: %clang -O2 -fsanitize=address,unreachable %s -emit-llvm -S -o - | FileCheck %s
+// REQUIRES: ubsan-asan
+
+void bar(void) __attribute__((noreturn));
+
+void foo() {
+  bar();
+}
+// CHECK-LABEL: define void @foo()
+// CHECK:       call void @__asan_handle_no_return
+// CHECK-NEXT:  call void @bar
+// CHECK-NEXT:  call void @__asan_handle_no_return
+// CHECK-NEXT:  call void @__ubsan_handle_builtin_unreachable
+// CHECK-NEXT:  unreachable

llvm/docs/LangRef.rst

 ``indirect-tls-seg-refs``
     This attribute indicates that the code generator should not use
     direct TLS access through segment registers, even if the
     target-specific ABI normally permits it.
 ``noreturn``
     This function attribute indicates that the function never returns
     normally. This produces undefined behavior at runtime if the
     function ever does dynamically return.
+``expect_noreturn``
+    This function attribute indicates that the function is unlikely to return
+    normally, but that it still allowed to do so. This is useful in cases where
+    ``noreturn`` is too strong a guarantee.
 ``norecurse``
     This function attribute indicates that the function does not call itself
     either directly or indirectly down any possible call path. This produces
     undefined behavior at runtime if the function ever does recurse.
 ``nounwind``
     This function attribute indicates that the function never raises an
     exception. If the function does raise an exception, its runtime
     behavior is undefined. However, functions marked nounwind may still

llvm/include/llvm/Bitcode/LLVMBitCodes.h

enum AttributeKindCodes {
   ATTR_KIND_WRITEONLY = 52,
   ATTR_KIND_SPECULATABLE = 53,
   ATTR_KIND_STRICT_FP = 54,
   ATTR_KIND_SANITIZE_HWADDRESS = 55,
   ATTR_KIND_NOCF_CHECK = 56,
   ATTR_KIND_OPT_FOR_FUZZING = 57,
   ATTR_KIND_SHADOWCALLSTACK = 58,
   ATTR_KIND_SPECULATIVE_LOAD_HARDENING = 59,
+  ATTR_KIND_EXPECT_NO_RETURN = 60,
 };
 
 enum ComdatSelectionKindCodes {
   COMDAT_SELECTION_KIND_ANY = 1,
   COMDAT_SELECTION_KIND_EXACT_MATCH = 2,
   COMDAT_SELECTION_KIND_LARGEST = 3,
   COMDAT_SELECTION_KIND_NO_DUPLICATES = 4,
   COMDAT_SELECTION_KIND_SAME_SIZE = 5,

llvm/include/llvm/IR/Attributes.td

 def NoRecurse : EnumAttr<"norecurse">;
 
 /// Disable redzone.
 def NoRedZone : EnumAttr<"noredzone">;
 
 /// Mark the function as not returning.
 def NoReturn : EnumAttr<"noreturn">;
 
+/// Mark the function as unlikely to return. This is useful in cases where
+/// `noreturn` is too strong a guarantee.
+def ExpectNoReturn : EnumAttr<"expect_noreturn">;
+
 /// Disable Indirect Branch Tracking.
 def NoCfCheck : EnumAttr<"nocf_check">;
 
 /// Function doesn't unwind stack.
 def NoUnwind : EnumAttr<"nounwind">;
 
 /// Select optimizations for best fuzzing signal.
 def OptForFuzzing : EnumAttr<"optforfuzzing">;

llvm/lib/AsmParser/LLLexer.cpp

#define KEYWORD(STR)                                                           \
   KEYWORD(noduplicate);
   KEYWORD(noimplicitfloat);
   KEYWORD(noinline);
   KEYWORD(norecurse);
   KEYWORD(nonlazybind);
   KEYWORD(nonnull);
   KEYWORD(noredzone);
   KEYWORD(noreturn);
+  KEYWORD(expect_noreturn);
   KEYWORD(nocf_check);
   KEYWORD(nounwind);
   KEYWORD(optforfuzzing);
   KEYWORD(optnone);
   KEYWORD(optsize);
   KEYWORD(readnone);
   KEYWORD(readonly);
   KEYWORD(returned);

llvm/lib/AsmParser/LLParser.cpp

while (true) {
     case lltok::kw_nobuiltin: B.addAttribute(Attribute::NoBuiltin); break;
     case lltok::kw_noduplicate: B.addAttribute(Attribute::NoDuplicate); break;
     case lltok::kw_noimplicitfloat:
       B.addAttribute(Attribute::NoImplicitFloat); break;
     case lltok::kw_noinline: B.addAttribute(Attribute::NoInline); break;
     case lltok::kw_nonlazybind: B.addAttribute(Attribute::NonLazyBind); break;
     case lltok::kw_noredzone: B.addAttribute(Attribute::NoRedZone); break;
     case lltok::kw_noreturn: B.addAttribute(Attribute::NoReturn); break;
+    case lltok::kw_expect_noreturn:
+      B.addAttribute(Attribute::ExpectNoReturn); break;
     case lltok::kw_nocf_check: B.addAttribute(Attribute::NoCfCheck); break;
     case lltok::kw_norecurse: B.addAttribute(Attribute::NoRecurse); break;
     case lltok::kw_nounwind: B.addAttribute(Attribute::NoUnwind); break;
     case lltok::kw_optforfuzzing:
       B.addAttribute(Attribute::OptForFuzzing); break;
     case lltok::kw_optnone: B.addAttribute(Attribute::OptimizeNone); break;
     case lltok::kw_optsize: B.addAttribute(Attribute::OptimizeForSize); break;
     case lltok::kw_readnone: B.addAttribute(Attribute::ReadNone); break;
while (true) {
     case lltok::kw_naked:
     case lltok::kw_nobuiltin:
     case lltok::kw_noduplicate:
     case lltok::kw_noimplicitfloat:
     case lltok::kw_noinline:
     case lltok::kw_nonlazybind:
     case lltok::kw_noredzone:
     case lltok::kw_noreturn:
+    case lltok::kw_expect_noreturn:
     case lltok::kw_nocf_check:
     case lltok::kw_nounwind:
     case lltok::kw_optforfuzzing:
     case lltok::kw_optnone:
     case lltok::kw_optsize:
     case lltok::kw_returns_twice:
     case lltok::kw_sanitize_address:
     case lltok::kw_sanitize_hwaddress:
while (true) {
     case lltok::kw_naked:
     case lltok::kw_nobuiltin:
     case lltok::kw_noduplicate:
     case lltok::kw_noimplicitfloat:
     case lltok::kw_noinline:
     case lltok::kw_nonlazybind:
     case lltok::kw_noredzone:
     case lltok::kw_noreturn:
+    case lltok::kw_expect_noreturn:
     case lltok::kw_nocf_check:
     case lltok::kw_nounwind:
     case lltok::kw_optforfuzzing:
     case lltok::kw_optnone:
     case lltok::kw_optsize:
     case lltok::kw_returns_twice:
     case lltok::kw_sanitize_address:
     case lltok::kw_sanitize_hwaddress:

llvm/lib/AsmParser/LLToken.h

enum Kind {
   kw_noduplicate,
   kw_noimplicitfloat,
   kw_noinline,
   kw_norecurse,
   kw_nonlazybind,
   kw_nonnull,
   kw_noredzone,
   kw_noreturn,
+  kw_expect_noreturn,
   kw_nocf_check,
   kw_nounwind,
   kw_optforfuzzing,
   kw_optnone,
   kw_optsize,
   kw_readnone,
   kw_readonly,
   kw_returned,

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

static uint64_t getRawAttributeMask(Attribute::AttrKind Val) {
   case Attribute::SwiftError:      return 1ULL << 52;
   case Attribute::WriteOnly:       return 1ULL << 53;
   case Attribute::Speculatable:    return 1ULL << 54;
   case Attribute::StrictFP:        return 1ULL << 55;
   case Attribute::SanitizeHWAddress: return 1ULL << 56;
   case Attribute::NoCfCheck:       return 1ULL << 57;
   case Attribute::OptForFuzzing:   return 1ULL << 58;
   case Attribute::ShadowCallStack: return 1ULL << 59;
-  case Attribute::SpeculativeLoadHardening:
-    return 1ULL << 60;
+  case Attribute::SpeculativeLoadHardening: return 1ULL << 60;
+  case Attribute::ExpectNoReturn:  return 1ULL << 61;
   case Attribute::Dereferenceable:
     llvm_unreachable("dereferenceable attribute not supported in raw format");
     break;
   case Attribute::DereferenceableOrNull:
     llvm_unreachable("dereferenceable_or_null attribute not supported in raw "
                      "format");
     break;
   case Attribute::ArgMemOnly:
static Attribute::AttrKind getAttrFromCode(uint64_t Code) {
   case bitc::ATTR_KIND_DEREFERENCEABLE_OR_NULL:
     return Attribute::DereferenceableOrNull;
   case bitc::ATTR_KIND_ALLOC_SIZE:
     return Attribute::AllocSize;
   case bitc::ATTR_KIND_NO_RED_ZONE:
     return Attribute::NoRedZone;
   case bitc::ATTR_KIND_NO_RETURN:
     return Attribute::NoReturn;
+  case bitc::ATTR_KIND_EXPECT_NO_RETURN:
+    return Attribute::ExpectNoReturn;
   case bitc::ATTR_KIND_NOCF_CHECK:
     return Attribute::NoCfCheck;
   case bitc::ATTR_KIND_NO_UNWIND:
     return Attribute::NoUnwind;
   case bitc::ATTR_KIND_OPT_FOR_FUZZING:
     return Attribute::OptForFuzzing;
   case bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE:
     return Attribute::OptimizeForSize;

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp

static uint64_t getAttrKindEncoding(Attribute::AttrKind Kind) {
   case Attribute::Dereferenceable:
     return bitc::ATTR_KIND_DEREFERENCEABLE;
   case Attribute::DereferenceableOrNull:
     return bitc::ATTR_KIND_DEREFERENCEABLE_OR_NULL;
   case Attribute::NoRedZone:
     return bitc::ATTR_KIND_NO_RED_ZONE;
   case Attribute::NoReturn:
     return bitc::ATTR_KIND_NO_RETURN;
+  case Attribute::ExpectNoReturn:
+    return bitc::ATTR_KIND_EXPECT_NO_RETURN;
   case Attribute::NoCfCheck:
     return bitc::ATTR_KIND_NOCF_CHECK;
   case Attribute::NoUnwind:
     return bitc::ATTR_KIND_NO_UNWIND;
   case Attribute::OptForFuzzing:
     return bitc::ATTR_KIND_OPT_FOR_FUZZING;
   case Attribute::OptimizeForSize:
     return bitc::ATTR_KIND_OPTIMIZE_FOR_SIZE;

llvm/lib/IR/Attributes.cpp

std::string Attribute::getAsString(bool InAttrGrp) const {
   if (hasAttribute(Attribute::NonLazyBind))
     return "nonlazybind";
   if (hasAttribute(Attribute::NonNull))
     return "nonnull";
   if (hasAttribute(Attribute::NoRedZone))
     return "noredzone";
   if (hasAttribute(Attribute::NoReturn))
     return "noreturn";
+  if (hasAttribute(Attribute::ExpectNoReturn))
+    return "expect_noreturn";
   if (hasAttribute(Attribute::NoCfCheck))
     return "nocf_check";
   if (hasAttribute(Attribute::NoRecurse))
     return "norecurse";
   if (hasAttribute(Attribute::NoUnwind))
     return "nounwind";
   if (hasAttribute(Attribute::OptForFuzzing))
     return "optforfuzzing";

llvm/lib/IR/Verifier.cpp

void Verifier::visitModuleFlagCGProfileEntry(const MDOperand &MDO) {
   Assert(Count && Count->getType()->isIntegerTy(),
          "expected an integer constant", Node->getOperand(2));
 }
 
 /// Return true if this attribute kind only applies to functions.
 static bool isFuncOnlyAttr(Attribute::AttrKind Kind) {
   switch (Kind) {
   case Attribute::NoReturn:
+  case Attribute::ExpectNoReturn:
   case Attribute::NoCfCheck:
   case Attribute::NoUnwind:
   case Attribute::NoInline:
   case Attribute::AlwaysInline:
   case Attribute::OptimizeForSize:
   case Attribute::StackProtect:
   case Attribute::StackProtectReq:
   case Attribute::StackProtectStrong:

llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp

return StringSwitch<Attribute::AttrKind>(Kind)
       .Case("naked", Attribute::Naked)
       .Case("nobuiltin", Attribute::NoBuiltin)
       .Case("noduplicate", Attribute::NoDuplicate)
       .Case("noimplicitfloat", Attribute::NoImplicitFloat)
       .Case("noinline", Attribute::NoInline)
       .Case("nonlazybind", Attribute::NonLazyBind)
       .Case("noredzone", Attribute::NoRedZone)
       .Case("noreturn", Attribute::NoReturn)
+      .Case("expect_noreturn", Attribute::ExpectNoReturn)
       .Case("nocf_check", Attribute::NoCfCheck)
       .Case("norecurse", Attribute::NoRecurse)
       .Case("nounwind", Attribute::NoUnwind)
       .Case("optforfuzzing", Attribute::OptForFuzzing)
       .Case("optnone", Attribute::OptimizeNone)
       .Case("optsize", Attribute::OptimizeForSize)
       .Case("readnone", Attribute::ReadNone)
       .Case("readonly", Attribute::ReadOnly)

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.hasFnAttr(Attribute::ExpectNoReturn))
+            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;

llvm/lib/Transforms/Utils/CodeExtractor.cpp

if (Attr.isStringAttribute()) {
       case Attribute::InaccessibleMemOrArgMemOnly:
       case Attribute::JumpTable:
       case Attribute::Naked:
       case Attribute::Nest:
       case Attribute::NoAlias:
       case Attribute::NoBuiltin:
       case Attribute::NoCapture:
       case Attribute::NoReturn:
+      case Attribute::ExpectNoReturn:
       case Attribute::None:
       case Attribute::NonNull:
       case Attribute::ReadNone:
       case Attribute::ReadOnly:
       case Attribute::Returned:
       case Attribute::ReturnsTwice:
       case Attribute::SExt:
       case Attribute::Speculatable:

llvm/test/Bitcode/attributes.ll

 }
 
 declare void @nobuiltin()
 
 define void @f34()
 ; CHECK: define void @f34()
 {
         call void @nobuiltin() nobuiltin
-; CHECK: call void @nobuiltin() #36
+; CHECK: call void @nobuiltin() #37
         ret void;
 }
 
 define void @f35() optnone noinline
 ; CHECK: define void @f35() #23
 {
         ret void;
 }

 }
 
 ; CHECK: define void @f59() #35
 define void @f59() shadowcallstack
 {
   ret void
 }
 
+; CHECK: define void @f60() #36
+define void @f60() expect_noreturn
+{
+  ret void
+}
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { readnone }
 ; CHECK: attributes #3 = { readonly }
 ; CHECK: attributes #4 = { noinline }
 ; CHECK: attributes #5 = { alwaysinline }
 ; CHECK: attributes #6 = { optsize }
 ; CHECK: attributes #7 = { ssp }

 ; CHECK: attributes #28 = { inaccessiblememonly }
 ; CHECK: attributes #29 = { inaccessiblemem_or_argmemonly }
 ; CHECK: attributes #30 = { allocsize(0) }
 ; CHECK: attributes #31 = { allocsize(0,1) }
 ; CHECK: attributes #32 = { writeonly }
 ; CHECK: attributes #33 = { speculatable }
 ; CHECK: attributes #34 = { sanitize_hwaddress }
 ; CHECK: attributes #35 = { shadowcallstack }
-; CHECK: attributes #36 = { nobuiltin }
+; CHECK: attributes #36 = { expect_noreturn }
+; CHECK: attributes #37 = { nobuiltin }

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.
 
 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 calls to noreturn functions (regardless of callsite)
+define i32 @Call1() sanitize_address {
+  call void @NoReturnFunc()
   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-LABEL:  @Call1
 ; CHECK:        call void @__asan_handle_no_return
-; CHECK-NEXT:   call void @MyNoReturnFunc
-; CHECK-NEXT: unreachable
+; CHECK-NEXT:   call void @NoReturnFunc
+
+; Instrument noreturn call sites (regardless of function)
+define i32 @Call2() sanitize_address {
+  call void @NormalFunc() noreturn
   unreachable
 }
+; CHECK-LABEL:  @Call2
+; CHECK:        call void @__asan_handle_no_return
+; CHECK-NEXT:   call void @NormalFunc
+
+; Also instrument expect_noreturn call sites
+define i32 @Call3() sanitize_address {
+  call void @NormalFunc() expect_noreturn
+  ret i32 0
+}
+; CHECK-LABEL:  @Call3
+; CHECK:        call void @__asan_handle_no_return
+; CHECK-NEXT:   call void @NormalFunc
 
 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(i8** %esc) 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