wasmtime
wasmtime copied to clipboard
bytecodealliance
Js-polyfill using wasi-common crate
This commit drafts out js-polyfill using wasi-common crate.
There is a couple of issues with this PR, so I'm gonna mark it as a draft PR until all (or at least a vast majority is satisfactorily resolved) before moving forward with it.
This PR addresses #520.
Rustc regression wrt Emscripten in beta/nightly channels
It seems there might be a compiler regression wrt wasm32-unknown-emscripten target on beta and nightly channels. I'm still to add a CI job for building the introduced js-polyfill crate, but if you try and build with the latest Emscripten upstream LLVM backend, you will be presented with cryptic compilation failure of the num-integer crate:
Called function must be a pointer!
call addrspace(7667714) void
error: could not compile `num-integer`.
Caused by:
process didn't exit successfully: `rustc --crate-name num_integer /Users/kubkon/.cargo/registry/src/github.com-1ecc6299db9ec823/num-integer-0.1.41/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts --crate-type lib --emit=dep-info,metadata,link -C debuginfo=2 -C metadata=4f0bdfeb6f9e43f4 -C extra-filename=-4f0bdfeb6f9e43f4 --out-dir /Users/kubkon/dev/wasmtime/crates/wasi-common/js-polyfill/target/wasm32-unknown-emscripten/debug/deps --target wasm32-unknown-emscripten -L dependency=/Users/kubkon/dev/wasmtime/crates/wasi-common/js-polyfill/target/wasm32-unknown-emscripten/debug/deps -L dependency=/Users/kubkon/dev/wasmtime/crates/wasi-common/js-polyfill/target/debug/deps --extern num_traits=/Users/kubkon/dev/wasmtime/crates/wasi-common/js-polyfill/target/wasm32-unknown-emscripten/debug/deps/libnum_traits-7aaa72299f995a0d.rmeta --cap-lints allow -C 'link-args=--js-library assets/load-files.js --shell-file assets/shell.html --pre-js assets/wasi.js -s EXPORTED_FUNCTIONS=['\''_main'\'','\''_get_wasi_context'\'','\''_handleFiles'\'','\''_old_wasi_common_args_get'\'','\''_old_wasi_common_args_sizes_get'\'','\''_old_wasi_common_clock_res_get'\'','\''_old_wasi_common_clock_time_get'\'','\''_old_wasi_common_environ_get'\'','\''_old_wasi_common_environ_sizes_get'\'','\''_old_wasi_common_fd_advise'\'','\''_old_wasi_common_fd_allocate'\'','\''_old_wasi_common_fd_close'\'','\''_old_wasi_common_fd_datasync'\'','\''_old_wasi_common_fd_fdstat_get'\'','\''_old_wasi_common_fd_fdstat_set_flags'\'','\''_old_wasi_common_fd_fdstat_set_rights'\'','\''_old_wasi_common_fd_filestat_get'\'','\''_old_wasi_common_fd_filestat_set_size'\'','\''_old_wasi_common_fd_filestat_set_times'\'','\''_old_wasi_common_fd_pread'\'','\''_old_wasi_common_fd_prestat_dir_name'\'','\''_old_wasi_common_fd_prestat_get'\'','\''_old_wasi_common_fd_pwrite'\'','\''_old_wasi_common_fd_read'\'','\''_old_wasi_common_fd_readdir'\'','\''_old_wasi_common_fd_renumber'\'','\''_old_wasi_common_fd_seek'\'','\''_old_wasi_common_fd_sync'\'','\''_old_wasi_common_fd_tell'\'','\''_old_wasi_common_fd_write'\'','\''_old_wasi_common_path_create_directory'\'','\''_old_wasi_common_path_filestat_get'\'','\''_old_wasi_common_path_filestat_set_times'\'','\''_old_wasi_common_path_link'\'','\''_old_wasi_common_path_open'\'','\''_old_wasi_common_path_readlink'\'','\''_old_wasi_common_path_remove_directory'\'','\''_old_wasi_common_path_rename'\'','\''_old_wasi_common_path_symlink'\'','\''_old_wasi_common_path_unlink_file'\'','\''_old_wasi_common_poll_oneoff'\'','\''_old_wasi_common_proc_exit'\'','\''_old_wasi_common_proc_raise'\'','\''_old_wasi_common_random_get'\'','\''_old_wasi_common_sched_yield'\'','\''_old_wasi_common_sock_recv'\'','\''_old_wasi_common_sock_send'\'','\''_old_wasi_common_sock_shutdown'\''] -o assets/polyfill.html' --cfg has_i128` (signal: 11, SIGSEGV: invalid memory reference)
If you decide to use the fastcomp and Rust stable channel instead, you will be greated by linker errors to do with our usage of u128, etc. Either way, this needs further investigation and potential bug filing in Rust itself.
Clean up of .cargo/config (or an alternative?)
I'm currently using .cargo/config to specify wasm32-unknown-emscripten as the default target, plus pass all the necessary link-args to emcc compiler. This is super messy, so if anyone comes up with a better alternative, please shout out! Another caveat of this approach here, is that the JS + Wasm artifacts land in the assets/ folder.
Dealing with Wasm memory on the WASI syscall/polyfill boundary
The way we have wasi-common designed currently, is that (most of) syscalls accept a combination of &mut WasiCtx and &mut [u8] where the latter is a mutable view at Wasm memory which we use to decode/encode the passed in pointers to and from. The generated C bindings using wasi-common-cbindgen crate the require the syscalls to accept *mut WasiCtx and *mut u8 + usize (memory's address and its length). In order to avoid a lot of changes in the original wasi.js glue-code, I'm simply passing in the entire Emscripten memory (HEAP8) to every wasi-common syscall, which certainly carries some overhead. This should be cleaned up one way or another. If you've got any ideas for this, please do shout out! Even if we don't necessarily fix it in this PR, it'll be good to have some discussion about the best approach for later.
Anyhow, my main idea for this is to rewrite translation routines on the JS side to always alloc a contiguous and aligned memory chunks that are big enough to fit the contents located in the WASI Guest heap and required by the syscall to operate on. The tricky bit here is ensuring contiguity and alignment at the same time. I noticed a bit of speed up when I’ve repacked ciovec’s manually this way into a contiguous (compressed if you will) chunk of memory allocated in Emscripten’s heap with only a single _malloc call.
Dealing with Wasi context on the WASI syscall/polyfill boundary
Currently, in this PR, we generate the WasiCtx struct as a thread-local struct in src/main.rs much like it was done in the original polyfill.c. In order to pass it as an arg in every WASI syscall in JS, I've exposed an unsafe "getter" fn get_wasi_ctx() -> *mut WasiCtx but I'm wondering if there could be a better way to handle this. One thing I've had in mind was to feature-gate the generated syscalls in wasi-common so that if compiled with js-polyfill feature on (for instance), WasiCtx would be accessed statically. All thoughts on this are much appreciated!
@sunfishcode @alexcrichton @peterhuene I'm not sure whether review requests work in Draft PR mode so I'm cc'ing all of you. Apologies for this additional spam!
I don't have a ton of thoughts about this myself per se, I'm pretty unfamiliar with the Emscripten target in Rust.
In terms of build config I'd have a slight preference for a script checked in vs .cargo/config myself, but I don't think there's a great way to orchestrate this
I don't have a ton of thoughts about this myself per se, I'm pretty unfamiliar with the Emscripten target in Rust.
In terms of build config I'd have a slight preference for a script checked in vs
.cargo/configmyself, but I don't think there's a great way to orchestrate this
OK cool, I can definitely do that! I actually can’t stand the way ones has to specify rustflags in .cargo/config so a script will do nicely here. Thanks for the suggestion!
OK, I'm gonna go ahead and mark this PR as ready for review even though there is quite a few rough edges I'd like to smooth out. Still, I reckon this is a reasonable step towards porting the wasi-common to Emscripten.
One thing to note here, in order to successfully build the js-polyfill crate using the build.sh script, it is necessary to remove cdylib from crate-type in wasi-common's Cargo.toml. This seems to be a bug in rustc (I've filed rust-lang/rust#67782 to track this). In the meantime, if you'd like to experiment with the code, remove the cdylib and the polyfill should build successfully.
Furthermore, also note that the polyfill requires WASI binaries targetting wasi_snapshot_preview1 so you'll need to use either the 1.41.0-beta or 1.42.0-nightly toolchains.
Finally, I haven't done extensive tests for the polyfill, but a simple println! works just fine ;-)
Hm so I'm not sure if I'm the best to review this, I'm not really familiar with how this is expected to be used or how we're looking to maintain this over time. Right now it looks like a huge amount of JS which is handwritten but doesn't have any tests?
Some questions I might have are:
- Would it be possible do auto-generate the JS files from
*.witxfiles? - How is this expected to be used? For example how to I "use the polyfill" to satisfy a wasi import on the web
- Should we perhaps deploy a built copy of this as an asset for each release?
I think we probably really do want to auto-generate any JS glue here (if necessary) from *.witx files, but I also am sort of surprised we need to much JS, I figured that the purpose of this was to basically call a function with some configuration options which gives you a JS object which has all of the wasi imports that can be directly hooked up into a wasm module
Hm so I'm not sure if I'm the best to review this, I'm not really familiar with how this is expected to be used or how we're looking to maintain this over time. Right now it looks like a huge amount of JS which is handwritten but doesn't have any tests?
Yeah, the tests would be great. I didn't add or figure that out for this PR yet, as I wanted this PR to lay some ground for future work in getting the js-polyfill optimised, tested, etc.
Some questions I might have are:
* Would it be possible do auto-generate the JS files from `*.witx` files?
I haven't looked into that in detail yet, but if it was possible, that would be great and I'm all for that.
* How is this expected to be used? For example how to I "use the polyfill" to satisfy a wasi import on the web
AFAIK, although not an expert here, this is meant to replace the polyfill used by https://wasi.dev/polyfill. The idea seems quite old, and reported in #520 (and much earlier in the original, archived wasi-common). I wanted this PR to be the first step in having the polyfill use wasi-common rather than wasi-c.
* Should we perhaps deploy a built copy of this as an asset for each release?
I think the main application is similar to https://wasi.dev/polyfill but perhaps @sunfishcode has more ideas about this one?
I think we probably really do want to auto-generate any JS glue here (if necessary) from
*.witxfiles, but I also am sort of surprised we need to much JS, I figured that the purpose of this was to basically call a function with some configuration options which gives you a JS object which has all of the wasi imports that can be directly hooked up into a wasm module
Yep, agreed! I basically tried to recreate the original js-polyfill that was based on wasi-c but now that we have witx, perhaps auto-generating the JS glue should be possible to set up.
Personally I think that anything new WASI-related being added to this repository should be driven from the *.witx files from now on, so I would prefer that this were switched to a code-generation strategy before landing. I'm a bit fearful of how we've been continually landing "groundwork" PRs which, while they make sense, needs to switch eventually to "fully fleshed out". For new WASI features I personally think we're at that threshold where new features related to WASI should be pretty fleshed out, which in my my includes the code generation aspect.
As to how this would be used, I'm not thinking so much what the purpose of this is but rather at a technical level how it's expected to be hooked up into other applications.
Personally I think that anything new WASI-related being added to this repository should be driven from the
*.witxfiles from now on, so I would prefer that this were switched to a code-generation strategy before landing. I'm a bit fearful of how we've been continually landing "groundwork" PRs which, while they make sense, needs to switch eventually to "fully fleshed out". For new WASI features I personally think we're at that threshold where new features related to WASI should be pretty fleshed out, which in my my includes the code generation aspect.
Agreed! Before I make those changes I'd really like @sunfishcode to have a look since I'm not even sure the code presented here is going in the right direction. Also, if this PR should include auto-generation from witx, I'd be keen to put in a little more effort and add some basic tests in. Much like yourself, I'm not overly keen on merging anything that's not been tested.
Also, while we're here, perhaps @kripken could have a look/join in the discussion and offer some guidance as well, if it's not too much to ask ofc :-)
As to how this would be used, I'm not thinking so much what the purpose of this is but rather at a technical level how it's expected to be hooked up into other applications.
Oh OK. That's an excellent question which I actually don't have an answer to :-D
Aside from one emscripten comment I don't see anything that looks wrong to me. But I just skimmed as I don't understand the setup here.
cc @tlively for the rust-emscripten questions above.
The Rust-emscripten regression mentioned in the opening post is fixed in https://github.com/rust-lang/rust/pull/67976, which has been approved but might take a while to get through the commit queue. Please cc me on any other issues you run into with Rust's Emscripten backends. There are certainly some rough corners left since we switched away from using Fastcomp with Rust.
Thanks for putting this together @kubkon! I think it makes sense to merge this, and iterate from here.
On second thought, I agree with @alexcrichton here. There's a lot of boilerplate here, and as WASI evolves, this is going to be more boilerplate that has to be manually maintained. I think we should look for ways to generate more of this code through witx first.
On second thought, I agree with @alexcrichton here. There's a lot of boilerplate here, and as WASI evolves, this is going to be more boilerplate that has to be manually maintained. I think we should look for ways to generate more of this code through witx first.
I agree with both @alexcrichton and you to have most of the boilerplate auto-generated from witx. If it's OK to have this PR lying around for a couple more weeks, I'll be able to look into this after @pchickey and I finish the first full draft of wiggle.
Subscribe to Label Action
This issue or pull request has been labeled: "w", "a", "s", "i"
To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Subscribe to Label Action
This issue or pull request has been labeled: "w", "a", "s", "i"
To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.
Subscribe to Label Action
This issue or pull request has been labeled: "wasi"
Users Subscribed to "wasi"
- @kubkon
To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.
I was looking at some older PRs on Wasmtime and I came across this. This is pretty old at this point and isn't entirely actionable as-is. Nowadays jco is probably the best go-to for WASI-on-the-web, however, if someone comes across this now or again in the future.