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

Disable traps by default in bindgen! imports #8310

Merged

Conversation

alexcrichton
Copy link
Member

By default previously all return types were wrapped in wasmtime::Result<T> to enable any import to return a trap to the wasm guest. This is a fair bit of boilerplate, however, and it's easy to accidentally turn a normal error into a trap. This is additionally somewhat of a power-user method as many imports probably don't end up wanting to trap.

This commit adds a new configuration option to bindgen!: trappable_imports, and switches the default to false. The previous behavior can be recovered with trappable_imports: true.

By default previously all return types were wrapped in
`wasmtime::Result<T>` to enable any import to return a trap to the wasm
guest. This is a fair bit of boilerplate, however, and it's easy to
accidentally turn a normal error into a trap. This is additionally
somewhat of a power-user method as many imports probably don't end up
wanting to trap.

This commit adds a new configuration option to `bindgen!`:
`trappable_imports`, and switches the default to `false`. The previous
behavior can be recovered with `trappable_imports: true`.
@alexcrichton alexcrichton requested a review from a team as a code owner April 5, 2024 21:59
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team April 5, 2024 21:59
@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself labels Apr 5, 2024
Copy link

github-actions bot commented Apr 5, 2024

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasi", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@fitzgen fitzgen requested review from elliottt and removed request for fitzgen April 8, 2024 15:40
Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Nice!

@alexcrichton alexcrichton added this pull request to the merge queue Apr 10, 2024
Merged via the queue into bytecodealliance:main with commit 1cf0060 Apr 10, 2024
19 checks passed
@alexcrichton alexcrichton deleted the no-trapping-imports branch April 10, 2024 20:21
@dearfl
Copy link

dearfl commented Jun 3, 2024

Hey guys, I'm fairly new to wasm/wasmtime/wasi, can you provide some per function trappable_imports example? [Nevermind, I found some example in this PR.]

Also I'd like to ask how should one handle errors in a Host impl?

Suppose I have some wit definition like this:

resource example {
    hello: func() -> result<string, error-type>,
}

The host side impl in Rust with trappable-imports is something like this

impl HostExample for ExampleContext {
    fn hello(&mut self, self_: Resource<Example>) -> wasmtime::Result<Result<String, ErrorType>> {
        // should I return a trap when resource table operations failed in here like this?
        // When should I return a trap/normal result? My current understanding is if something is
        // caused by wasmtime itself (like ResourceTable operation failed here), then I should
        // return a trap, else normal result. Is my understanding correct/sane?
        let ext: &mut Example = self.table.get_mut(&self_)?;
        // let say Example::hello returns Result<String, ErrorType>
        Ok(ext.hello())
    }

    fn drop(&mut self, self_: Resource<Example>) -> wasmtime::Result<()> {
        // same here
        self.table.delete(self_)?;
        Ok(())
    }
}

@alexcrichton
Copy link
Member Author

Looks like you might have already found your answer @dearfl but I can try to fill in any possible gaps and/or confirm:

  • Whether something should be a trap or a WIT-level error depends on whether the guest should be able to recover from it. Guests can't recover from traps but can recover from WIT-level errors.
  • Table-related errors are best represented as traps since that indicates a bug in the host or resource limits being hit from the guest if an error is returned.
  • You might be interested in the trappable_error_type configuration key to have a single Result<T,E> instead of Result<Result<T, E>>, but that's not required.

In any case the code you have gisted above looks reasonable to me!

@dearfl
Copy link

dearfl commented Jun 4, 2024

Looks like you might have already found your answer @dearfl but I can try to fill in any possible gaps and/or confirm:

* Whether something should be a trap or a WIT-level error depends on whether the guest should be able to recover from it. Guests can't recover from traps but can recover from WIT-level errors.

* Table-related errors are best represented as traps since that indicates a bug in the host or resource limits being hit from the guest if an error is returned.

* You might be interested in the `trappable_error_type` configuration key to have a single `Result<T,E>` instead of `Result<Result<T, E>>`, but that's not required.

In any case the code you have gisted above looks reasonable to me!

Thank you for your detailed reply.

maxdeviant added a commit to zed-industries/zed that referenced this pull request Jul 25, 2024
maxdeviant added a commit to zed-industries/zed that referenced this pull request Jul 25, 2024
This PR upgrades the version of `wasmtime` and `wasmtime-wasi` in use to
v21.0.1.

We have to skip v20 because Tree-sitter also skipped it.

Here are the changes that had to be made:

### v19 -> v20

After upgrading the `wasmtime` packages to v20, I also had to run `cargo
update -p mach2` to pull in
[v0.4.2](https://github.com/JohnTitor/mach2/releases/tag/0.4.2) to fix
some compile errors.

There were a few minor API changes in `wasmtime-wasi` from
bytecodealliance/wasmtime#8228 that we needed to
account for.

### v20 -> v21

Since there isn't a Tree-sitter version that depends on `wasmtime@v20`,
we're jumping straight to v21.

The published version of Tree-sitter (v0.22.6) still depends on
`wasmtime@v19`, but there was a commit
(tree-sitter/tree-sitter@7f4a578)
later that month that upgrades the `wasmtime` dependency to v21.

We're patching Tree-sitter to that commit so we can get the new
`wasmtime` version.

The main change in v21 is that imports generated by `bindgen!` are no
longer automatically trapped
(bytecodealliance/wasmtime#8310), so we need to
add `trappable_imports: true` to our `bindgen!` calls.

Release Notes:

- N/A
CharlesChen0823 pushed a commit to CharlesChen0823/zed that referenced this pull request Jul 29, 2024
This PR upgrades the version of `wasmtime` and `wasmtime-wasi` in use to
v21.0.1.

We have to skip v20 because Tree-sitter also skipped it.

Here are the changes that had to be made:

### v19 -> v20

After upgrading the `wasmtime` packages to v20, I also had to run `cargo
update -p mach2` to pull in
[v0.4.2](https://github.com/JohnTitor/mach2/releases/tag/0.4.2) to fix
some compile errors.

There were a few minor API changes in `wasmtime-wasi` from
bytecodealliance/wasmtime#8228 that we needed to
account for.

### v20 -> v21

Since there isn't a Tree-sitter version that depends on `wasmtime@v20`,
we're jumping straight to v21.

The published version of Tree-sitter (v0.22.6) still depends on
`wasmtime@v19`, but there was a commit
(tree-sitter/tree-sitter@7f4a578)
later that month that upgrades the `wasmtime` dependency to v21.

We're patching Tree-sitter to that commit so we can get the new
`wasmtime` version.

The main change in v21 is that imports generated by `bindgen!` are no
longer automatically trapped
(bytecodealliance/wasmtime#8310), so we need to
add `trappable_imports: true` to our `bindgen!` calls.

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants