From fb584d1c2b99b5169271603801da2914a492b82b Mon Sep 17 00:00:00 2001 From: Pure White Date: Fri, 10 Jun 2022 01:04:33 +0800 Subject: [PATCH] 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 | 10 ++ crates/c-api/include/wasmtime/config.h | 2 +- crates/c-api/src/config.rs | 5 +- crates/cli-flags/src/lib.rs | 4 +- crates/fuzzing/src/generators.rs | 8 +- crates/wasmtime/src/config.rs | 230 ++++++++++++++++--------- crates/wasmtime/src/engine.rs | 13 +- fuzz/fuzz_targets/compile.rs | 2 +- tests/all/relocs.rs | 4 +- 9 files changed, 181 insertions(+), 97 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 035213f4b003..03490b5c097b 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -8,6 +8,16 @@ Unreleased. ### Changed +* The `Config::strategy`, `Config::cranelift_flag_enable` 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 + +* 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.38.0 diff --git a/crates/c-api/include/wasmtime/config.h b/crates/c-api/include/wasmtime/config.h index dcef84fa6a67..8a25e65378bf 100644 --- a/crates/c-api/include/wasmtime/config.h +++ b/crates/c-api/include/wasmtime/config.h @@ -201,7 +201,7 @@ WASMTIME_CONFIG_PROP(void, wasm_memory64, bool) * If the compilation strategy selected could not be enabled then an error is * returned. */ -WASMTIME_CONFIG_PROP(wasmtime_error_t*, strategy, wasmtime_strategy_t) +WASMTIME_CONFIG_PROP(void, strategy, wasmtime_strategy_t) /** * \brief Configures whether Cranelift's debug verifier is enabled. diff --git a/crates/c-api/src/config.rs b/crates/c-api/src/config.rs index f928ff39fb36..9e60abbeb0da 100644 --- a/crates/c-api/src/config.rs +++ b/crates/c-api/src/config.rs @@ -103,13 +103,12 @@ pub extern "C" fn wasmtime_config_wasm_memory64_set(c: &mut wasm_config_t, enabl pub extern "C" fn wasmtime_config_strategy_set( c: &mut wasm_config_t, strategy: wasmtime_strategy_t, -) -> Option> { +) { use wasmtime_strategy_t::*; - let result = c.config.strategy(match strategy { + c.config.strategy(match strategy { WASMTIME_STRATEGY_AUTO => Strategy::Auto, WASMTIME_STRATEGY_CRANELIFT => Strategy::Cranelift, }); - handle_result(result, |_cfg| {}) } #[no_mangle] diff --git a/crates/cli-flags/src/lib.rs b/crates/cli-flags/src/lib.rs index 532fd3213be4..7029b0d6b743 100644 --- a/crates/cli-flags/src/lib.rs +++ b/crates/cli-flags/src/lib.rs @@ -267,13 +267,13 @@ impl CommonOptions { for name in &self.cranelift_enable { unsafe { - config.cranelift_flag_enable(name)?; + config.cranelift_flag_enable(name); } } for (name, value) in &self.cranelift_set { unsafe { - config.cranelift_flag_set(name, value)?; + config.cranelift_flag_set(name, value); } } diff --git a/crates/fuzzing/src/generators.rs b/crates/fuzzing/src/generators.rs index 74bdd7f423e0..ee0e0f509176 100644 --- a/crates/fuzzing/src/generators.rs +++ b/crates/fuzzing/src/generators.rs @@ -421,8 +421,7 @@ impl Config { if self.wasmtime.force_jump_veneers { unsafe { - cfg.cranelift_flag_set("wasmtime_linkopt_force_jump_veneer", "true") - .unwrap(); + cfg.cranelift_flag_set("wasmtime_linkopt_force_jump_veneer", "true"); } } @@ -431,8 +430,7 @@ impl Config { cfg.cranelift_flag_set( "wasmtime_linkopt_padding_between_functions", &pad.to_string(), - ) - .unwrap(); + ); } } @@ -658,7 +656,7 @@ impl CodegenSettings { config.target(target).unwrap(); for (key, value) in flags { unsafe { - config.cranelift_flag_set(key, value).unwrap(); + config.cranelift_flag_set(key, value); } } } diff --git a/crates/wasmtime/src/config.rs b/crates/wasmtime/src/config.rs index ed800bacafb3..94666c5dbe4a 100644 --- a/crates/wasmtime/src/config.rs +++ b/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, + pub(crate) compiler_config: CompilerConfig, pub(crate) tunables: Tunables, #[cfg(feature = "cache")] pub(crate) cache_config: CacheConfig, @@ -103,6 +106,54 @@ 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, + pub(crate) settings: HashMap, + pub(crate) flags: HashSet, +} + +#[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 equals to the given value. + /// If the key is not set, it will be set to the given value. + /// + /// # Returns + /// + /// Returns true if successfully set or already had the given setting + /// value, or false if the setting was explicitly set to something + /// else previously. + fn ensure_setting_unset_or_given(&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. @@ -110,7 +161,7 @@ impl Config { 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 +217,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 +341,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 } @@ -533,38 +570,18 @@ impl Config { /// This feature gates items such as the `externref` and `funcref` types as /// well as allowing a module to define multiple tables. /// - /// Note that enabling the reference types feature will also enable the bulk - /// memory feature. + /// Note that the reference types proposal depends on the bulk memory proposal. /// /// This feature is `true` by default. /// + /// # Errors + /// + /// The validation of this feature are deferred until the engine is being built, + /// and thus may cause `Engine::new` fail if the `bulk_memory` feature is disabled. + /// /// [proposal]: https://github.com/webassembly/reference-types 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); - } - self } @@ -586,13 +603,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 } @@ -604,6 +614,15 @@ impl Config { /// /// This is `true` by default. /// + /// Feature `reference_types`, which is also `true` by default, requires + /// this feature to be enabled. Thus disabling this feature must also disable + /// `reference_types` as well using [`wasm_reference_types`](crate::Config::wasm_reference_types). + /// + /// # Errors + /// + /// Disabling this feature without disabling `reference_types` will cause + /// `Engine::new` to fail. + /// /// [proposal]: https://github.com/webassembly/bulk-memory-operations pub fn wasm_bulk_memory(&mut self, enable: bool) -> &mut Self { self.features.bulk_memory = enable; @@ -674,17 +693,11 @@ impl Config { /// and its documentation. /// /// The default value for this is `Strategy::Auto`. - /// - /// # Errors - /// - /// Some compilation strategies require compile-time options of `wasmtime` - /// itself to be set, but if they're not set and the strategy is specified - /// 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 +725,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 +746,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 +764,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 } @@ -770,12 +783,14 @@ impl Config { /// /// # Errors /// - /// This method can fail if the flag's name does not exist. + /// The validation of the flags are deferred until the engine is being built, and thus may + /// cause `Engine::new` fail if the flag's name does not exist, or the value is not appropriate + /// for the flag type. #[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)?; - Ok(self) + pub unsafe fn cranelift_flag_enable(&mut self, flag: &str) -> &mut Self { + self.compiler_config.flags.insert(flag.to_string()); + self } /// Allows settings another Cranelift flag defined by a flag name and value. This allows @@ -789,13 +804,15 @@ impl Config { /// /// # Errors /// - /// This method can fail if the flag's name does not exist, or the value is not appropriate for - /// the flag type. + /// The validation of the flags are deferred until the engine is being built, and thus may + /// cause `Engine::new` fail if the flag's name does not exist. #[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`. @@ -1294,6 +1311,11 @@ impl Config { self } + pub(crate) fn validate(&self) -> Result<()> { + self.features.validate(); + Ok(()) + } + pub(crate) fn build_allocator(&self) -> Result> { #[cfg(feature = "async")] let stack_size = self.async_stack_size; @@ -1318,6 +1340,50 @@ impl Config { )?)), } } + + #[cfg(compiler)] + pub(crate) fn build_compiler(&mut self) -> Result> { + 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_unset_or_given("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_unset_or_given("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_unset_or_given("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)?; + } + for flag in self.compiler_config.flags.iter() { + compiler.enable(flag)?; + } + + compiler.build() + } } #[cfg(compiler)] @@ -1345,7 +1411,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 +1464,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 +1474,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. diff --git a/crates/wasmtime/src/engine.rs b/crates/wasmtime/src/engine.rs index fbef9592313f..50d4e8c3a307 100644 --- a/crates/wasmtime/src/engine.rs +++ b/crates/wasmtime/src/engine.rs @@ -55,6 +55,16 @@ struct EngineInner { impl Engine { /// Creates a new [`Engine`] with the specified compilation and /// configuration settings. + /// + /// # Errors + /// + /// This method can fail if the `config` is invalid or some + /// configurations are incompatible. + /// + /// For example, feature `reference_types` will need to set + /// the compiler setting `enable_safepoints` and `unwind_info` + /// to `true`, but explicitly disable these two compiler settings + /// will cause errors. pub fn new(config: &Config) -> Result { // Ensure that wasmtime_runtime's signal handlers are configured. This // is the per-program initialization required for handling traps, such @@ -64,13 +74,14 @@ impl Engine { let registry = SignatureRegistry::new(); let mut config = config.clone(); + config.validate()?; let allocator = config.build_allocator()?; allocator.adjust_tunables(&mut config.tunables); Ok(Engine { inner: Arc::new(EngineInner { #[cfg(compiler)] - compiler: config.compiler.build()?, + compiler: config.build_compiler()?, config, allocator, signatures: registry, diff --git a/fuzz/fuzz_targets/compile.rs b/fuzz/fuzz_targets/compile.rs index 9af4b5ae0f40..93ff23c1b0c0 100644 --- a/fuzz/fuzz_targets/compile.rs +++ b/fuzz/fuzz_targets/compile.rs @@ -12,7 +12,7 @@ fn create_engine() -> Engine { // the generated code at all; it only does extra checking after // compilation. unsafe { - config.cranelift_flag_enable("regalloc_checker").unwrap(); + config.cranelift_flag_enable("regalloc_checker"); } Engine::new(&config).expect("Could not construct Engine") } diff --git a/tests/all/relocs.rs b/tests/all/relocs.rs index fdd0730dfa52..c799fe1cbe37 100644 --- a/tests/all/relocs.rs +++ b/tests/all/relocs.rs @@ -21,7 +21,7 @@ fn store_with_padding(padding: usize) -> Result> { 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, ()))