Skip to content

Reduce boilerplate in wasmtime-wasi#707

Merged
alexcrichton merged 1 commit intobytecodealliance:masterfrom
alexcrichton:wasi-witx
Dec 16, 2019
Merged

Reduce boilerplate in wasmtime-wasi#707
alexcrichton merged 1 commit intobytecodealliance:masterfrom
alexcrichton:wasi-witx

Conversation

@alexcrichton
Copy link
Member

This commit uses the *.witx files describing the current wasi API to
reduce the boilerplate used to define implementations in the
wasmtime-wasi crate. Eventually I'd like to remove lots of boilerplate
in the wasi-common crate too, but this should at least be a good start!

The boilerplate removed here is:

  • No need to list each function to add it to the
    wasmtime_runtime::Module being created

  • No need to list the signature of the function in a separate
    syscalls.rs file.

Instead the *.witx file is processed in a single-use macro inside the
wasmtime-wasi crate. This macro uses the signatures known from
*.witx to automatically register with the right type in the wasm
module as well as define a wrapper that the wasm module will call into.
Functionally this is all the same as before, it's just defined in a
different way now!

The shim generated by this macro which wasmtime calls into only uses
i32/i64/etc wasm types, and it internally uses as casts to convert
to the right wasi types when delegating into the wasi-common crate.

One change was necessary to get this implemented, however. The functions
in wasi-common sometimes took WasiCtx and sometimes took a slice of
memory. After this PR they uniformly all require both WasiCtx and
memory so the wrappers can be auto-generated. The arguments are ignored
if they weren't previously required.

This commit uses the `*.witx` files describing the current wasi API to
reduce the boilerplate used to define implementations in the
`wasmtime-wasi` crate. Eventually I'd like to remove lots of boilerplate
in the `wasi-common` crate too, but this should at least be a good start!

The boilerplate removed here is:

* No need to list each function to add it to the
  `wasmtime_runtime::Module` being created

* No need to list the signature of the function in a separate
  `syscalls.rs` file.

Instead the `*.witx` file is processed in a single-use macro inside the
`wasmtime-wasi` crate. This macro uses the signatures known from
`*.witx` to automatically register with the right type in the wasm
module as well as define a wrapper that the wasm module will call into.
Functionally this is all the same as before, it's just defined in a
different way now!

The shim generated by this macro which wasmtime calls into only uses
`i32`/`i64`/etc wasm types, and it internally uses `as` casts to convert
to the right wasi types when delegating into the `wasi-common` crate.

One change was necessary to get this implemented, however. The functions
in `wasi-common` sometimes took `WasiCtx` and sometimes took a slice of
memory. After this PR they uniformly all require both `WasiCtx` and
memory so the wrappers can be auto-generated. The arguments are ignored
if they weren't previously required.
@kubkon
Copy link
Member

kubkon commented Dec 16, 2019

This looks really good to me! I thought I might have been a little too clever in trying to have syscalls not requiring all input args omit them which unnecessarily complicated all macro use. I'm wondering here, do you think we could reuse the witx approach in autogenerating the JS wrappers for the polyfill in #720?

cc @sunfishcode should have a look this as well.

@alexcrichton
Copy link
Member Author

Certainly! I think for basically anything that enumerates all of WASI would likely be best done via the witx files. There's a bit more in wasi-common I'd like to auto-generated, but I figured it'd be best to do incrementally

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do expect we'll eventually want to infer whether we need to pass in a memory slice or WasiCtx -- a slice is needed iff a function takes a pointer, const_pointer, string, array, or, for now, has multiple results, and a WasiCtx is needed iff a function takes a handle. But I'm also ok deferring that for later, so that we can focus on iterating on the bigger picture here. I agree there's more we can auto-generate!

@alexcrichton alexcrichton merged commit cc4be18 into bytecodealliance:master Dec 16, 2019
@alexcrichton alexcrichton deleted the wasi-witx branch December 16, 2019 22:37
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jan 21, 2020
This commit is effectively just doing what bytecodealliance#707 already did, but
applying it to the `snapshot_0` module as well. The end result is the
same, where we cut down on all the boilerplate in `snapshot_0` and bring
it in line with the main `wasi_snapshot_preview1` implementation. The
goal here is to make it easier to change the two in tandem since they're
both doing the same thing.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jan 21, 2020
This commit is effectively just doing what bytecodealliance#707 already did, but
applying it to the `snapshot_0` module as well. The end result is the
same, where we cut down on all the boilerplate in `snapshot_0` and bring
it in line with the main `wasi_snapshot_preview1` implementation. The
goal here is to make it easier to change the two in tandem since they're
both doing the same thing.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jan 22, 2020
This commit is effectively just doing what bytecodealliance#707 already did, but
applying it to the `snapshot_0` module as well. The end result is the
same, where we cut down on all the boilerplate in `snapshot_0` and bring
it in line with the main `wasi_snapshot_preview1` implementation. The
goal here is to make it easier to change the two in tandem since they're
both doing the same thing.
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jan 22, 2020
This commit is effectively just doing what bytecodealliance#707 already did, but
applying it to the `snapshot_0` module as well. The end result is the
same, where we cut down on all the boilerplate in `snapshot_0` and bring
it in line with the main `wasi_snapshot_preview1` implementation. The
goal here is to make it easier to change the two in tandem since they're
both doing the same thing.
alexcrichton added a commit that referenced this pull request Jan 22, 2020
* Auto-generate shims for old `wasi_unstable` module

This commit is effectively just doing what #707 already did, but
applying it to the `snapshot_0` module as well. The end result is the
same, where we cut down on all the boilerplate in `snapshot_0` and bring
it in line with the main `wasi_snapshot_preview1` implementation. The
goal here is to make it easier to change the two in tandem since they're
both doing the same thing.

* Migrate `wasi_common::hostcalls` to a macro

This commit migrates the `hostcalls` module to being auto-generated by a
macro rather than duplicating a handwritten signature for each wasi
syscall.

* Auto-generate snapshot_0's `hostcalls` module

Similar to the previous commit, but for `snapshot_0`

* Delete the `wasi-common-cbindgen` crate

This is no longer needed with the hostcalls macro now, we can easily
fold the definition of the cbindgen macro into the same crate.

* Rustfmt

* Fix windows build errors

* Rustfmt

* Remove now no-longer-necessary code

* rustfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants