-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Don't use __gxx_personality_v0 in panic_unwind on emscripten target #97888
Conversation
This resolves rust-lang#85821. See also the discussion here: emscripten-core/emscripten#17128 The consensus seems to be that rust_eh_personality is never invoked. I patched __gxx_personality_v0 to log invocations and then ran various panic tests and it was never called, so this analysis matches what seems to happen in practice. This replaces the definition with an abort, modeled on the structured exception handling implementation.
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
@bors r+ |
📌 Commit 0ec3174 has been approved by |
@bors r- Can you remove |
@bors r+ |
📌 Commit 46a3f0f has been approved by |
… r=Amanieu Don't use __gxx_personality_v0 in panic_unwind on emscripten target This resolves rust-lang#85821. See also the discussion here: emscripten-core/emscripten#17128 The consensus seems to be that rust_eh_personality is never invoked. I patched __gxx_personality_v0 to log invocations and then ran various panic tests and it was never called, so this analysis matches what seems to happen in practice. This replaces the definition with an abort, modeled on the structured exception handling implementation.
Failed in rollup: #97909 (comment) |
@bors r+ |
📌 Commit d2d205d has been approved by |
Rollup of 6 pull requests Successful merges: - rust-lang#97718 (Fix `delayed_good_path_bug` ice for expected diagnostics (RFC 2383)) - rust-lang#97876 (update docs for `std::future::IntoFuture`) - rust-lang#97888 (Don't use __gxx_personality_v0 in panic_unwind on emscripten target) - rust-lang#97922 (Remove redundant calls to reserve in impl Write for VecDeque) - rust-lang#97927 (Do not introduce bindings for types and consts in HRTB.) - rust-lang#97937 (Fix a typo in `test/ui/hrtb/hrtb-just-for-static.rs`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
I've actually had a PR open for a while now (#95950) that disables unwinding for Emscripten completely, also to address the breakage in #85821. I'm glad to see that @hoodmane found an alternative solution. However as part of the discussion around #95950, the compiler team agreed not to re-enable unwinding on Emscripten without an MCP, because of the risk that a future change to Emscripten could break everything again. It doesn't look like that process was followed here. @pnkfelix thoughts? |
This PR didn't re-enable unwinding since it was not disabled. |
Probably it would be good to discuss with the emscripten team. Ideally Rust should have an error handling implementation that relies on stable interfaces from Emscripten, and the Emscripten team should maintain a stable interface that Rust can use. Also, Emscripten has two different error handling ABIs but currently Rust only supports one, in the ideal case Rust could support both. I will open a new issue about this. |
This resolves #85821. See also the discussion here:
emscripten-core/emscripten#17128
The consensus seems to be that rust_eh_personality is never invoked.
I patched __gxx_personality_v0 to log invocations and then ran
various panic tests and it was never called, so this analysis matches
what seems to happen in practice. This replaces the definition with
an abort, modeled on the structured exception handling implementation.