-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Allow core with unwinding functions to be used in -C panic=abort crates
#148269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Some changes occurred in compiler/rustc_codegen_ssa
cc @Amanieu, @folkertdev, @sayantn Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| #[allow(ffi_unwind_calls)] | ||
| // Allow `core` built with `-C panic=unwind` to be linked into `-C panic=abort` programs. | ||
| #[rustc_propagate_ffi_unwind] | ||
| pub unsafe fn throw<const TAG: i32>(ptr: *mut u8) -> ! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function must not be callable if any crate is compiled panic=abort. Otherwise you can unwind through panic=abort functions and thus skip destructors, which is unsound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the declaration of this intrinsic should be moved back to the panic_unwind crate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's the case -- since #[rustc_propagate_ffi_unwind] functions are always considered unwinding, the abort_unwinding_calls pass will insert the correct logic whenever it's called from a panic=abort crate; and if it's called indirectly via some other rustic function, then that nested function will be recognized as unwinding and ffi_unwind_calls will force the panic strategy to unwind. I think that #[rustc_propagate_ffi_unwind] functions behave identically to FFI functions (i.e. without available source) in this regard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the propagate-ffi-unwind.rs test demonstrates this behavior; please correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that #[rustc_propagate_ffi_unwind] functions behave identically to FFI functions (i.e. without available source) in this regard.
When calling extern "C" we assume that no panic can happen, when defining an extern "C" function unwinding out of it will abort unconditionally. When calling extern "C-unwind" from a panic=abort crate we insert an aborting landingpad. And if the extern "C-unwind" function is declared in a panic=unwind crate that forces the final compilation to be panic=unwind as we don't know if any panic=unwind function called it that could allow exceptions to escape into panic=abort crates. For calling extern "Rust" functions with a hidden origin no landingpads get inserted because if any crate is panic=abort, we ensure there is no way to unwind through any rust crate thanks to the extern "C-unwind" landingpad + guarantee that if any crate is panic=abort, all extern "C-unwind" functions are declared in panic=abort crates too and thus the landingpads are forced.
AFAICT with this PR you could call throw from a panic=unwind crate (which thus doesn't have an aborting landingpad) and then call this function from a panic=abort crate which also doesn't have an aborting landingpad due to the assumption that you can't unwind into any Rust code if any crate is panic=abort. This unwinding into a panic=abort crate then results in UB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right -- I forgot to update has_ffi_unwind_calls to consider calls to #[rustc_propagate_ffi_unwind] functions as tainting even if the function has rustic ABI. I think that's sufficient to fix the issue?
EDIT: scratch that, that doesn't work due to function pointers. It kind of seems like this would require a new ABI, then...
EDIT 2: Maybe we can just make throw C-unwind (and also use this attribute)? That sounds like it works.
I want to reply to this question outside the review thread because I think it's fundamental to whether the approach in this PR is the one we want to pursue. My argument is that supporting user-controlled unwinding is a goal to keep in mind. Even today, userland Rust crates can throw arbitrary exceptions on most platforms by calling foreign functions like I guess it could be argued that this intrinsic can still be reasonably moved into |
Which would be an
Outside of wasm32-unknown-unknown
I don't think we can reasonably expose the raw intrinsic to end users given that there is no specification for what arbitrary tag indices should do. LLVM doesn't expose a way for the user to generate arbitrary tags and then point to those. Currently it only supports a tag for C++ exceptions, so I think we should only expose a function that specifically throws C++ exceptions. By the way it seems like |
That's kind of my point -- typically, this rule is not restrictive, since users can opt into linking to Other than that, agreed. I didn't know that
Well, that looks wrong... |
|
Well, this is awkward: |
|
Oh, that explains it... we do in fact use external |
Move wasm `throw` intrinsic back to `unwind` Fixes #148246, less invasive than the previously proposed #148269. Removes the publicly visible unstable intrinsic tracked in #122465 since it's not clear how to export it in a sound manner. r? `@bjorn3` --- rustc assumes that regular `extern "Rust"` functions unwind only if the `unwind` panic runtime is linked. `throw` was annotated as such, but unwound unconditionally. This could cause UB when a crate built with `-C panic=abort` called `throw` from `core` built with `-C panic=unwind`, since no terminator was added to handle the panic arising from calling an allegedly non-unwinding `extern "Rust"` function. rustc was taught to recognize this condition since #144225 and prevented such linkage, but this caused regressions in #148246, since this meant that Emscripten projects could not be built with `-C panic=abort` without recompiling std. The most straightforward solution would be to move `throw` into the `panic_unwind` crate, so that it's only compiled if the panic runtime is guaranteed to be `unwind`, but this is messy due to our architecture. Instead, move it into `unwind::wasm`, which is only compiled for bare-metal targets that default to `panic = "abort"`, rendering the issue moot.
Move wasm `throw` intrinsic back to `unwind` Fixes rust-lang/rust#148246, less invasive than the previously proposed rust-lang/rust#148269. Removes the publicly visible unstable intrinsic tracked in rust-lang/rust#122465 since it's not clear how to export it in a sound manner. r? `@bjorn3` --- rustc assumes that regular `extern "Rust"` functions unwind only if the `unwind` panic runtime is linked. `throw` was annotated as such, but unwound unconditionally. This could cause UB when a crate built with `-C panic=abort` called `throw` from `core` built with `-C panic=unwind`, since no terminator was added to handle the panic arising from calling an allegedly non-unwinding `extern "Rust"` function. rustc was taught to recognize this condition since rust-lang/rust#144225 and prevented such linkage, but this caused regressions in rust-lang/rust#148246, since this meant that Emscripten projects could not be built with `-C panic=abort` without recompiling std. The most straightforward solution would be to move `throw` into the `panic_unwind` crate, so that it's only compiled if the panic runtime is guaranteed to be `unwind`, but this is messy due to our architecture. Instead, move it into `unwind::wasm`, which is only compiled for bare-metal targets that default to `panic = "abort"`, rendering the issue moot.
Move wasm `throw` intrinsic back to `unwind` Fixes rust-lang/rust#148246, less invasive than the previously proposed rust-lang/rust#148269. Removes the publicly visible unstable intrinsic tracked in rust-lang/rust#122465 since it's not clear how to export it in a sound manner. r? `@bjorn3` --- rustc assumes that regular `extern "Rust"` functions unwind only if the `unwind` panic runtime is linked. `throw` was annotated as such, but unwound unconditionally. This could cause UB when a crate built with `-C panic=abort` called `throw` from `core` built with `-C panic=unwind`, since no terminator was added to handle the panic arising from calling an allegedly non-unwinding `extern "Rust"` function. rustc was taught to recognize this condition since rust-lang/rust#144225 and prevented such linkage, but this caused regressions in rust-lang/rust#148246, since this meant that Emscripten projects could not be built with `-C panic=abort` without recompiling std. The most straightforward solution would be to move `throw` into the `panic_unwind` crate, so that it's only compiled if the panic runtime is guaranteed to be `unwind`, but this is messy due to our architecture. Instead, move it into `unwind::wasm`, which is only compiled for bare-metal targets that default to `panic = "abort"`, rendering the issue moot.
core::intrinsics::wasm32::throwis a function that unwinds by calling thethrowintrinsic. When suchcoreis linked into a-C panic=abortbinary crate, rustc recognizes thatthrowis a non-FFI function that nevertheless unwinds. Before this PR, this caused rustc to emit an error.With this PR, functions can opt into being recognized as unwinding even if the ambient panic strategy is
abort. This allows wrappers around unwinding FFI functions, like thethrowfunction wrapping thethrowWasm intrinsic, to behave just like the wrapped function would behave with regards to theffi_unwind_callslint and the required panic strategy logic.I don't like adding a new attribute, but this functionality seems to be very rarely needed: it only matters for
-C panic=unwindcrates statically linked into binary crates with-C panic=abort, and onlycore/stdare typically compiled separately from the whole crate tree.Fixes #148246.
r? @alexcrichton