Use a StoreOpaque during backtraces for metadata (#4325)

Previous to this commit Wasmtime would use the `GlobalModuleRegistry`
when learning information about a trap such as its trap code, the
symbols for each frame, etc. This has a downside though of holding a
global read-write lock for the duration of this operation which hinders
registration of new modules in parallel. In addition there was a fair
amount of internal duplication between this "global module registry" and
the store-local module registry. Finally relying on global state for
information like this gets a bit more brittle over time as it seems best
to scope global queries to precisely what's necessary rather than
holding extra information.

With the refactoring in wasm backtraces done in #4183 it's now possible
to always have a `StoreOpaque` reference when a backtrace is collected
for symbolication and otherwise Trap-identification purposes. This
commit adds a `StoreOpaque` parameter to the `Trap::from_runtime`
constructor and then plumbs that everywhere. Note that while doing this
I changed the internal `traphandlers::lazy_per_thread_init` function to
no longer return a `Result` and instead just `panic!` on Unix if memory
couldn't be allocated for a stack. This removed quite a lot of
error-handling code for a case that's expected to quite rarely happen.
If necessary in the future we can add a fallible initialization point
but this feels like a better default balance for the code here.

With a `StoreOpaque` in use when a trap is being symbolicated that means
we have a `ModuleRegistry` which can be used for queries and such. This
meant that the `GlobalModuleRegistry` state could largely be dismantled
and moved to per-`Store` state (within the `ModuleRegistry`, mostly just
moving methods around).

The final state is that the global rwlock is not exclusively scoped
around insertions/deletions/`is_wasm_trap_pc` which is just a lookup and
atomic add. Otherwise symbolication for a backtrace exclusively uses
store-local state now (as intended).

The original motivation for this commit was that frame information
lookup and pieces were looking to get somewhat complicated with the
addition of components which are a new vector of traps coming out of
Cranelift-generated code. My hope is that by having a `Store` around for
more operations it's easier to plumb all this through.

crates/runtime/src/traphandlers.rs

@@ -132,22 +132,6 @@ pub enum TrapReason {
 
     /// A trap raised from a wasm libcall
     Wasm(TrapCode),
-
-    /// A trap indicating that the runtime was unable to allocate sufficient memory.
-    OOM,
-}
-
-impl Trap {
-    /// 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 {
-            reason: TrapReason::OOM,
-            backtrace: None,
-        }
-    }
 }
 
 /// Catches any wasm traps that happen within the execution of `closure`,
@@ -213,7 +197,7 @@ impl CallThreadState {
     }
 
     fn with(self, closure: impl FnOnce(&CallThreadState) -> i32) -> Result<(), Box<Trap>> {
-        let ret = tls::set(&self, || closure(&self))?;
+        let ret = tls::set(&self, || closure(&self));
         if ret != 0 {
             Ok(())
         } else {
@@ -329,7 +313,6 @@ impl<T: Copy> Drop for ResetCell<'_, T> {
 // the caller to the trap site.
 mod tls {
     use super::CallThreadState;
-    use crate::Trap;
     use std::ptr;
 
     pub use raw::Ptr;
@@ -350,7 +333,6 @@ mod tls {
     // these functions are free to be inlined.
     mod raw {
         use super::CallThreadState;
-        use crate::Trap;
         use std::cell::Cell;
         use std::ptr;
 
@@ -365,17 +347,17 @@ mod tls {
 
         #[cfg_attr(feature = "async", inline(never))] // see module docs
         #[cfg_attr(not(feature = "async"), inline)]
-        pub fn replace(val: Ptr) -> Result<Ptr, Box<Trap>> {
+        pub fn replace(val: Ptr) -> Ptr {
             PTR.with(|p| {
                 // When a new value is configured that means that we may be
                 // entering WebAssembly so check to see if this thread has
                 // performed per-thread initialization for traps.
                 let (prev, initialized) = p.get();
                 if !initialized {
-                    super::super::sys::lazy_per_thread_init()?;
+                    super::super::sys::lazy_per_thread_init();
                 }
                 p.set((val, true));
-                Ok(prev)
+                prev
             })
         }
 
@@ -383,15 +365,14 @@ mod tls {
         /// lazily by the runtime if users do not perform it eagerly.
         #[cfg_attr(feature = "async", inline(never))] // see module docs
         #[cfg_attr(not(feature = "async"), inline)]
-        pub fn initialize() -> Result<(), Box<Trap>> {
+        pub fn initialize() {
             PTR.with(|p| {
                 let (state, initialized) = p.get();
                 if initialized {
-                    return Ok(());
+                    return;
                 }
-                super::super::sys::lazy_per_thread_init()?;
+                super::super::sys::lazy_per_thread_init();
                 p.set((state, true));
-                Ok(())
             })
         }
 
@@ -414,7 +395,7 @@ mod tls {
         ///
         /// This is not a safe operation since it's intended to only be used
         /// with stack switching found with fibers and async wasmtime.
-        pub unsafe fn take() -> Result<TlsRestore, Box<Trap>> {
+        pub unsafe fn take() -> TlsRestore {
             // Our tls pointer must be set at this time, and it must not be
             // null. We need to restore the previous pointer since we're
             // removing ourselves from the call-stack, and in the process we
@@ -423,30 +404,29 @@ mod tls {
             let raw = raw::get();
             if !raw.is_null() {
                 let prev = (*raw).prev.replace(ptr::null());
-                raw::replace(prev)?;
+                raw::replace(prev);
             }
             // Null case: we aren't in a wasm context, so theres no tls
             // to save for restoration.
-            Ok(TlsRestore(raw))
+            TlsRestore(raw)
         }
 
         /// Restores a previous tls state back into this thread's TLS.
         ///
         /// This is unsafe because it's intended to only be used within the
         /// context of stack switching within wasmtime.
-        pub unsafe fn replace(self) -> Result<(), Box<super::Trap>> {
+        pub unsafe fn replace(self) {
             // Null case: we aren't in a wasm context, so theres no tls
             // to restore.
             if self.0.is_null() {
-                return Ok(());
+                return;
             }
             // We need to configure our previous TLS pointer to whatever is in
             // TLS at this time, and then we set the current state to ourselves.
             let prev = raw::get();
             assert!((*self.0).prev.get().is_null());
             (*self.0).prev.set(prev);
-            raw::replace(self.0)?;
-            Ok(())
+            raw::replace(self.0);
         }
     }
 
@@ -454,21 +434,20 @@ mod tls {
     /// execution of `closure` any call to `with` will yield `ptr`, unless this
     /// is recursively called again.
     #[inline]
-    pub fn set<R>(state: &CallThreadState, closure: impl FnOnce() -> R) -> Result<R, Box<Trap>> {
+    pub fn set<R>(state: &CallThreadState, closure: impl FnOnce() -> R) -> R {
         struct Reset<'a>(&'a CallThreadState);
 
         impl Drop for Reset<'_> {
             #[inline]
             fn drop(&mut self) {
-                raw::replace(self.0.prev.replace(ptr::null()))
-                    .expect("tls should be previously initialized");
+                raw::replace(self.0.prev.replace(ptr::null()));
             }
         }
 
-        let prev = raw::replace(state)?;
+        let prev = raw::replace(state);
         state.prev.set(prev);
         let _reset = Reset(state);
-        Ok(closure())
+        closure()
     }
 
     /// Returns the last pointer configured with `set` above. Panics if `set`

crates/runtime/src/traphandlers/macos.rs

@@ -33,7 +33,7 @@
 
 #![allow(non_snake_case)]
 
-use crate::traphandlers::{tls, wasmtime_longjmp, Trap};
+use crate::traphandlers::{tls, wasmtime_longjmp};
 use mach::exception_types::*;
 use mach::kern_return::*;
 use mach::mach_init::*;
@@ -410,7 +410,7 @@ unsafe extern "C" fn unwind(wasm_pc: *const u8) -> ! {
 /// task-level port which is where we'd expected things like breakpad/crashpad
 /// exception handlers to get registered.
 #[cold]
-pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
+pub fn lazy_per_thread_init() {
     unsafe {
         assert!(WASMTIME_PORT != MACH_PORT_NULL);
         let this_thread = mach_thread_self();
@@ -424,5 +424,4 @@ pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
         mach_port_deallocate(mach_task_self(), this_thread);
         assert_eq!(kret, KERN_SUCCESS, "failed to set thread exception port");
     }
-    Ok(())
 }

crates/runtime/src/traphandlers/unix.rs

@@ -1,4 +1,4 @@
-use crate::traphandlers::{tls, wasmtime_longjmp, Trap};
+use crate::traphandlers::{tls, wasmtime_longjmp};
 use std::cell::RefCell;
 use std::io;
 use std::mem::{self, MaybeUninit};
@@ -252,7 +252,7 @@ unsafe fn set_pc(cx: *mut libc::c_void, pc: usize, arg1: usize) {
 /// and registering our own alternate stack that is large enough and has a guard
 /// page.
 #[cold]
-pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
+pub fn lazy_per_thread_init() {
     // This thread local is purely used to register a `Stack` to get deallocated
     // when the thread exists. Otherwise this function is only ever called at
     // most once per-thread.
@@ -270,11 +270,10 @@ pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
     }
 
     return STACK.with(|s| {
-        *s.borrow_mut() = unsafe { allocate_sigaltstack()? };
-        Ok(())
+        *s.borrow_mut() = unsafe { allocate_sigaltstack() };
     });
 
-    unsafe fn allocate_sigaltstack() -> Result<Option<Stack>, Box<Trap>> {
+    unsafe fn allocate_sigaltstack() -> Option<Stack> {
         // Check to see if the existing sigaltstack, if it exists, is big
         // enough. If so we don't need to allocate our own.
         let mut old_stack = mem::zeroed();
@@ -286,7 +285,7 @@ pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
             io::Error::last_os_error()
         );
         if old_stack.ss_flags & libc::SS_DISABLE == 0 && old_stack.ss_size >= MIN_STACK_SIZE {
-            return Ok(None);
+            return None;
         }
 
         // ... but failing that we need to allocate our own, so do all that
@@ -301,7 +300,7 @@ pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
             rustix::mm::ProtFlags::empty(),
             rustix::mm::MapFlags::PRIVATE,
         )
-        .map_err(|_| Box::new(Trap::oom()))?;
+        .expect("failed to allocate memory for sigaltstack");
 
         // Prepare the stack with readable/writable memory and then register it
         // with `sigaltstack`.
@@ -325,10 +324,10 @@ pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
             io::Error::last_os_error()
         );
 
-        Ok(Some(Stack {
+        Some(Stack {
             mmap_ptr: ptr,
             mmap_size: alloc_size,
-        }))
+        })
     }
 
     impl Drop for Stack {

crates/runtime/src/traphandlers/windows.rs

@@ -1,4 +1,4 @@
-use crate::traphandlers::{tls, wasmtime_longjmp, Trap};
+use crate::traphandlers::{tls, wasmtime_longjmp};
 use std::io;
 use winapi::um::errhandlingapi::*;
 use winapi::um::minwinbase::*;
@@ -74,7 +74,6 @@ unsafe extern "system" fn exception_handler(exception_info: PEXCEPTION_POINTERS)
     })
 }
 
-pub fn lazy_per_thread_init() -> Result<(), Box<Trap>> {
+pub fn lazy_per_thread_init() {
     // Unused on Windows
-    Ok(())
 }