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

@@ -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<usize, Trap> {
+    fn validate_inbounds(&self, max: usize, ptr: u64, len: u64) -> Result<usize, TrapCode> {
-        let oob = || Trap::wasm(TrapCode::HeapOutOfBounds);
+        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);

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(
+        return Err(InstantiationError::Trap(TrapCode::HeapOutOfBounds));
-            TrapCode::HeapOutOfBounds,
-        )));
     }
 
     Ok(())

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,

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(|()| {
+        validate_atomic_addr(instance, memory, addr_to_check)
-            Err(Trap::user(anyhow::anyhow!(
-                "unimplemented: wasm atomics (fn memory_atomic_notify) unsupported",
-            )))
-        })
     };
     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(|()| {
+        validate_atomic_addr(instance, memory, addr_to_check)
-            Err(Trap::user(anyhow::anyhow!(
-                "unimplemented: wasm atomics (fn memory_atomic_wait32) unsupported",
-            )))
-        })
     };
     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(|()| {
+        validate_atomic_addr(instance, memory, addr_to_check)
-            Err(Trap::user(anyhow::anyhow!(
-                "unimplemented: wasm atomics (fn memory_atomic_wait64) unsupported",
-            )))
-        })
     };
     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(())
 }

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<Item = *mut VMCallerCheckedAnyfunc>,
-    ) -> 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!(

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) -> ! {
+pub unsafe fn raise_lib_trap(trap: TrapCode) -> ! {
-    tls::with(|info| info.unwrap().unwind_with(UnwindReason::LibTrap(trap)))
+    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<dyn Any + Send>) -> ! {
 
 /// Stores trace message with backtrace.
 #[derive(Debug)]
-pub enum Trap {
+pub struct Trap {
-    /// A user-raised trap through `raise_user_trap`.
+    /// Original reason from where this trap originated.
-    User {
+    pub reason: TrapReason,
-        /// The user-provided error
+    /// Wasm backtrace of the trap, if any.
-        error: Error,
+    pub backtrace: Option<Backtrace>,
-        /// Native stack backtrace at the time the trap occurred
+}
-        backtrace: Option<Backtrace>,
-    },
 
-    /// A trap raised from jit code
+/// Enumeration of different methods of raising a trap.
-    Jit {
+#[derive(Debug)]
-        /// The program counter in JIT code where this trap happened.
+pub enum TrapReason {
-        pc: usize,
+    /// A user-raised trap through `raise_user_trap`.
-        /// Native stack backtrace at the time the trap occurred
+    User(Error),
-        backtrace: Option<Backtrace>,
+
-    },
+    /// 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 {
+    Wasm(TrapCode),
-        /// Code of the trap.
-        trap_code: TrapCode,
-        /// Native stack backtrace at the time the trap occurred
-        backtrace: Option<Backtrace>,
-    },
 
     /// A trap indicating that the runtime was unable to allocate sufficient memory.
-    OOM {
+    OOM,
-        /// Native stack backtrace at the time the OOM occurred
-        backtrace: Option<Backtrace>,
-    },
 }
 
 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 }
+        Trap {
-    }
+            reason: TrapReason::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),
         }
     }
 }
@@ -226,9 +193,7 @@ pub struct CallThreadState {
 
 enum UnwindReason {
     Panic(Box<dyn Any + Send>),
-    UserTrap(Error),
+    Trap(TrapReason),
-    LibTrap(Trap),
-    JitTrap { pc: usize }, // Removed a backtrace here
 }
 
 impl CallThreadState {
@@ -258,17 +223,12 @@ impl CallThreadState {
 
     #[cold]
     unsafe fn read_trap(&self) -> Box<Trap> {
-        Box::new(match (*self.unwind.get()).as_ptr().read() {
+        let (unwind_reason, backtrace) = (*self.unwind.get()).as_ptr().read();
-            (UnwindReason::UserTrap(error), backtrace) => Trap::User { error, backtrace },
+        let reason = match unwind_reason {
-            (UnwindReason::LibTrap(mut trap), backtrace) => {
+            UnwindReason::Trap(trap) => trap,
-                if let Some(backtrace) = backtrace {
+            UnwindReason::Panic(panic) => std::panic::resume_unwind(panic),
-                    trap.insert_backtrace(backtrace);
+        };
-                }
+        Box::new(Trap { reason, backtrace })
-                trap
-            }
-            (UnwindReason::JitTrap { pc }, backtrace) => Trap::Jit { pc, backtrace },
-            (UnwindReason::Panic(panic), _) => std::panic::resume_unwind(panic),
-        })
     }
 
     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));
         }
     }
 }

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(())

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(),
                 }
             })?;

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 {
+        let wasmtime_runtime::Trap { reason, backtrace } = runtime_trap;
-            wasmtime_runtime::Trap::User { error, backtrace } => {
+        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 {
+            wasmtime_runtime::TrapReason::Wasm(trap_code) => {
-                trap_code,
-                backtrace,
-            } => {
                 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)