nounwind declaration of _Unwind_Resume generates bad gcc_except_table

[smeenai]

See https://godbolt.org/z/19c3ohsKs. The generated gcc_except_table is missing the call site table entry for the end of the function:

GCC_except_table0:
.Lexception0:
        .byte   255                             // @LPStart Encoding = omit
        .byte   255                             // @TType Encoding = omit
        .byte   1                               // Call site Encoding = uleb128
        .uleb128 .Lcst_end0-.Lcst_begin0
.Lcst_begin0:
        .uleb128 .Ltmp0-.Lfunc_begin0           // >> Call Site 1 <<
        .uleb128 .Ltmp1-.Ltmp0                  //   Call between .Ltmp0 and .Ltmp1
        .uleb128 .Ltmp2-.Lfunc_begin0           //     jumps to .Ltmp2
        .byte   0                               //   On action: cleanup
.Lcst_end0:

As a result, any exception thrown through this function will end up terminating, because we'll enter the cleanup, resume unwinding, and then the personality function will be unable to find any call site table entry corresponding to the cleanup PC.

Removing the nounwind attribute from the declaration of either _ZN14has_destructorD1Ev or _Unwind_Resume produces a correct gcc_except_table:

GCC_except_table0:
.Lexception0:
        .byte   255                             // @LPStart Encoding = omit
        .byte   255                             // @TType Encoding = omit
        .byte   1                               // Call site Encoding = uleb128
        .uleb128 .Lcst_end0-.Lcst_begin0
.Lcst_begin0:
        .uleb128 .Ltmp0-.Lfunc_begin0           // >> Call Site 1 <<
        .uleb128 .Ltmp1-.Ltmp0                  //   Call between .Ltmp0 and .Ltmp1
        .uleb128 .Ltmp2-.Lfunc_begin0           //     jumps to .Ltmp2
        .byte   0                               //   On action: cleanup
        .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
.Lcst_end0:

This repros with any target using DWARF unwinding, not just Android AArch64.

I discovered this when building a combined libc++, libc++abi, and libunwind library with LTO, with libunwind providing the actual _Unwind_Resume definition; it resulted in various libc++ functions having bad gcc_except_tables, leading to exception handling issues. That's probably an unusual setup, but I'd still expect it to work.

[smeenai]

CCing some exception handling folks: @compnerd and @rnk

[smeenai]

I looked into this a bit more. My understanding of the cleanup sequence when using DWARF EH is:

• In phase 2 of unwinding, the unwinder steps to the function with the cleanup and calls its personality function, which installs a context for the cleanup landing pad.
• This landing pad cleans up and then calls _Unwind_Resume to resume unwinding.
• _Unwind_Resume resumes phase 2 of the unwind, and now the unwind cursor points to the instruction after the _Unwind_Resume call.
• The unwinder calls the personality function again, which is supposed to find the call site table entry covering the landing pad, which just tells it to resume unwinding.

(It seems kinda inefficient to have to call into the personality function twice for each function with a cleanup, but I imagine there's some other scenarios for which that's important?)

I believe the relevant code in LLVM is EHStreamer::computeCallSiteTable. It has logic to emit the call site table entry covering the end of the function if a potentially throwing instruction was seen. When all the functions inside the cleanup are marked as nounwind, this condition is no longer true, so we omit the call site table entry covering the end of the function.

I'm not sure if it conceptually makes sense for _Unwind_Resume to be marked nounwind or not. It doesn't technically raise an exception, but it'll also result in its call sites being iterated during an unwind (so they need an associated call site entry). In my case it's being marked nounwind by virtue of being defined in a C file. I can think of a few potential options here:

1. Teach LLVM to ignore any nounwind attributes seen for _Unwind_Resume.
2. Get EHStreamer::callToNoUnwindFunction to special-case _Unwind_Resume, which is just a more targeted version of 1.
3. Force emission of a call site table entry when you see a cleanup pad (something like https://reviews.llvm.org/D130828, though I haven't thought through that particular implementation at all).
4. Make the unwinder do an extra step inside _Unwind_Resume, to skip over the frame which called _Unwind_Resume. None of libgcc, nongnu libunwind, or LLVM libunwind do this though, so I'm assuming there's a good reason for that.

Thoughts on the best way forward here? Also CC @MaskRay, since I forgot in the last comment.

[jyknight]

As you suggested, this is only a bug if you try to LTO libunwind -- which I expect nobody else has tried before. In other cases, the nounwind on the _Unwind_Resume implementation won't be seen by the callers, so everything works out.

I believe it's indeed incorrect to mark _Unwind_Resume as nounwind, since it most definitely does unwind. But, I think the right fix here is to ensure that the runtime functions are built correctly, and don't get that attribute -- not to add any special cases.

And I think we can do that just by building the sources with -fexceptions. (I'll note that libgcc compiles its unwinder code with -fexceptions already, though I suspect that was done for some other reason than LTO). For the C files, that seems likely to be straightforward (add_c_flags(-fexceptions)) but in some build modes, the unwind APIs are written in C++, and enabling exceptions probably triggers the addition of exceptional destructors, which would be undesired. So that might need more work.

[rnk]

It seems to me like you're on the right track: _Unwind_Resume must not be marked nounwind, and surely it cannot be inlined.

[smeenai]

Thank you both for the comments!

-fexceptions doesn't appear to be enough to get the nounwind attribute removed: https://godbolt.org/z/KfqP8csKn. Not sure if there's an explicit way to force that.

For now I can work around this effectively enough by not LTOing libunwind (technically I only need to not LTO UnwindLevel1.c, but I don't know if there's other weird cases lurking here). I'm now a little worried about LTOing libc++ and libc++abi together as well, but I know that at least Android builds the two of those together into a single library (though I'm not sure if they LTO them), so hopefully I'm not in fully uncharted territory there.

[modiking]

(It seems kinda inefficient to have to call into the personality function twice for each function with a cleanup, but I imagine there's some other scenarios for which that's important?)

During clean-up, we're executing arbitrary user code. At the end of it, the user code has to pass back control if we want to continue with the runtime. Given the personality function can handle unwind starting at arbitrary places it makes sense to re-use the existing functionality.

I believe, based on how the MSVC model works, the reason we want the PC to go _Unwind_Resume is to track that we have successfully unwound the throw site (when PC was at original invoke) and are now going up the stack to process the next frame.

_Unwind_Resume should not be nounwind but it is a special case of it: it doesn't cause unwind because of an exception being thrown but because it directly calls into the runtime unwind. The current analysis looks for potential throw sites but because there are none (by design) it concludes that _Unwind_Resume is nounwind.

For the LTO scenario, I think we want to ensure _Unwind_Resume in libunwind is properly annotated rather than special casing it in the compiler. The trouble comes in that there's not an easy way to specify that a function is noexcept(false)/throwing compiled as a "C" file. Disabling LTO is a decent work-around because LLVM always goes conservative on native functions but obviously that extends to all optimizations which is not ideal.

technically I only need to not LTO UnwindLevel1.c

This sounds good enough for now. It's also possible disabling optimizations on _Unwind_Resume prevents propagation and suppresses the addition of nounwind in combination with -fexceptions.

[smeenai]

Yup, makes sense. Compiling with -fexceptions, as suggested by @jyknight, actually does work to remove nounwind; my previous Godbolt example was too trivial, but https://godbolt.org/z/ofP81G3b3 shows a function with calls not getting marked as nounwind. I haven't gotten the chance to test that with libunwind yet though.