Merge #1401

1401: Make runtime and trap errors well defined r=syrusakbary a=MarkMcCaskey

Resolves #1328 

This PR goes through and gives explicit types for all the errors instead of using `Box<dyn Any + Send>` where possible.  This gives users better insight into what the specific errors are and should help with debugging in the case of mistakes in our code.

The remaining uses of `Box<dyn Any>` are due to the structure of our dependency graph -- this is probably solvable but it seems fine as is as all error types are now explicit and the remaining `Box<dyn Any>`s are either fully user controlled or not for end-user consumption.

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <mark@wasmer.io>

lib/runtime-core/src/backend.rs

@@ -1,5 +1,5 @@
 use crate::{
-    error::CompileResult,
+    error::{CompileResult, RuntimeError},
     module::ModuleInner,
     state::ModuleStateMap,
     typed_func::Wasm,
@@ -282,7 +282,7 @@ pub trait RunnableModule: Send + Sync {
     fn get_trampoline(&self, info: &ModuleInfo, sig_index: SigIndex) -> Option<Wasm>;
 
     /// Trap an error.
-    unsafe fn do_early_trap(&self, data: Box<dyn Any + Send>) -> !;
+    unsafe fn do_early_trap(&self, data: RuntimeError) -> !;
 
     /// Returns the machine code associated with this module.
     fn get_code(&self) -> Option<&[u8]> {

lib/runtime-core/src/codegen.rs

@@ -6,13 +6,12 @@ use crate::{
     backend::RunnableModule,
     backend::{CacheGen, Compiler, CompilerConfig, Features, Token},
     cache::{Artifact, Error as CacheError},
-    error::{CompileError, CompileResult},
+    error::{CompileError, CompileResult, RuntimeError},
     module::{ModuleInfo, ModuleInner},
     structures::Map,
     types::{FuncIndex, FuncSig, SigIndex},
 };
 use smallvec::SmallVec;
-use std::any::Any;
 use std::collections::HashMap;
 use std::fmt;
 use std::fmt::Debug;
@@ -23,7 +22,7 @@ use wasmparser::{Operator, Type as WpType};
 
 /// A type that defines a function pointer, which is called when breakpoints occur.
 pub type BreakpointHandler =
-    Box<dyn Fn(BreakpointInfo) -> Result<(), Box<dyn Any + Send>> + Send + Sync + 'static>;
+    Box<dyn Fn(BreakpointInfo) -> Result<(), RuntimeError> + Send + Sync + 'static>;
 
 /// Maps instruction pointers to their breakpoint handlers.
 pub type BreakpointMap = Arc<HashMap<usize, BreakpointHandler>>;

lib/runtime-core/src/error.rs

@@ -173,13 +173,99 @@ impl std::fmt::Display for LinkError {
 
 impl std::error::Error for LinkError {}
 
-/// This is the error type returned when calling
-/// a WebAssembly function.
+/// An error that happened while invoking a Wasm function.
+#[derive(Debug)]
+pub enum InvokeError {
+    /// Indicates an exceptional circumstance such as a bug in Wasmer (please file an issue!)
+    /// or a hardware failure.
+    FailedWithNoError,
+    /// Indicates that a trap occurred that is not known to Wasmer.
+    UnknownTrap {
+        /// The address that the trap occurred at.
+        address: usize,
+        /// The name of the signal.
+        signal: &'static str,
+    },
+    /// A trap that Wasmer knows about occurred.
+    TrapCode {
+        /// The type of exception.
+        code: ExceptionCode,
+        /// Where in the Wasm file this trap orginated from.
+        srcloc: u32,
+    },
+    /// A trap occurred that Wasmer knows about but it had a trap code that
+    /// we weren't expecting or that we do not handle.  This error may be backend-specific.
+    UnknownTrapCode {
+        /// The trap code we saw but did not recognize.
+        trap_code: String,
+        /// Where in the Wasm file this trap orginated from.
+        srcloc: u32,
+    },
+    /// An "early trap" occurred.  TODO: document this properly
+    EarlyTrap(Box<RuntimeError>),
+    /// Indicates that a breakpoint was hit. The inner value is dependent upon
+    /// the middleware or backend being used.
+    Breakpoint(Box<RuntimeError>),
+}
+
+impl From<InvokeError> for RuntimeError {
+    fn from(other: InvokeError) -> RuntimeError {
+        match other {
+            InvokeError::EarlyTrap(re) | InvokeError::Breakpoint(re) => *re,
+            _ => RuntimeError::InvokeError(other),
+        }
+    }
+}
+
+impl std::error::Error for InvokeError {}
+
+impl std::fmt::Display for InvokeError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        match self {
+            InvokeError::FailedWithNoError => write!(f, "Invoke failed with no error"),
+            InvokeError::UnknownTrap { address, signal } => write!(
+                f,
+                "An unknown trap (`{}`) occured at 0x{:X}",
+                signal, address
+            ),
+            InvokeError::TrapCode { code, srcloc } => {
+                write!(f, "A `{}` trap was thrown at code offset {}", code, srcloc)
+            }
+            InvokeError::UnknownTrapCode { trap_code, srcloc } => write!(
+                f,
+                "A trap with an unknown trap code (`{}`) was thrown at code offset {}",
+                trap_code, srcloc
+            ),
+            InvokeError::EarlyTrap(rte) => write!(f, "Early trap: {}", rte),
+            InvokeError::Breakpoint(rte) => write!(f, "Breakpoint hit: {}", rte),
+        }
+    }
+}
+
+/// An `InternalError` is an error that happened inside of Wasmer and is a
+/// catch-all for errors that would otherwise be returned as
+/// `RuntimeError(Box::new(<string>))`.
 ///
-/// The main way to do this is `Instance.call`.
-///
-/// Comparing two `RuntimeError`s always evaluates to false.
-pub struct RuntimeError(pub Box<dyn Any + Send>);
+/// This type provides greater visibility into the kinds of things that may fail
+/// and improves the ability of users to handle them, though these errors may be
+/// extremely rare and impossible to handle.
+#[derive(Debug)]
+pub enum RuntimeError {
+    /// An error relating to the invocation of a Wasm function.
+    InvokeError(InvokeError),
+    /// A metering triggered error value.
+    ///
+    /// An error of this type indicates that it was returned by the metering system.
+    Metering(Box<dyn Any + Send>),
+    /// A frozen state of Wasm used to pause and resume execution.  Not strictly an
+    /// "error", but this happens while executing and therefore is a `RuntimeError`
+    /// from the persective of the caller that expected the code to fully execute.
+    InstanceImage(Box<dyn Any + Send>),
+    /// A user triggered error value.
+    ///
+    /// An error returned from a host function.
+    User(Box<dyn Any + Send>),
+}
 
 impl PartialEq for RuntimeError {
     fn eq(&self, _other: &RuntimeError) -> bool {
@@ -187,29 +273,33 @@ impl PartialEq for RuntimeError {
     }
 }
 
+impl std::error::Error for RuntimeError {}
+
 impl std::fmt::Display for RuntimeError {
     fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        let data = &*self.0;
-        if let Some(s) = data.downcast_ref::<String>() {
-            write!(f, "\"{}\"", s)
-        } else if let Some(s) = data.downcast_ref::<&str>() {
-            write!(f, "\"{}\"", s)
-        } else if let Some(exc_code) = data.downcast_ref::<ExceptionCode>() {
-            write!(f, "Caught exception of type \"{:?}\".", exc_code)
-        } else {
-            write!(f, "unknown error")
+        match self {
+            RuntimeError::InvokeError(ie) => write!(f, "Error when calling invoke: {}", ie),
+            RuntimeError::Metering(_) => write!(f, "unknown metering error type"),
+            RuntimeError::InstanceImage(_) => write!(
+                f,
+                "Execution interrupted by a suspend signal: instance image returned"
+            ),
+            RuntimeError::User(user_error) => {
+                write!(f, "User supplied error: ")?;
+                if let Some(s) = user_error.downcast_ref::<String>() {
+                    write!(f, "\"{}\"", s)
+                } else if let Some(s) = user_error.downcast_ref::<&str>() {
+                    write!(f, "\"{}\"", s)
+                } else if let Some(n) = user_error.downcast_ref::<i32>() {
+                    write!(f, "{}", n)
+                } else {
+                    write!(f, "unknown user error type")
+                }
+            }
         }
     }
 }
 
-impl std::fmt::Debug for RuntimeError {
-    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-        write!(f, "{}", self)
-    }
-}
-
-impl std::error::Error for RuntimeError {}
-
 /// This error type is produced by resolving a wasm function
 /// given its name.
 ///

lib/runtime-core/src/fault.rs

@@ -28,6 +28,7 @@ pub mod raw {
 }
 
 use crate::codegen::{BreakpointInfo, BreakpointMap};
+use crate::error::{InvokeError, RuntimeError};
 use crate::state::x64::{build_instance_image, read_stack, X64Register, GPR};
 use crate::state::{CodeVersion, ExecutionStateImage};
 use crate::vm;
@@ -36,7 +37,6 @@ use nix::sys::signal::{
     sigaction, SaFlags, SigAction, SigHandler, SigSet, Signal, SIGBUS, SIGFPE, SIGILL, SIGINT,
     SIGSEGV, SIGTRAP,
 };
-use std::any::Any;
 use std::cell::{Cell, RefCell, UnsafeCell};
 use std::ffi::c_void;
 use std::process;
@@ -61,7 +61,7 @@ type SetJmpBuffer = [i32; SETJMP_BUFFER_LEN];
 struct UnwindInfo {
     jmpbuf: SetJmpBuffer, // in
     breakpoints: Option<BreakpointMap>,
-    payload: Option<Box<dyn Any + Send>>, // out
+    payload: Option<Box<RuntimeError>>, // out
 }
 
 /// A store for boundary register preservation.
@@ -182,7 +182,7 @@ pub unsafe fn clear_wasm_interrupt() {
 pub unsafe fn catch_unsafe_unwind<R, F: FnOnce() -> R>(
     f: F,
     breakpoints: Option<BreakpointMap>,
-) -> Result<R, Box<dyn Any + Send>> {
+) -> Result<R, RuntimeError> {
     let unwind = UNWIND.with(|x| x.get());
     let old = (*unwind).take();
     *unwind = Some(UnwindInfo {
@@ -195,7 +195,7 @@ pub unsafe fn catch_unsafe_unwind<R, F: FnOnce() -> R>(
         // error
         let ret = (*unwind).as_mut().unwrap().payload.take().unwrap();
         *unwind = old;
-        Err(ret)
+        Err(*ret)
     } else {
         let ret = f();
         // implicit control flow to the error case...
@@ -205,7 +205,7 @@ pub unsafe fn catch_unsafe_unwind<R, F: FnOnce() -> R>(
 }
 
 /// Begins an unsafe unwind.
-pub unsafe fn begin_unsafe_unwind(e: Box<dyn Any + Send>) -> ! {
+pub unsafe fn begin_unsafe_unwind(e: Box<RuntimeError>) -> ! {
     let unwind = UNWIND.with(|x| x.get());
     let inner = (*unwind)
         .as_mut()
@@ -279,7 +279,11 @@ extern "C" fn signal_trap_handler(
     static ARCH: Architecture = Architecture::Aarch64;
 
     let mut should_unwind = false;
-    let mut unwind_result: Box<dyn Any + Send> = Box::new(());
+    let mut unwind_result: Option<Box<RuntimeError>> = None;
+    let get_unwind_result = |uw_result: Option<Box<RuntimeError>>| -> Box<RuntimeError> {
+        uw_result
+            .unwrap_or_else(|| Box::new(RuntimeError::InvokeError(InvokeError::FailedWithNoError)))
+    };
 
     unsafe {
         let fault = get_fault_info(siginfo as _, ucontext);
@@ -302,7 +306,7 @@ extern "C" fn signal_trap_handler(
                         ) {
                             match ib.ty {
                                 InlineBreakpointType::Middleware => {
-                                    let out: Option<Result<(), Box<dyn Any + Send>>> =
+                                    let out: Option<Result<(), RuntimeError>> =
                                         with_breakpoint_map(|bkpt_map| {
                                             bkpt_map.and_then(|x| x.get(&ip)).map(|x| {
                                                 x(BreakpointInfo {
@@ -313,7 +317,7 @@ extern "C" fn signal_trap_handler(
                                     if let Some(Ok(())) = out {
                                     } else if let Some(Err(e)) = out {
                                         should_unwind = true;
-                                        unwind_result = e;
+                                        unwind_result = Some(Box::new(e));
                                     }
                                 }
                             }
@@ -328,7 +332,7 @@ extern "C" fn signal_trap_handler(
             })
         });
         if should_unwind {
-            begin_unsafe_unwind(unwind_result);
+            begin_unsafe_unwind(get_unwind_result(unwind_result));
         }
         if early_return {
             return;
@@ -342,20 +346,22 @@ extern "C" fn signal_trap_handler(
             match Signal::from_c_int(signum) {
                 Ok(SIGTRAP) => {
                     // breakpoint
-                    let out: Option<Result<(), Box<dyn Any + Send>>> =
-                        with_breakpoint_map(|bkpt_map| {
-                            bkpt_map.and_then(|x| x.get(&(fault.ip.get()))).map(|x| {
-                                x(BreakpointInfo {
-                                    fault: Some(&fault),
-                                })
-                            })
+                    let out: Option<Result<(), RuntimeError>> =
+                        with_breakpoint_map(|bkpt_map| -> Option<Result<(), RuntimeError>> {
+                            bkpt_map.and_then(|x| x.get(&(fault.ip.get()))).map(
+                                |x| -> Result<(), RuntimeError> {
+                                    x(BreakpointInfo {
+                                        fault: Some(&fault),
+                                    })
+                                },
+                            )
                         });
                     match out {
                         Some(Ok(())) => {
                             return false;
                         }
                         Some(Err(e)) => {
-                            unwind_result = e;
+                            unwind_result = Some(Box::new(e));
                             return true;
                         }
                         None => {}
@@ -387,7 +393,7 @@ extern "C" fn signal_trap_handler(
             if is_suspend_signal {
                 // If this is a suspend signal, we parse the runtime state and return the resulting image.
                 let image = build_instance_image(ctx, es_image);
-                unwind_result = Box::new(image);
+                unwind_result = Some(Box::new(RuntimeError::InstanceImage(Box::new(image))));
             } else {
                 // Otherwise, this is a real exception and we just throw it to the caller.
                 if !es_image.frames.is_empty() {
@@ -415,7 +421,12 @@ extern "C" fn signal_trap_handler(
                     None
                 });
                 if let Some(code) = exc_code {
-                    unwind_result = Box::new(code);
+                    unwind_result =
+                        Some(Box::new(RuntimeError::InvokeError(InvokeError::TrapCode {
+                            code,
+                            // TODO:
+                            srcloc: 0,
+                        })));
                 }
             }
 
@@ -423,7 +434,7 @@ extern "C" fn signal_trap_handler(
         });
 
         if should_unwind {
-            begin_unsafe_unwind(unwind_result);
+            begin_unsafe_unwind(get_unwind_result(unwind_result));
         }
     }
 }

lib/runtime-core/src/instance.rs

@@ -5,7 +5,7 @@
 use crate::{
     backend::RunnableModule,
     backing::{ImportBacking, LocalBacking},
-    error::{CallResult, ResolveError, ResolveResult, Result, RuntimeError},
+    error::{CallResult, InvokeError, ResolveError, ResolveResult, Result, RuntimeError},
     export::{Context, Export, ExportIter, Exportable, FuncPointer},
     global::Global,
     import::{ImportObject, LikeNamespace},
@@ -584,25 +584,30 @@ pub(crate) fn call_func_with_index_inner(
         invoke_env,
     } = wasm;
 
-    let run_wasm = |result_space: *mut u64| unsafe {
+    let run_wasm = |result_space: *mut u64| -> CallResult<()> {
         let mut error_out = None;
 
-        let success = invoke(
-            trampoline,
-            ctx_ptr,
-            func_ptr,
-            raw_args.as_ptr(),
-            result_space,
-            &mut error_out,
-            invoke_env,
-        );
+        let success = unsafe {
+            invoke(
+                trampoline,
+                ctx_ptr,
+                func_ptr,
+                raw_args.as_ptr(),
+                result_space,
+                &mut error_out,
+                invoke_env,
+            )
+        };
 
         if success {
             Ok(())
         } else {
-            Err(error_out
-                .map(RuntimeError)
-                .unwrap_or_else(|| RuntimeError(Box::new("invoke(): Unknown error".to_string()))))
+            let error: RuntimeError = error_out.map_or_else(
+                || RuntimeError::InvokeError(InvokeError::FailedWithNoError),
+                Into::into,
+            );
+            dbg!(&error);
+            Err(error.into())
         }
     };
 

lib/runtime-core/src/state.rs

@@ -699,13 +699,13 @@ pub mod x64 {
     pub use super::x64_decl::*;
     use super::*;
     use crate::codegen::BreakpointMap;
+    use crate::error::RuntimeError;
     use crate::fault::{
         catch_unsafe_unwind, get_boundary_register_preservation, run_on_alternative_stack,
     };
     use crate::structures::TypedIndex;
     use crate::types::LocalGlobalIndex;
     use crate::vm::Ctx;
-    use std::any::Any;
 
     #[allow(clippy::cast_ptr_alignment)]
     unsafe fn compute_vmctx_deref(vmctx: *const Ctx, seq: &[usize]) -> u64 {
@@ -738,7 +738,7 @@ pub mod x64 {
         image: InstanceImage,
         vmctx: &mut Ctx,
         breakpoints: Option<BreakpointMap>,
-    ) -> Result<u64, Box<dyn Any + Send>> {
+    ) -> Result<u64, RuntimeError> {
         let mut stack: Vec<u64> = vec![0; 1048576 * 8 / 8]; // 8MB stack
         let mut stack_offset: usize = stack.len();
 

lib/runtime-core/src/tiering.rs

@@ -2,6 +2,7 @@
 //! as runtime.
 use crate::backend::{Compiler, CompilerConfig};
 use crate::compile_with_config;
+use crate::error::RuntimeError;
 use crate::fault::{
     catch_unsafe_unwind, ensure_sighandler, pop_code_version, push_code_version, with_ctx,
 };
@@ -223,23 +224,26 @@ pub unsafe fn run_tiering<F: Fn(InteractiveShellContext) -> ShellExitOperation>(
             }
         });
         if let Err(e) = ret {
-            if let Ok(new_image) = e.downcast::<InstanceImage>() {
+            match e {
                 // Tier switch event
-                if !was_sigint_triggered_fault() && opt_state.outcome.lock().unwrap().is_some() {
-                    resume_image = Some(*new_image);
-                    continue;
-                }
-                let op = interactive_shell(InteractiveShellContext {
-                    image: Some(*new_image),
-                    patched: n_versions.get() > 1,
-                });
-                match op {
-                    ShellExitOperation::ContinueWith(new_image) => {
-                        resume_image = Some(new_image);
+                RuntimeError::InstanceImage(ii_value) => {
+                    let new_image = ii_value.downcast::<InstanceImage>().unwrap();
+                    if !was_sigint_triggered_fault() && opt_state.outcome.lock().unwrap().is_some()
+                    {
+                        resume_image = Some(*new_image);
+                        continue;
+                    }
+                    let op = interactive_shell(InteractiveShellContext {
+                        image: Some(*new_image),
+                        patched: n_versions.get() > 1,
+                    });
+                    match op {
+                        ShellExitOperation::ContinueWith(new_image) => {
+                            resume_image = Some(new_image);
+                        }
                     }
                 }
-            } else {
-                return Err("Error while executing WebAssembly".into());
+                _ => return Err("Error while executing WebAssembly".into()),
             }
         } else {
             return Ok(());

lib/runtime-core/src/typed_func.rs

@@ -1,7 +1,7 @@
 //! The typed func module implements a way of representing a wasm function
 //! with the correct types from rust. Function calls using a typed func have a low overhead.
 use crate::{
-    error::RuntimeError,
+    error::{InvokeError, RuntimeError},
     export::{Context, Export, FuncPointer},
     import::IsExport,
     types::{FuncSig, NativeWasmType, Type, WasmExternType},
@@ -37,7 +37,7 @@ pub type Invoke = unsafe extern "C" fn(
     func: NonNull<vm::Func>,
     args: *const u64,
     rets: *mut u64,
-    error_out: *mut Option<Box<dyn Any + Send>>,
+    error_out: *mut Option<RuntimeError>,
     extra: Option<NonNull<c_void>>,
 ) -> bool;
 
@@ -340,7 +340,9 @@ impl<'a> DynamicFunc<'a> {
                 Err(e) => {
                     // At this point, there is an error that needs to be trapped.
                     drop(args); // Release the Vec which will leak otherwise.
-                    (&*vmctx.module).runnable_module.do_early_trap(e)
+                    (&*vmctx.module)
+                        .runnable_module
+                        .do_early_trap(RuntimeError::User(e))
                 }
             }
         }
@@ -588,9 +590,7 @@ macro_rules! impl_traits {
                 ) {
                     Ok(Rets::from_ret_array(rets))
                 } else {
-                    Err(error_out.map(RuntimeError).unwrap_or_else(|| {
-                        RuntimeError(Box::new("invoke(): Unknown error".to_string()))
-                    }))
+                    Err(error_out.map_or_else(|| RuntimeError::InvokeError(InvokeError::FailedWithNoError), Into::into))
                 }
             }
         }
@@ -678,9 +678,10 @@ macro_rules! impl_traits {
                         Ok(Ok(returns)) => return returns.into_c_struct(),
                         Ok(Err(err)) => {
                             let b: Box<_> = err.into();
-                            b as Box<dyn Any + Send>
+                            RuntimeError::User(b as Box<dyn Any + Send>)
                         },
-                        Err(err) => err,
+                        // TODO(blocking): this line is wrong!
+                        Err(err) => RuntimeError::User(err),
                     };
 
                     // At this point, there is an error that needs to
@@ -791,9 +792,10 @@ macro_rules! impl_traits {
                         Ok(Ok(returns)) => return returns.into_c_struct(),
                         Ok(Err(err)) => {
                             let b: Box<_> = err.into();
-                            b as Box<dyn Any + Send>
+                            RuntimeError::User(b as Box<dyn Any + Send>)
                         },
-                        Err(err) => err,
+                        // TODO(blocking): this line is wrong!
+                        Err(err) => RuntimeError::User(err),
                     };
 
                     // At this point, there is an error that needs to

lib/runtime-core/src/vm.rs

@@ -1079,10 +1079,10 @@ mod vm_ctx_tests {
         use super::Func;
         use crate::backend::{sys::Memory, CacheGen, RunnableModule};
         use crate::cache::Error as CacheError;
+        use crate::error::RuntimeError;
         use crate::typed_func::Wasm;
         use crate::types::{LocalFuncIndex, SigIndex};
         use indexmap::IndexMap;
-        use std::any::Any;
         use std::collections::HashMap;
         use std::ptr::NonNull;
         struct Placeholder;
@@ -1098,7 +1098,7 @@ mod vm_ctx_tests {
             fn get_trampoline(&self, _module: &ModuleInfo, _sig_index: SigIndex) -> Option<Wasm> {
                 unimplemented!("generate_module::get_trampoline")
             }
-            unsafe fn do_early_trap(&self, _: Box<dyn Any + Send>) -> ! {
+            unsafe fn do_early_trap(&self, _: RuntimeError) -> ! {
                 unimplemented!("generate_module::do_early_trap")
             }
         }