Skip to content

Commit

Permalink
Code review feedback.
Browse files Browse the repository at this point in the history
* 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`.
  • Loading branch information
peterhuene committed Mar 31, 2021
1 parent 859484e commit 52bbe00
Show file tree
Hide file tree
Showing 9 changed files with 180 additions and 178 deletions.
68 changes: 32 additions & 36 deletions crates/wasmtime/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(),
Expand All @@ -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,
Expand All @@ -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`.
Expand Down Expand Up @@ -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
Expand Down
45 changes: 37 additions & 8 deletions crates/wasmtime/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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)
}
Expand Down
Loading

0 comments on commit 52bbe00

Please sign in to comment.