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

Implemented compatibility mode with wasm32-wasi #3324

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Rob2309
Copy link

@Rob2309 Rob2309 commented Feb 19, 2023

This PR adds the new --wasi-abi flag to wasm-bindgen. With this flag, wasm-bindgen generates bindings that are compatible with the ABI used by the rust target wasm32-wasi.
The changes should not affect the behavior of wasm-bindgen when the flag is not specified.

For implementing the behavior of wasm32-wasi, I introduced a few new Instructions to the "Standard".
They are PackSlice, UnpackSlice, PackOption and UnpackOption. These instructions translate between the passed-by-pointer structs used in wasm32-wasi and the format expected by the rest of wasm-bindgen. Since I think dereferencing pointers is not quite possible in WIT, I just assumed that the wasi ABI is incompatible with most WIT stuff. If I am wrong about this I am happy to change this.

I tried to make the existing tests work with the wasi target, but for this I needed to add a few "mock" system calls in from of empty javascript functions, since there doesn't seem to be a way to disable the std lib with this target.

I am very open to suggestions on this.

@Rob2309
Copy link
Author

Rob2309 commented Feb 21, 2023

I am thinking about implementing the WASI systemcalls as optional intrinsics, so that a wasi environment is not needed when running the generated module.

@Rob2309
Copy link
Author

Rob2309 commented Feb 21, 2023

I just added the environment variable flag "WASM_BINDGEN_EMULATE_WASI" that introduces many WASI systemcalls as intrinsics. While not all syscalls are implemented yet, the tests run fine on wasm32-wasi without any external wasi shims.

@daxpedda
Copy link
Collaborator

What is the exact use-case of wasm-bindgen for WASI? As far as I understood #1841, it was for things like using the interface proposal, but that never came into fruition.

So currently I'm not sure what the purpose of wasm-bindgen is without access to JS. Right now I'm more in favor of #3233, which seeks to disable the functionality of wasm-bindgen when targeting WASI, like it works for non-Wasm targets right now.

I'm wholly ignorant about WASI btw, so I'm probably missing the big-picture here.

@daxpedda daxpedda added the waiting for author Waiting for author to respond label May 11, 2023
@daxpedda daxpedda mentioned this pull request May 11, 2023
@Rob2309
Copy link
Author

Rob2309 commented May 16, 2023

What is the exact use-case of wasm-bindgen for WASI? As far as I understood #1841, it was for things like using the interface proposal, but that never came into fruition.

So currently I'm not sure what the purpose of wasm-bindgen is without access to JS. Right now I'm more in favor of #3233, which seeks to disable the functionality of wasm-bindgen when targeting WASI, like it works for non-Wasm targets right now.

I'm wholly ignorant about WASI btw, so I'm probably missing the big-picture here.

The main point for doing this was to support compiling C code (e.g. with the cc crate) and linking it into a library that uses wasm-bindgen. Since C does not support the wasm-bindgen calling convention, I instead opted for supporting the wasi calling convention in wasm-bindgen.

@daxpedda
Copy link
Collaborator

The main point for doing this was to support compiling C code (e.g. with the cc crate) and linking it into a library that uses wasm-bindgen.

Apologies for any cluelessness here: would it be possible to compile Rust with the usual wasm32-unknown-unknown target while linking to a C library compiled with wasm32-wasi?

@Liamolucko
Copy link
Collaborator

Apologies for any cluelessness here: would it be possible to compile Rust with the usual wasm32-unknown-unknown target while linking to a C library compiled with wasm32-wasi?

It should be possible, but currently isn't, mainly because Rust's "C" ABI on wasm32-unknown-unknown doesn't match the one used by clang (which is defined in https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md). There are a lot of different issues opened about this, but rustwasm/team#291 seems to be the main one.

The difference between them is that Rust flattens structs out into one parameter per field, whereas clang passes them via. a pointer. The wasm32-wasi target does match, which is why it works for linking to C code.

There's also then the issue of somehow providing the WASI imports needed by the C library, which would probably involve having to add a way to pass wasm imports to init. I'm not sure how you could get that to work on targets without init, though.

@daxpedda
Copy link
Collaborator

There's also then the issue of somehow providing the WASI imports needed by the C library, which would probably involve having to add a way to pass wasm imports to init. I'm not sure how you could get that to work on targets without init, though.

Maybe that's the part we could provide here in wasm-bindgen, but that's useless right now if the problem is still in Rust.

@ranile
Copy link
Collaborator

ranile commented May 19, 2023

As I understand it, this will allow compiling Rust code to wasm32-wasi and executing it in browser, right? I'm in favor of this as long as the compilation is for wasm32-unknown-unknown target but the generated code is compatible with wasm32-wasi ABI. That should allow C code.

That combined with #3233, means that wasm32-wasi is explicitly incompatible with wasm-bindgen but there is an option to use an ABI, that is the same as clang, thus allowing linking with C code

Question: assuming my understanding is correct (please correct me if it isn't), why not make this the default?

@daxpedda
Copy link
Collaborator

Question: assuming my understanding is correct (please correct me if it isn't), why not make this the default?

What do you mean with "default"?

@ranile
Copy link
Collaborator

ranile commented May 19, 2023

Question: assuming my understanding is correct (please correct me if it isn't), why not make this the default?

What do you mean with "default"?

The wasi-abi flag this PR introduces. Why not make it the default?

@daxpedda
Copy link
Collaborator

Yeah, that sounds reasonable to me too.

@Rob2309
Copy link
Author

Rob2309 commented May 19, 2023

The wasi-abi flag this PR introduces. Why not make it the default?

The problem I see with this is that this ABI is currently completely incompatible with any wasm target except wasm32-wasi. I don't think there is a good way to make this the default without breaking most stuff.

The main usecase I had for this PR is allowing to run a rust program linked to C code in a browser. For this, the PR also introduces basic wasi shims that do enough to make programs work that expect correct wasi system calls.

@temeddix
Copy link

1+ for making wasm32-wasi the defaut. It will allow Rust wasm modules to use many things that wasn't possible before. Things like std::thread or std::time::Instant

@daxpedda
Copy link
Collaborator

daxpedda commented Oct 25, 2023

1+ for making wasm32-wasi the defaut. It will allow Rust wasm modules to use many things that wasn't possible before. Things like std::thread or std::time::Instant

wasm-bindgen doesn't control how you compile your Rust code. We were discussing the default for adding the shim/ABI compatibility, not which target is chosen.

The whole point here is to build on what Hamza said:

I'm in favor of this as long as the compilation is for wasm32-unknown-unknown target but the generated code is compatible with wasm32-wasi ABI. That should allow C code.

Though I guess making wasm-bindgen compatible with Rust code that is compiled to wasm32-wasi is possible as well, it is a different discussion.

@Rob2309
Copy link
Author

Rob2309 commented Oct 25, 2023

Though I guess making wasm-bindgen compatible with Rust code that is compiled to wasm32-wasi is possible as well, it is a different discussion.

If I remember correctly, this is exactly what this PR does. I am honestly not quite sure anymore since so much time has passed.

@temeddix
Copy link

temeddix commented Oct 25, 2023

Thanks for the correction :)

I hope this gets reviewed by the authors, as this PR basically fixes some limitations that arise from wasm32-unknown-unknown. wasm32-wasi is actively being developed and popular crates like tokio are adding support for it. Would this change make into the master? @Rob2309 mentioned that he's open to suggestions.

@daxpedda
Copy link
Collaborator

As mentioned above, I agree mostly with Hamza that I would prefer a solution that adds WASI compatibility by adding a shim to make linked WASI libraries work. This means Rust should be compiled with wasm32-unknown-unknown but linked C/C++ libraries can be compiled with wasm32-wasi.

This isn't a solution that can be applied today because of #3454, so I would argue that fixing #3454 is the way forward right now.

Supporting wasm32-wasi directly can be discussed here: #3421. Though I would generally prefer not to do that for reasons I wrote up there, but I'm happy to hear more arguments and views.

@ranile
Copy link
Collaborator

ranile commented Oct 25, 2023

To add to what I said previously, I would rather have wasm32-unknown-unknown be compatible with C ABI than wasm32-wasi be supported. wasm32-wasi has functions like file system access which obviously can't be provided in a browser environment so I'm a little hesitant to add wasm32-wasi compatibility mode. We would effectively be shipping a shim like browser_wasi_shim if we add this support

@daxpedda
Copy link
Collaborator

We would effectively be shipping a shim like browser_wasi_shim if we add this support

That's the idea.
But if this can be done without the help of wasm-bindgen, or make adjustments to enable that, even better.

@temeddix
Copy link

temeddix commented Oct 25, 2023

wasmer-js claimed that it supports polyfills for system I/O, and that threads polyfill(using webworkers) is also coming soon.

IMHO, maybe wasmer-js can be the standard WASI shim for wasm-bindgen. Perhaps a comparison between browser_wasi_shim and wasmer-js can be done.

image

Either way, this change will allow std::fs, std::time::Instant, std::thread work on the web as it is, allowing so many crates work on the web just like they do on native.

@Rob2309
Copy link
Author

Rob2309 commented Apr 24, 2024

I haven't really been paying attention on the wasm progress in rust. Is there still a realistic usecase for this PR?

@daxpedda
Copy link
Collaborator

Now that rust-lang/rust#117919 was merged, we could start thinking about the proposal I outlined in #3324 (comment).

@Rob2309
Copy link
Author

Rob2309 commented May 21, 2024

Now that rust-lang/rust#117919 was merged, we could start thinking about the proposal I outlined in #3324 (comment).

If I understand correctly, one would simply compile a crate with wasm32-unknown-unknown with wasm-bindgen and import c functions with extern "wasm-c"?
This would render this PR unnecessary right?

@daxpedda
Copy link
Collaborator

If I understand correctly, one would simply compile a crate with wasm32-unknown-unknown with wasm-bindgen and import c functions with extern "wasm-c"?

The PR didn't introduce a new ABI, just adjusted the "C" ABI to behave correctly, ergo compatible with wasm32-wasi.

wasm-bindgen should allow adding the WASI shim, whether this requires any changes to wasm-bindgen or not is whats missing here. Preferably a test should be added. I guess it might be best to do this in a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for author Waiting for author to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants