Return `anyhow::Error` from host functions instead of `Trap`, redesign `Trap` #5149

alexcrichton · 2022-10-28T16:21:21Z

This series of commits is a large refactoring of how Trap works and is used in Wasmtime. Individual commits have more details but at a high-level the changes implemented here are:

All functions in Wasmtime now work with Result<T> instead of some working with Result<T, Trap>. For example TypedFunc::call reutrns Result<T> now, in addition to the host-provided closure to Func::wrap.
Instances of anyhow::Error are threaded around as-is, so if the host returns a specific error that error pops out the other end.
The Trap::i32_exit function has been removed in favor of a concrete wasi_common::I32Exit error type
The Trap::new function has been removed in favor of anyhow::Error constructors like bail!
The Trap type has been removed and replaced with TrapCode. It's now just a simple enum
Wasm backtraces are modeled through a new WasmBacktrace type that's attached with .context(...) to anyhow::Error errors.

This is a large refactoring of how traps work in Wasmtime where the main goal is to pare down the responsibility of Trap and instead let anyhow do all the heavy lifting. Embedders which handle various kinds of traps various ways will need to be adjusted accordingly.

github-actions · 2022-10-28T16:34:11Z

Subscribe to Label Action

cc @fitzgen, @kubkon, @peterhuene

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

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

fitzgen: fuzzing
kubkon: wasi
peterhuene: wasmtime:api, wasmtime:c-api

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

Learn more.

alexcrichton · 2022-11-01T17:58:16Z

Ok I'm actually working through a good deal more refactorings for Trap, namely I've done away with all the internal structure of Trap in favor of just enum Trap { ... } where backtrace information is always attached via a .context(...) call. This fixes a prior issue with this PR that accidentally brought back the O(n^2) behavior if a non-Trap was returned.

I still have some more documentation I'd like to update, however, and additionally rename BacktraceContext to WasmBacktrace and shuffle around its struct definition. This has what should be the main meat of what @pchickey and I have settled on.

pchickey

I've sent all my suggestions in as diffs, this is awesome!

This commit refactors how errors are modeled when returned from host functions and additionally refactors how custom errors work with `Trap`. At a high level functions in Wasmtime that previously worked with `Result<T, Trap>` now work with `Result<T>` instead where the error is `anyhow::Error`. This includes functions such as: * Host-defined functions in a `Linker<T>` * `TypedFunc::call` * Host-related callbacks like call hooks Errors are now modeled primarily as `anyhow::Error` throughout Wasmtime. This subsequently removes the need for `Trap` to have the ability to represent all host-defined errors as it previously did. Consequently the `From` implementations for any error into a `Trap` have been removed here and the only embedder-defined way to create a `Trap` is to use `Trap::new` with a custom string. After this commit the distinction between a `Trap` and a host error is the wasm backtrace that it contains. Previously all errors in host functions would flow through a `Trap` and get a wasm backtrace attached to them, but now this only happens if a `Trap` itself is created meaning that arbitrary host-defined errors flowing from a host import to the other side won't get backtraces attached. Some internals of Wasmtime itself were updated or preserved to use `Trap::new` to capture a backtrace where it seemed useful, such as when fuel runs out. The main motivation for this commit is that it now enables hosts to thread a concrete error type from a host function all the way through to where a wasm function was invoked. Previously this could not be done since the host error was wrapped in a `Trap` that didn't provide the ability to get at the internals. A consequence of this commit is that when a host error is returned that isn't a `Trap` we'll capture a backtrace and then won't have a `Trap` to attach it to. To avoid losing the contextual information this commit uses the `Error::context` method to attach the backtrace as contextual information to ensure that the backtrace is itself not lost. This is a breaking change for likely all users of Wasmtime, but it's hoped to be a relatively minor change to workaround. Most use cases can likely change `-> Result<T, Trap>` to `-> Result<T>` and otherwise explicit creation of a `Trap` is largely no longer necessary.

* Trap: avoid a trailing newline in the Display impl which in turn ends up with three newlines between the end of the backtrace and the `Caused by` in the anyhow Debug impl * make BacktraceContext pub, and add tests showing downcasting behavior of anyhow::Error to traps or backtraces

This commit removes special-handling in the `wasmtime::Trap` type for the i32 exit code required by WASI. This is now instead modeled as a specific `I32Exit` error type in the `wasmtime-wasi` crate which is returned by the `proc_exit` hostcall. Embedders which previously tested for i32 exits now downcast to the `I32Exit` value.

This commit removes the ability to create a trap with an arbitrary error message. The purpose of this commit is to continue the prior trend of leaning into the `anyhow::Error` type instead of trying to recreate it with `Trap`. A subsequent simplification to `Trap` after this commit is that `Trap` will simply be an `enum` of trap codes with no extra information. This commit is doubly-motivated by the desire to always use the new `BacktraceContext` type instead of sometimes using that and sometimes using `Trap`. Most of the changes here were around updating `Trap::new` calls to `bail!` calls instead. Tests which assert particular error messages additionally often needed to use the `:?` formatter instead of the `{}` formatter because the prior formats the whole `anyhow::Error` and the latter only formats the top-most error, which now contains the backtrace.

With prior refactorings there's no more need for `Trap` to be opaque or otherwise contain a backtrace. This commit parse down `Trap` to simply an `enum` which was the old `TrapCode`. All various tests and such were updated to handle this. The main consequence of this commit is that all errors have a `BacktraceContext` context attached to them. This unfortunately means that the backtrace is printed first before the error message or trap code, but given all the prior simplifications that seems worth it at this time.

This feels like a better name given how this has turned out, and additionally this commit removes having both `WasmBacktrace` and `BacktraceContext`.

alexcrichton · 2022-11-01T19:18:39Z

@pchickey mind double-checking the most recent few commits to confirm they look ok to you?

pchickey

Latest commits look good. Sending you the commit to get the c-api building again, basically we need to derive Clone on all the backtrace structures because theres no more hiding it behind an arc. (Should we use an arc inside?)

This commit was a mistake from bytecodealliance#5149

This commit was a mistake from #5149

* c-api: Avoid losing error context with instance traps This commit was a mistake from #5149 * Update how exits are modeled in the C API (#5215) Previously extracting an exit code was only possibly on a `wasm_trap_t` which will never successfully have an exit code on it, so the exit code extractor is moved over to `wasmtime_error_t`. Additionally extracting a wasm trace from a `wasmtime_error_t` is added since traces happen on both traps and errors now.

Change the return value of this function to a `wasmtime_error_t*` instead of the prior `wasm_trap_t*`. This is a leftover from bytecodealliance#5149. Closes bytecodealliance#5257

…5262) Change the return value of this function to a `wasmtime_error_t*` instead of the prior `wasm_trap_t*`. This is a leftover from #5149. Closes #5257

…ytecodealliance#5262) Change the return value of this function to a `wasmtime_error_t*` instead of the prior `wasm_trap_t*`. This is a leftover from bytecodealliance#5149. Closes bytecodealliance#5257

…5262) (#5263) Change the return value of this function to a `wasmtime_error_t*` instead of the prior `wasm_trap_t*`. This is a leftover from #5149. Closes #5257

In Wasmtime 3, `Trap` is now only an Enum (just like `TrapCode` was). See bytecodealliance/wasmtime#5149 I've adjusted the Ruby bindings to match that change: - Moved the trap code constants on `Trap`, removing the `TrapCode` module. - Renamed `Trap#trap_code` to `Trap#trap`. - Made `Trap#message` exclude the Wasm backtrace by default, and instead added a `wasm_backtrace_message` method for that. I chose this name so we can still implement `wasm_backtrace` that returns an Enumerable of traces, if we ever have the need. This is probably fine for now. In the spirit of matching Wasmtime closely, I wonder if we should instead have a single error type (`Wasmtime::Error`) and define `trap_code` and `wasm_backtrace` on it. We can take that on later and suggest merging this as it's a smaller diff and still gets us on Wasmtime 3.

Fixes compatibility with redesigned error handling (Result<T, wasmtime::Trap> -> anyhow::Result<T>). Refs: bytecodealliance/wasmtime#5149 Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

alexcrichton requested a review from pchickey October 28, 2022 16:21

github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasi Issues pertaining to WASI wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Oct 28, 2022

alexcrichton force-pushed the refactor-trap branch from 5f4d08f to 74784cd Compare October 31, 2022 15:19

github-actions bot added the cranelift Issues related to the Cranelift code generator label Oct 31, 2022

pchickey approved these changes Nov 1, 2022

View reviewed changes

alexcrichton and others added 10 commits November 1, 2022 12:15

Fix some doc links

46d0ce8

Remove now-unnecesary Trap downcasts in Linker::module

1ff3d78

Fix test output expectations

35d68e1

Rename BacktraceContext to WasmBacktrace

9b0275e

This feels like a better name given how this has turned out, and additionally this commit removes having both `WasmBacktrace` and `BacktraceContext`.

Soup up documentation for errors and traps

a651f23

alexcrichton force-pushed the refactor-trap branch from c111eb2 to a651f23 Compare November 1, 2022 19:18

alexcrichton changed the title ~~Return anyhow::Error from host functions instead of Trap~~ Return anyhow::Error from host functions instead of Trap, redesign Trap Nov 1, 2022

github-actions bot added the wasmtime:config Issues related to the configuration of Wasmtime label Nov 1, 2022

This comment was marked as resolved.

Sign in to view

pchickey approved these changes Nov 1, 2022

View reviewed changes

Fix build of the C API

053943a

alexcrichton enabled auto-merge (squash) November 2, 2022 14:30

Fix a build warning

a15448e

alexcrichton merged commit 2afaac5 into bytecodealliance:main Nov 2, 2022

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Nov 8, 2022

c-api: Avoid losing error context with instance traps

7ab20a1

This commit was a mistake from bytecodealliance#5149

alexcrichton mentioned this pull request Nov 8, 2022

c-api: Avoid losing error context with instance traps #5223

Merged

alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Nov 8, 2022

c-api: Avoid losing error context with instance traps

c61b1ce

This commit was a mistake from bytecodealliance#5149

alexcrichton mentioned this pull request Nov 8, 2022

[3.0.0] Backport some C-API trap-related changes #5224

Merged

alexcrichton added a commit that referenced this pull request Nov 8, 2022

c-api: Avoid losing error context with instance traps (#5223)

7b5fd84

This commit was a mistake from #5149

kpreisser mentioned this pull request Nov 12, 2022

Wasmtime C API: Missing way to retrieve the WASI exit code when using wasmtime_func_call_unchecked #5257

Closed

alexcrichton mentioned this pull request Nov 14, 2022

c-api: Fix wasmtime_func_call_unchecked to communicate all errors #5262

Merged

alexcrichton mentioned this pull request Nov 14, 2022

[3.0.0] c-api: Fix wasmtime_func_call_unchecked to communicate all errors (… #5263

Merged

jbourassa mentioned this pull request Nov 22, 2022

Wasmtime 3.0.0 bytecodealliance/wasmtime-rb#41

Merged

etehtsea mentioned this pull request Nov 27, 2022

Bump wasmtime to 3.0.0 fermyon/wit-bindgen-backport#1

Merged

rvolosatovs mentioned this pull request Dec 7, 2022

build: update wasmtime to 3.0.1 enarx/vfs#27

Merged

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 4, 2023

update to wasmtime 3.0.0

e6f0b8b

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion mentioned this pull request Jan 4, 2023

update to wasmtime 3.0.0 bytecodealliance/wizer#65

Merged

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 4, 2023

update to wasmtime 3.0.0

63deecd

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 4, 2023

update to wasmtime 3.0.0

62af22b

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 4, 2023

update to wasmtime 3.0.0

40d59ea

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 8, 2023

update to wasmtime 3.0.0

624cb13

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 8, 2023

update to wasmtime 3.0.0

9e02d39

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Jan 9, 2023

update to wasmtime 3.0.0

52c4070

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Apr 15, 2023

update to wasmtime 3.0.0

4caa57d

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

JakeChampion added a commit to bytecodealliance/wizer that referenced this pull request Apr 15, 2023

update to wasmtime 3.0.0

f8b023f

this update required changes from the removed `Trap::new` to anyhow! macro, Trap::new was removed in bytecodealliance/wasmtime#5149

Manishearth mentioned this pull request Feb 26, 2024

Update wasi-common to 17.0 wasmi-labs/wasmi#941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return `anyhow::Error` from host functions instead of `Trap`, redesign `Trap` #5149

Return `anyhow::Error` from host functions instead of `Trap`, redesign `Trap` #5149

alexcrichton commented Oct 28, 2022 •

edited

Loading

github-actions bot commented Oct 28, 2022

alexcrichton commented Nov 1, 2022

pchickey left a comment

alexcrichton commented Nov 1, 2022

This comment was marked as resolved.

pchickey left a comment

Return anyhow::Error from host functions instead of Trap, redesign Trap #5149

Return anyhow::Error from host functions instead of Trap, redesign Trap #5149

Conversation

alexcrichton commented Oct 28, 2022 • edited Loading

github-actions bot commented Oct 28, 2022

Subscribe to Label Action

alexcrichton commented Nov 1, 2022

pchickey left a comment

Choose a reason for hiding this comment

alexcrichton commented Nov 1, 2022

This comment was marked as resolved.

pchickey left a comment

Choose a reason for hiding this comment

Return `anyhow::Error` from host functions instead of `Trap`, redesign `Trap` #5149

Return `anyhow::Error` from host functions instead of `Trap`, redesign `Trap` #5149

alexcrichton commented Oct 28, 2022 •

edited

Loading