From 77e06213b76fe45d78e94e290270db2b1b5c1ec8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 27 Jun 2022 12:35:14 -0500 Subject: [PATCH] Refactor the internals of traps in `wasmtime_runtime` (#4326) This commit is a small refactoring of `wasmtime_runtime::Trap` and various internals. The `Trap` structure is now a reason plus backtrace, and the old `Trap` enum is mostly in `TrapReason` now. Additionally all `Trap`-returning methods of `wasmtime_runtime` are changed to returning a `TrapCode` to indicate that they never capture a backtrace. Finally the `UnwindReason` internally now no longer duplicates the trap reasons, instead only having two variants of "panic" and "trap". The motivation for this commit is mostly just cleaning up trap internals and removing the need for methods like `wasmtime_runtime::Trap::insert_backtrace` to leave it only happening at the `wasmtime` layer. --- crates/runtime/src/instance.rs | 19 ++-- crates/runtime/src/instance/allocator.rs | 7 +- crates/runtime/src/lib.rs | 2 +- crates/runtime/src/libcalls.rs | 36 ++++---- crates/runtime/src/table.rs | 16 ++-- crates/runtime/src/traphandlers.rs | 107 +++++++---------------- crates/wasmtime/src/externals.rs | 8 +- crates/wasmtime/src/instance.rs | 2 +- crates/wasmtime/src/trap.rs | 14 ++- 9 files changed, 81 insertions(+), 130 deletions(-) diff --git a/crates/runtime/src/instance.rs b/crates/runtime/src/instance.rs index 1ae33e3e92..08f4f4c634 100644 --- a/crates/runtime/src/instance.rs +++ b/crates/runtime/src/instance.rs @@ -6,7 +6,6 @@ use crate::export::Export; use crate::externref::VMExternRefActivationsTable; use crate::memory::{Memory, RuntimeMemoryCreator}; use crate::table::{Table, TableElement, TableElementType}; -use crate::traphandlers::Trap; use crate::vmcontext::{ VMBuiltinFunctionsArray, VMCallerCheckedAnyfunc, VMContext, VMFunctionImport, VMGlobalDefinition, VMGlobalImport, VMMemoryDefinition, VMMemoryImport, VMOpaqueContext, @@ -561,7 +560,7 @@ impl Instance { dst: u32, src: u32, len: u32, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { // TODO: this `clone()` shouldn't be necessary but is used for now to // inform `rustc` that the lifetime of the elements here are // disconnected from the lifetime of `self`. @@ -583,7 +582,7 @@ impl Instance { dst: u32, src: u32, len: u32, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { // https://webassembly.github.io/bulk-memory-operations/core/exec/instructions.html#exec-table-init let table = unsafe { &mut *self.get_table(table_index) }; @@ -593,7 +592,7 @@ impl Instance { .and_then(|s| s.get(..usize::try_from(len).unwrap())) { Some(elements) => elements, - None => return Err(Trap::wasm(TrapCode::TableOutOfBounds)), + None => return Err(TrapCode::TableOutOfBounds), }; match table.element_type() { @@ -643,7 +642,7 @@ impl Instance { src_index: MemoryIndex, src: u64, len: u64, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { // https://webassembly.github.io/reference-types/core/exec/instructions.html#exec-memory-copy let src_mem = self.get_memory(src_index); @@ -665,8 +664,8 @@ impl Instance { Ok(()) } - fn validate_inbounds(&self, max: usize, ptr: u64, len: u64) -> Result { - let oob = || Trap::wasm(TrapCode::HeapOutOfBounds); + fn validate_inbounds(&self, max: usize, ptr: u64, len: u64) -> Result { + let oob = || TrapCode::HeapOutOfBounds; let end = ptr .checked_add(len) .and_then(|i| usize::try_from(i).ok()) @@ -689,7 +688,7 @@ impl Instance { dst: u64, val: u8, len: u64, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { let memory = self.get_memory(memory_index); let dst = self.validate_inbounds(memory.current_length(), dst, len)?; @@ -719,7 +718,7 @@ impl Instance { dst: u64, src: u32, len: u32, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { let range = match self.module().passive_data_map.get(&data_index).cloned() { Some(range) if !self.dropped_data.contains(data_index) => range, _ => 0..0, @@ -738,7 +737,7 @@ impl Instance { dst: u64, src: u32, len: u32, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { // https://webassembly.github.io/bulk-memory-operations/core/exec/instructions.html#exec-memory-init let memory = self.get_memory(memory_index); diff --git a/crates/runtime/src/instance/allocator.rs b/crates/runtime/src/instance/allocator.rs index bede02c42b..b52e8aa836 100644 --- a/crates/runtime/src/instance/allocator.rs +++ b/crates/runtime/src/instance/allocator.rs @@ -2,7 +2,6 @@ use crate::imports::Imports; use crate::instance::{Instance, InstanceHandle, RuntimeMemoryCreator}; use crate::memory::{DefaultMemoryCreator, Memory}; use crate::table::Table; -use crate::traphandlers::Trap; use crate::ModuleRuntimeInfo; use crate::Store; use anyhow::Result; @@ -103,7 +102,7 @@ pub enum InstantiationError { /// A trap ocurred during instantiation, after linking. #[error("Trap occurred during instantiation")] - Trap(Trap), + Trap(TrapCode), /// A limit on how many instances are supported has been reached. #[error("Limit of {0} concurrent instances has been reached")] @@ -384,9 +383,7 @@ fn initialize_memories(instance: &mut Instance, module: &Module) -> Result<(), I }, ); if !ok { - return Err(InstantiationError::Trap(Trap::wasm( - TrapCode::HeapOutOfBounds, - ))); + return Err(InstantiationError::Trap(TrapCode::HeapOutOfBounds)); } Ok(()) diff --git a/crates/runtime/src/lib.rs b/crates/runtime/src/lib.rs index e8ed16c6a3..4f7567dd35 100644 --- a/crates/runtime/src/lib.rs +++ b/crates/runtime/src/lib.rs @@ -65,7 +65,7 @@ pub use crate::mmap_vec::MmapVec; pub use crate::table::{Table, TableElement}; pub use crate::traphandlers::{ catch_traps, init_traps, raise_lib_trap, raise_user_trap, resume_panic, tls_eager_initialize, - Backtrace, SignalHandler, TlsRestore, Trap, + Backtrace, SignalHandler, TlsRestore, Trap, TrapReason, }; pub use crate::vmcontext::{ VMCallerCheckedAnyfunc, VMContext, VMFunctionBody, VMFunctionImport, VMGlobalDefinition, diff --git a/crates/runtime/src/libcalls.rs b/crates/runtime/src/libcalls.rs index 6173d9f83e..1a29a8195f 100644 --- a/crates/runtime/src/libcalls.rs +++ b/crates/runtime/src/libcalls.rs @@ -59,7 +59,7 @@ use crate::externref::VMExternRef; use crate::instance::Instance; use crate::table::{Table, TableElementType}; -use crate::traphandlers::{raise_lib_trap, resume_panic, Trap}; +use crate::traphandlers::{raise_lib_trap, raise_user_trap, resume_panic}; use crate::vmcontext::{VMCallerCheckedAnyfunc, VMContext}; use std::mem; use std::ptr::{self, NonNull}; @@ -506,14 +506,12 @@ pub unsafe extern "C" fn memory_atomic_notify( // or it's been validated to be in-bounds already. Double-check for now // just to be sure. let addr_to_check = addr.checked_add(4).unwrap(); - validate_atomic_addr(instance, memory, addr_to_check).and_then(|()| { - Err(Trap::user(anyhow::anyhow!( - "unimplemented: wasm atomics (fn memory_atomic_notify) unsupported", - ))) - }) + validate_atomic_addr(instance, memory, addr_to_check) }; match result { - Ok(n) => n, + Ok(()) => raise_user_trap(anyhow::anyhow!( + "unimplemented: wasm atomics (fn memory_atomic_notify) unsupported", + )), Err(e) => raise_lib_trap(e), } } @@ -533,14 +531,12 @@ pub unsafe extern "C" fn memory_atomic_wait32( // see wasmtime_memory_atomic_notify for why this shouldn't overflow // but we still double-check let addr_to_check = addr.checked_add(4).unwrap(); - validate_atomic_addr(instance, memory, addr_to_check).and_then(|()| { - Err(Trap::user(anyhow::anyhow!( - "unimplemented: wasm atomics (fn memory_atomic_wait32) unsupported", - ))) - }) + validate_atomic_addr(instance, memory, addr_to_check) }; match result { - Ok(n) => n, + Ok(()) => raise_user_trap(anyhow::anyhow!( + "unimplemented: wasm atomics (fn memory_atomic_wait32) unsupported", + )), Err(e) => raise_lib_trap(e), } } @@ -560,14 +556,12 @@ pub unsafe extern "C" fn memory_atomic_wait64( // see wasmtime_memory_atomic_notify for why this shouldn't overflow // but we still double-check let addr_to_check = addr.checked_add(8).unwrap(); - validate_atomic_addr(instance, memory, addr_to_check).and_then(|()| { - Err(Trap::user(anyhow::anyhow!( - "unimplemented: wasm atomics (fn memory_atomic_wait64) unsupported", - ))) - }) + validate_atomic_addr(instance, memory, addr_to_check) }; match result { - Ok(n) => n, + Ok(()) => raise_user_trap(anyhow::anyhow!( + "unimplemented: wasm atomics (fn memory_atomic_wait64) unsupported", + )), Err(e) => raise_lib_trap(e), } } @@ -585,9 +579,9 @@ unsafe fn validate_atomic_addr( instance: &Instance, memory: MemoryIndex, addr: usize, -) -> Result<(), Trap> { +) -> Result<(), TrapCode> { if addr > instance.get_memory(memory).current_length() { - return Err(Trap::wasm(TrapCode::HeapOutOfBounds)); + return Err(TrapCode::HeapOutOfBounds); } Ok(()) } diff --git a/crates/runtime/src/table.rs b/crates/runtime/src/table.rs index 63b4e440d7..b71d21ffc6 100644 --- a/crates/runtime/src/table.rs +++ b/crates/runtime/src/table.rs @@ -3,7 +3,7 @@ //! `Table` is to WebAssembly tables what `LinearMemory` is to WebAssembly linear memories. use crate::vmcontext::{VMCallerCheckedAnyfunc, VMTableDefinition}; -use crate::{Store, Trap, VMExternRef}; +use crate::{Store, VMExternRef}; use anyhow::{bail, format_err, Error, Result}; use std::convert::{TryFrom, TryInto}; use std::ops::Range; @@ -267,7 +267,7 @@ impl Table { &mut self, dst: u32, items: impl ExactSizeIterator, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { assert!(self.element_type() == TableElementType::Func); let elements = match self @@ -276,7 +276,7 @@ impl Table { .and_then(|s| s.get_mut(..items.len())) { Some(elements) => elements, - None => return Err(Trap::wasm(TrapCode::TableOutOfBounds)), + None => return Err(TrapCode::TableOutOfBounds), }; for (item, slot) in items.zip(elements) { @@ -288,14 +288,14 @@ impl Table { /// Fill `table[dst..dst + len]` with `val`. /// /// Returns a trap error on out-of-bounds accesses. - pub fn fill(&mut self, dst: u32, val: TableElement, len: u32) -> Result<(), Trap> { + pub fn fill(&mut self, dst: u32, val: TableElement, len: u32) -> Result<(), TrapCode> { let start = dst as usize; let end = start .checked_add(len as usize) - .ok_or_else(|| Trap::wasm(TrapCode::TableOutOfBounds))?; + .ok_or_else(|| TrapCode::TableOutOfBounds)?; if end > self.size() as usize { - return Err(Trap::wasm(TrapCode::TableOutOfBounds)); + return Err(TrapCode::TableOutOfBounds); } debug_assert!(self.type_matches(&val)); @@ -410,7 +410,7 @@ impl Table { dst_index: u32, src_index: u32, len: u32, - ) -> Result<(), Trap> { + ) -> Result<(), TrapCode> { // https://webassembly.github.io/bulk-memory-operations/core/exec/instructions.html#exec-table-copy if src_index @@ -420,7 +420,7 @@ impl Table { .checked_add(len) .map_or(true, |m| m > (*dst_table).size()) { - return Err(Trap::wasm(TrapCode::TableOutOfBounds)); + return Err(TrapCode::TableOutOfBounds); } debug_assert!( diff --git a/crates/runtime/src/traphandlers.rs b/crates/runtime/src/traphandlers.rs index 053d1261fb..60e8291d66 100644 --- a/crates/runtime/src/traphandlers.rs +++ b/crates/runtime/src/traphandlers.rs @@ -80,7 +80,8 @@ pub fn init_traps(is_wasm_pc: fn(usize) -> bool) { /// 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(data: Error) -> ! { - tls::with(|info| info.unwrap().unwind_with(UnwindReason::UserTrap(data))) + let trap = TrapReason::User(data); + tls::with(|info| info.unwrap().unwind_with(UnwindReason::Trap(trap))) } /// Raises a trap from inside library code immediately. @@ -93,8 +94,9 @@ pub unsafe fn raise_user_trap(data: Error) -> ! { /// 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_lib_trap(trap: Trap) -> ! { - tls::with(|info| info.unwrap().unwind_with(UnwindReason::LibTrap(trap))) +pub unsafe fn raise_lib_trap(trap: TrapCode) -> ! { + let trap = TrapReason::Wasm(trap); + tls::with(|info| info.unwrap().unwind_with(UnwindReason::Trap(trap))) } /// Carries a Rust panic across wasm code and resumes the panic on the other @@ -111,74 +113,39 @@ pub unsafe fn resume_panic(payload: Box) -> ! { /// Stores trace message with backtrace. #[derive(Debug)] -pub enum Trap { - /// A user-raised trap through `raise_user_trap`. - User { - /// The user-provided error - error: Error, - /// Native stack backtrace at the time the trap occurred - backtrace: Option, - }, +pub struct Trap { + /// Original reason from where this trap originated. + pub reason: TrapReason, + /// Wasm backtrace of the trap, if any. + pub backtrace: Option, +} - /// A trap raised from jit code - Jit { - /// The program counter in JIT code where this trap happened. - pc: usize, - /// Native stack backtrace at the time the trap occurred - backtrace: Option, - }, +/// Enumeration of different methods of raising a trap. +#[derive(Debug)] +pub enum TrapReason { + /// A user-raised trap through `raise_user_trap`. + User(Error), + + /// A trap raised from Cranelift-generated code with the pc listed of where + /// the trap came from. + Jit(usize), /// A trap raised from a wasm libcall - Wasm { - /// Code of the trap. - trap_code: TrapCode, - /// Native stack backtrace at the time the trap occurred - backtrace: Option, - }, + Wasm(TrapCode), /// A trap indicating that the runtime was unable to allocate sufficient memory. - OOM { - /// Native stack backtrace at the time the OOM occurred - backtrace: Option, - }, + OOM, } impl Trap { - /// Construct a new Wasm trap with the given trap code. - /// - /// Internally saves a backtrace when passed across a setjmp boundary, if the - /// engine is configured to save backtraces. - pub fn wasm(trap_code: TrapCode) -> Self { - Trap::Wasm { - trap_code, - backtrace: None, - } - } - - /// Construct a new Wasm trap from a user Error. - /// - /// Internally saves a backtrace when passed across a setjmp boundary, if the - /// engine is configured to save backtraces. - pub fn user(error: Error) -> Self { - Trap::User { - error, - backtrace: None, - } - } /// Construct a new OOM trap. /// /// Internally saves a backtrace when passed across a setjmp boundary, if the /// engine is configured to save backtraces. pub fn oom() -> Self { - Trap::OOM { backtrace: None } - } - - fn insert_backtrace(&mut self, bt: Backtrace) { - match self { - Trap::User { backtrace, .. } => *backtrace = Some(bt), - Trap::Jit { backtrace, .. } => *backtrace = Some(bt), - Trap::Wasm { backtrace, .. } => *backtrace = Some(bt), - Trap::OOM { backtrace, .. } => *backtrace = Some(bt), + Trap { + reason: TrapReason::OOM, + backtrace: None, } } } @@ -226,9 +193,7 @@ pub struct CallThreadState { enum UnwindReason { Panic(Box), - UserTrap(Error), - LibTrap(Trap), - JitTrap { pc: usize }, // Removed a backtrace here + Trap(TrapReason), } impl CallThreadState { @@ -258,17 +223,12 @@ impl CallThreadState { #[cold] unsafe fn read_trap(&self) -> Box { - Box::new(match (*self.unwind.get()).as_ptr().read() { - (UnwindReason::UserTrap(error), backtrace) => Trap::User { error, backtrace }, - (UnwindReason::LibTrap(mut trap), backtrace) => { - if let Some(backtrace) = backtrace { - trap.insert_backtrace(backtrace); - } - trap - } - (UnwindReason::JitTrap { pc }, backtrace) => Trap::Jit { pc, backtrace }, - (UnwindReason::Panic(panic), _) => std::panic::resume_unwind(panic), - }) + let (unwind_reason, backtrace) = (*self.unwind.get()).as_ptr().read(); + let reason = match unwind_reason { + UnwindReason::Trap(trap) => trap, + UnwindReason::Panic(panic) => std::panic::resume_unwind(panic), + }; + Box::new(Trap { reason, backtrace }) } fn unwind_with(&self, reason: UnwindReason) -> ! { @@ -344,10 +304,11 @@ impl CallThreadState { } else { None }; + let trap = TrapReason::Jit(pc as usize); unsafe { (*self.unwind.get()) .as_mut_ptr() - .write((UnwindReason::JitTrap { pc: pc as usize }, backtrace)); + .write((UnwindReason::Trap(trap), backtrace)); } } } diff --git a/crates/wasmtime/src/externals.rs b/crates/wasmtime/src/externals.rs index 8942e87195..953801d273 100644 --- a/crates/wasmtime/src/externals.rs +++ b/crates/wasmtime/src/externals.rs @@ -464,7 +464,7 @@ impl Table { let table = Table::from_wasmtime_table(wasmtime_export, store); (*table.wasmtime_table(store, std::iter::empty())) .fill(0, init, ty.minimum()) - .map_err(Trap::from_runtime)?; + .map_err(|c| Trap::new_wasm(c, None))?; Ok(table) } @@ -653,7 +653,7 @@ impl Table { let src_table = src_table.wasmtime_table(store, src_range); unsafe { runtime::Table::copy(dst_table, src_table, dst_index, src_index, len) - .map_err(Trap::from_runtime)?; + .map_err(|c| Trap::new_wasm(c, None))?; } Ok(()) } @@ -681,7 +681,9 @@ impl Table { let table = self.wasmtime_table(store, std::iter::empty()); unsafe { - (*table).fill(dst, val, len).map_err(Trap::from_runtime)?; + (*table) + .fill(dst, val, len) + .map_err(|c| Trap::new_wasm(c, None))?; } Ok(()) diff --git a/crates/wasmtime/src/instance.rs b/crates/wasmtime/src/instance.rs index 8d38339a1b..9cf1365a73 100644 --- a/crates/wasmtime/src/instance.rs +++ b/crates/wasmtime/src/instance.rs @@ -322,7 +322,7 @@ impl Instance { ) .map_err(|e| -> Error { match e { - InstantiationError::Trap(trap) => Trap::from_runtime(trap).into(), + InstantiationError::Trap(trap) => Trap::new_wasm(trap, None).into(), other => other.into(), } })?; diff --git a/crates/wasmtime/src/trap.rs b/crates/wasmtime/src/trap.rs index 85d61ee6f9..2fae8457fa 100644 --- a/crates/wasmtime/src/trap.rs +++ b/crates/wasmtime/src/trap.rs @@ -218,15 +218,16 @@ impl Trap { #[cold] // see Trap::new pub(crate) fn from_runtime(runtime_trap: wasmtime_runtime::Trap) -> Self { - match runtime_trap { - wasmtime_runtime::Trap::User { error, backtrace } => { + let wasmtime_runtime::Trap { reason, backtrace } = runtime_trap; + match reason { + wasmtime_runtime::TrapReason::User(error) => { let trap = Trap::from(error); if let Some(backtrace) = backtrace { trap.record_backtrace(TrapBacktrace::new(backtrace, None)); } trap } - wasmtime_runtime::Trap::Jit { pc, backtrace } => { + wasmtime_runtime::TrapReason::Jit(pc) => { let code = GlobalModuleRegistry::with(|modules| { modules .lookup_trap_code(pc) @@ -235,14 +236,11 @@ impl Trap { let backtrace = backtrace.map(|bt| TrapBacktrace::new(bt, Some(pc))); Trap::new_wasm(code, backtrace) } - wasmtime_runtime::Trap::Wasm { - trap_code, - backtrace, - } => { + wasmtime_runtime::TrapReason::Wasm(trap_code) => { let backtrace = backtrace.map(|bt| TrapBacktrace::new(bt, None)); Trap::new_wasm(trap_code, backtrace) } - wasmtime_runtime::Trap::OOM { backtrace } => { + wasmtime_runtime::TrapReason::OOM => { let reason = TrapReason::Message("out of memory".to_string()); let backtrace = backtrace.map(|bt| TrapBacktrace::new(bt, None)); Trap::new_with_trace(reason, backtrace)