[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

Tue Nov 15 08:51:24 PST 2022

lebedev.ri added a comment.

@vitalybuka @rjmccall thank you for taking a look!
Since posting this, there has been a conversation in `#llvm` with @jyknight,
where it was established that the situation is even worse than i though.
Essentially, there are two situations:

1. exception escape out of non-unwinding function is guaranteed to cause program termination. That happens for:
  - the C++ `noexcept`
  - `__attribute__((nothrow))` (for some non-obvious reason)
  - OpenMP region lowering (even though it's not mandated by the standard)
2. exception escape out of non-unwinding function is wild UB, this is the semantics of
  - `__attribute__((pure))`/`__attribute__((const))`.

It is possible that `__attribute__((nothrow))` should also be in UB category,
otherwise what benefit does it bring over C++'s `noexcept`?

So, i have correctly felt that there is a problem, but the fix is going in the wrong way.
While i still think that we should handle case 1. in UBSan, let's first deal with 2.

In D137381#3926121 <https://reviews.llvm.org/D137381#3926121>, @rjmccall wrote:

> I don't have a problem with adding a sanitizer like this.  IIRC, though, there's a review process for adding new sanitizers; I don't remember if this is the normal RFC process or something more streamlined.

For the record, i have added the entirety of `-fsanitize=implicit-conversion` (part of `-fsanitize=integer`)
and `-fsanitize=alignment`, and this is the first time i'm hearing of an RFC process.

> Also, I know @jyknight is interested in a mode that checks `-fno-exceptions` more efficiently, which is of direct relevance to what you're trying here; that was discussed in a forums thread (https://discourse.llvm.org/t/rfc-add-call-unwindabort-to-llvm-ir/62543).

Yes, we've talked in `#llvm` about that a bit.

> The implementation and tests here don't seem to match the stated problem, though.  Trying to throw out of a `noexcept` function is actually well-defined — the function has to catch the exception and call `std::terminate`, and we do actually generate code that way.  Your patch is just adding a check at the start of the handler we emit, which means it's not catching anything we're not already catching; also, again, this is a well-defined execution path, so it doesn't seem appropriate to trigger UBSan on it.
>
> IIRC, there are really only two potential sources of UB with `noexcept`.  The first is that you can declare a function `noexcept(true)` but implement it as `noexcept(false)`, which is in a class of problems (ODR violations) that I think UBSan doesn't normally try to diagnose.  The second is that I think it's possible to get a `noexcept(true)` function pointer for a `noexcept(false)` function, maybe via some chain of casts; that would be a little closer to what UBSan normally diagnoses.
>
> Meanwhile, your original test case has nothing to do with `noexcept`. `__attribute__((pure))` (like `nothrow` and `-fno-exceptions`) really does introduce a novel UB rule which it would be reasonable to ask the compiler to check under UBSan.
>
> I think the right way to handle what you're trying to handle would be to introduce some different kind of scope, a "it's UB to unwind out of this" scope, and then push that scope around calls and implementations of functions that use one of the UB-to-throw attributes.  You could also potentially push that scope around calls to `nounwind` functions if you want to catch those ODR / function-pointer cases.

Yes. Somehow i did not think that those attributes introduce UB, and not are just handled incorrectly,
and tried to solve this the wrong way around. That being said, this diff is not incorrect.
I would like to have a sanitizer for the "exception escape is guaranteed to cause program termination" mode,
because all of the bugs with that mode i had to deal with had rather bad diagnostic,
but that can be done afterwards.

Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381