fix(wasmtime):Config methods should be idempotent

This commit refactored `Config` to use a seperate `CompilerConfig` field instead
of operating on `CompilerBuilder` directly to make all its methods idempotent.

Fixes #4189

RELEASES.md

@@ -53,6 +53,10 @@ Unreleased.
  the associated pointer.
  [#4192](https://github.com/bytecodealliance/wasmtime/pull/4192)
 
+* The `Config::strategy` and `Config::cranelift_flag_set` APIs now return `&mut Self`
+ instead of `Result<&mut Self>` since the validation is deferred until `Engine::new`.
+ [#4252](https://github.com/bytecodealliance/wasmtime/pull/4252)
+
 ### Fixed
 
 * An improvement was made to the spill-slot allocation in code generation to fix
@@ -61,6 +65,10 @@ Unreleased.
  larger-than-intended increases in stack frame sizes.
  [#4222](https://github.com/bytecodealliance/wasmtime/pull/4222)
 
+* A refactor of `Config` was made to fix an issue that the order of calls to `Config`
+ matters now, which may lead to unexpected behavior.
+ [#4252](https://github.com/bytecodealliance/wasmtime/pull/4252)
+
 --------------------------------------------------------------------------------
 
 ## 0.37.0
@@ -1474,3 +1482,4 @@ Released 2020-02-26.
 * Automated releases for tags should be up and running again, along with
  automatic publication of the `wasmtime` Python package.
  [#971](https://github.com/bytecodealliance/wasmtime/pull/971)
+

crates/cli-flags/src/lib.rs

@@ -273,7 +273,7 @@ impl CommonOptions {
 
  for (name, value) in &self.cranelift_set {
  unsafe {
- config.cranelift_flag_set(name, value)?;
+ config.cranelift_flag_set(name, value);
  }
  }
 

crates/wasmtime/src/config.rs

@@ -2,11 +2,14 @@ use crate::memory::MemoryCreator;
 use crate::trampoline::MemoryCreatorProxy;
 use anyhow::{bail, Result};
 use serde::{Deserialize, Serialize};
-use std::cmp;
 use std::fmt;
 #[cfg(feature = "cache")]
 use std::path::Path;
 use std::sync::Arc;
+use std::{
+ cmp,
+ collections::{HashMap, HashSet},
+};
 use wasmparser::WasmFeatures;
 #[cfg(feature = "cache")]
 use wasmtime_cache::CacheConfig;
@@ -82,7 +85,7 @@ impl Default for ModuleVersionStrategy {
 /// [`Engine::new()`](crate::Engine::new)
 pub struct Config {
  #[cfg(compiler)]
- pub(crate) compiler: Box<dyn CompilerBuilder>,
+ pub(crate) compiler_config: CompilerConfig,
  pub(crate) tunables: Tunables,
  #[cfg(feature = "cache")]
  pub(crate) cache_config: CacheConfig,
@@ -103,14 +106,56 @@ pub struct Config {
  pub(crate) force_memory_init_memfd: bool,
 }
 
+/// User-provided configuration for the compiler.
+#[cfg(compiler)]
+#[derive(Debug, Clone)]
+pub(crate) struct CompilerConfig {
+ pub(crate) strategy: Strategy,
+ pub(crate) target: Option<target_lexicon::Triple>,
+ pub(crate) settings: HashMap<String, String>,
+ pub(crate) flags: HashSet<String>,
+}
+
+#[cfg(compiler)]
+impl CompilerConfig {
+ pub(crate) fn new(strategy: Strategy) -> Self {
+ Self {
+ strategy,
+ target: None,
+ settings: HashMap::new(),
+ flags: HashSet::new(),
+ }
+ }
+
+ /// Ensures that the key is not set or equal to the desired value.
+ /// If the given key is not set, then it will be set to the desired value.
+ fn ensure_setting(&mut self, k: &str, v: &str) -> bool {
+ if let Some(value) = self.settings.get(k) {
+ if value != v {
+ return false;
+ }
+ } else {
+ self.settings.insert(k.to_string(), v.to_string());
+ }
+ true
+ }
+}
+
+#[cfg(compiler)]
+impl Default for CompilerConfig {
+ fn default() -> Self {
+ Self::new(Strategy::Auto)
+ }
+}
+
 impl Config {
  /// Creates a new configuration object with the default configuration
  /// specified.
  pub fn new() -> Self {
  let mut ret = Self {
  tunables: Tunables::default(),
  #[cfg(compiler)]
- compiler: compiler_builder(Strategy::Auto).unwrap(),
+ compiler_config: CompilerConfig::default(),
  #[cfg(feature = "cache")]
  cache_config: CacheConfig::new_cache_disabled(),
  profiler: Arc::new(NullProfilerAgent),
@@ -166,8 +211,8 @@ impl Config {
  #[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
  pub fn target(&mut self, target: &str) -> Result<&mut Self> {
  use std::str::FromStr;
- self.compiler
- .target(target_lexicon::Triple::from_str(target).map_err(|e| anyhow::anyhow!(e))?)?;
+ self.compiler_config.target =
+ Some(target_lexicon::Triple::from_str(target).map_err(|e| anyhow::anyhow!(e))?);
 
  Ok(self)
  }
@@ -290,20 +335,6 @@ impl Config {
  /// will always return `None`.
  pub fn wasm_backtrace(&mut self, enable: bool) -> &mut Self {
  self.wasm_backtrace = enable;
- #[cfg(compiler)]
- {
- // unwind_info must be enabled when either backtraces or reference types are enabled:
- self.compiler
- .set(
- "unwind_info",
- if enable || self.features.reference_types {
- "true"
- } else {
- "false"
- },
- )
- .unwrap();
- }
  self
  }
 
@@ -542,24 +573,6 @@ impl Config {
  pub fn wasm_reference_types(&mut self, enable: bool) -> &mut Self {
  self.features.reference_types = enable;
 
- #[cfg(compiler)]
- {
- self.compiler
- .set("enable_safepoints", if enable { "true" } else { "false" })
- .unwrap();
- // unwind_info must be enabled when either backtraces or reference types are enabled:
- self.compiler
- .set(
- "unwind_info",
- if enable || self.wasm_backtrace {
- "true"
- } else {
- "false"
- },
- )
- .unwrap();
- }
-
  // The reference types proposal depends on the bulk memory proposal.
  if enable {
  self.wasm_bulk_memory(true);
@@ -586,13 +599,6 @@ impl Config {
  /// [relaxed simd proposal]: https://github.com/WebAssembly/relaxed-simd
  pub fn wasm_simd(&mut self, enable: bool) -> &mut Self {
  self.features.simd = enable;
- #[cfg(compiler)]
- {
- let val = if enable { "true" } else { "false" };
- self.compiler
- .set("enable_simd", val)
- .expect("should be valid flag");
- }
  self
  }
 
@@ -606,6 +612,9 @@ impl Config {
  ///
  /// [proposal]: https://github.com/webassembly/bulk-memory-operations
  pub fn wasm_bulk_memory(&mut self, enable: bool) -> &mut Self {
+ if !enable {
+ assert!(!self.features.reference_types, "'reference_types' requires 'bulk_memory', cannot disable 'bulk_memory' without disable 'reference_types' first");
+ }
  self.features.bulk_memory = enable;
  self
  }
@@ -682,9 +691,9 @@ impl Config {
  /// here then an error will be returned.
  #[cfg(compiler)]
  #[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
- pub fn strategy(&mut self, strategy: Strategy) -> Result<&mut Self> {
- self.compiler = compiler_builder(strategy)?;
- Ok(self)
+ pub fn strategy(&mut self, strategy: Strategy) -> &mut Self {
+ self.compiler_config.strategy = strategy;
+ self
  }
 
  /// Creates a default profiler based on the profiling strategy chosen.
@@ -712,9 +721,9 @@ impl Config {
  #[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
  pub fn cranelift_debug_verifier(&mut self, enable: bool) -> &mut Self {
  let val = if enable { "true" } else { "false" };
- self.compiler
- .set("enable_verifier", val)
- .expect("should be valid flag");
+ self.compiler_config
+ .settings
+ .insert("enable_verifier".to_string(), val.to_string());
  self
  }
 
@@ -733,9 +742,9 @@ impl Config {
  OptLevel::Speed => "speed",
  OptLevel::SpeedAndSize => "speed_and_size",
  };
- self.compiler
- .set("opt_level", val)
- .expect("should be valid flag");
+ self.compiler_config
+ .settings
+ .insert("opt_level".to_string(), val.to_string());
  self
  }
 
@@ -751,9 +760,9 @@ impl Config {
  #[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
  pub fn cranelift_nan_canonicalization(&mut self, enable: bool) -> &mut Self {
  let val = if enable { "true" } else { "false" };
- self.compiler
- .set("enable_nan_canonicalization", val)
- .expect("should be valid flag");
+ self.compiler_config
+ .settings
+ .insert("enable_nan_canonicalization".to_string(), val.to_string());
  self
  }
 
@@ -774,7 +783,7 @@ impl Config {
  #[cfg(compiler)]
  #[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
  pub unsafe fn cranelift_flag_enable(&mut self, flag: &str) -> Result<&mut Self> {
- self.compiler.enable(flag)?;
+ self.compiler_config.flags.insert(flag.to_string());
  Ok(self)
  }
 
@@ -793,9 +802,11 @@ impl Config {
  /// the flag type.
  #[cfg(compiler)]
  #[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
- pub unsafe fn cranelift_flag_set(&mut self, name: &str, value: &str) -> Result<&mut Self> {
- self.compiler.set(name, value)?;
- Ok(self)
+ pub unsafe fn cranelift_flag_set(&mut self, name: &str, value: &str) -> &mut Self {
+ self.compiler_config
+ .settings
+ .insert(name.to_string(), value.to_string());
+ self
  }
 
  /// Loads cache configuration specified at `path`.
@@ -1318,6 +1329,48 @@ impl Config {
  )?)),
  }
  }
+
+ #[cfg(compiler)]
+ pub(crate) fn build_compiler(&mut self) -> Result<Box<dyn wasmtime_environ::Compiler>> {
+ let mut compiler = compiler_builder(self.compiler_config.strategy)?;
+ if let Some(target) = &self.compiler_config.target {
+ compiler.target(target.clone())?;
+ }
+
+ // check for incompatible compiler options and set required values
+ if self.wasm_backtrace || self.features.reference_types {
+ if !self.compiler_config.ensure_setting("unwind_info", "true") {
+ bail!("compiler option 'unwind_info' must be enabled when either 'backtraces' or 'reference types' are enabled");
+ }
+ }
+ if self.features.reference_types {
+ if !self
+ .compiler_config
+ .ensure_setting("enable_safepoints", "true")
+ {
+ bail!("compiler option 'enable_safepoints' must be enabled when 'reference types' is enabled");
+ }
+ }
+ if self.features.simd {
+ if !self.compiler_config.ensure_setting("enable_simd", "true") {
+ bail!("compiler option 'enable_simd' must be enabled when 'simd' is enabled");
+ }
+ }
+
+ // Apply compiler settings and flags
+ for (k, v) in self.compiler_config.settings.iter() {
+ compiler.set(k, v).expect(&format!(
+ "should be valid compiler setting, name: {k}, val: {v}"
+ ));
+ }
+ for flag in self.compiler_config.flags.iter() {
+ compiler
+ .enable(flag)
+ .expect(&format!("should be valid compiler flag, name: {flag}"));
+ }
+
+ compiler.build()
+ }
 }
 
 #[cfg(compiler)]
@@ -1345,7 +1398,7 @@ impl Clone for Config {
  fn clone(&self) -> Config {
  Config {
  #[cfg(compiler)]
- compiler: self.compiler.clone(),
+ compiler_config: self.compiler_config.clone(),
  tunables: self.tunables.clone(),
  #[cfg(feature = "cache")]
  cache_config: self.cache_config.clone(),
@@ -1398,7 +1451,7 @@ impl fmt::Debug for Config {
  .field("parallel_compilation", &self.parallel_compilation);
  #[cfg(compiler)]
  {
- f.field("compiler", &self.compiler);
+ f.field("compiler_config", &self.compiler_config);
  }
  f.finish()
  }
@@ -1408,7 +1461,7 @@ impl fmt::Debug for Config {
 ///
 /// This is used as an argument to the [`Config::strategy`] method.
 #[non_exhaustive]
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Copy)]
 pub enum Strategy {
  /// An indicator that the compilation strategy should be automatically
  /// selected.

crates/wasmtime/src/engine.rs

@@ -70,7 +70,7 @@ impl Engine {
  Ok(Engine {
  inner: Arc::new(EngineInner {
  #[cfg(compiler)]
- compiler: config.compiler.build()?,
+ compiler: config.build_compiler()?,
  config,
  allocator,
  signatures: registry,

tests/all/relocs.rs

@@ -21,7 +21,7 @@ fn store_with_padding(padding: usize) -> Result<Store<()>> {
  config.cranelift_flag_set(
  "wasmtime_linkopt_padding_between_functions",
  &padding.to_string(),
- )?;
+ );
  }
  let engine = Engine::new(&config)?;
  Ok(Store::new(&engine, ()))
@@ -78,7 +78,7 @@ fn mixed() -> Result<()> {
 fn mixed_forced() -> Result<()> {
  let mut config = Config::new();
  unsafe {
- config.cranelift_flag_set("wasmtime_linkopt_force_jump_veneer", "true")?;
+ config.cranelift_flag_set("wasmtime_linkopt_force_jump_veneer", "true");
  }
  let engine = Engine::new(&config)?;
  test_many_call_module(Store::new(&engine, ()))