-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Move wasm throw intrinsic back to unwind
#148291
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
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#144225 and prevented such linkage, but this caused regressions in rust-lang#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.
|
cc @Amanieu, @folkertdev, @sayantn |
|
@bors r+ |
In preparation for changes in rust-lang/rust#148291.
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6906167 (parent) -> 8205e6b (this PR) Test differencesShow 4 test diffsStage 1
Stage 2
Additionally, 2 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 8205e6b75ec656305ac235d4726d2c7a1ddcef14 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (8205e6b): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 0.3%, secondary -3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.6%, secondary 2.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -1.1%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 475.565s -> 475.886s (0.07%) |
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 theunwindpanic runtime is linked.throwwas annotated as such, but unwound unconditionally. This could cause UB when a crate built with-C panic=abortcalledthrowfromcorebuilt with-C panic=unwind, since no terminator was added to handle the panic arising from calling an allegedly non-unwindingextern "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=abortwithout recompiling std.The most straightforward solution would be to move
throwinto thepanic_unwindcrate, so that it's only compiled if the panic runtime is guaranteed to beunwind, but this is messy due to our architecture. Instead, move it intounwind::wasm, which is only compiled for bare-metal targets that default topanic = "abort", rendering the issue moot.