From 52bbe003bfbbf46722057741ced0d52173d74f4d Mon Sep 17 00:00:00 2001 From: Peter Huene Date: Tue, 30 Mar 2021 17:20:15 -0700 Subject: [PATCH] Code review feedback. * Remove `Config::for_target` in favor of setter `Config::target`. * Remove explicit setting of Cranelift flags in `Config::new` in favor of calling the `Config` methods that do the same thing. * Serialize the package version independently of the data when serializing a module. * Use struct deconstructing in module serialization to ensure tunables and features aren't missed. * Move common log initialization in the CLI into `CommonOptions`. --- crates/wasmtime/src/config.rs | 68 ++++---- crates/wasmtime/src/module.rs | 45 +++++- crates/wasmtime/src/module/serialization.rs | 162 +++++++++----------- src/commands/compile.rs | 11 +- src/commands/run.rs | 11 +- src/commands/wasm2obj.rs | 11 +- src/commands/wast.rs | 11 +- src/lib.rs | 22 ++- tests/all/module_serialize.rs | 17 ++ 9 files changed, 180 insertions(+), 178 deletions(-) diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs index 1fd5ee8b3ea0..2ccfca22a043 100644 --- a/crates/wasmtime/src/config.rs +++ b/crates/wasmtime/src/config.rs @@ -392,20 +392,6 @@ impl Config { /// Creates a new configuration object with the default configuration /// specified. pub fn new() -> Self { - Self::new_with_isa_flags(native::builder()) - } - - /// Creates a [`Config`] for the given target triple. - /// - /// No CPU flags will be enabled for the config. - pub fn for_target(target: &str) -> Result { - use std::str::FromStr; - Ok(Self::new_with_isa_flags(native::lookup( - target_lexicon::Triple::from_str(target).map_err(|e| anyhow::anyhow!(e))?, - )?)) - } - - fn new_with_isa_flags(isa_flags: isa::Builder) -> Self { let mut flags = settings::builder(); // There are two possible traps for division, and this way @@ -414,30 +400,15 @@ impl Config { .enable("avoid_div_traps") .expect("should be valid flag"); - // Invert cranelift's default-on verification to instead default off. - flags - .set("enable_verifier", "false") - .expect("should be valid flag"); - - // Turn on cranelift speed optimizations by default - flags - .set("opt_level", "speed") - .expect("should be valid flag"); - // We don't use probestack as a stack limit mechanism flags .set("enable_probestack", "false") .expect("should be valid flag"); - // Reference types are enabled by default, so enable safepoints - flags - .set("enable_safepoints", "true") - .expect("should be valid flag"); - let mut ret = Self { tunables: Tunables::default(), flags, - isa_flags, + isa_flags: native::builder(), strategy: CompilationStrategy::Auto, #[cfg(feature = "cache")] cache_config: CacheConfig::new_cache_disabled(), @@ -446,12 +417,7 @@ impl Config { allocation_strategy: InstanceAllocationStrategy::OnDemand, max_wasm_stack: 1 << 20, wasm_backtrace_details_env_used: false, - features: WasmFeatures { - reference_types: true, - bulk_memory: true, - multi_value: true, - ..WasmFeatures::default() - }, + features: WasmFeatures::default(), max_instances: 10_000, max_tables: 10_000, max_memories: 10_000, @@ -460,10 +426,38 @@ impl Config { host_funcs: HostFuncMap::new(), async_support: false, }; + ret.cranelift_debug_verifier(false); + ret.cranelift_opt_level(OptLevel::Speed); + ret.wasm_reference_types(true); + ret.wasm_multi_value(true); + ret.wasm_bulk_memory(true); ret.wasm_backtrace_details(WasmBacktraceDetails::Environment); ret } + /// Sets the target triple for the [`Config`]. + /// + /// By default, the host target triple is used for the [`Config`]. + /// + /// This method can be used to change the target triple. + /// + /// Note that any no Cranelift flags will be inferred for the given target. + /// + /// [`Config::cranelift_clear_cpu_flags`] will reset the target triple back to + /// the host's target. + /// + /// # Errors + /// + /// This method will error if the given target triple is not supported. + pub fn target(&mut self, target: &str) -> Result<&mut Self> { + use std::str::FromStr; + self.isa_flags = native::lookup( + target_lexicon::Triple::from_str(target).map_err(|e| anyhow::anyhow!(e))?, + )?; + + Ok(self) + } + /// Whether or not to enable support for asynchronous functions in Wasmtime. /// /// When enabled, the config can optionally define host functions with `async`. @@ -906,6 +900,8 @@ impl Config { /// Clears native CPU flags inferred from the host. /// + /// Note: this method will change the target to that of the host. + /// /// By default Wasmtime will tune generated code for the host that Wasmtime /// itself is running on. If you're compiling on one host, however, and /// shipping artifacts to another host then this behavior may not be diff --git a/crates/wasmtime/src/module.rs b/crates/wasmtime/src/module.rs index 6d1d9b3ebcc7..bb3fcf0da663 100644 --- a/crates/wasmtime/src/module.rs +++ b/crates/wasmtime/src/module.rs @@ -17,7 +17,7 @@ mod serialization; use serialization::SerializedModule; -const COMPILED_MODULE_HEADER: &[u8] = b"\0wasmtimeaot"; +const COMPILED_MODULE_HEADER: &[u8] = b"\0wasmtime-aot"; /// A compiled WebAssembly module, ready to be instantiated. /// @@ -364,12 +364,10 @@ impl Module { // Write a header that marks this as a compiled module output.write_all(COMPILED_MODULE_HEADER)?; - bincode_options().serialize_into( - output, + Self::serialize_module( &SerializedModule::from_artifacts(engine.compiler(), &artifacts, &types), - )?; - - Ok(()) + output, + ) } /// Returns the type signature of this module. @@ -392,10 +390,24 @@ impl Module { /// Serialize compilation artifacts to the buffer. See also `deserialize`. pub fn serialize(&self) -> Result> { let mut buffer = Vec::new(); - bincode_options().serialize_into(&mut buffer, &SerializedModule::new(self))?; + Self::serialize_module(&SerializedModule::new(self), &mut buffer)?; Ok(buffer) } + fn serialize_module(module: &SerializedModule, mut output: impl Write) -> Result<()> { + // Preface the data with a version so we can do a version check independent + // of the serialized data. + let version = env!("CARGO_PKG_VERSION"); + assert!( + version.len() < 256, + "package version must be less than 256 bytes" + ); + output.write(&[version.len() as u8])?; + output.write_all(version.as_bytes())?; + bincode_options().serialize_into(output, module)?; + Ok(()) + } + /// Deserializes and creates a module from the compilation artifacts. /// The `serialize` saves the compilation artifacts along with the host /// fingerprint, which consists of target, compiler flags, and wasmtime @@ -406,8 +418,25 @@ impl Module { /// for modifications or corruptions. All responsibly of signing and its /// verification falls on the embedder. pub fn deserialize(engine: &Engine, serialized: &[u8]) -> Result { + if serialized.is_empty() { + bail!("serialized data data is empty"); + } + + let version_len = serialized[0] as usize; + if serialized.len() < version_len + 1 { + bail!("serialized data is malformed"); + } + + let version = std::str::from_utf8(&serialized[1..1 + version_len])?; + if version != env!("CARGO_PKG_VERSION") { + bail!( + "Module was compiled with incompatible Wasmtime version '{}'", + version + ); + } + bincode_options() - .deserialize::>(serialized) + .deserialize::>(&serialized[1 + version_len..]) .context("Deserialize compilation artifacts")? .into_module(engine) } diff --git a/crates/wasmtime/src/module/serialization.rs b/crates/wasmtime/src/module/serialization.rs index 9c469924ad86..3daa7b925992 100644 --- a/crates/wasmtime/src/module/serialization.rs +++ b/crates/wasmtime/src/module/serialization.rs @@ -32,18 +32,32 @@ struct WasmFeatures { impl From<&wasmparser::WasmFeatures> for WasmFeatures { fn from(other: &wasmparser::WasmFeatures) -> Self { + let wasmparser::WasmFeatures { + reference_types, + multi_value, + bulk_memory, + module_linking, + simd, + threads, + tail_call, + deterministic_only, + multi_memory, + exceptions, + memory64, + } = other; + Self { - reference_types: other.reference_types, - multi_value: other.multi_value, - bulk_memory: other.bulk_memory, - module_linking: other.module_linking, - simd: other.simd, - threads: other.threads, - tail_call: other.tail_call, - deterministic_only: other.deterministic_only, - multi_memory: other.multi_memory, - exceptions: other.exceptions, - memory64: other.memory64, + reference_types: *reference_types, + multi_value: *multi_value, + bulk_memory: *bulk_memory, + module_linking: *module_linking, + simd: *simd, + threads: *threads, + tail_call: *tail_call, + deterministic_only: *deterministic_only, + multi_memory: *multi_memory, + exceptions: *exceptions, + memory64: *memory64, } } } @@ -149,7 +163,6 @@ impl<'a> SerializedModuleData<'a> { #[derive(Serialize, Deserialize)] pub struct SerializedModule<'a> { - version: String, target: String, flags_hash: u64, // Record the opt level as it is the most common Cranelift flag users might change @@ -192,7 +205,6 @@ impl<'a> SerializedModule<'a> { let isa = compiler.isa(); Self { - version: env!("CARGO_PKG_VERSION").to_string(), target: isa.triple().to_string(), opt_level: isa.flags().opt_level().into(), flags_hash: Self::simple_hash(isa.flags()), @@ -209,7 +221,6 @@ impl<'a> SerializedModule<'a> { let compiler = engine.compiler(); let isa = compiler.isa(); - self.check_version()?; self.check_triple(isa)?; self.check_isa_flags(isa)?; self.check_strategy(compiler)?; @@ -262,17 +273,6 @@ impl<'a> SerializedModule<'a> { } } - fn check_version(&self) -> Result<()> { - if self.version != env!("CARGO_PKG_VERSION") { - bail!( - "Module was compiled with Wasmtime version '{}'", - self.version - ); - } - - Ok(()) - } - fn check_triple(&self, isa: &dyn TargetIsa) -> Result<()> { let triple = target_lexicon::Triple::from_str(&self.target).map_err(|e| anyhow!(e))?; @@ -368,120 +368,115 @@ impl<'a> SerializedModule<'a> { } fn check_tunables(&self, compiler: &Compiler) -> Result<()> { + let Tunables { + static_memory_bound, + static_memory_offset_guard_size, + dynamic_memory_offset_guard_size, + generate_native_debuginfo, + parse_wasm_debuginfo, + interruptable, + consume_fuel, + static_memory_bound_is_maximum, + } = self.tunables; + let other = compiler.tunables(); Self::check_int( - self.tunables.static_memory_bound, + static_memory_bound, other.static_memory_bound, "static memory bound", )?; Self::check_int( - self.tunables.static_memory_offset_guard_size, + static_memory_offset_guard_size, other.static_memory_offset_guard_size, "static memory guard size", )?; Self::check_int( - self.tunables.dynamic_memory_offset_guard_size, + dynamic_memory_offset_guard_size, other.dynamic_memory_offset_guard_size, "dynamic memory guard size", )?; Self::check_bool( - self.tunables.generate_native_debuginfo, + generate_native_debuginfo, other.generate_native_debuginfo, "debug information support", )?; Self::check_bool( - self.tunables.parse_wasm_debuginfo, + parse_wasm_debuginfo, other.parse_wasm_debuginfo, "WebAssembly backtrace support", )?; + Self::check_bool(interruptable, other.interruptable, "interruption support")?; + Self::check_bool(consume_fuel, other.consume_fuel, "fuel support")?; Self::check_bool( - self.tunables.interruptable, - other.interruptable, - "interruption support", - )?; - Self::check_bool( - self.tunables.consume_fuel, - other.consume_fuel, - "fuel support", - )?; - Self::check_bool( - self.tunables.static_memory_bound_is_maximum, + static_memory_bound_is_maximum, other.static_memory_bound_is_maximum, "pooling allocation support", )?; - // At this point, the hashes should match (if not we're missing a check) - assert_eq!( - Self::simple_hash(&self.tunables), - Self::simple_hash(other), - "unexpected hash difference" - ); - Ok(()) } fn check_features(&self, compiler: &Compiler) -> Result<()> { + let WasmFeatures { + reference_types, + multi_value, + bulk_memory, + module_linking, + simd, + threads, + tail_call, + deterministic_only, + multi_memory, + exceptions, + memory64, + } = self.features; + let other = compiler.features(); Self::check_bool( - self.features.reference_types, + reference_types, other.reference_types, "WebAssembly reference types support", )?; Self::check_bool( - self.features.multi_value, + multi_value, other.multi_value, "WebAssembly multi-value support", )?; Self::check_bool( - self.features.bulk_memory, + bulk_memory, other.bulk_memory, "WebAssembly bulk memory support", )?; Self::check_bool( - self.features.module_linking, + module_linking, other.module_linking, "WebAssembly module linking support", )?; - Self::check_bool(self.features.simd, other.simd, "WebAssembly SIMD support")?; - Self::check_bool( - self.features.threads, - other.threads, - "WebAssembly threads support", - )?; - Self::check_bool( - self.features.tail_call, - other.tail_call, - "WebAssembly tail-call support", - )?; + Self::check_bool(simd, other.simd, "WebAssembly SIMD support")?; + Self::check_bool(threads, other.threads, "WebAssembly threads support")?; + Self::check_bool(tail_call, other.tail_call, "WebAssembly tail-call support")?; Self::check_bool( - self.features.deterministic_only, + deterministic_only, other.deterministic_only, "WebAssembly deterministic-only support", )?; Self::check_bool( - self.features.multi_memory, + multi_memory, other.multi_memory, "WebAssembly multi-memory support", )?; Self::check_bool( - self.features.exceptions, + exceptions, other.exceptions, "WebAssembly exceptions support", )?; Self::check_bool( - self.features.memory64, + memory64, other.memory64, "WebAssembly 64-bit memory support", )?; - // At this point, the hashes should match (if not we're missing a check) - assert_eq!( - Self::simple_hash(&self.features), - Self::simple_hash(other), - "unexpected hash difference" - ); - Ok(()) } } @@ -491,25 +486,6 @@ mod test { use super::*; use crate::Config; - #[test] - fn test_version_mismatch() -> Result<()> { - let engine = Engine::default(); - let module = Module::new(&engine, "(module)")?; - - let mut serialized = SerializedModule::new(&module); - serialized.version = "0.0.1".to_string(); - - match serialized.into_module(&engine) { - Ok(_) => unreachable!(), - Err(e) => assert_eq!( - e.to_string(), - "Module was compiled with Wasmtime version '0.0.1'" - ), - } - - Ok(()) - } - #[test] fn test_architecture_mismatch() -> Result<()> { let engine = Engine::default(); diff --git a/src/commands/compile.rs b/src/commands/compile.rs index 79ea0305326c..77a38940c66c 100644 --- a/src/commands/compile.rs +++ b/src/commands/compile.rs @@ -1,6 +1,6 @@ //! The module that implements the `wasmtime wast` command. -use crate::{init_file_per_thread_logger, CommonOptions}; +use crate::CommonOptions; use anyhow::{anyhow, bail, Context, Result}; use std::fs::{self, File}; use std::io::BufWriter; @@ -148,14 +148,7 @@ pub struct CompileCommand { impl CompileCommand { /// Executes the command. pub fn execute(mut self) -> Result<()> { - if !self.common.disable_logging { - if self.common.log_to_files { - let prefix = "wasmtime.dbg."; - init_file_per_thread_logger(prefix); - } else { - pretty_env_logger::init(); - } - } + self.common.init_logging(); let target = self .target diff --git a/src/commands/run.rs b/src/commands/run.rs index d20b8275de99..83ede6a06caf 100644 --- a/src/commands/run.rs +++ b/src/commands/run.rs @@ -1,6 +1,6 @@ //! The module that implements the `wasmtime run` command. -use crate::{init_file_per_thread_logger, CommonOptions}; +use crate::CommonOptions; use anyhow::{bail, Context as _, Result}; use std::thread; use std::time::Duration; @@ -126,14 +126,7 @@ pub struct RunCommand { impl RunCommand { /// Executes the command. pub fn execute(&self) -> Result<()> { - if !self.common.disable_logging { - if self.common.log_to_files { - let prefix = "wasmtime.dbg."; - init_file_per_thread_logger(prefix); - } else { - pretty_env_logger::init(); - } - } + self.common.init_logging(); let mut config = self.common.config(None)?; if self.wasm_timeout.is_some() { diff --git a/src/commands/wasm2obj.rs b/src/commands/wasm2obj.rs index 4a88a3051ff1..efb118d615c7 100644 --- a/src/commands/wasm2obj.rs +++ b/src/commands/wasm2obj.rs @@ -1,7 +1,7 @@ //! The module that implements the `wasmtime wasm2obj` command. use crate::obj::compile_to_obj; -use crate::{init_file_per_thread_logger, parse_target, pick_compilation_strategy, CommonOptions}; +use crate::{parse_target, pick_compilation_strategy, CommonOptions}; use anyhow::{Context as _, Result}; use std::{ fs::File, @@ -43,14 +43,7 @@ pub struct WasmToObjCommand { impl WasmToObjCommand { /// Executes the command. pub fn execute(self) -> Result<()> { - if !self.common.disable_logging { - if self.common.log_to_files { - let prefix = "wasm2obj.dbg."; - init_file_per_thread_logger(prefix); - } else { - pretty_env_logger::init(); - } - } + self.common.init_logging(); let strategy = pick_compilation_strategy(self.common.cranelift, self.common.lightbeam)?; diff --git a/src/commands/wast.rs b/src/commands/wast.rs index 848e9179e653..b757db41b1d8 100644 --- a/src/commands/wast.rs +++ b/src/commands/wast.rs @@ -1,6 +1,6 @@ //! The module that implements the `wasmtime wast` command. -use crate::{init_file_per_thread_logger, CommonOptions}; +use crate::CommonOptions; use anyhow::{Context as _, Result}; use std::path::PathBuf; use structopt::{clap::AppSettings, StructOpt}; @@ -26,14 +26,7 @@ pub struct WastCommand { impl WastCommand { /// Executes the command. pub fn execute(self) -> Result<()> { - if !self.common.disable_logging { - if self.common.log_to_files { - let prefix = "wast.dbg."; - init_file_per_thread_logger(prefix); - } else { - pretty_env_logger::init(); - } - } + self.common.init_logging(); let config = self.common.config(None)?; let store = Store::new(&Engine::new(&config)?); diff --git a/src/lib.rs b/src/lib.rs index 7f89db761403..259be6de6483 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -196,12 +196,24 @@ struct CommonOptions { } impl CommonOptions { - fn config(&self, target: Option<&str>) -> Result { - let mut config = if let Some(target) = target { - Config::for_target(target)? + fn init_logging(&self) { + if self.disable_logging { + return; + } + if self.log_to_files { + let prefix = "wasmtime.dbg."; + init_file_per_thread_logger(prefix); } else { - Config::new() - }; + pretty_env_logger::init(); + } + } + fn config(&self, target: Option<&str>) -> Result { + let mut config = Config::new(); + + // Set the target before setting any cranelift options + if let Some(target) = target { + config.target(target)?; + } config .cranelift_debug_verifier(self.enable_cranelift_debug_verifier) diff --git a/tests/all/module_serialize.rs b/tests/all/module_serialize.rs index a77f62b077a2..dab73f67757f 100644 --- a/tests/all/module_serialize.rs +++ b/tests/all/module_serialize.rs @@ -11,6 +11,23 @@ fn deserialize_and_instantiate(store: &Store, buffer: &[u8]) -> Result Ok(Instance::new(&store, &module, &[])?) } +#[test] +fn test_version_mismatch() -> Result<()> { + let engine = Engine::default(); + let mut buffer = serialize(&engine, "(module)")?; + buffer[1] = 'x' as u8; + + match Module::deserialize(&engine, &buffer) { + Ok(_) => bail!("expected deserialization to fail"), + Err(e) => assert_eq!( + e.to_string(), + "Module was compiled with incompatible Wasmtime version 'x.25.0'" + ), + } + + Ok(()) +} + #[test] fn test_module_serialize_simple() -> Result<()> { let buffer = serialize(