Skip to content
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

Disable unwinding for emscripten again #95950

Conversation

Jules-Bertholet
Copy link
Contributor

@Jules-Bertholet Jules-Bertholet commented Apr 11, 2022

Unwinding on wasm32-unknown-emscripten seems to be broken again, so this PR disables it. Undoes 62c3443, fixes #85821, redux of #89762 which was abandoned by its author.

@rust-highfive
Copy link
Collaborator

Thank you for submitting a new PR for the library teams! If this PR contains a stabilization of a library feature that has not already completed FCP in its tracking issue, introduces new or changes existing unstable library APIs, or changes our public documentation in ways that create new stability guarantees then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 11, 2022
@rust-highfive
Copy link
Collaborator

r? @nagisa

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

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2022
@nagisa
Copy link
Member

nagisa commented Apr 11, 2022

Given that this is not the first time this default changes, I think we should have a write-up detailing a final decision and stick with it, instead of switching it back and forth every time the upstream project releases a new version.

Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but lets wait the final decision on that problem

@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Apr 11, 2022

From my limited sleuthing:
According to #85821 (comment), apparently Rust unwinding has never really worked properly on Emscripten. Emscripten offers two alternative exception handing methods: via JS and natively via WASM. The JS method is slow, requires JS (which not all WASM environments have), and it's not clear how easy it would be to support with Rust. The WASM exceptions seem much more straightforward, but they rely on a not-yet-finalized proposal that most WASM engines don't support.

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch 2 times, most recently from ccf0ba8 to d373daa Compare April 14, 2022 22:08
@rust-log-analyzer

This comment has been minimized.

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from d373daa to 5ac753d Compare April 15, 2022 01:41
@rust-log-analyzer

This comment has been minimized.

@nagisa
Copy link
Member

nagisa commented Apr 16, 2022

I'm going to nominate this for a T-compiler discussion.

I think it is a correct thing to disable unwinding here, but I worry that doing so will break somebody's CI or something. Probably is worth it over a target that doesn't work at all, but also if we make a decision like this now, I feel like we should make it such that it is permanent. That is, we would commit to wasm32-unknown-emscripten always using the abort panic scheme. We'd commit here so that users of this target can actually rely on a specific panicking mechanism here rather than being forced to specify --panic=abort to get any semblance of stability here.

@nagisa nagisa added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 16, 2022
@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from 5ac753d to 4a51f3c Compare April 16, 2022 19:55
@Jules-Bertholet
Copy link
Contributor Author

Jules-Bertholet commented Apr 16, 2022

// Unwinding doesn't work right now, so the whole target unconditionally
// defaults to panic=abort. Note that this is guaranteed to change in
// the future once unwinding is implemented. Don't rely on this as we're
// basically guaranteed to change it once WebAssembly supports
// exceptions.

I don't think committing to never ever having any form of unwinding for the Emscripten target makes sense, given that Rust doesn't commit to that for the other Wasm targets; once the Wasm exception handling proposal is finalized and stabilized everywhere, it can be used to implement unwinding. A weaker commitment which I think does make sense is that unwinding won't be implemented with any mechanism other than native Wasm exceptions—Emscripten's homegrown exception handling doesn't provide a API that Rust can rely on.

@rust-log-analyzer

This comment has been minimized.

@Jules-Bertholet
Copy link
Contributor Author

Another option is to gate WebAssembly unwinding behind an exceptions target feature, or some similar mechanism. See also #77839

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from 4a51f3c to e791894 Compare April 16, 2022 20:34
@rust-log-analyzer

This comment has been minimized.

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from e791894 to 2caadf4 Compare April 16, 2022 23:41
@rust-log-analyzer

This comment has been minimized.

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch 4 times, most recently from 358bfc5 to cb316c9 Compare April 17, 2022 15:53
@apiraino
Copy link
Contributor

The topic of disabling unwind has been discussed during T-compiler meeting on Zulip (notes). One idea was to disable unwind (so this PR can move forward) and define a process involving a writeup to reinstate it back if/when deemed necessary.

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 21, 2022
@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from 832e4f5 to 7406384 Compare June 5, 2022 22:46
@Jules-Bertholet
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2022
@rust-log-analyzer

This comment has been minimized.

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch 2 times, most recently from 00726cb to e88d5fe Compare June 5, 2022 23:13
@bors
Copy link
Contributor

bors commented Jun 7, 2022

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

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from e88d5fe to 37af9e0 Compare June 8, 2022 13:27
@bors
Copy link
Contributor

bors commented Jun 10, 2022

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

@Jules-Bertholet
Copy link
Contributor Author

This should not be merged until the discussion around #97888 is resolved.

@Jules-Bertholet Jules-Bertholet marked this pull request as draft June 10, 2022 16:08
@bors
Copy link
Contributor

bors commented Jun 11, 2022

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

@Jules-Bertholet Jules-Bertholet force-pushed the disable_emscripten_exceptions_again branch from de9f48f to 16140ab Compare June 11, 2022 02:54
@bors
Copy link
Contributor

bors commented Jun 13, 2022

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

@tlively
Copy link
Contributor

tlively commented Jun 13, 2022

Hi, I work on Emscripten and have historically worked on the Emscripten beckends in Rust, too. I am the one who previously enabled unwinding for the Emscripten target. From briefly looking at the issues that motivated this, it looks like the reporters were trying to use versions of Emscripten that use versions of LLVM that are incompatible with Rust's version of LLVM. In other words, I think the root problem here is a lack of documentation or automation about what version of Emscripten should be used with Rust, not that there is an actual bug with Emscripten or Rust.

@Jules-Bertholet
Copy link
Contributor Author

No, this wasn't an LLVM compatibility issue as far as I can tell, this was due to a change in Emscripten's libunwind.

@tlively
Copy link
Contributor

tlively commented Jun 13, 2022

Sure, but the principle is the same. Using Emscripten versions from before that change to Emscripten's libunwind is the "correct" fix to this problem, since the latest version (or any other recent version) of Emscripten is not compatible with Rust in general.

@Jules-Bertholet
Copy link
Contributor Author

I would think that the "correct" fix would be to rely only on APIs that the Emscripten project makes stability guarantees for, so users don't need to be bound to a specific version.

@tlively
Copy link
Contributor

tlively commented Jun 13, 2022

I agree that that would be even better, but Emscripten does not currently make stability guarantees for anything beyond the basic C ABI, so unfortunately that is not realistic right now. We're aiming to stabilize our C++ ABI soon, which would include libunwind, but it is not stable yet. Relatedly, we would like to stabilize a dynamic linking ABI soon, but again, that's not stable yet.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emscripten wasm32 compilation broken