llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.53k stars 11.79k forks source link

[Itanium ABI] If we can make __cxa_end_catch calls nounwind conditionally #57375

Open ChuanqiXu9 opened 2 years ago

ChuanqiXu9 commented 2 years ago

This was discussed in https://reviews.llvm.org/D108905.

The simple background is:

void bar();
void foo() {
    try {
        bar();
    } catch(...) {}
}

From the perspective of programmers, foo() should be clearly noexcept (although this is not completely correct). And the root cause here is that __cxa_end_catch is not unwind.

In D108905 we want to make __cxa_end_catch calls nounwind unconditionally. But it gets blocked since the behavior is wrong in certain cases if the destroying exception can throw an exception: https://godbolt.org/z/PYnj85Pdq. Like the case shows, foo() in the above case is not 100% noexcept. So the current behavior is correct.

However, we know it is really rare that the destroying exception can throw and this problem is relatively important in C++ coroutines. Since C++ coroutines are nearly wrapped in a big try catch generated by compiler:

try {
   coroutine function body
} catch (...) {
   promise.unhandled_exception()
}
final_suspend: // final suspend is guaranteed to not throw

So if we can make __cxa_end_catch as nounwind, we can make many coroutines as noexcept automatically.

Given the above two reasons (the exceptional case is rare and it is helpful for a lot of functions), I think it makes sense to make __cxa_end_catch calls nounwind conditionally (e.g., by a compilation flag) at least.

llvmbot commented 2 years ago

@llvm/issue-subscribers-c-1

yuanfang-chen commented 2 years ago

@rjmccall

rjmccall commented 2 years ago

Conditional on what? Honestly, what you've written comes across as "it would be really convenient if this property were true, but it isn't, but maybe we can get our optimization potential back by just ignoring that".

I agree that it's really bad that catch (...) {} can throw, but I think you need to take it up with the C++ committee.

ChuanqiXu9 commented 2 years ago

Yeah, it is best to get a consensus with the C++ committee. But due to it may break existing codes, it may be a long time or not approved. What I imaged is to add a flag -fno-throw-destructor or -fno-throw-exception-destructor. So we can solve the problem and not violate the standard language.

mclow commented 2 years ago
try  { foo (); }
catch (...) {
    log("exception thrown by Foo");
    throw;
}
ChuanqiXu9 commented 2 years ago

try { foo (); } catch (...) { log("exception thrown by Foo"); throw; }

May I ask the intention?

mclow commented 2 years ago

To log the fact that foo exited via an exception, and then continue processing the exception elsewhere.

ChuanqiXu9 commented 2 years ago

To log the fact that foo exited via an exception, and then continue processing the exception elsewhere.

I still don't get your point. Do you mean https://godbolt.org/z/PYnj85Pdq?

rjmccall commented 2 years ago

Marshall, I think you're missing Chuanqi's point. Of course it's possible to throw an exception out of the body of a catch (...) clause, either by rethrowing the caught exception or just throwing a new exception. Chuanqi's complaint arises from the fact that merely exiting a catch (...) clause can throw because of the possibility that you've caught an exception type with a throwing destructor.

Chuanqi, the targeted flag is an interesting idea. We could also emit an error if you try to throw an exception with a throwing destructor, just to make the dialect as self-consistent as possible. (I think we'd want to allow catching such types with an explicit clause, to allow programmers to work around problems if they run into them because of separate compilation.)

ChuanqiXu9 commented 2 years ago

Thanks for the suggestion. I'll try to make it.

smeenai commented 12 months ago

We would appreciate such an option as well. I ran across this recently while looking into exception metadata size, and found it surprising given that C++11 make destructors implicitly noexcept. I've read through all the discussions and understand the rationale now, but an option to opt-in to this behavior would be useful while the standardization aspects are being sorted out.

smeenai commented 11 months ago

For anyone following along here, https://reviews.llvm.org/D108905 was updated to add an opt-in option for this.