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

@@ -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<Self> {
-        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 {
+            features: WasmFeatures::default(),
-                reference_types: true,
-                bulk_memory: true,
-                multi_value: true,
-                ..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

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(
+        Self::serialize_module(
-            output,
             &SerializedModule::from_artifacts(engine.compiler(), &artifacts, &types),
-        )?;
+            output,
-
+        )
-        Ok(())
     }
 
     /// 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<Vec<u8>> {
         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<Module> {
+        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::<SerializedModule<'_>>(serialized)
+            .deserialize::<SerializedModule<'_>>(&serialized[1 + version_len..])
             .context("Deserialize compilation artifacts")?
             .into_module(engine)
     }

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,
+            reference_types: *reference_types,
-            multi_value: other.multi_value,
+            multi_value: *multi_value,
-            bulk_memory: other.bulk_memory,
+            bulk_memory: *bulk_memory,
-            module_linking: other.module_linking,
+            module_linking: *module_linking,
-            simd: other.simd,
+            simd: *simd,
-            threads: other.threads,
+            threads: *threads,
-            tail_call: other.tail_call,
+            tail_call: *tail_call,
-            deterministic_only: other.deterministic_only,
+            deterministic_only: *deterministic_only,
-            multi_memory: other.multi_memory,
+            multi_memory: *multi_memory,
-            exceptions: other.exceptions,
+            exceptions: *exceptions,
-            memory64: other.memory64,
+            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,
+            static_memory_bound_is_maximum,
-            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,
             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(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.threads,
+            deterministic_only,
-            other.threads,
-            "WebAssembly threads support",
-        )?;
-        Self::check_bool(
-            self.features.tail_call,
-            other.tail_call,
-            "WebAssembly tail-call support",
-        )?;
-        Self::check_bool(
-            self.features.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();

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 {
+        self.common.init_logging();
-            if self.common.log_to_files {
-                let prefix = "wasmtime.dbg.";
-                init_file_per_thread_logger(prefix);
-            } else {
-                pretty_env_logger::init();
-            }
-        }
 
         let target = self
             .target

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 {
+        self.common.init_logging();
-            if self.common.log_to_files {
-                let prefix = "wasmtime.dbg.";
-                init_file_per_thread_logger(prefix);
-            } else {
-                pretty_env_logger::init();
-            }
-        }
 
         let mut config = self.common.config(None)?;
         if self.wasm_timeout.is_some() {

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 {
+        self.common.init_logging();
-            if self.common.log_to_files {
-                let prefix = "wasm2obj.dbg.";
-                init_file_per_thread_logger(prefix);
-            } else {
-                pretty_env_logger::init();
-            }
-        }
 
         let strategy = pick_compilation_strategy(self.common.cranelift, self.common.lightbeam)?;
 

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 {
+        self.common.init_logging();
-            if self.common.log_to_files {
-                let prefix = "wast.dbg.";
-                init_file_per_thread_logger(prefix);
-            } else {
-                pretty_env_logger::init();
-            }
-        }
 
         let config = self.common.config(None)?;
         let store = Store::new(&Engine::new(&config)?);

src/lib.rs

@@ -196,12 +196,24 @@ struct CommonOptions {
 }
 
 impl CommonOptions {
-    fn config(&self, target: Option<&str>) -> Result<Config> {
+    fn init_logging(&self) {
-        let mut config = if let Some(target) = target {
+        if self.disable_logging {
-            Config::for_target(target)?
+            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<Config> {
+        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)

tests/all/module_serialize.rs

@@ -11,6 +11,23 @@ fn deserialize_and_instantiate(store: &Store, buffer: &[u8]) -> Result<Instance>
     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(