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

[Julian Lettner via Phabricator via cfe-commits]

yln added a comment.

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 <https://reviews.llvm.org/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.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56624/new/

https://reviews.llvm.org/D56624