Abort in panic_abort eh_personality

[bjorn3]

This ensures that it is impossible to unwind through rust code when there is any -Cpanic=abort rust code, even once extern "C-unwind" is implemented and used in -Cpanic=unwind code.

See https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/next.20steps/near/244594484

Blocked on #92845

[rust-highfive]

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

[dtolnay]

😳 This is not code that I am interested in ramping up on. Maybe r? @cuviper?

[RalfJung]

This ensures that it is impossible to unwind through rust code when there is any -Cpanic=abort rust code, even once extern "C-unwind" is implemented and used in -Cpanic=unwind code.

How is that possible? How does this affect what happens when C++ throws an exception and it bubbles into Rust via C-unwind?

[bjorn3]

Say you have

//- crate_a.rs
// rustflags: -Cpanic=unwind

extern "C-unwind" {
    fn may_throw_foreign_exception();
}

pub fn wrapper() {
    unsafe { may_throw_foreign_exception(); }
}

//- crate_b.rs
// rustflags: -Cpanic=abort
fn main() {
    a::wrapper();
}

If the call to wrapper doesn't have an additional abort on panic landing pad, the foreign exception would simply unwind into wrapper and then main. The later setting nounwind on the wrapper declaration. In addition cleanup in the panic=unwind crate never gets run due to the eh_personality saying that unwinding should continue. This PR instead causes the process to abort the moment the exception reaches rust code by aborting in the personality function. In this case it reaches wrapper.

[RalfJung]

This PR instead causes the process to abort the moment the exception reaches rust code by aborting in the personality function.

Yes that is exactly the magic I am asking about. :) I know nothing at all about "personality functions", so could you explain what is happening here (or provide some links to documents that properly explain this stuff)?

[bjorn3]

When using DWARF based unwinding (used on pretty much all systems except windows), the .eh_frame section contains instructions on how to unwind the stack. It is conceptually a table from instruction address to where every register of the caller stack frame is located. (in practice it is encoded in a more compact form and some registers like caller saved registers may be unrecoverable) This is enough information to get a backtrace. The .eh_frame section however doesn't contain information about which variables need to be dropped. To drop variables, the function contains additional "landing pads". The unwinder doesn't have any knowledge about this though. Instead the .eh_frame entry for a function contains a reference to a so called personality function. This function is called first to determine which frame catches the exception. And then once the actual unwinding happens, it is called again and given the chance to change the instruction pointer to a landing pad. At the end of the landing pad there is a tail call to _Unwind_Resume to let the unwinder know that it should continue unwinding or if it catches the exception, the landing pad just jumps to the regular code in the function. This PR changes the personality function for panic_abort to abort the process instead of tell the unwinder that there is nothing to cleanup and that it should continue unwinding.

https://itanium-cxx-abi.github.io/cxx-abi/abi-eh.html may be helpful.

[RalfJung]

Okay, so basically there's a "magic function pointer" that we can set for each stack frame that is called on all kinds of unwinding, and we can make that one abort if the function was compiled with panic=abort? That sounds nice, yes.

But you said this is not used on Windows. So the original unwinding problem remains on Windows then?

[bjorn3]

Correct. I will need to read into SEH (the unwinding system on windows) to see if there is a way to achieve something similar on windows.

[bjorn3]

https://docs.microsoft.com/en-us/windows/win32/debug/frame-based-exception-handling:

The filter expression of a frame-based exception handler is an expression that is evaluated by the system when an exception occurs within the guarded body. This evaluation results in one of the following actions by the system.
[...]
The filter expression can be a simple expression, or it can invoke a filter function that attempts to handle the exception. You can call the GetExceptionCode and GetExceptionInformation functions from within a filter expression to get information about the exception being filtered. GetExceptionCode returns a code that identifies the type of exception, and GetExceptionInformation returns a pointer to an EXCEPTION_POINTERS structure that contains pointers to CONTEXT and EXCEPTION_RECORD structures.

(emphasis mine)

Looks like SEH has an equivalent to the personality function for DWARF based unwinding. To get the same effect as for DWARF with this PR, it would be necessary to unconditionally use such a filter function though I think. This may be a small perf loss when unwinding. I think this would be acceptable as panicking should rarely happen anyway, so it isn't perf critical.

[alexcrichton]

My main concern about this strategy is indeed platform support, namely Windows support. As pointed out here this PR, as-is at least, does not support MSVC. I attempted oh-so-long-ago to use a custom personality on MSVC as well (similar to how rust_eh_personality is a custom one for other platforms). At the time IIRC it wasn't supported because LLVM would classify certain kinds of functions and their exception-capbilities based on the name of the personality function used. For whatever reason __CxxFrameHandler3 ended up matching the best, I think because C++ uses that.

Things may have change in LLVM over time, but supporting a custom personality in the same way that we support it today may require changes on LLVM's side of things otherwise that may not be trivial.

[bjorn3]

Would this PR be fine to merge as is given that it is strictly an improvement for systems using DWARF based unwinding and doesn't hurt SEH based unwinding? Even if a completely different implementation strategy is found, this will simply function as a fail-safe on most systems in case of a bug in rust and otherwise not do any harm.

[alexcrichton]

I'm not the reviewer of this PR, but AFAIK the main motivation is to remove a blocker from stabilizing C-unwind, and without a fix on Windows (or a known fix in the near future), then C-unwind continues to be blocked. As to whether this is worthwhile to land in its own right, I'll leave that up to reviewers.

[bjorn3]

At the time IIRC it wasn't supported because LLVM would classify certain kinds of functions and their exception-capbilities based on the name of the personality function used. For whatever reason __CxxFrameHandler3 ended up matching the best, I think because C++ uses that.

I think I know the solution. If the current crate is not a panic runtime, then if the current crate is panic=abort a local __CxxFrameHandler3 function is generated that aborts. If the current crate is panic=unwind, a local __CxxFrameHandler3 function is generated that calls __rust_CxxFrameHandler3. The panic_abort runtime then contains this function with an aborting implementation. The panic_unwind runtime contains this function, but forwards it to the real __CxxFrameHandler3. This way LLVM sees a __CxxFrameHandler3 function, but rust has control over it's implementation.

[alexcrichton]

I think that could work yeah. You'd want to test to ensure that it doesn't interfere with C++'s usaage of __CxxFrameHandler3 because C++ embeds should still be able to throw/catch exceptions, just so long as they don't cross the Rust boundary.

I also think you can probably eschew __rust_CxxFrameHandler3. If the compiler is already synthesizing a __CxxFrameHandler3 symbol then it could stick a llvm.trap() call in their itself.

[bjorn3]

I think that could work yeah. You'd want to test to ensure that it doesn't interfere with C++'s usaage of __CxxFrameHandler3 because C++ embeds should still be able to throw/catch exceptions, just so long as they don't cross the Rust boundary.

That is why I suggested making the generated __CxxFrameHandler3 local. That way it can't override the __CxxFrameHandler3 used outside of rust codegen units.

I also think you can probably eschew __rust_CxxFrameHandler3. If the compiler is already synthesizing a __CxxFrameHandler3 symbol then it could stick a llvm.trap() call in their itself.

If code is compiled with panic=unwind, an abort would not always be a correct implementation. If the final program is panic=unwind, it needs to forward to the real __CxxFrameHandler3, however if the final program is panic=abort, it would need to be an abort. The only way to do this would be to call into the panic runtime I think, which is exactly what the purpose of __rust_CxxFrameHandler3 is. In addition because rustc would generate a local __CxxFrameHandler3, it wouldn't be able to directly call into the real _CxxFrameHandler3. Only the panic runtime for which no rustc generated _CxxFrameHandler3 exists can call the real _CxxFrameHandler3.

[cuviper]

If the personality function is an unconditional abort, doesn't that prevent foreign functions from performing any cleanup? e.g. your may_throw_foreign_exception() may have some non-catching cleanup to do. I was expecting to see the search phase report that we have a handler, and then its cleanup phase handle it by aborting.

Or is the idea to make this effectively the same as an uncaught-exception's terminate()?

[bjorn3]

If the personality function is an unconditional abort, doesn't that prevent foreign functions from performing any cleanup?

The personality function only applies to rust functions. C++ functions will still be able to unwind. Only when the exception reaches a rust function will it abort per the RFC.

e.g. your may_throw_foreign_exception() may have some non-catching cleanup to do. I was expecting to see the search phase report that we have a handler, and then its cleanup phase handle it by aborting.

When a panic happens with panic=abort we don't run the cleanup for any crate compiled with panic=unwind either. It makes sense to me to extend this to foreign exceptions when they would unwind through rust code.

[alexcrichton]

Ah ok I think that can work then. It feels a little convoluted but so long as LLVM is doing name matching on personalities we seem kinda forced to do this. I think it would be good to confirm that the name matching still happens though and it would probably be good to consult with an MSVC expert before committing to this pattern, though. Naturally I think it would all want to be well tested to ensure it's behaving as we expect. I don't know enough about MSVC to say with certainty that it will behave exactly as described here.

[cuviper]

@alexcrichton

I'm not the reviewer of this PR,

Do you want to be? I was chosen somewhat arbitrarily, but I feel you better understand these details already.
(I especially don't know about the Windows or Emscripten parts.)

[bors]

☔ The latest upstream changes (presumably #90382) made this pull request unmergeable. Please resolve the merge conflicts.

[apiraino]

Discussed in T-compiler meeting on Zulip.

[danielframpton]

I wanted to come back here and write down my thoughts on where this issue is. Although the discussion here started around Windows support, I wanted to confirm that my concerns are Windows specific. If we have some code that would need cleanups (e.g., variables with destructors) but is compiled with panic=abort, and then calls a function that has a C-unwind with panic=unwind, would the second function end up referencing the personality function on Linux?

If it does then I agree the above solution will work. For Windows to be certain to catch the unwind at the point it enters Rust code it seems like we would need to either:

1. Remove the requirement to support mixing panic runtimes, such that the protection we provide at the call sites to external unwind functions is sufficient to prevent unwinding into Rust code in panic=abort. This feels like it might be possible in the future, with sufficient progress in cargo for supporting build-std in stable, but it doesn't feel likely in the short term.
2. Introduce a call in unwind code around external unwind functions that calls down to a new entrypoint in the panic runtime. I think this is feasible, but I am not sure of the implications of choices here, in particular if this is platform specific. Should this just be an undefined symbol (e.g., __rust_assert_unwind) or would this require something more (such as stabilizing a new part of the panic handler). Should we use it for all platforms as an alternative to the approach in this PR? I would be happy to look at prototyping that, but I would be interested to hear suggestions about the right way to do that, and what we think we need there in terms of stabilizing. If the long-term direction is 1 then I am a little concerned about investing too much in this.

[Amanieu]

I believe this PR is mostly superseded by #92845. You need a valid personality function even with panic=abort.

[bjorn3]

It partly is. #92845 doesn't implement logic to make the personality function abort when trying to unwind in case of panic=abort though.

[Amanieu]

That logic already exists since #86155. The MIR pass will generate an abort landing pad since it sees a potentially unwinding call (the "C-unwind" function) in a no-unwind function.

[bjorn3]

That only happens for crates compiled with -Cpanic=abort. This PR is to also cause it to abort for crates compiled with -Cpanic=unwind when any crate is compiled with -Cpanic=abort as required for sound mixing of the two.

[apiraino]

@bjorn3 checking the progress for this PR: anything specific you need to be reviewed since last comments (I'm trying to recap the current status). thanks :)

[bjorn3]

This doesn't work for windows yet. In addition once #92845 lands, I will need to change this PR.

[apiraino]

ok thanks for the update. I'll flip the switch and set it waiting-on-author so it reflects the current status (in general terms). When this will be ready for another round of review, it can go back to waiting-on-review

@rustbot author

[Dylan-DPC]

Marking this as blocked on #92845

[BatmanAoD]

Does #97235 completely supersede this change?

[bjorn3]

I don't think so. I suspect but haven't confirmed that the current implementation of panic_abort will cause foreign exceptions to unwind past rust code despite the landingpads. The current personality function in panic_abort says that unwinding should just continue. Either aborting in the panic_abort eh personality (as this PR does) or reusing panic_unwind's eh personality would fix this.

[bjorn3]

I don't believe this is necessary anymore given changes that happened since.

[RalfJung]

Which other changes? Would be good to leave a reference.

[bjorn3]

I don't quite recall...

[Amanieu]

I believe this is mainly #92845. Now the same personality function is used with both -C panic=unwind and -C panic=abort.

[RalfJung]

#92845 is listed as a prerequisite for this PR, not an alternative, though.

This was intended to fix #96926, right? The the alternative that happened instead is probably #97235.