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

wiggle: choose between &mut self and &self #5428

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Dec 13, 2022

Previously, all Wiggle-generated traits were generated with &mut self signatures. With the addition of the mutable configuration option to from_witx! and wasmtime_integration!, one can disable this, emitting instead traits that use &self (i.e., mutable: false). This change is helpful for implementing wasi-threads: WASI implementations with interior mutability will now be able to communitcate this to their Wiggle-generated code.

The other side of this change is the get_cx closure passed to Wiggle's generated add_to_linker function. When mutability is set to true (default), the get_cx function takes a &mut data structure from the store and returns a corresponding &mut reference, usually to a field of the passed-in structure. When mutability: false, the get_cx closure will still take a &mut data structure but now will return a & reference.

Previously, all Wiggle-generated traits were generated with `&mut self`
signatures. With the addition of the `mutable` configuration option to
`from_witx!` and `wasmtime_integration!`, one can disable this, emitting
instead traits that use `&self` (i.e., `mutable: false`). This change is
helpful for implementing wasi-threads: WASI implementations with
interior mutability will now be able to communitcate this to their
Wiggle-generated code.

The other side of this change is the `get_cx` closure passed to Wiggle's
generated `add_to_linker` function. When `mutability` is set to `true`
(default), the `get_cx` function takes a `&mut` data structure from the
store and returns a corresponding `&mut` reference, usually to a field
of the passed-in structure. When `mutability: false`, the `get_cx`
closure will still take a `&mut` data structure but now will return a
`&` reference.
@abrown abrown marked this pull request as ready for review December 13, 2022 20:01
@abrown abrown merged commit 3ce896f into bytecodealliance:main Dec 13, 2022
@abrown abrown deleted the wiggle-mutability branch December 13, 2022 22:38
abrown added a commit to abrown/wasmtime that referenced this pull request Dec 20, 2022
After bytecodealliance#5236, it is now easier to modify the `wasi-common`
implementations (both `preview0` and `preview1`) to use `&self` instead
of `&mut self` in their function signatures. This makes it possible to
access and use a `WasiCtx` from an `Arc<Host>` (the top-level Wasmtime
CLI structure holding the WASI implementations). An `Arc<Host>` will
only give out immutable access to the WASI implementations it contains.
This relies on bytecodealliance#5428 to configure Wiggle to emit `&self`-receiving
traits.

This change also wraps `wasi-common`'s source of randomness in `WasiCtx`
with a `Mutex` to enable this `&mut self` to `&self` refactoring.

One remaining issue to figure out with this change is the interaction
with the `async` side of wiggle. E.g., `cargo build --all-features -p
wasmtime-wasi` will fail due to the `tokyo` feature being enabled. The
errors have to do with incorrect `Send` and `Sync` bounds on various
parts of the Wiggle-generated API. We had previously discussed turning
off these `async` parts when the `wasi-threads` feature is enabled, or
at least prevent these features from being enabled at the same time,
since as-is even `cargo test --all` is affected.
abrown added a commit to abrown/wasmtime that referenced this pull request Jan 6, 2023
After bytecodealliance#5236, it is now easier to modify the `wasi-common`
implementations (both `preview0` and `preview1`) to use `&self` instead
of `&mut self` in their function signatures. This makes it possible to
access and use a `WasiCtx` from an `Arc<Host>` (the top-level Wasmtime
CLI structure holding the WASI implementations). An `Arc<Host>` will
only give out immutable access to the WASI implementations it contains.
This relies on bytecodealliance#5428 to configure Wiggle to emit `&self`-receiving
traits.

This change also wraps `wasi-common`'s source of randomness in `WasiCtx`
with a `Mutex` to enable this `&mut self` to `&self` refactoring.

One remaining issue to figure out with this change is the interaction
with the `async` side of wiggle. E.g., `cargo build --all-features -p
wasmtime-wasi` will fail due to the `tokyo` feature being enabled. The
errors have to do with incorrect `Send` and `Sync` bounds on various
parts of the Wiggle-generated API. We had previously discussed turning
off these `async` parts when the `wasi-threads` feature is enabled, or
at least prevent these features from being enabled at the same time,
since as-is even `cargo test --all` is affected.
abrown added a commit to abrown/wasmtime that referenced this pull request Jan 9, 2023
After bytecodealliance#5236, it is now easier to modify the `wasi-common`
implementations (both `preview0` and `preview1`) to use `&self` instead
of `&mut self` in their function signatures. This makes it possible to
access and use a `WasiCtx` from an `Arc<Host>` (the top-level Wasmtime
CLI structure holding the WASI implementations). An `Arc<Host>` will
only give out immutable access to the WASI implementations it contains.
This relies on bytecodealliance#5428 to configure Wiggle to emit `&self`-receiving
traits.

This change also wraps `wasi-common`'s source of randomness in `WasiCtx`
with a `Mutex` to enable this `&mut self` to `&self` refactoring.

One remaining issue to figure out with this change is the interaction
with the `async` side of wiggle. E.g., `cargo build --all-features -p
wasmtime-wasi` will fail due to the `tokyo` feature being enabled. The
errors have to do with incorrect `Send` and `Sync` bounds on various
parts of the Wiggle-generated API. We had previously discussed turning
off these `async` parts when the `wasi-threads` feature is enabled, or
at least prevent these features from being enabled at the same time,
since as-is even `cargo test --all` is affected.
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.

None yet

2 participants