Page MenuHomePhabricator

[ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind
Changes PlannedPublic

Authored by MaskRay on Aug 29 2021, 11:36 PM.

Details

Summary

See https://lists.llvm.org/pipermail/cfe-dev/2021-August/068740.html

A catch clause calls __cxa_begin_catch/__cxa_end_catch.
invoke void @__cxa_end_catch is used in some cases.
Switching to nounwind can avoid a landing pad for resume.

In the absence of out-of-scope variable destructors, e.g. `void test() { try {
can_throw(); } catch (...) {}}` this can at least avoid unneeded call site
records whose action record offset is 0 (cleanup). In addition, the
__clang_call_terminate definition may be possibly avoided.

-       .uleb128 .Ltmp1-.Lfunc_begin0           # >> Call Site 2 <<
-       .uleb128 .Lfunc_end0-.Ltmp1             #   Call between .Ltmp1 and .Lfunc_end0
-       .byte   0                               #     has no landing pad
-       .byte   0                               #   On action: cleanup

Note: libcxxrt/libsupc++/libc++abi __cxa_end_catch may call _Unwind_DeleteException (_Unwind_Exception::exception_cleanup) for a foreign exception
and __cxa_exception::exceptionDestructor for a native exception.
We can expect that the destructor/cleanup functions do not throw (the behavior would be unclear
if they throw). GCC assumes that __cxa_end_catch does not throw since 2000.

Diff Detail

Event Timeline

MaskRay requested review of this revision.Aug 29 2021, 11:36 PM
MaskRay created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2021, 11:36 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Aug 29 2021, 11:43 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay edited the summary of this revision. (Show Details)

Thanks for looking into this!
I am not familiar with exception handling. But from the comments, I worried that if it is a problem if there are exceptions whose destructor could throw (although it is not usual nor good).

MaskRay edited the summary of this revision. (Show Details)Aug 29 2021, 11:50 PM
MaskRay edited the summary of this revision. (Show Details)Aug 29 2021, 11:56 PM

Thanks for looking into this!
I am not familiar with exception handling. But from the comments, I worried that if it is a problem if there are exceptions whose destructor could throw (although it is not usual nor good).

GCC does not emit call site records for the __cxa_end_catch region, so I believe it makes the same assumption.
Setting nounwind should be safe...

MaskRay retitled this revision from [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch nounwind to [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch.Aug 30 2021, 12:00 AM
MaskRay edited the summary of this revision. (Show Details)
ChuanqiXu accepted this revision.Aug 30 2021, 12:01 AM

Thanks for looking into this!
I am not familiar with exception handling. But from the comments, I worried that if it is a problem if there are exceptions whose destructor could throw (although it is not usual nor good).

GCC does not emit call site records for the __cxa_end_catch region, so I believe it makes the same assumption.
Setting nounwind should be safe...

Make sense to me. Thanks for looking into this!

This revision is now accepted and ready to land.Aug 30 2021, 12:01 AM

I confirm that GCC has had this behavior since 2000 (3.0).

ChuanqiXu added inline comments.Aug 30 2021, 1:00 AM
clang/lib/CodeGen/ItaniumCXXABI.cpp
4418–4423

If __cxa_end_catch is nounwind always now, do we need to refactor this one? Like:

struct CallEndCatch final : EHScopeStack::Cleanup {
    CallEndCatch() {}

    void Emit(CodeGenFunction &CGF, Flags flags) override {
      CGF.EmitNounwindRuntimeCall(getEndCatchFn(CGF.CGM));
    }
  };

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.

MaskRay updated this revision to Diff 369467.Aug 30 2021, 9:39 AM
MaskRay retitled this revision from [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch to [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind.
MaskRay edited the summary of this revision. (Show Details)

set nounwind in call sites

Peanut gallery says: Does this godbolt demonstrate the situation where "destroying an exception can throw an exception"?
https://godbolt.org/z/Ghjz53rrj
You're not proposing to change the behavior of this program, are you?
(If you're proposing to change it: I don't think you should. If you're not proposing to change it, but rather the issue is something subtler: never mind me, then.)

Well, I'm saying two things. First, it is not clear to me what the expected behavior of that code is under the standard. The fact that it appears to work in one particular implementation is not in any way conclusive; we have to look at the specification. Second, I think it only appears to work: looking at the runtime code in both libc++abi and libsupc++, it leaks the memory of the exception object, which it's clearly not allowed to do. You should be able to fairly easy prove that by looking at heap usage if you do it in a loop.

Well, I'm saying two things. First, it is not clear to me what the expected behavior of that code is under the standard. The fact that it appears to work in one particular implementation is not in any way conclusive; we have to look at the specification.

Aha, I had thought it worked on "both Clang and GCC," but now I see that Godbolt uses libsupc++ for both Clang and GCC (and that's where the issue is). Switching to a branch that uses libc++abi, I see that libc++abi just calls std::terminate in this situation: https://godbolt.org/z/4s8aMvr3K

Second, I think it only appears to work: looking at the runtime code in both libc++abi and libsupc++, it leaks the memory of the exception object, which it's clearly not allowed to do. You should be able to fairly easy prove that by looking at heap usage if you do it in a loop.

Yep, memory leak confirmed. (Via the same Godbolt: https://godbolt.org/z/4s8aMvr3K ) So okay, never mind me.

MaskRay planned changes to this revision.Aug 30 2021, 11:23 AM

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.

Scanning through the standard this to me also looks like an overlooked corner case in the standard (TBF this is very corner case).

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

In this case, the destructor is throwing during normal scope exit so I don't think terminate behavior from [except.throw]p7 is enforced since we're not currently handling an uncaught exception. The handler is already active since we're past the initialization of the catch. Given that, I think this is akin to the example in [except.ctor]p2 and if the destructor is noexcept(false) should trigger a proper unwind like how an exception in the destructor of a simple automatic variable inside the handler scope would also do.

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.

Scanning through the standard this to me also looks like an overlooked corner case in the standard (TBF this is very corner case).

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

In this case, the destructor is throwing during normal scope exit so I don't think terminate behavior from [except.throw]p7 is enforced since we're not currently handling an uncaught exception. The handler is already active since we're past the initialization of the catch. Given that, I think this is akin to the example in [except.ctor]p2 and if the destructor is noexcept(false) should trigger a proper unwind like how an exception in the destructor of a simple automatic variable inside the handler scope would also do.

Yeah, I think this is the most natural interpretation of the current standard. But that would be a very unfortunate rule, because people who write catch (...) {} do reasonably expect that that code will never throw. In fact, it would be quite difficult — perhaps impossible — to write code that actually swallowed all exceptions: even try { try { foo() } catch(...) {} } catch (...) {} wouldn't work, because you could throw an exception whose destructor throws an exception whose destructor throws an exception ad infinitum.

Yeah, I think this is the most natural interpretation of the current standard. But that would be a very unfortunate rule, because people who write catch (...) {} do reasonably expect that that code will never throw. In fact, it would be quite difficult — perhaps impossible — to write code that actually swallowed all exceptions: even try { try { foo() } catch(...) {} } catch (...) {} wouldn't work, because you could throw an exception whose destructor throws an exception whose destructor throws an exception ad infinitum.

Yeah it's not great and also something that practically will never happen. I think terminate guards are the only thing that really swallows all exceptions except here you can't guard the catch variable destructor unless you want to change up and depend on library implementation. My immediate thought is something like catch(...) noexcept {} to express this but it's a solution to a problem that really shouldn't exist.

Yeah, I think this is the most natural interpretation of the current standard. But that would be a very unfortunate rule, because people who write catch (...) {} do reasonably expect that that code will never throw. In fact, it would be quite difficult — perhaps impossible — to write code that actually swallowed all exceptions: even try { try { foo() } catch(...) {} } catch (...) {} wouldn't work, because you could throw an exception whose destructor throws an exception whose destructor throws an exception ad infinitum.

Yeah it's not great and also something that practically will never happen. I think terminate guards are the only thing that really swallows all exceptions except here you can't guard the catch variable destructor unless you want to change up and depend on library implementation. My immediate thought is something like catch(...) noexcept {} to express this but it's a solution to a problem that really shouldn't exist.

Well, I think catch (...) { std::terminate(); } should work to express that outer catch, if only because the end of the catch is never reached. But yeah, I agree that this is clearly a problem that shouldn't exist.

We might be able to require exception types to have noexcept destructors. Formally, it would have source compatibility / language-version incompatibility implications, but in practice I doubt it would cause many problems, especially if it was just a "if this actually throws it'll have UB" warning in pre-C++23 language modes for throwing types with explicitly noexcept destructors.

I've forwarded the question here onto CWG, with Arthur's example and the suggestion that maybe we shouldn't allow throwing an exception with a non-noexcept destructor.

Thanks, Richard. Fangrui, I think we can't do anything on this patch without a CWG decision about the right semantics, so this will have to sit.

No decision as yet, but so far it looks very likely that we'll settle on the rule that exceptions cannot have potentially-throwing destructors, and that we should reject throws of such types. I don't think that should be applied retroactively to C++98, though, because destructors were not implicitly non-throwing back then.

No decision as yet, but so far it looks very likely that we'll settle on the rule that exceptions cannot have potentially-throwing destructors, and that we should reject throws of such types. I don't think that should be applied retroactively to C++98, though, because destructors were not implicitly non-throwing back then.

Is the committee comfortable with implementations causing potentially-throwing exception destructors to trigger std::terminate? I understand that this is a weird question because it implies the use of / interoperation with an old language standard, but we do need to know how to compile in C++98 mode, and we may need to demote this to a warning pre-C++23. If it's not an error in old modes, but the committee doesn't approve of calling std::terminate if it happens, then we still need to compile catch (...) as throwing in case we're interoperating.

No decision as yet, but so far it looks very likely that we'll settle on the rule that exceptions cannot have potentially-throwing destructors, and that we should reject throws of such types. I don't think that should be applied retroactively to C++98, though, because destructors were not implicitly non-throwing back then.

Is the committee comfortable with implementations causing potentially-throwing exception destructors to trigger std::terminate? I understand that this is a weird question because it implies the use of / interoperation with an old language standard, but we do need to know how to compile in C++98 mode, and we may need to demote this to a warning pre-C++23. If it's not an error in old modes, but the committee doesn't approve of calling std::terminate if it happens, then we still need to compile catch (...) as throwing in case we're interoperating.

@rjmccall @rsmith Do you think it makes sense to make __cxa_end_catch nounwind conditionally? I mean, in higher standard than C++98, we could make __cxa_end_catch nounwind.