Merge #1962

1962: Reduce scope of wasmparser::BinaryReaderError in the codebase r=MarkMcCaskey a=webmaster128

~Based on #1963~

Closes #1950

# Description

There is an error conversion pileline `BinaryReaderError` -> `WasmError`. In this PR, the same conversion happens earlier, such that `WasmError`/`WasmResult` can be used in middlewares.

Questions:
- [ ] Should middlewares be allowed to produce all `WasmError` cases? Or should we introduce a `WasmError::MiddlewareError(String)`?
- [x] ~Should `to_wasm_error` be removed as part of this PR?~ Extracted to #1963

# Review

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


Co-authored-by: Simon Warta <[email protected]>

CHANGELOG.md

@@ -17,6 +17,7 @@
 * [#1955](https://github.com/wasmerio/wasmer/pull/1955) Set `jit` as a default feature of the `wasmer-wasm-c-api` crate
 * [#1944](https://github.com/wasmerio/wasmer/pull/1944) Require `WasmerEnv` to be `Send + Sync` even in dynamic functions.
 * [#1963](https://github.com/wasmerio/wasmer/pull/1963) Removed `to_wasm_error` in favour of `impl From<BinaryReaderError> for WasmError`
+* [#1962](https://github.com/wasmerio/wasmer/pull/1962) Replace `wasmparser::Result<()>` with `Result<(), MiddlewareError>` in middleware, allowing implementors to return errors in `FunctionMiddleware::feed`
 
 ### Fixed
 

lib/api/src/lib.rs

@@ -304,10 +304,11 @@ pub use crate::utils::is_wasm;
 pub use target_lexicon::{Architecture, CallingConvention, OperatingSystem, Triple, HOST};
 #[cfg(feature = "compiler")]
 pub use wasmer_compiler::{
-    wasmparser, CompilerConfig, FunctionMiddleware, MiddlewareReaderState, ModuleMiddleware,
+    wasmparser, CompilerConfig, FunctionMiddleware, MiddlewareError, MiddlewareReaderState,
+    ModuleMiddleware,
 };
 pub use wasmer_compiler::{
-    CompileError, CpuFeature, Features, ParseCpuFeatureError, Target, WasmError,
+    CompileError, CpuFeature, Features, ParseCpuFeatureError, Target, WasmError, WasmResult,
 };
 pub use wasmer_engine::{
     ChainableNamedResolver, DeserializeError, Engine, Export, FrameInfo, LinkError, NamedResolver,

lib/compiler-singlepass/src/compiler.rs

@@ -9,7 +9,6 @@ use crate::codegen_x64::{
 use crate::config::Singlepass;
 use rayon::prelude::{IntoParallelIterator, IntoParallelRefIterator, ParallelIterator};
 use std::sync::Arc;
-use wasmer_compiler::wasmparser::BinaryReaderError;
 use wasmer_compiler::TrapInformation;
 use wasmer_compiler::{
     Architecture, CompileModuleInfo, CompilerConfig, MiddlewareBinaryReader, ModuleMiddlewareChain,
@@ -92,9 +91,9 @@ impl Compiler for SinglepassCompiler {
 
                 // This local list excludes arguments.
                 let mut locals = vec![];
-                let num_locals = reader.read_local_count().map_err(to_compile_error)?;
+                let num_locals = reader.read_local_count()?;
                 for _ in 0..num_locals {
-                    let (count, ty) = reader.read_local_decl().map_err(to_compile_error)?;
+                    let (count, ty) = reader.read_local_decl()?;
                     for _ in 0..count {
                         locals.push(ty);
                     }
@@ -113,7 +112,7 @@ impl Compiler for SinglepassCompiler {
 
                 while generator.has_control_frames() {
                     generator.set_srcloc(reader.original_position() as u32);
-                    let op = reader.read_operator().map_err(to_compile_error)?;
+                    let op = reader.read_operator()?;
                     generator.feed_operator(op).map_err(to_compile_error)?;
                 }
 
@@ -157,12 +156,6 @@ trait ToCompileError {
     fn to_compile_error(self) -> CompileError;
 }
 
-impl ToCompileError for BinaryReaderError {
-    fn to_compile_error(self) -> CompileError {
-        CompileError::Codegen(self.message().into())
-    }
-}
-
 impl ToCompileError for CodegenError {
     fn to_compile_error(self) -> CompileError {
         CompileError::Codegen(self.message)

lib/compiler/src/error.rs

@@ -48,6 +48,27 @@ impl From<WasmError> for CompileError {
     }
 }
 
+/// A error in the middleware.
+#[derive(Debug)]
+#[cfg_attr(feature = "std", derive(Error))]
+#[cfg_attr(feature = "std", error("Error in middleware {name}: {message}"))]
+pub struct MiddlewareError {
+    /// The name of the middleware where the error was created
+    pub name: String,
+    /// The error message
+    pub message: String,
+}
+
+impl MiddlewareError {
+    /// Create a new `MiddlewareError`
+    pub fn new<A: Into<String>, B: Into<String>>(name: A, message: B) -> Self {
+        Self {
+            name: name.into(),
+            message: message.into(),
+        }
+    }
+}
+
 /// A WebAssembly translation error.
 ///
 /// When a WebAssembly function can't be translated, one of these error codes will be returned
@@ -80,11 +101,21 @@ pub enum WasmError {
     #[cfg_attr(feature = "std", error("Implementation limit exceeded"))]
     ImplLimitExceeded,
 
+    /// An error from the middleware error.
+    #[cfg_attr(feature = "std", error("{0}"))]
+    Middleware(MiddlewareError),
+
     /// A generic error.
     #[cfg_attr(feature = "std", error("{0}"))]
     Generic(String),
 }
 
+impl From<MiddlewareError> for WasmError {
+    fn from(original: MiddlewareError) -> Self {
+        Self::Middleware(original)
+    }
+}
+
 /// The error that can happen while parsing a `str`
 /// to retrieve a [`CpuFeature`](crate::target::CpuFeature).
 #[derive(Debug)]
@@ -97,3 +128,28 @@ pub enum ParseCpuFeatureError {
 
 /// A convenient alias for a `Result` that uses `WasmError` as the error type.
 pub type WasmResult<T> = Result<T, WasmError>;
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn middleware_error_can_be_created() {
+        let msg = String::from("Something went wrong");
+        let error = MiddlewareError::new("manipulator3000", msg);
+        assert_eq!(error.name, "manipulator3000");
+        assert_eq!(error.message, "Something went wrong");
+    }
+
+    #[test]
+    fn middleware_error_be_converted_to_wasm_error() {
+        let error = WasmError::from(MiddlewareError::new("manipulator3000", "foo"));
+        match error {
+            WasmError::Middleware(MiddlewareError { name, message }) => {
+                assert_eq!(name, "manipulator3000");
+                assert_eq!(message, "foo");
+            }
+            err => panic!("Unexpected error: {:?}", err),
+        }
+    }
+}

lib/compiler/src/lib.rs

@@ -69,7 +69,9 @@ mod sourceloc;
 pub use crate::address_map::{FunctionAddressMap, InstructionAddressMap};
 #[cfg(feature = "translator")]
 pub use crate::compiler::{Compiler, CompilerConfig, Symbol, SymbolRegistry};
-pub use crate::error::{CompileError, ParseCpuFeatureError, WasmError, WasmResult};
+pub use crate::error::{
+    CompileError, MiddlewareError, ParseCpuFeatureError, WasmError, WasmResult,
+};
 pub use crate::function::{
     Compilation, CompiledFunction, CompiledFunctionFrameInfo, CustomSections, Dwarf, FunctionBody,
     Functions,

lib/compiler/src/translator/middleware.rs

@@ -7,7 +7,9 @@ use std::fmt::Debug;
 use std::ops::Deref;
 use wasmer_types::LocalFunctionIndex;
 use wasmer_vm::ModuleInfo;
-use wasmparser::{BinaryReader, Operator, Result as WpResult, Type};
+use wasmparser::{BinaryReader, Operator, Type};
+
+use crate::error::{MiddlewareError, WasmResult};
 
 /// A shared builder for function middlewares.
 pub trait ModuleMiddleware: Debug + Send + Sync {
@@ -32,7 +34,7 @@ pub trait FunctionMiddleware: Debug {
         &mut self,
         operator: Operator<'a>,
         state: &mut MiddlewareReaderState<'a>,
-    ) -> WpResult<()> {
+    ) -> Result<(), MiddlewareError> {
         state.push_operator(operator);
         Ok(())
     }
@@ -127,22 +129,22 @@ impl<'a> MiddlewareBinaryReader<'a> {
     }
 
     /// Read a `count` indicating the number of times to call `read_local_decl`.
-    pub fn read_local_count(&mut self) -> WpResult<u32> {
-        self.state.inner.read_var_u32()
+    pub fn read_local_count(&mut self) -> WasmResult<u32> {
+        Ok(self.state.inner.read_var_u32()?)
     }
 
     /// Read a `(count, value_type)` declaration of local variables of the same type.
-    pub fn read_local_decl(&mut self) -> WpResult<(u32, Type)> {
+    pub fn read_local_decl(&mut self) -> WasmResult<(u32, Type)> {
         let count = self.state.inner.read_var_u32()?;
         let ty = self.state.inner.read_type()?;
         Ok((count, ty))
     }
 
     /// Reads the next available `Operator`.
-    pub fn read_operator(&mut self) -> WpResult<Operator<'a>> {
+    pub fn read_operator(&mut self) -> WasmResult<Operator<'a>> {
         if self.chain.is_empty() {
             // We short-circuit in case no chain is used
-            return self.state.inner.read_operator();
+            return Ok(self.state.inner.read_operator()?);
         }
 
         // Try to fill the `self.pending_operations` buffer, until it is non-empty.

lib/middlewares/src/metering.rs

@@ -4,12 +4,10 @@
 use std::convert::TryInto;
 use std::fmt;
 use std::sync::Mutex;
-use wasmer::wasmparser::{
-    Operator, Result as WpResult, Type as WpType, TypeOrFuncType as WpTypeOrFuncType,
-};
+use wasmer::wasmparser::{Operator, Type as WpType, TypeOrFuncType as WpTypeOrFuncType};
 use wasmer::{
     ExportIndex, FunctionMiddleware, GlobalInit, GlobalType, Instance, LocalFunctionIndex,
-    MiddlewareReaderState, ModuleMiddleware, Mutability, Type,
+    MiddlewareError, MiddlewareReaderState, ModuleMiddleware, Mutability, Type,
 };
 use wasmer_types::GlobalIndex;
 use wasmer_vm::ModuleInfo;
@@ -118,7 +116,7 @@ impl<F: Fn(&Operator) -> u64 + Copy + Clone + Send + Sync> FunctionMiddleware
         &mut self,
         operator: Operator<'a>,
         state: &mut MiddlewareReaderState<'a>,
-    ) -> WpResult<()> {
+    ) -> Result<(), MiddlewareError> {
         // Get the cost of the current operator, and add it to the accumulator.
         // This needs to be done before the metering logic, to prevent operators like `Call` from escaping metering in some
         // corner cases.

tests/compilers/middlewares.rs

@@ -2,7 +2,7 @@ use crate::utils::get_store_with_middlewares;
 use anyhow::Result;
 
 use std::sync::Arc;
-use wasmer::wasmparser::{Operator, Result as WpResult};
+use wasmer::wasmparser::Operator;
 use wasmer::*;
 
 #[derive(Debug)]
@@ -28,7 +28,7 @@ impl FunctionMiddleware for Add2Mul {
         &mut self,
         operator: Operator<'a>,
         state: &mut MiddlewareReaderState<'a>,
-    ) -> WpResult<()> {
+    ) -> Result<(), MiddlewareError> {
         match operator {
             Operator::I32Add => {
                 state.push_operator(Operator::I32Mul);
@@ -66,7 +66,7 @@ impl FunctionMiddleware for Fusion {
         &mut self,
         operator: Operator<'a>,
         state: &mut MiddlewareReaderState<'a>,
-    ) -> WpResult<()> {
+    ) -> Result<(), MiddlewareError> {
         match (operator, self.state) {
             (Operator::I32Add, 0) => {
                 self.state = 1;