@Alex Crichton, @Chris Fallin: I just merged https://github.com/bytecodealliance/wasmtime/pull/2957 since it seemed to make sense but I asked myself, "are these tests not being run on ARM in CI? Shouldn't we be seeing failures there?" IIRC, the S390 testing was pending some changes to QEMU, but aren't the tests being run in qemu-aarch64
?
Yes, we do run aarch64 tests with qemu... I haven't looked closely at that PR yet but I should
yeah we're running the test, I don't think the PR was correct, it should probably only disable the tests when the x86_64 backend isn't built. On CI we build tests with all backends enabled
Ah, OK, so it's just building and then checking CLIF, it's not executing anything... the x86-64 is just a placeholder for "we need some backend"
the PR disables the tests unless it's on the x86_64 platform, but I think the right fix is to require that the backend is built in
I don't remember the crate dependency graph offhand but if we could use cranelift-native
here we could just get a backend for the current machine
I think the right fix is to require that the backend is built in
Yeah, but isn't that what @Benjamin Bouvier is getting at in https://github.com/bytecodealliance/wasmtime/pull/2957#discussion_r643136838? In the absence of a way of knowing "is the x86 backend present?" we have to check the target architecture.
we could just get a backend for the current machine
I'm looking at those tests now and I think this makes sense to try... they assert a specific generated CLIF for memory operations so hopefully the emitted CLIF would be the same for different backends? If we think that is true and I can submit a PR to bring in cranelift-native
as a dev dependency and use the current backend for the test.
Or perhaps @Alex Crichton there is some way to figure out incranelift-frontend
which features were enabled for its cranelift-codegen
dependency?
I think cranelift-codegen
would have to expose a function of some kind. Or like you could catch the error when loading the target and "pass" the test because support wasn't compiled in
Yeah, that's not a bad idea... Just print out Skipping test foo because the x86 backend is not present
or something like that?
It would be nicer to be able to ignore tests at runtime but I'm not sure that is possible...
Well, let's see how https://github.com/bytecodealliance/wasmtime/pull/2961 goes: basically I just made the tests use the host system's target ISA (pulled in by cranelift-codegen) which should have the effect that these tests would run for everyone regardless of host. I'm not sure if the emitted CLIF will always be the same but I don't really see why not so let's run it and see...
Thanks for tackling this! Yeah I don't think the CLIF should differ -- given that cranelift-frontend did not previously depend on cranelift-native
to determine the platform
@Alex Crichton, when you get a chance: not sure why the verify-publish step is failing: https://github.com/bytecodealliance/wasmtime/pull/2961/checks?check_run_id=2731435666#step:5:868. Is there something different that has to happen for dev dependencies?
@Andrew Brown dependencies are topologically sorted in scripts/publish.rs
and you'll probably need to reorder something
Ok, I used @bjorn3's suggestion to bring in the x86 backend as a dev dependency in https://github.com/bytecodealliance/wasmtime/pull/2961. I think that is a better solution than what we previously merged. Thoughts?
Did you see my last suggestion of creating a FrontendConfig
directly? https://github.com/bytecodealliance/wasmtime/pull/2961#issuecomment-853748761
Yeah, I just tried the first suggestion first... You think that's a way better approach?
Oh, I see... you mean in addition to what is currently there?
No, ok, I see what you mean... completely replace the use of TargetIsa
Last updated: Dec 23 2024 at 13:07 UTC