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

Rewrite WASI implemention from C to Rust #120

Closed
sunfishcode opened this issue Apr 24, 2019 · 15 comments
Closed

Rewrite WASI implemention from C to Rust #120

sunfishcode opened this issue Apr 24, 2019 · 15 comments

Comments

@sunfishcode
Copy link
Member

Wasmtime's current WASI implementation is derived from what CloudABI calls its userspace emulator code. This was an expedient way to get to a relatively complete WASI implementation that we could use to do early API iteration and to prototype the rest of the ecosystem on, however it also serves as a reference implementation of sorts, as WASI and CloudABI are relatively thin on API-level documentation.

However, an implementation in Rust would be preferable, because it would be easier to maintain along with the rest of Wasmtime's Rust code, it would be safer (there will likely still be unsafe code, however that's still an improvement over all the code being effectively "unsafe"), and it'd allow us to reduce our dependencies on cmake and cc. It may even make sense to build WASI as a crate that can be independent of Wasmtime entirely, so that it can be used in other contexts.

And, since CloudABI considers this "emulation" code, it isn't as robust as we'd ideally like it to be. For example, poll_oneoff only supports one clock subscription, some obscure features aren't implemented yet, and there may be other limitations.

This is a big project, though we can implement it one step at a time, starting with simple functions and building up to more complex ones. The very first function to do is probably proc_exit, and possibly next the args_get/args_sizes_get/environ_get/environ_sizes_get functions.

I'll probably start work on this as I have time, though if anyone else wants to take a look at these, I'd be happy to mentor!

@kubkon
Copy link
Member

kubkon commented Apr 25, 2019

I'd be happy to help out with this especially since it intersects with the areas of my interests #121.

@sunfishcode
Copy link
Member Author

That'd be great! As I mentioned above, proc_exit and the args/environ functions are good ones to start with, as the implementations are fairly simple, so it'll be good for getting started.

@kubkon
Copy link
Member

kubkon commented Apr 26, 2019

Awesome! So, I've played somewhat with the code, and to get an idea of how things are stitched together, I've had a stab at implementing proc_exit purely in Rust. You can see the changes in my fork kubkon/wasmtime.

To quickly sum them up though, I've removed wasmtime_ssp_proc_exit (and the relevant entries in posix.h), and moved the logic into separate submodule of syscalls/proc_exit.rs. I've replaced the call to wasmtime_ssp_proc_exit with ::std::process::exit so that proc_exit function is now implemented in Rust. I've left the C shims as they were however. Are we after completely removing those as well or are we going to continue storing C-style function pointers in the relevant PrimaryMaps in instantiate::instantiate_wasi? And more importantly, is this the direction you've had in mind?

@sunfishcode
Copy link
Member Author

That's a good question. I don't have a specific plan in mind for how to handle that yet. I'll think about it, though if you have ideas about how best to do that, I'd be interested in hearing them too.

@sunfishcode
Copy link
Member Author

Ok, here's a concrete idea. What if we leave the shim logic and the rest of syscalls.rs as is, and just replace the wasmtime_ssp_proc_exit calls right now? The logic in syscalls.rs for other system calls handles translating arguments from wasm32 to host values, including translating pointers, which is relatively mechanical, so it's nice to keep that code separate from the code that actually implements the main functionality. Does that make sense?

@kubkon
Copy link
Member

kubkon commented Apr 27, 2019

SGTM! I'll draft it out and submit a PR, and I guess we can take it from there.

@sunfishcode
Copy link
Member Author

sunfishcode commented Apr 27, 2019

Multiple people are interested in this, which is great! To help avoid duplicating work, here are a few rough groups of functions that people can start with, with the more complex ones toward the end.

  • {environ, args}_sizes/get} (being worked on by @kubkon and/or Replace {environ,size}_{sizes_get,get} C code with Rust #126)
  • prestat_*
  • sched_yield
  • random_get, clock_res_get, clock_time_get
  • fd_sync, fd_datasync, fd_close, fd_advise (of the fd_* functions, let's do these first)
  • fd_seek, fd_tell, fd_renumber,
  • fd_fdstat_set_flags, fd_fdstat_set_rights
  • fd_read, fd_write, fd_fdstat_get
  • fd_pread, fd_pwrite,
  • path_create_directory, path_remove_directory, path_unlink_file, path_filestat_get (of the path_* functions, let's do these first)
  • path_filestat_get, path_filestat_set_times,
  • path_rename, path_symlink
  • fd_readdir
  • path_open
  • poll_oneoff

If you want to start one of these groups, please check here and then post about it, and we'll figure out the details as we go. As always, feel free to ask questions!

@pchickey
Copy link
Contributor

Some of these already have implementations over in lucet-wasi. Lucet uses the same license as Cranelift, so you can lift the source into this project with just copyright attribution. Or you can just use them as a reference and design your own.

As a separate note, we could probably figure out a way for Lucet runtime and wasmtime to present the same API to this sort of function, so that the same crate could be used in both runtimes.

@kubkon
Copy link
Member

kubkon commented Apr 28, 2019

@pchickey I don't mind lifting the implementation from lucet-wasi. What is your preference @sunfishcode? I also really like the idea of sharing the implementations between the runtimes!

If it's OK with everyone, I'd like to have a go at the prestat_*s; i.e., the fd_prestat_get and fd_prestat_dir_name.

EDIT: and while I'm at it, I can also have a look at sched_yield although initially probably rewriting as *nix compatible. Any guidance here (as well anywhere else in fact) will be much appreciated!

@sunfishcode
Copy link
Member Author

As an update here, I talked with @pchickey and others and they suggested a way we can proceed here, which is to add a cargo git dependency on lucet-wasi (it's not on crates.io yet, but a git dependency is fine for now). I think it makes sense to finish up the fd_prestat_dir_name support here, but it looks like the next logical step is to start working with the lucet-wasi code as a git dependency and see where that takes us.

@kubkon Would you be willing to create a branch with such a depedency, so we can start exploring it?

@kubkon
Copy link
Member

kubkon commented May 3, 2019

@sunfishcode Sure thing, I'll be happy to do that!

@kubkon
Copy link
Member

kubkon commented May 6, 2019

So, I've had a closer look at lucet-wasi and I've done some preliminary work at extracting the functionality which I thought could be reused between the Lucet and Wastime projects. You can find the extracted bits in the repo wasi-common. I've also made sure that it will work fine with both Wasmtime and Lucet runtimes which you can access in my local forks: wasmtime#lucet-wasi and lucet#lucet-wasi. I should warn you though that in order to make that happen I've had to resort to some pretty messy hacks, so please do bear that in mind. If we'll decide to continue down that road, however, I'm sure we'll be able to clean it all up.

Anyhow, here's a brief summary of the changes to lucet-wasi crate I've had to make to make it pluggable in both projects:

  • the proc_exit hostcall relies on stack unwinding and custom panic handling specific to lucet-runtime which I had to strip away (this not to say that it could not be ported over to wasmtime as well, or perhaps, if I got it right, the trap in wasmtime could be extended to handle this)
  • as in the point above, all of the hostcalls in lucet-wasi were wrapped inside custom stack unwinding in the form lucet_hostcalls! macro, which I essentially left out for now
  • the actual Rust implementation of all the hostcalls provided by lucet-wasi is left pretty much intact; the caveat here was to create a common interface between wasmtime's VMContext and lucet's VmCtx structs. I've done that in the form of a VmContext trait which requires provision of 3 methods: as_wasi_ctx, as_wasi_ctx_mut and dec_ptr. The former two are meant to allow to "deref"/convert the struct into lucet's WasiCtx struct which is essentially Rust's implementation of WasiState. The latter dec_ptr corresponds to wasmtime's decode_ptr and is the only fn which is runtime specific.
  • since the code now relies on WasiCtx to hold the prestats, I've had to slightly modify how wasmtime handles preopens, etc.

All in all, as you can see in wasmtime#lucet-wasi, the wasmtime-wasi crate is now reduced to instantiate_wasi fn, the wrapper around the hostcalls macro syscalls! (I tried not to modify the bit responsible for storing the fn pointers in wasmtime just yet), and decode_ptr fn, and that's it! Similar truth holds for lucet-wasi crate in lucet#lucet-wasi.

Finally, I've tested both runtimes with the introduced changes and they work and test out fine. As I said before, however, the code is rather messy and will need to be cleaned up should we decide to continue going down that road. Anyhow, @sunfishcode and @pchickey I'm curious to hear out your opinions!

@pchickey
Copy link
Contributor

pchickey commented May 6, 2019

I'm going to hand this off to @acfoltzer and @jedisct1 - they wrote lucet-wasi, and Adam designed the lucet_hostcalls! macro and is taking the lead on all things unwinding.

@kubkon
Copy link
Member

kubkon commented May 14, 2019

For those of you tracking the issue, you can find our efforts for this in CraneStation/wasi-common repo.

@kubkon
Copy link
Member

kubkon commented Jun 26, 2019

Since it’s well and ongoing in CraneStation/wasi-common, I’m going to close this issue as it’s no longer directly applicable to wasmtime.

@kubkon kubkon closed this as completed Jun 26, 2019
grishasobol pushed a commit to grishasobol/wasmtime that referenced this issue Nov 29, 2021
pchickey added a commit to pchickey/wasmtime that referenced this issue May 16, 2023
mooori pushed a commit to mooori/wasmtime that referenced this issue Dec 20, 2023
dhil pushed a commit to dhil/wasmtime that referenced this issue Mar 6, 2024
This PR adds tests for the backtrace support added in earlier PRs in
this series
avanhatt pushed a commit to wellesley-prog-sys/wasmtime that referenced this issue Oct 9, 2024
Generate `AluRRRShift` spec with ASLp.

This is the second example of a spec that relies on symbolic opcodes,
but it's slightly more challenging because the shift amount determines
the size of the symbolic field. Not only that, but the spec cases split
on `shiftop`, which is a struct type. In addition, the `lshl_from_imm64`
spec needed to be fixed.

Updates avanhatt#36 avanhatt#35
Closes bytecodealliance#120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants