[PATCH] D108905: [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: rsmith.
rjmccall added a comment.

Yes, this seems extremely reasonable; it was really just an oversight.  Patch is semantically fine, although what we do in several other places is use `EmitNounwindRuntimeCall` to emit the call, and I think I'd like to do that here: I'd like to eventually eliminate `EmitRuntimeCall` entirely in favor of expecting the client to be explicit about whether the runtime call is allowed to throw.

Although... actually, it occurs to me that there is a reason `__cxa_end_catch` *could* throw in the abstract, which is that the exception destructor could throw.  My first instinct is that this should call std::terminate, and if so, the runtime should handle that; but I don't see anything in either the standard or the Itanium ABI that directly supports that.

I'm not really sure what the standard expects to happen if an exception destructor throws.  The standard tells us when the destruction happens — the latest point it can — but that clause doesn't mention exceptions from the destructor.  If there's a specific clause on this, I can't find it.  [except.throw]p7 says that "if the exception handling mechanism handling an uncaught exception directly invokes a function that exits via an exception, the function std::terminate is called", which is meant to cover exceptions thrown when initializing a catch variable.  Once the catch clause is entered, though, the exception is considered caught unless it's rethrown, so this clause doesn't apply when destroying the exception at the end of the clause.  If the catch variable's destructor throws, that seems to be specified to unwind normally (including destroying the exception, and if the destructor throws at that point then std::terminate gets called, as normal for exceptions during unwinding).

Neither libc++abi nor libsupc++ seem to handle the possibility of the exception destructor throwing when destroying exceptions; they'll both happily leak the exception object if it does.  That's reasonable if the assumption is that the destructor function passed to `__cxa_throw` never throws, which is itself reasonable for the frontend to ensure if indeed the language rule is that `std::terminate` is supposed to be called.  Clang does not actually do that, though: it just passes the address of the destructor, which would only be legal if the destructor was `noexcept` (which is, of course, now the default).

So tentatively I think this is okay, but it looks like it's uncovered some bugs in our handling of throwing destructors for exceptions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108905