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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
