Run the full test suite on 32-bit platforms #9837
Conversation
This commit switches to running the full test suite in its entirety (`./ci/run-tests.sh`) on 32-bit platforms in CI in addition to 64-bit platforms. This notably adds i686 and armv7 as architectures that are tested in CI. Lots of little fixes here and there were applied to a number of tests. Many tests just don't run on 32-bit platforms or a platform without Cranelift support, and they've been annotated as such where necessary. Other tests were adjusted to run on all platforms a few minor bug fixes are here as well. prtest:full
alexcrichton
requested review from
cfallin and
fitzgen
and removed request for
a team
Don't require the `pulley` feature opt-in on 32-bit platforms to get wasm code running.
There was a problem hiding this comment.
I'll defer to @fitzgen on the Pulley interpreter changes but a few thoughts on the conditional logic -- my main thought is that I'd prefer we didn't conflate "32-bit host" with "has no backend" and instead of a specific feature (or other method) meaning "we have a native codegen backend for the host platform"; seems less likely to cause unintentional issues later.
github-actions
bot
added
cranelift
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "pulley", "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| // Flag pulley as enabled unconditionally on 32-bit targets to ensure that | ||
| // wasm is runnable by default like it is on other 64-bit native platforms. | ||
| // Note that this doesn't actually enable the Cargo feature, it just changes | ||
| // the cfg's passed to the crate, so for example care is still taken in | ||
| // `Cargo.toml` to handle pulley-specific dependencies on 32-bit platforms. | ||
| let target_pointer_width = std::env::var("CARGO_CFG_TARGET_POINTER_WIDTH").unwrap(); | ||
| if target_pointer_width == "32" { | ||
| println!("cargo:rustc-cfg=feature=\"pulley\""); | ||
| } |
There was a problem hiding this comment.
Here I think it is worth explicitly checking against our ISAs supported by Cranelift.
| # On 32-bit platforms where Cranelift currently doesn't have any supported host | ||
| # unconditionally pull in the `pulley-interpreter` crate. Runtime support is | ||
| # enabled via `build.rs` which prints `feature="pulley"` by default here and | ||
| # compilation support, if enabled via the `cranelift` feature, is handled in the | ||
| # `wasmtime-cranelift` crate enabling Pulley on 32-bit platforms. | ||
| [target.'cfg(target_pointer_width = "32")'.dependencies] | ||
| pulley-interpreter = { workspace = true } |
There was a problem hiding this comment.
Shouldn't this happen automatically when the build script enables the feature?
|
Ok I've been thinking about how to clean this up. For the tests that @cfallin flagged I added logic to them all to check @fitzgen for your points though I'm not sure how to make it better. I'll note that the [target.'cfg(not(any(target_arch = "x86_64", target_arch = "aarch64", target_arch = "s390x", target_arch = "riscv64")))'.dependencies]
pulley-interpreter = { workspace = true }And then reflecting that same condition into This feels "zoomed out" enough to me that leaving |
|
That cfg is certainly annoying but seems like it is probably better than baking in new assumptions that we know are unlikely to be true for too long? |
|
To clarify though the precise assumption here is that 32-bit platforms don't have a Cranelift backend so pulley is pulled in as a dependency. This doesn't actually change any logic internally within Cranelift or Wasmtime, it just compiles in Pulley by default without a way to turn that off. That's only a problem with respect to binary size. If all of Cranelift's currently supported architectures are listed out here then that's duplicated in two places in Wasmtime (not counting the ones in Cranelift) and they're just going to get blindly added to in the future. Alternatively if Cranelift gets a 32-bit backend then that would be added as an exception to the cfg as-is when binary size becomes a problem. To me this is six-to-one-half-dozen-or-the-other where I have a preference for what's as-is as it's a managable cfg that's by-default workable for most platforms (it doesn't work for 64-bit platforms without a Cranelift backend). Listing out architectures individually feels "just as bad" to me basically. The main alternative to me is that we leave pulley disabled by default on all platforms, regardless of Cranelift support. That feels like a worse experience to me though because it means that Wasmtime only works by default on Cranelift-supported platforms, which if someone's new to Wasmtime they might not understand to enable the Pulley backend. |
|
ping @fitzgen on ^ |
This commit switches to running the full test suite in its entirety (
./ci/run-tests.sh) on 32-bit platforms in CI in addition to 64-bit platforms. This notably adds i686 and armv7 as architectures that are tested in CI.Lots of little fixes here and there were applied to a number of tests. Many tests just don't run on 32-bit platforms or a platform without Cranelift support, and they've been annotated as such where necessary. Other tests were adjusted to run on all platforms a few minor bug fixes are here as well.