alexcrichton opened PR #3899 from validate-flags
to main
:
This commit fills out a few FIXME annotations by implementing run-time
checks that when aModule
is created it has compatible codegen
settings for the current host (asModule
is proof of "this code can
run"). This is done by implementing newEngine
-level methods which
validate compiler settings. These settings are validated on
Module::new
as well as when loading serialized modules.Settings are split into two categories, one for "shared" top-level
settings and one for ISA-specific settings. Both categories now have
allow-lists hardcoded intoEngine
which indicate the acceptable values
for each setting (if applicable). ISA-specific settings are checked with
the Rust standard library'sstd::is_x86_feature_detected!
macro. Other
macros for other platforms are not stable at this time but can be added
here if necessary.Closes #3897
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Can we use a
OnceCell
to implement the lazy computation here? Effectively we're computing either a boolean ("are we compatible") or aResult
, depending on how you want to look at it; maybe aOnceCell<Result>
to cache our return value?
cfallin created PR review comment:
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 toplevelEngine
forwasmtime
feels like the wrong spot to be defining specific CPU flags and detecting them if we also have the same code elsewhere...
cfallin created PR review comment:
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 inwasmtime-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.
alexcrichton updated PR #3899 from validate-flags
to main
.
alexcrichton updated PR #3899 from validate-flags
to main
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
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...
alexcrichton submitted PR review.
alexcrichton created PR review comment:
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)
cfallin submitted PR review.
alexcrichton merged PR #3899.
Last updated: Dec 23 2024 at 12:05 UTC