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

[John McCall via Phabricator via cfe-commits]

rjmccall added a subscriber: jyknight.
rjmccall added a comment.

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.  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).

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.



================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:2021
 
-  llvm::BasicBlock *getInvokeDest() {
+  llvm::BasicBlock *getInvokeDest(SourceLocation Loc = SourceLocation()) {
     if (!EHStack.requiresLandingPad()) return nullptr;
----------------
I'm not sure why this has a default argument.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:2028
+    // FIXME: what about FuncletPads?
+    if (llvm::BasicBlock *InvokeBB = Builder.GetInsertBlock();
+        SanOpts.has(SanitizerKind::ExceptionEscape) &&
----------------
The purpose of `getInvokeDestImpl` is to outline the slow path, and this definitely belongs in the slow path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137381