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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

In D56624#1369940 <https://reviews.llvm.org/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 <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?


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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56624