Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/environ/src/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ pub trait Compiler: Send + Sync {
}

/// Value of a configured setting for a [`Compiler`]
#[derive(Serialize, Deserialize, Hash, Eq, PartialEq)]
#[derive(Serialize, Deserialize, Hash, Eq, PartialEq, Debug)]
pub enum FlagValue {
/// Name of the value that has been configured for this setting.
Enum(Cow<'static, str>),
Expand Down
212 changes: 212 additions & 0 deletions crates/wasmtime/src/engine.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use crate::signatures::SignatureRegistry;
use crate::{Config, Trap};
use anyhow::Result;
use once_cell::sync::OnceCell;
#[cfg(feature = "parallel-compilation")]
use rayon::prelude::*;
use std::sync::atomic::{AtomicU64, Ordering};
use std::sync::Arc;
#[cfg(feature = "cache")]
use wasmtime_cache::CacheConfig;
use wasmtime_environ::FlagValue;
use wasmtime_runtime::{debug_builtins, CompiledModuleIdAllocator, InstanceAllocator};

/// An `Engine` which is a global context for compilation and management of wasm
Expand Down Expand Up @@ -44,6 +46,10 @@ struct EngineInner {
signatures: SignatureRegistry,
epoch: AtomicU64,
unique_id_allocator: CompiledModuleIdAllocator,

// One-time check of whether the compiler's settings, if present, are
// compatible with the native host.
compatible_with_native_host: OnceCell<Result<(), String>>,
}

impl Engine {
Expand All @@ -70,6 +76,7 @@ impl Engine {
signatures: registry,
epoch: AtomicU64::new(0),
unique_id_allocator: CompiledModuleIdAllocator::new(),
compatible_with_native_host: OnceCell::new(),
}),
})
}
Expand Down Expand Up @@ -217,6 +224,211 @@ impl Engine {
.map(|a| f(a))
.collect::<Result<Vec<B>, E>>()
}

/// Returns the target triple which this engine is compiling code for
/// and/or running code for.
pub(crate) fn target(&self) -> target_lexicon::Triple {
// If a compiler is configured, use that target.
#[cfg(compiler)]
return self.compiler().triple().clone();

// ... otherwise it's the native target
#[cfg(not(compiler))]
return target_lexicon::Triple::host();
}

/// Verify that this engine's configuration is compatible with loading
/// modules onto the native host platform.
///
/// This method is used as part of `Module::new` to ensure that this
/// engine can indeed load modules for the configured compiler (if any).
/// Note that if cranelift is disabled this trivially returns `Ok` because
/// loaded serialized modules are checked separately.
pub(crate) fn check_compatible_with_native_host(&self) -> Result<()> {
self.inner
.compatible_with_native_host
.get_or_init(|| self._check_compatible_with_native_host())
.clone()
.map_err(anyhow::Error::msg)
}
fn _check_compatible_with_native_host(&self) -> Result<(), String> {
#[cfg(compiler)]
{
let compiler = self.compiler();

// Check to see that the config's target matches the host
let target = compiler.triple();
if *target != target_lexicon::Triple::host() {
return Err(format!(
"target '{}' specified in the configuration does not match the host",
target
));
}

// Also double-check all compiler settings
for (key, value) in compiler.flags().iter() {
self.check_compatible_with_shared_flag(key, value)?;
}
for (key, value) in compiler.isa_flags().iter() {
self.check_compatible_with_isa_flag(key, value)?;
}
}
Ok(())
}

/// Checks to see whether the "shared flag", something enabled for
/// individual compilers, is compatible with the native host platform.
///
/// This is used both when validating an engine's compilation settings are
/// compatible with the host as well as when deserializing modules from
/// disk to ensure they're compatible with the current host.
///
/// Note that most of the settings here are not configured by users that
/// often. While theoretically possible via `Config` methods the more
/// interesting flags are the ISA ones below. Typically the values here
/// represent global configuration for wasm features. Settings here
/// currently rely on the compiler informing us of all settings, including
/// those disabled. Settings then fall in a few buckets:
///
/// * Some settings must be enabled, such as `avoid_div_traps`.
/// * Some settings must have a particular value, such as
/// `libcall_call_conv`.
/// * Some settings do not matter as to their value, such as `opt_level`.
pub(crate) fn check_compatible_with_shared_flag(
&self,
flag: &str,
value: &FlagValue,
) -> Result<(), String> {
let ok = match flag {
// These settings must all have be enabled, since their value
// can affect the way the generated code performs or behaves at
// runtime.
"avoid_div_traps" => *value == FlagValue::Bool(true),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm -- I definitely understand the convenience of putting this compatibility-table here, but it feels like it should either be part of the wasmtime-cranelift crate or, maybe, something in Cranelift (or some combination). Basically there are two sources of truth we really need to query:

  • Does a given flag change codegen, such that if flipped, the generated code will be incompatible (this covers e.g. the calling-convention-related stuff);
  • Is a given flag required to be on in order for some Wasm feature to work (this covers e.g. reftypes requiring safepoints).

Maybe the former in cranelift-codegen -- are_flags_abi_compatible perhaps? -- and the latter in wasmtime-cranelift?

Seen another way, this is kind of a subtyping relationship. Cranelift should be able to tell us if one set of flags are a "subtype" of another (are compatible); and we get the minimum flags from Wasmtime's Cranelift glue, given the features it needs, then check if actual flags are a subtype of that. Does that make sense?

To be concrete, the downside of the current approach that I worry about is just that this creates some unfortunate tight coupling and distributed definition of truth in the source. It's manageable here in any case because we're all in one big repo, but it's sort of an indication for me that we should have better compatibility abstractions in the API in general.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me but the difficulty here I think is that we need this check to work even if cranelift is not compiled into Wasmtime. When we're loading precompiled modules from disk into a Wasmtime without Cranelift we still want to check that all the various features are compatible.

I do agree that this is a tight coupling, but it was sort of the best I could come up with for now. I figured this was small enough it'd be ok to punt to some future state as the settings haven't changed that much in awhile I think. I also don't know of a better way to do this...

"unwind_info" => *value == FlagValue::Bool(true),
"libcall_call_conv" => *value == FlagValue::Enum("isa_default".into()),

// Features wasmtime doesn't use should all be disabled, since
// otherwise if they are enabled it could change the behavior of
// generated code.
"baldrdash_prologue_words" => *value == FlagValue::Num(0),
"enable_llvm_abi_extensions" => *value == FlagValue::Bool(false),
"emit_all_ones_funcaddrs" => *value == FlagValue::Bool(false),
"enable_pinned_reg" => *value == FlagValue::Bool(false),
"enable_probestack" => *value == FlagValue::Bool(false),
"use_colocated_libcalls" => *value == FlagValue::Bool(false),
"use_pinned_reg_as_heap_base" => *value == FlagValue::Bool(false),

// If reference types are enabled this must be enabled, otherwise
// this setting can have any value.
"enable_safepoints" => {
if self.config().features.reference_types {
*value == FlagValue::Bool(true)
} else {
return Ok(())
}
}

// These settings don't affect the interface or functionality of
// the module itself, so their configuration values shouldn't
// matter.
"enable_heap_access_spectre_mitigation"
| "enable_nan_canonicalization"
| "enable_jump_tables"
| "enable_float"
| "enable_simd"
| "enable_verifier"
| "is_pic"
| "machine_code_cfg_info"
| "tls_model" // wasmtime doesn't use tls right now
| "opt_level" // opt level doesn't change semantics
| "probestack_func_adjusts_sp" // probestack above asserted disabled
| "probestack_size_log2" // probestack above asserted disabled
| "regalloc" // shouldn't change semantics
| "enable_atomics" => return Ok(()),

// Everything else is unknown and needs to be added somewhere to
// this list if encountered.
_ => {
return Err(format!("unknown shared setting {:?} configured to {:?}", flag, value))
}
};

if !ok {
return Err(format!(
"setting {:?} is configured to {:?} which is not supported",
flag, value,
));
}
Ok(())
}

/// Same as `check_compatible_with_native_host` except used for ISA-specific
/// flags. This is used to test whether a configured ISA flag is indeed
/// available on the host platform itself.
pub(crate) fn check_compatible_with_isa_flag(
&self,
flag: &str,
value: &FlagValue,
) -> Result<(), String> {
match value {
// ISA flags are used for things like CPU features, so if they're
// disabled then it's compatible with the native host.
FlagValue::Bool(false) => return Ok(()),

// Fall through below where we test at runtime that features are
// available.
FlagValue::Bool(true) => {}

// Only `bool` values are supported right now, other settings would
// need more support here.
_ => {
return Err(format!(
"isa-specific feature {:?} configured to unknown value {:?}",
flag, value
))
}
}
#[cfg(target_arch = "x86_64")]
{
let enabled = match flag {
"has_sse3" => Some(std::is_x86_feature_detected!("sse3")),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a duplication of code from Cranelift and ideally we should be able to (i) get the native flags from cranelift-native, then (ii) use a "compatibility" or "subtyping" predicate, I think. The toplevel Engine for wasmtime feels like the wrong spot to be defining specific CPU flags and detecting them if we also have the same code elsewhere...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was wondering originally if we may want to drop Wasmtime's usage of cranelift-native, but the tests here are all subtly different where here we're enumerating based on what was enabled and checking whether the feature is available, whereas cranelift-native is the reverse where it's based on what's available it enables features. I could maybe export something like a fancy macro which iterates over the feature/name pairs but that didn't seem really all that better (and additionally doesn't solve the issue of Wasmtime-not-depending-on-Cranelift still needs to check this)

"has_ssse3" => Some(std::is_x86_feature_detected!("ssse3")),
"has_sse41" => Some(std::is_x86_feature_detected!("sse4.1")),
"has_sse42" => Some(std::is_x86_feature_detected!("sse4.2")),
"has_popcnt" => Some(std::is_x86_feature_detected!("popcnt")),
"has_avx" => Some(std::is_x86_feature_detected!("avx")),
"has_avx2" => Some(std::is_x86_feature_detected!("avx2")),
"has_bmi1" => Some(std::is_x86_feature_detected!("bmi1")),
"has_bmi2" => Some(std::is_x86_feature_detected!("bmi2")),
"has_avx512bitalg" => Some(std::is_x86_feature_detected!("avx512bitalg")),
"has_avx512dq" => Some(std::is_x86_feature_detected!("avx512dq")),
"has_avx512f" => Some(std::is_x86_feature_detected!("avx512f")),
"has_avx512vl" => Some(std::is_x86_feature_detected!("avx512vl")),
"has_avx512vbmi" => Some(std::is_x86_feature_detected!("avx512vbmi")),
"has_lzcnt" => Some(std::is_x86_feature_detected!("lzcnt")),

// fall through to the very bottom to indicate that support is
// not enabled to test whether this feature is enabled on the
// host.
_ => None,
};
match enabled {
Some(true) => return Ok(()),
Some(false) => {
return Err(format!(
"compilation setting {:?} is enabled but not available on the host",
flag
))
}
// fall through
None => {}
}
}
Err(format!(
"cannot test if target-specific flag {:?} is available at runtime",
flag
))
}
}

impl Default for Engine {
Expand Down
15 changes: 3 additions & 12 deletions crates/wasmtime/src/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,18 +295,9 @@ impl Module {
#[cfg(compiler)]
#[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
pub fn from_binary(engine: &Engine, binary: &[u8]) -> Result<Module> {
// Check to see that the config's target matches the host
let target = engine.compiler().triple();
if *target != target_lexicon::Triple::host() {
bail!(
"target '{}' specified in the configuration does not match the host",
target
);
}

// FIXME: we may want to validate that the ISA flags in the config match those that
// would be inferred for the host, otherwise the JIT might produce unrunnable code
// for the features the host's CPU actually has.
engine
.check_compatible_with_native_host()
.context("compilation settings are not compatible with the native host")?;

cfg_if::cfg_if! {
if #[cfg(feature = "cache")] {
Expand Down
Loading