Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wasmtime+Cranelift: strip out some dead x86-32 code. #5226

Merged
merged 2 commits into from
Nov 8, 2022

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Nov 8, 2022

I was recently pointed to fastly/Viceroy#200 where it seems some folks are trying to compile Wasmtime (via Viceroy) for Windows x86-32 and the failures may not be loud enough. I've tried to reproduce this cross-compiling to i686-pc-windows-gnu from Linux and hit build failures (as expected) in several places. Nevertheless, while trying to discern what others may be attempting, I noticed some dead x86-32-specific code in our repo, and figured it would be a good idea to clean this up. Otherwise, it (i) sends some mixed messages -- "hey look, this codebase does support x86-32" -- and (ii) keeps untested code around, which is generally not great.

This PR removes x86-32-specific cases in traphandlers and unwind code, and Cranelift's native feature detection. It adds helpful compile-error messages in a few cases. If we ever support x86-32 (contributors welcome! The big missing piece is Cranelift support; see #1980), these compile errors and git history should be enough to recover any knowledge we are now encoding in the source.

I left the x86-32 support in wasmtime-fiber alone because that seems like a bit of a special case -- foundation library, separate from the rest of Wasmtime, with specific care to provide a (presumably working) full 32-bit version.

@cfallin cfallin requested a review from pchickey November 8, 2022 18:08
I was recently pointed to fastly/Viceroy#200 where it seems some folks
are trying to compile Wasmtime (via Viceroy) for Windows x86-32 and the
failures may not be loud enough. I've tried to reproduce this
cross-compiling to i686-pc-windows-gnu from Linux and hit build failures
(as expected) in several places.  Nevertheless, while trying to discern
what others may be attempting, I noticed some dead x86-32-specific code
in our repo, and figured it would be a good idea to clean this up.
Otherwise, it (i) sends some mixed messages -- "hey look, this codebase
does support x86-32" -- and (ii) keeps untested code around, which is
generally not great.

This PR removes x86-32-specific cases in traphandlers and unwind code,
and Cranelift's native feature detection. It adds helpful compile-error
messages in a few cases. If we ever support x86-32 (contributors
welcome! The big missing piece is Cranelift support; see bytecodealliance#1980), these
compile errors and git history should be enough to recover any knowledge
we are now encoding in the source.

I left the x86-32 support in `wasmtime-fiber` alone because that seems
like a bit of a special case -- foundation library, separate from the
rest of Wasmtime, with specific care to provide a (presumably working)
full 32-bit version.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, but I think it's ok to remove the intentional compile_error! for specifically x86 and let it fall back into the other compile_error! blocks we have for platform support. In theory at least all our cfg_if! stuff has an else { compile_error!("...") }

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 24, 2023

This broke doing a check build of cg_clif on x86, which rust's CI does. I use cranelift-native for implementing -Ctarget-cpu=native in cg_clif. For this purpose just returning no features on unsupported platforms is fine.

https://github.com/rust-lang/rust/actions/runs/3999165664/jobs/6862725564#step:26:1343

@cfallin cfallin deleted the remove-x86-32-isms branch January 24, 2023 18:23
@cfallin
Copy link
Member Author

cfallin commented Jan 24, 2023

@bjorn3 IMHO it's reasonable for cranelift-native to refuse to compile on a platform (i686) that Cranelift doesn't support, since its purpose is to query what the native platform is from Cranelift's point of view. Is there a way to exclude the cg_clif check build from running there?

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 24, 2023

Is there a way to exclude the cg_clif check build from running there?

Probably, but it would also break cross-compiling scenarios where cranelift-native would never be called at runtime.

@cfallin
Copy link
Member Author

cfallin commented Jan 24, 2023

Hmm, yeah, actually it seems that any other non-supported platform will compile here (returning an Err at runtime); that's probably the more reasonable option. Happy to review a PR to remove the block with the compile-error. The original issue we were trying to solve was that folks were trying to build Cranelift embedders (like Wasmtime or Viceroy wrapping it) for i686 and were misled by a successful compile; but I suppose a runtime error is still good enough for that purpose since it's clear enough ("unsupported architecture").

@bjorn3
Copy link
Contributor

bjorn3 commented Jan 24, 2023

Opened #5627

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants