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

Match wasm32-unknown-unknown ABI with clang #117236

Closed
wants to merge 2 commits into from
Closed

Match wasm32-unknown-unknown ABI with clang #117236

wants to merge 2 commits into from

Conversation

temeddix
Copy link

@temeddix temeddix commented Oct 26, 2023

Currently, wasm-bindgen does not support wasm32-wasi target.

The goal is to make wasm-bindgen support wasm32-wasi target, and this PR is a part of that effort which aims to match the ABI of wasm-32-unknown-unknown target to that of wasm32-wasi and clang.

There are discussions going on at wasm-bindgen repository about extending the support to wasm32-wasi. In short, wasm-bindgen needs a consistent C ABI between wasm32-unknown-unknown and wasm32-wasi to support both of them.

These links might also be helpful:

Take a look at the comment by @alexcrichton who wrote this code. We can understand why this legacy ABI was introduced in the first place from this post.

This is a default for backwards-compatibility with the original definition of this target oh-so-long-ago. Once the "wasm" ABI is stable and the wasm-bindgen project has switched to using it then there's no need for this and it can be removed. Currently this is the reason that this target's ABI is mismatched with clang's ABI. This means that, in the limit, you can't merge C and Rust code on this target due to this ABI mismatch.

After more than 2 years, wasm32-unknown-unknown still uses the legacy ABI. wasm-bindgen says that Rust should switch to the new ABI first, but Rust is saying that wasm-bindgen should support the new ABI first. It's somewhat like a chicken-egg problem.

Up until now, this legacy ABI was working fine. However recently wasm32-wasi started becoming a thing and the Rust ecosystem is actively being built around it. That's why I and some other contributors came up to suggest this change. Since most of the users do not use C ABI directly, but instead generate .wasm and .js code with wasm-bindgen, they will just need to update wasm-bindgen and re-generate the code. Appropriate warning and version-specific workaround should be provided from Rust compiler, though.

To narrow the focus on web browsers, many native functionalities including std::thread, std::time::Instant, std::fs are not working right now with wasm32-unknown-unknown. This limitation prevents many crates like tokio and rayon from working on the web, leaving the vast majority of Rust ecosystem being fallen out from the web platform. Consequently, fragmentation arises as many crates now ship with separated wasm and non-wasm versions, simply because they can't use std.

If wasm-bindgen gets to support wasm32-wasi, various crates will be able to use thread, time, file, network operations on the web only with std without any custom JavaScript glue, just like they do on native platforms. JavaScript polyfill libraries(such as wasmer-js, browser_wasi_shim) claims that they can handle that WASI syscalls, utilizing existing web APIs to mimic native functionalities.

I hope we can achieve consensus between Rust developers and wasm-bindgen developers soon enough!

@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

@daxpedda
Copy link
Contributor

I believe it would be better for Rust to follow bjorn3's recommendation here: #71871 (comment). Providing a crate specific version warning would be great!
I think the warning is also important for non-wasm-bindgen users, because there might be some users who rely on the current ABI.

Also want to note that wasm-bindgen does not currently support the fixed ABI, this would require a new release first.


Additionally I would like to point out some misunderstandings that might arise from the OP:

The goal is to make wasm-bindgen support wasm32-wasi target, and this PR is a part of that effort which aims to match the ABI of wasm-32-unknown-unknown target to that of wasm32-wasi and clang.

The current non-standard ABI doesn't stop wasm-bindgen from supporting wasm32-wasi, it only stops it from supporting both at the same time. This is not a wasm-bindgen specific problem, it is a rustc problem. Of course if wasm-bindgen wants to support that, Rust support is required.

To narrow the focus on web browsers, many native functionalities including std::thread, std::time::Instant, std::fs are not working. This limitation prevents many crates like tokio and rayon from working on the web, leaving the vast majority of Rust ecosystem being fallen out from the web platform. Consequently, fragmentation arises as many crates now ship with separated wasm and non-wasm versions, simply because they can't use std.

This PR would not fix this issue. Maybe std::thread will be implemented for wasm32-unknown-unknown with a proposal (like the thread spawn one), but there is no active proposals I'm aware of for std::time and std::fs. None of these problems are an ABI problem.

If wasm-bindgen gets to support wasm32-wasi, various crates will be able to use thread, time, file, network operations only with std without any custom JavaScript glue, just like they do on native platforms.

wasm-bindgen could already support wasm32-wasi today! What it can't support is wasm32-unknown-unknown code that tries to access WASI code through FFI (without funny workarounds at least), but again, this isn't a wasm-bindgen issue, but a rustc issue.


wasm-bindgen only comes in at:

  • This change would break wasm-bindgen users. But it would also break any users relying on the current ABI.
  • If wasm-bindgen wants to offer WASI shim support, ABI compatibility would be required.

To clarify: I'm absolutely in favor of this change, but it should be done with care, imo.

@temeddix
Copy link
Author

temeddix commented Oct 26, 2023

The current non-standard ABI doesn't stop wasm-bindgen from supporting wasm32-wasi, it only stops it from supporting both at the same time.

Yeah, it's the main reason this PR was made. If wasm-bindgen doesn't need to support both wasm32-wasi and wasm32-unknown-unknown, we could have just ignored this legacy ABI.

This PR would not fix this issue. Maybe std::thread will be implemented for wasm32-unknown-unknown with a proposal (like the thread spawn one), but there is no active proposals I'm aware of for std::time and std::fs. None of these problems are an ABI problem.

Yeah, looks like this is something that cannot be solved with Rust ABI or wasm-bindgen. Rather, it should be done by wasm32-wasi with JS polyfills such as wasmer-js or browser_wasi_shim(links above) that takes care of WASI syscalls. However, it does open up a whole new possibilities, as the thread-related instructions will be written in the .wasm file, though 'HOW' can have multiple answers.

To clarify: I'm absolutely in favor of this change, but it should be done with care, imo.

Glad to hear that. Hope that other developers also investigate in this topic to reach consensus :)

@temeddix temeddix changed the title Match wasm32-unknown-unknown ABI with clang Match wasm32-unknown-unknown ABI with clang Oct 26, 2023
@temeddix
Copy link
Author

temeddix commented Oct 26, 2023

I believe it would be better for Rust to follow bjorn3's recommendation here: #71871 (comment). Providing a crate specific version warning would be great!

Do you have any suggestions to integrate this idea in this PR? Should we search for crates that uses the legacy ABI directly without wasm-bindgen?

@daxpedda
Copy link
Contributor

daxpedda commented Oct 26, 2023

This PR would not fix this issue. Maybe std::thread will be implemented for wasm32-unknown-unknown with a proposal (like the thread spawn one), but there is no active proposals I'm aware of for std::time and std::fs. None of these problems are an ABI problem.

Yeah, looks like this is something that cannot be solved with Rust ABI or wasm-bindgen. Rather, it should be done by wasm32-wasi with JS polyfills such as wasmer-js or browser_wasi_shim(links above) that takes care of WASI syscalls.

Shims wouldn't solve that as well. You can't compile to wasm32-unknown-unknown and make std::thread & co work without fixing this in std.

The WASI shim solution for wasm-bindgen is to allow linked libraries to use these APIs, but they have to be compiled to wasm32-wasi. I'm not aware of a way Cargo allows you to compile dependencies to different targets.

Do you have any suggestions to integrate this idea in this PR?

I'm not familiar with crate-specific warnings in Rust (not that I'm familiar with rustc at all really). But that would be my (actually bjorn3's) suggestion: detect old versions of wasm-bindgen and emit a future-compat warning. Like mentioned in the other thread: this could start as a warning and then turn into an error later on.

Should we search for crates that uses the legacy ABI directly without wasm32-unknown-unknown?

I wouldn't know how, but my impression so far is that this wouldn't be necessary.

@temeddix
Copy link
Author

Yeah, I also think that can be a sane way to warn the users.

To clarify, I don't expect wasm32-unknown-unknown to work with std::thread or std::time. Those expectations are about wasm32-wasi. Still, thanks for the info!

@temeddix temeddix closed this by deleting the head repository Oct 27, 2023
@workingjubilee workingjubilee added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Oct 21, 2024
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/ S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

6 participants