Stream: git-wasmtime

Topic: wasmtime / PR #3899 Implement runtime checks for compilat...

Wasmtime GitHub notifications bot (Mar 08 2022 at 19:54):

alexcrichton opened PR #3899 from validate-flags to main:

This commit fills out a few FIXME annotations by implementing run-time
checks that when a Module is created it has compatible codegen
settings for the current host (as Module is proof of "this code can
run"). This is done by implementing new Engine-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 into Engine which indicate the acceptable values
for each setting (if applicable). ISA-specific settings are checked with
the Rust standard library's std::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.
-->

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:12):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:12):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:12):

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 a Result, depending on how you want to look at it; maybe a OnceCell<Result> to cache our return value?

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:12):

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 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...

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:12):

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 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.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:30):

alexcrichton updated PR #3899 from validate-flags to main.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:40):

alexcrichton updated PR #3899 from validate-flags to main.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:43):

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...

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:44):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2022 at 20:44):

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)

Wasmtime GitHub notifications bot (Mar 08 2022 at 21:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2022 at 15:46):

alexcrichton merged PR #3899.

Last updated: Nov 22 2024 at 17:03 UTC