CAD97 commented 3 weeks ago

Proposal

Problem statement

Various use cases exist that can't or don't want to support being unwound through. Today, there are multiple ways of coverting an in-progress unwind into an abort, each with subtly different user experience when the abort occurs. By providing a function abort_unwind as part of core, we can standardize the experience.

Motivating examples or use cases

Perhaps the most notorious case of unwind-abort is the take_mut/replace_with pattern. The take_mut crate uses catch_unwind to respond to unwinds. The replace_with crate uses drop flag conditional drop glue to respond to unwinds. Both crates simply call abort on an unwind and don't show a more specific reason for what caused the abort.

Also highly notable is the soundness footgun that dropping the panic payload returned from catch_unwind can itself cause an unwind. std necessarily uses catch_unwind[^1] and responds to an unwind with rtabort!.

[^1]: There needs to be at least one catch frame on the stack to ensure that raising an unwind can actually unwind the stack.

I myself have used impl Drop types to cause a deliberate double panic (and thus an abort) in code that cannot soundly repair state when it gets unwound. This strategy generally creates a more useful crash message, as the runtime can see that the unwind will not be completed.

Solution sketch

The newly stabilized abort on extern "C" edges (“panic in a function that cannot unwind”) is the exact handling that we would like to have for all code regions that cannot be unwound. We also have catch_unwind already establishing the API shape of establishing an unwind handler.

/// Invokes a closure, aborting if the closure unwinds.
///
/// When compiled with aborting panics, this function is effectively a no-op.
/// With unwinding panics, an unwind results in another call into the panic
/// hook followed by a process abort.
///
/// # Notes
///
/// Instead of using this function, code should attempt to support unwinding.
/// Implementing [`Drop`] allows you to restore invariants uniformly in both
/// return and unwind paths.
///
/// If an unwind can lead to logical issues but not soundness issues, you
/// should allow the unwind. Opting out of [`UnwindSafe`] indicates to your
/// consumers that they need to consider correctness in the face of unwinds.
///
/// If an unwind would be unsound, then this function should be used in order
/// to prevent unwinds. However, note that `extern "C" fn` will automatically
/// convert unwinds to aborts, so using this function isn't necessary for FFI.
pub fn abort_unwind<F: FnOnce() -> R, R>(f: F) -> R {
    // extern "C" converts unwinding edges to unwind abort(abi)
    extern "C" fn abort_unwind_inner<F: FnOnce() -> R, R>(f: F) -> R {
        f()
    }

    abort_unwind_inner(f)
}

Alternatives

We can always do nothing; existing solutions in the ecosystem are sufficient. However, the way that this is usually done results in suboptimal diagnostics on an abort, and the required user code required to be sound is “obvious” enough that pulling in a 3rd party crate is unlikely.

I've chosen to use an inner function with extern "C" such that the public API signature doesn't show this implementation choice. It would be a valid choice to skip this step, and/or to use the #[rustc_nounwind] attribute instead of ABI.

Similarly, the language could provide a public #[nounwind] attribute to prevent unwinding instead of abort_unwind being library API. I argue that providing the library API is the better choice since the added friction discouraging the use of nounwind shims is a good thing overall.

Links and related work

I have participated in wg-unwind, but this is my own individual idea.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

We think this problem seems worth solving, and the standard library might be the right place to solve it.
We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

dtolnay commented 3 weeks ago

LGTM. I think this is worth having.

One thing that makes this strange is libcore does not currently expose any stable fn abort() -> !. It would be unfortunate if core::panic::abort_unwind(|| panic!()) needs to become the preferred no-std way to express an abort.

dtolnay commented 2 weeks ago

Accepting this together with https://github.com/rust-lang/libs-team/issues/442. (But they can be implemented and tracked independently, and perhaps stabilized independently.)

I wonder if there will be any reason to have separate abort_immediate vs abort flavors of abort_unwind.

CAD97 commented 2 weeks ago

I wonder if there will be any reason to have separate abort_immediate vs abort flavors of abort_unwind.

This seems highly unlikely. Existing usages of intrinsics::abort are micro-optimizations for code size (ud2 is cheaper than call abort@GOTPCREL) and/or compile time (intrinsic is cheaper than a function call edge). The impact of [unwind abort(abi)] edges is already tuned to have minimal impact, and the case of wanting to avoid the panic machinery entirely is served by panic_immediate_abort.

If someone absolutely must abort_immediate on unwind while also still supporting unwinding panics normally elsewhere, using a manual drop guard to do so seems like a reasonable solution for this edge case of a requirement.

RalfJung commented 1 week ago

std necessarily uses catch_unwind

Couldn't it use abort_unwind now?

CAD97 commented 1 week ago

lang_start needs to use catch_unwind because it's not only UB to unwind out of generic system main, it's effectively UB to start an unwind without a catch somewhere on the stack. (C++ says that std::terminate() is called, but impl-defined whether unwinding takes place.)

On Windows, using abort_unwind would likely work; an unwind without any catch will unwind out of main then abort (experimental proof), meaning we can intercept the unwind with our abort messaging.

On Itanium EH targets (e.g. Linux), an unwind without any catch target will fail to start (by the spec). Currently Rust reserves the right to abort before the unwind when an unwind would end with a runtime-introduced abort, but this isn't a great user experience. It could be acceptable for unwinding from the drop of a panic payload, if the error improves from “fatal runtime error: failed to initiate panic, code 5”. (Unfortunately, _Unwind_RaiseException is allowed to return _URC_FATAL_PHASE2_ERROR indicating an arbitrarily corrupted stack and that all registers are clobbered, making immediate rtabort! the only somewhat safe option in that case. But we don't even handle this correctly today, so…)

rust-lang / libs-team

ACP: panic::abort_unwind #441