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

wasm32-emscripten stability issues (exception handling) #98030

Open
hoodmane opened this issue Jun 12, 2022 · 14 comments
Open

wasm32-emscripten stability issues (exception handling) #98030

hoodmane opened this issue Jun 12, 2022 · 14 comments
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/

Comments

@hoodmane
Copy link
Contributor

hoodmane commented Jun 12, 2022

There have been repeated issues with Rust support for the wasm32-emscripten target, particularly involving exception handling (e.g., #97888 #85821), but also involving dynamic linking (e.g., #80775, #92738). Building a rust program as a wasm32-emscripten dynamic library is currently a very delicate business which I would like to see get better.

In my opinion, the main issue here is one of coordination between the Emscripten and rust teams. Ideally I think the following changes would be made:

  1. one of the Emscripten developers should review changes to the Rust Emscripten support to ensure that they know what is going on and consider the implementation to be correct
  2. Emscripten CI should build a Rust program so they can tell whether they are breaking Rust compilation. I would be happy to contribute this.

Emscripten supports two different error handling ABIs: Emscripten exception handling, and wasm exception handling. Wasm exception handling is more efficient, but it is only supported on Safari >= 15.2 which is of concern because ~3% of web users are on older Safari versions, particularly people with old iphones. Ideally, Rust would be able to support both ABIs. In particular, in the case where we are building a dynamically linked library for emscripten (e.g., for Python packages) the ABI used by the module has to match the ABI that the main module was built with.

We would need some configuration that allows the user to choose between abort, emscripten exceptions, and wasm exceptions. I think picking abort as the default is the most reasonable thing to do because this matches Emscripten's behavior: by default C++ exceptions are not supported, but one can opt in to Emscipten eh with -fexceptions or wasm eh with -fwasm-exceptions.

It was suggested that we need a major change proposal, which I think may be a good idea. I am willing to write one after we have a discussion and figure out what should be done. I don't understand much about the Rust consensus process nor really much about error handling so I would have to learn a bit first.

@Jules-Bertholet @Amanieu @brson @nagisa @apiraino @pnkfelix @sbc100 @kripken @aheejin

@kripken
Copy link

kripken commented Jun 13, 2022

cc @tlively who knows most about Rust/Emscripten from the Emscripten side.

@tlively
Copy link
Contributor

tlively commented Jun 13, 2022

One problem here is that we don't have a good way to automatically ensure or even document that users' Emscripten versions are compatible with their Rust versions. New versions of Emscripten breaking things with Rust is WAI because it is not possible for Rust to be compatible with arbitrary Emscripten versions. There have been multiple attempts to improve the documentation situation (emscripten-core/emscripten#14394, rustwasm/book#243, rust-lang/rustup#2791), but none have gone anywhere.

Ideally we would find the most compatible Emscripten version every time Rust updates its LLVM, we would document that version clearly both in documentation and in the output of tools like rustup, and we would have rustup automatically install the correct version of Emscripten when installing the wasm32-emscripten backend. We would also want to update the version of Emscripten used in Rust CI (https://github.com/rust-lang/rust/blob/master/src/ci/docker/scripts/emscripten.sh#L23-L24) and fix any subsequent test failures before Rust's LLVM upgrade rolls out to stable. We should add test coverage to Rust for non-default Emscriptent configurations that folks might want to use, such as with different EH options, or building as a dynamic library, or using Asyncify, etc.

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Jun 13, 2022

The unwinding breakage wasn't an LLVM compatibility issue as far as I can tell, it was due to a change in Emscripten's libunwind to emscripten-core/emscripten@b43d3de.

@tlively
Copy link
Contributor

tlively commented Jun 13, 2022

Right. More generally, we have to periodically do work on the Rust side to upgrade the known-compatible Emscripten version. In principle we could do that in response to Emscripten changes like the change to libunwind, but it would be much more scalable to do that in response to changes on the Rust side such as pulling in a new LLVM.

@Jules-Bertholet
Copy link
Contributor

#95950 (comment)

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.

@aheejin
Copy link

aheejin commented Jun 13, 2022

@Jules-Bertholet

The unwinding breakage wasn't an LLVM compatibility issue as far as I can tell, it was due to a change in Emscripten's libunwind.

Can you point out which change in libunwind this is about? I don't think we have changed libunwind in a long while, and we didn't touch the API side so far AFAICT.

@aheejin
Copy link

aheejin commented Jun 13, 2022

Hello, I work on Wasm exception handling. I mainly worked on the new Wasm EH proposal and its toolchain support, but I also worked on the old Emscripten EH support for our LLVM backend as well. I am happy to be cc'ed on or review any EH related changes, but I don't know much about Rust or Rust toolchain, with which I may need help.

@Jules-Bertholet
Copy link
Contributor

Can you point out which change in libunwind this is about? I don't think we have changed libunwind in a long while, and we didn't touch the API side so far AFAICT.

Sorry, I was wrong, the change that led to the breaage was emscripten-core/emscripten@b43d3de.

@hoodmane
Copy link
Contributor Author

Emscripten does not currently make stability guarantees for anything beyond the basic C ABI, so unfortunately that is not realistic right now.

the change that led to the breakage was emscripten-core/emscripten@b43d3de.

We can see is that the change that led to the breakage was very minor. I think "we can't guarantee stability" isn't the same as "we won't put any effort into trying not to break things." In particular, if Emscripten CI had shown that removing the __gxx_personality_v0 was going to break Rust, they might have left the stub. @sbc100 I think putting it back for now would be a good idea. It seems not worth it to break Rust linking for such a minor improvement.

If there is a good reason to break the ABI (e.g., fixing a design flaw that can't easily be fixed without an ABI breakage), then at least if it's a known breakage there could be a discussion, documentation of the issue and workarounds, and fixes.

@sbc100
Copy link

sbc100 commented Jun 14, 2022

Assuming we can re-add this symbol, along with some kind of test, and some justification as to why we should include it all, I'm happy to see it return.

@hoodmane
Copy link
Contributor Author

I will open a PR then. Ideally we can drop it again when the LLVM version of Emscripten is next updated (since the next version of Rust won't need the patch).

@tlively
Copy link
Contributor

tlively commented Jun 14, 2022

Ideally we can drop it again when the LLVM version of Emscripten is next updated

Emscripten's LLVM is continuously updated, usually multiple times per day. Did you mean Rust's LLVM?

@hoodmane
Copy link
Contributor Author

I was thinking about the major version of LLVM. But I guess the issue of when we can remove the patch is maybe a bit complicated. Anyways just having a comment to say "remove this some day" may be okay.

@Jules-Bertholet
Copy link
Contributor

Emscripten (as far as I can tell) uses the latest LLVM nightly (currently 15.x), so it will have a higher major version than Rust's LLVM (currently 14.0).

@jyn514 jyn514 added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-wasm Target: WASM (WebAssembly), http://webassembly.org/
Projects
None yet
Development

No branches or pull requests

7 participants