Implement runtime checks for compilation settings

[alexcrichton]

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

[alexcrichton]

This actually ended up catching a preexisting bug in our spectests implementation during cargo test where reference types are enabled but by calling Config::strategy it overwrites the compiler builder meaning existing settings are removed (such as enable_safepoints) and later when a module is instantiated reference types are enabled but safepoints aren't. Previously this was allowed but now it's rejected at Module-creation time. For now the fix I implemented was to avoid calling Config::strategy, but now that I say this out loud it might be better to simply remove the Config::strategy method since it doesn't do anything anyway and is sort of dangerous to blow away existing settings.

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[cfallin]

Thanks for this -- it's definitely a necessary fix and the overall approach of more explicitly defining what is compatible makes a lot of sense to me!

I have some specific thoughts below on how this might be better factored and organized; I think there might be a better way to pull out a "subtyping" or "ordering" relation on the flags, let Cranelift worry about that, then use that here, to significantly cut down on hardcoded-ness and code duplication.

[cfallin]

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.

[alexcrichton]

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

[cfallin]

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

[alexcrichton]

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]

Thanks -- on further thought and given your responses above I think I'm happy with this landing as-is for now, pushing some refactorings off for later as you say...

Specifically, it'd be good to eventually factor out the flags/settings infra from Cranelift into maybe just a cranelift-settings crate; and then Wasmtime-without-Cranelift can pull in cranelift-native (just for the CPUID bits) and cranelift-settings. We should have some mechanism to indicate on a flag that setting it either does or doesn't change ABI compatibility of the generated code (ie code built with one setting can work with a runtime expecting the other); and we should have something in wasmtime-cranelift that generates a "minimum flags" result.

I see this as analogous to how Target is a separate thing in target-lexicon; we're logically extending that space with more dimensions.

I'll write up an issue for this now and would be happy to discuss further there!

[uweigand]

I just noticed that this seems to have broken the test suite on IBM Z when running on z15 or later. All wasi_tokio tests now fail with:

    0: compilation settings are not compatible with the native host
    1: cannot test if target-specific flag "has_mie2" is available at runtime

It looks like the JIT auto-detects the availability of the MIE2 feature (from cranelift-native), but then wasmtime checks again but now thinks that feature is unsupported (since there is no IBM Z support in check_compatible_with_isa_flag yet).

What's the right approach here? Just re-implement the feature check? (I must admit I'm not sure what the point of the double checking here is, in particular with a duplicated code base ...)

[alexcrichton]

Yeah in this case implementing the check is good. The purpose of this is to guard against loading precompiled modules with the wrong configuration settings. The duplication is unfortunate but I didn't see any obvious way to unify things (one says "activate the right features for the host" and the other says "verify each feature against the host", different enough I couldn't figure out how to abstract).

[uweigand]

OK, I've now implemented this here: #4101