[cfe-dev] [Exception Handling] Could we mark __cxa_end_catch as nounwind conditionally?

[John McCall via cfe-dev]

On 30 Aug 2021, at 2:59, Fāng-ruì Sòng wrote:
> On 2021-08-30, chuanqi.xcq wrote:
>> Hi all,
>>
>>   Let me introduce about the background:
>>   I find that the compiler couldn't mark `foo` as `nounwind` in the 
>> following example:
>>
>> ```
>> void bar();
>> void foo() {
>>    try {
>>        bar();
>>    } catch(...) {}
>> }
>> ```
>>
>>   From my perspective, it is clear that foo wouldn't throw any 
>> exception. So it is natural to me that the compiler could mark foo as 
>> nounwind as an optimization. But it didn't.
>>   This pattern occurs frequently in C++20 coroutine. So I tried to 
>> handle coroutine specially before in: 
>> https://reviews.llvm.org/D108277.
>>   But the all the reviewers strongly suggested that we should handle 
>> this case generally for all of functions instead of coroutines only.
>>
>>   Then when I looked into the details in IR, I found the reason is 
>> that __cxa_end_catch isn't nounwind.
>>   Here is the IR generated:
>>
>> ```
>> ; Function Attrs: mustprogress uwtable
>> define dso_local void @_Z3foov() local_unnamed_addr #0 personality 
>> i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
>>  invoke void @_Z3barv()
>>          to label %5 unwind label %1
>>
>> 1:                                                ; preds = %0
>>  %2 = landingpad { i8*, i32 }
>>          catch i8* null
>>  %3 = extractvalue { i8*, i32 } %2, 0
>>  %4 = tail call i8* @__cxa_begin_catch(i8* %3) #2 ; nounwind
>>  tail call void @__cxa_end_catch()
>>  br label %5
>>
>> 5:                                                ; preds = %0, %1
>>  ret void
>> }
>> ```
>>
>>    I found that if I marked the call to __cxa_end_catch() as 
>> `nounwind`, the foo could be marked as `nounwind`. So I start to 
>> survey why __cxa_end_catch() isn't 'nounwind'.
>>    First is the comment on __cxa_end_catch() in libcxxabi:
>>
>> ```
>> Upon exit for any reason, a handler must call:
>>    void __cxa_end_catch ();
>>
>> This routine can be called for either a native or foreign exception.
>> For a native exception:
>> * Locates the most recently caught exception and decrements its 
>> handler count.
>> * Removes the exception from the caught exception stack, if the 
>> handler count goes to zero.
>> * If the handler count goes down to zero, and the exception was not 
>> re-thrown
>>  by throw, it locates the primary exception (which may be the same as 
>> the one
>>  it's handling) and decrements its reference count. If that reference 
>> count
>>  goes to zero, the function destroys the exception. In any case, if 
>> the current
>>  exception is a dependent exception, it destroys that.
>>
>> For a foreign exception:
>> * If it has been rethrown, there is nothing to do.
>> * Otherwise delete the exception and pop the catch stack to empty.
>> ```
>>
>>   I am not familiar with exception handling. But from the comment 
>> above, it looks like that __cxa_end_catch wouldn't throw.
>>   Then in  clang::ItaniumCXXABI, I found this:
>>
>> ```
>> A cleanup to call __cxa_end_catch.  In many cases, the caught
>> exception type lets us state definitively that the thrown exception
>> type does not have a destructor.  In particular:
>>  - Catch-alls tell us nothing, so we have to conservatively
>>    assume that the thrown exception might have a destructor.
>>  - Catches by reference behave according to their base types.
>>  - Catches of non-record types will only trigger for exceptions
>>    of non-record types, which never have destructors.
>>  - Catches of record types can trigger for arbitrary subclasses
>>    of the caught type, so we have to assume the actual thrown
>>    exception type might have a throwing destructor, even if the
>>    caught type's destructor is trivial or nothrow.
>> ```
>>
>>     It looks like that __cxa_end_catch would throw only if the 
>> exception caught has an destructor which may throw.
>
> Yes...
>
>>     But I think the situation is rare. First as the comment says, an 
>> exception type doesn't have a destructor usually.
>> Then if it has a destructor, it is also rare that it may throw. 
>> Finally, it is a bad practice to throw from destructor which occurs 
>> in catch block.
>>     So I want to provide an option to tell the compiler whether the 
>> exceptions in current project has a may-throw destructor. In this 
>> way, we could optimize the example in the beginning.
>
> From GCC produced .gcc_except_table, it seems that GCC unconditionally
> assumes that __cxa_begin_catch/__cxa_end_catch do not throw. GCC does
> not emit call site records for the region with __cxa_end_catch.
>
> So I think we should unconditionally assume that 
> __cxa_begin_catch/__cxa_end_catch don't throw as well.
> Sent https://reviews.llvm.org/D108905

That is not how this works.  Here’s the path we need to take.

First, as I mentioned in the review, we need to figure out what the 
language standard expects to happen when the destructor for an exception 
throws at the end of a `catch`.  Currently we are essentially treating 
this as undefined behavior, which I think is highly unlikely to be the 
intent of the standard.

If it’s allowed to unwind, then `__cxa_end_catch` can throw, and so 
the second step is to:
(1) fix the bugs in the language runtimes to handle this situation 
correctly and not leak the exception object, and then
(2) fix GCC to stop unconditionally assuming that `__cxa_end_catch` does 
not throw.
We could then add an opt-in flag to globally assume that exception 
destructors never throw as an optimization if that’s really important 
for getting good code.

If it’s supposed to call `std::terminate`, then we need to come to an 
agreement with the language runtimes about whose responsibility it is to 
ensure that the exception is caught and `std::terminate` is called.  
That means opening an issue with the Itanium C++ ABI.  If we decide 
it’s the compiler’s responsibility to pass a noexcept function as 
the exception destructor, then we need to fix Clang to wrap the 
destructor in a noexcept function when it’s not noexcept.  If we 
decide it’s the runtime’s responsibility, we need to fix the 
runtimes to catch this exception and call `std::terminate`.  In either 
of those casess, `__cxa_end_catch` is known not to throw.  We could also 
decide it’s the compiler’s responsibility to call `std::terminate` 
if `__cxa_end_catch` throws, and if so then we have to do that; however, 
I think that would be a bad outcome.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210830/3054dfaa/attachment.html>