From c41451a5474a44ff9d16fc86269b42c694d3fd2c Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Mon, 21 Dec 2020 22:55:37 +0100 Subject: [PATCH 1/2] Replace `wasmparser::Result` with `wasmer::WasmResult` in middleware --- CHANGELOG.md | 1 + lib/api/src/lib.rs | 2 +- lib/compiler-singlepass/src/compiler.rs | 13 +++---------- lib/compiler/src/translator/middleware.rs | 16 +++++++++------- lib/middlewares/src/metering.rs | 8 +++----- tests/compilers/middlewares.rs | 6 +++--- 6 files changed, 20 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 788858a4cf3..b3dc8a2aa45 100644 --- a/CHANGELOG.md +++ b/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 for WasmError` +* [#1962](https://github.com/wasmerio/wasmer/pull/1962) Replace `wasmparser::Result` with `wasmer::WasmResult` in middleware ### Fixed diff --git a/lib/api/src/lib.rs b/lib/api/src/lib.rs index 8adef3b737f..738925df8b2 100644 --- a/lib/api/src/lib.rs +++ b/lib/api/src/lib.rs @@ -307,7 +307,7 @@ pub use wasmer_compiler::{ wasmparser, CompilerConfig, FunctionMiddleware, 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, diff --git a/lib/compiler-singlepass/src/compiler.rs b/lib/compiler-singlepass/src/compiler.rs index bda9a8f0023..0f2f6520b62 100644 --- a/lib/compiler-singlepass/src/compiler.rs +++ b/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) diff --git a/lib/compiler/src/translator/middleware.rs b/lib/compiler/src/translator/middleware.rs index e7f21c574b7..e7f7f300aa5 100644 --- a/lib/compiler/src/translator/middleware.rs +++ b/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::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<()> { + ) -> WasmResult<()> { 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 { - self.state.inner.read_var_u32() + pub fn read_local_count(&mut self) -> WasmResult { + 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> { + pub fn read_operator(&mut self) -> WasmResult> { 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. diff --git a/lib/middlewares/src/metering.rs b/lib/middlewares/src/metering.rs index a0a2919d31f..130c39961e5 100644 --- a/lib/middlewares/src/metering.rs +++ b/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, + MiddlewareReaderState, ModuleMiddleware, Mutability, Type, WasmResult, }; use wasmer_types::GlobalIndex; use wasmer_vm::ModuleInfo; @@ -118,7 +116,7 @@ impl u64 + Copy + Clone + Send + Sync> FunctionMiddleware &mut self, operator: Operator<'a>, state: &mut MiddlewareReaderState<'a>, - ) -> WpResult<()> { + ) -> WasmResult<()> { // 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. diff --git a/tests/compilers/middlewares.rs b/tests/compilers/middlewares.rs index 939c72b090f..4564cf50ff5 100644 --- a/tests/compilers/middlewares.rs +++ b/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<()> { + ) -> WasmResult<()> { 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<()> { + ) -> WasmResult<()> { match (operator, self.state) { (Operator::I32Add, 0) => { self.state = 1; From df74a4812c058d9cec06d1f8975cc54f005a9721 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Tue, 22 Dec 2020 22:27:28 +0100 Subject: [PATCH 2/2] Create dedicated MiddlewareError --- CHANGELOG.md | 2 +- lib/api/src/lib.rs | 3 +- lib/compiler/src/error.rs | 56 +++++++++++++++++++++++ lib/compiler/src/lib.rs | 4 +- lib/compiler/src/translator/middleware.rs | 4 +- lib/middlewares/src/metering.rs | 4 +- tests/compilers/middlewares.rs | 4 +- 7 files changed, 68 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3dc8a2aa45..5cb38691658 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +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 for WasmError` -* [#1962](https://github.com/wasmerio/wasmer/pull/1962) Replace `wasmparser::Result` with `wasmer::WasmResult` in middleware +* [#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 diff --git a/lib/api/src/lib.rs b/lib/api/src/lib.rs index 738925df8b2..b35fe6a0460 100644 --- a/lib/api/src/lib.rs +++ b/lib/api/src/lib.rs @@ -304,7 +304,8 @@ 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, WasmResult, diff --git a/lib/compiler/src/error.rs b/lib/compiler/src/error.rs index 0d554c15e8c..cf107ad6363 100644 --- a/lib/compiler/src/error.rs +++ b/lib/compiler/src/error.rs @@ -48,6 +48,27 @@ impl From 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, B: Into>(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 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 = Result; + +#[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), + } + } +} diff --git a/lib/compiler/src/lib.rs b/lib/compiler/src/lib.rs index 090a484f8d2..e814c6bdf4b 100644 --- a/lib/compiler/src/lib.rs +++ b/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, diff --git a/lib/compiler/src/translator/middleware.rs b/lib/compiler/src/translator/middleware.rs index e7f7f300aa5..5efcc562f9b 100644 --- a/lib/compiler/src/translator/middleware.rs +++ b/lib/compiler/src/translator/middleware.rs @@ -9,7 +9,7 @@ use wasmer_types::LocalFunctionIndex; use wasmer_vm::ModuleInfo; use wasmparser::{BinaryReader, Operator, Type}; -use crate::error::WasmResult; +use crate::error::{MiddlewareError, WasmResult}; /// A shared builder for function middlewares. pub trait ModuleMiddleware: Debug + Send + Sync { @@ -34,7 +34,7 @@ pub trait FunctionMiddleware: Debug { &mut self, operator: Operator<'a>, state: &mut MiddlewareReaderState<'a>, - ) -> WasmResult<()> { + ) -> Result<(), MiddlewareError> { state.push_operator(operator); Ok(()) } diff --git a/lib/middlewares/src/metering.rs b/lib/middlewares/src/metering.rs index 130c39961e5..f02ac1348ba 100644 --- a/lib/middlewares/src/metering.rs +++ b/lib/middlewares/src/metering.rs @@ -7,7 +7,7 @@ use std::sync::Mutex; use wasmer::wasmparser::{Operator, Type as WpType, TypeOrFuncType as WpTypeOrFuncType}; use wasmer::{ ExportIndex, FunctionMiddleware, GlobalInit, GlobalType, Instance, LocalFunctionIndex, - MiddlewareReaderState, ModuleMiddleware, Mutability, Type, WasmResult, + MiddlewareError, MiddlewareReaderState, ModuleMiddleware, Mutability, Type, }; use wasmer_types::GlobalIndex; use wasmer_vm::ModuleInfo; @@ -116,7 +116,7 @@ impl u64 + Copy + Clone + Send + Sync> FunctionMiddleware &mut self, operator: Operator<'a>, state: &mut MiddlewareReaderState<'a>, - ) -> WasmResult<()> { + ) -> 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. diff --git a/tests/compilers/middlewares.rs b/tests/compilers/middlewares.rs index 4564cf50ff5..3a9e5516656 100644 --- a/tests/compilers/middlewares.rs +++ b/tests/compilers/middlewares.rs @@ -28,7 +28,7 @@ impl FunctionMiddleware for Add2Mul { &mut self, operator: Operator<'a>, state: &mut MiddlewareReaderState<'a>, - ) -> WasmResult<()> { + ) -> 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>, - ) -> WasmResult<()> { + ) -> Result<(), MiddlewareError> { match (operator, self.state) { (Operator::I32Add, 0) => { self.state = 1;