Skip to content

Commit

Permalink
Simplify some trap handling in wasmtime runtime (#9673)
Browse files Browse the repository at this point in the history
The `needs_backtrace` field on `TrapReason` is an old artifact of having
the `wasmtime` and `wasmtime-runtime` crates split and there's no longer
any need for that. This commit removes the field and directly queries it
from the error as necessary when capturing backtraces.
  • Loading branch information
alexcrichton authored Nov 25, 2024
1 parent 6767488 commit 6c97d5e
Show file tree
Hide file tree
Showing 7 changed files with 19 additions and 51 deletions.
2 changes: 1 addition & 1 deletion crates/wasmtime/src/runtime/component/func/host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ unsafe fn call_host_and_handle_result<T>(

match res {
Ok(()) => {}
Err(e) => crate::trap::raise(e),
Err(e) => crate::runtime::vm::raise_user_trap(e),
}
}

Expand Down
2 changes: 1 addition & 1 deletion crates/wasmtime/src/runtime/func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2371,7 +2371,7 @@ impl HostContext {

match result {
Ok(val) => val,
Err(err) => crate::trap::raise(err),
Err(err) => crate::runtime::vm::raise_user_trap(err),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/wasmtime/src/runtime/trampoline/func.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ unsafe extern "C" fn array_call_shim<F>(
// call-site, which gets unwrapped in `Trap::from_runtime` later on as we
// convert from the internal `Trap` type to our own `Trap` type in this
// crate.
Err(trap) => crate::trap::raise(trap.into()),
Err(err) => crate::runtime::vm::raise_user_trap(err),
}
}

Expand Down
17 changes: 1 addition & 16 deletions crates/wasmtime/src/runtime/trap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,6 @@ use wasmtime_environ::{demangle_function_name, demangle_function_name_or_index,
/// ```
pub use wasmtime_environ::Trap;

// Same safety requirements and caveats as
// `crate::runtime::vm::raise_user_trap`.
pub(crate) unsafe fn raise(error: anyhow::Error) -> ! {
let needs_backtrace = error.downcast_ref::<WasmBacktrace>().is_none();
crate::runtime::vm::raise_user_trap(error, needs_backtrace)
}

#[cold] // traps are exceptional, this helps move handling off the main path
pub(crate) fn from_runtime_box(
store: &mut StoreOpaque,
Expand All @@ -99,15 +92,7 @@ pub(crate) fn from_runtime_box(
// provide useful information to debug with for the embedder/caller,
// otherwise the information about what the wasm was doing when the
// error was generated would be lost.
crate::runtime::vm::TrapReason::User {
error,
needs_backtrace,
} => {
debug_assert!(
needs_backtrace == backtrace.is_some() || !store.engine().config().wasm_backtrace
);
(error, None)
}
crate::runtime::vm::TrapReason::User(error) => (error, None),
#[cfg(all(feature = "signals-based-traps", not(miri)))]
crate::runtime::vm::TrapReason::Jit {
pc,
Expand Down
5 changes: 1 addition & 4 deletions crates/wasmtime/src/runtime/vm/component/libcalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,7 @@ mod trampolines {
match result {
Ok(ret) => shims!(@convert_ret ret $($pname: $param)*),
Err(err) => crate::runtime::vm::traphandlers::raise_trap(
crate::runtime::vm::traphandlers::TrapReason::User {
error: err,
needs_backtrace: true,
},
crate::runtime::vm::traphandlers::TrapReason::User(err)
),
}
}
Expand Down
7 changes: 1 addition & 6 deletions crates/wasmtime/src/runtime/vm/libcalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,7 @@ fn memory32_grow(
memory_index: u32,
) -> Result<*mut u8, TrapReason> {
let memory_index = MemoryIndex::from_u32(memory_index);
let result = match instance
.memory_grow(store, memory_index, delta)
.map_err(|error| TrapReason::User {
error,
needs_backtrace: true,
})? {
let result = match instance.memory_grow(store, memory_index, delta)? {
Some(size_in_bytes) => size_in_bytes / instance.memory_page_size(memory_index),
None => usize::max_value(),
};
Expand Down
35 changes: 13 additions & 22 deletions crates/wasmtime/src/runtime/vm/traphandlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub use self::signals::*;
use crate::prelude::*;
use crate::runtime::vm::sys::traphandlers;
use crate::runtime::vm::{Instance, VMContext, VMRuntimeLimits};
use crate::WasmBacktrace;
use core::cell::{Cell, UnsafeCell};
use core::mem::MaybeUninit;
use core::ops::Range;
Expand Down Expand Up @@ -58,11 +59,8 @@ pub unsafe fn raise_trap(reason: TrapReason) -> ! {
/// Only safe to call when wasm code is on the stack, aka `catch_traps` must
/// have been previously called. Additionally no Rust destructors can be on the
/// stack. They will be skipped and not executed.
pub unsafe fn raise_user_trap(error: Error, needs_backtrace: bool) -> ! {
raise_trap(TrapReason::User {
error,
needs_backtrace,
})
pub unsafe fn raise_user_trap(error: Error) -> ! {
raise_trap(TrapReason::User(error))
}

/// Invokes the closure `f` and returns the result.
Expand Down Expand Up @@ -113,12 +111,7 @@ pub struct Trap {
#[derive(Debug)]
pub enum TrapReason {
/// A user-raised trap through `raise_user_trap`.
User {
/// The actual user trap error.
error: Error,
/// Whether we need to capture a backtrace for this error or not.
needs_backtrace: bool,
},
User(Error),

/// A trap raised from Cranelift-generated code.
#[cfg(all(feature = "signals-based-traps", not(miri)))]
Expand Down Expand Up @@ -150,17 +143,14 @@ pub enum TrapReason {

impl TrapReason {
/// Create a new `TrapReason::User` that does not have a backtrace yet.
pub fn user_without_backtrace(error: Error) -> Self {
TrapReason::User {
error,
needs_backtrace: true,
}
pub fn user(error: Error) -> Self {
TrapReason::User(error)
}
}

impl From<Error> for TrapReason {
fn from(err: Error) -> Self {
TrapReason::user_without_backtrace(err)
TrapReason::user(err)
}
}

Expand Down Expand Up @@ -359,7 +349,7 @@ impl CallThreadState {
}

fn unwind_with(&self, reason: UnwindReason) -> ! {
let (backtrace, coredump) = match reason {
let (backtrace, coredump) = match &reason {
// Panics don't need backtraces. There is nowhere to attach the
// hypothetical backtrace to and it doesn't really make sense to try
// in the first place since this is a Rust problem rather than a
Expand All @@ -369,10 +359,11 @@ impl CallThreadState {
// And if we are just propagating an existing trap that already has
// a backtrace attached to it, then there is no need to capture a
// new backtrace either.
UnwindReason::Trap(TrapReason::User {
needs_backtrace: false,
..
}) => (None, None),
UnwindReason::Trap(TrapReason::User(err))
if err.downcast_ref::<WasmBacktrace>().is_some() =>
{
(None, None)
}
UnwindReason::Trap(_) => (
self.capture_backtrace(self.limits, None),
self.capture_coredump(self.limits, None),
Expand Down

0 comments on commit 6c97d5e

Please sign in to comment.