fix for issue 132802: x86 code in wasm32-unknown-unknown binaries
#137457
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Kobzol (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-run-make
|
This PR modifies cc @jieyouxu |
src/bootstrap/src/core/sanity.rs
Outdated
|
|
||
| fn is_gcc_compiler(path: PathBuf, build: &Build) -> bool { | ||
| let cc_output = command(&path).arg("--version").run_capture_stdout(build).stdout(); | ||
| cc_output.contains("GCC") |
There was a problem hiding this comment.
i don't think this does what you want. on my machine, cc --version prints "cc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0", even though it's really gcc.
i am not sure how to check this ... maybe the rust equivalent of realpath $(which cc) | rg gcc? or we could check cc.is_like_clang(): https://docs.rs/cc/latest/cc/struct.Tool.html#method.is_like_clang
There was a problem hiding this comment.
I'll try to find a gcc-specific flag to run. If all else fails, we can try compiling a C program and check if some headers are defined.
There was a problem hiding this comment.
When going down this path I might recommend something like clang -E -dM -x c /dev/null | rg -i clang which will print #define __clang__ 1. That's a test for clang rather than a test for "not gcc" but at this time only clang has support for wasm, so it might work as a detection mechanism nonetheless?
Although IMO rejecting compilers like this is a bit of a tricky business that's bound to have false positives and negatives so personally I'd probably just skip this part and rely on the test you added instead.
There was a problem hiding this comment.
This can just be a UI test (tests/ui), which are cheaper to run than rmake (one thing to compile rather than 2-3). You'll just need the directives //@ only-wasm32 and //@ compile-flags: -Dlinker-messages.
Please make sure tests have a doc comment describing what they are testing as well.
There was a problem hiding this comment.
Got it. I'll rewrite this next week - is there anything else?
There was a problem hiding this comment.
Just to be clear - this should be a //@ build-pass test without an expected output, right?
src/bootstrap/src/core/sanity.rs
Outdated
| @@ -314,6 +314,17 @@ than building it. | |||
| .entry(*target) | |||
| .or_insert_with(|| Target::from_triple(&target.triple)); | |||
|
|
|||
| // compiler-rt c fallbacks for wasm cannot be built with gcc | |||
| if (target.triple == "wasm32-unknown-unknown" || target.triple == "wasm32v1-none") // bare metal targets without wasi sdk | |||
There was a problem hiding this comment.
Could you please add a method to TargetSelection named e.g. is_bare_wasm, or is_wasm_without_wasi or something like that that describes this family of targets?
There was a problem hiding this comment.
If going down this compiler-detection route I'd recommend enabling this for all wasm targets, not just the unknown/none targets. The WASI targets have some auto-configuration but the same principle applies here where if gcc is used then that's a bug for WASI as well.
There was a problem hiding this comment.
If going down this compiler-detection route I'd recommend enabling this for all wasm targets, not just the unknown/none targets.
Would this entail some sort of check to see if the wasi sdk is using gcc?
There was a problem hiding this comment.
In theory (unless I'm misremembering something) nothing more than what's already here other than updating this conditional. The detected compiler for WASI targets should make its way to here and always be clang (wasi-sdk doesn't have gcc and won't for the forseeable future)
|
cc @alexcrichton (target maintainer for |
|
Do note that bootstrap |
|
Hey yall, just wanted everyone to be aware I'm in a power outage so I probably can't work on this today. |
There is never any requirement to work on a PR every day, thanks for being active but take your time :) |
Thanks, I'll probably finish the changes on Sunday or so then |
|
I've changed the test to a UI test, and changed the compiler detection. I'm not sure about the consensus on that - are we still going use it? The commits for that can be scrapped if so |
|
The UI and runtime tests sadly won't help that much for this issue, because it's possible that we might use Clang on test builders, but GCC on dist builders. But the bootstrap sanity check should hopefully do the trick. |
|
For wasm specifically targets I think that Clang should be used everywhere because gcc doesn't have support for wasm. If any builder is using gcc for wasm that's definitely a bug that needs fixing (which the tests might help detect?) |
|
Well, the dist builders only build stuff, it's not actually tested on most dist builders, which is why #132802 was a thing. So the UI/run-make tests are nice to have, but wouldn't help for this situation. But with the bootstrap sanity test I think it's fine. You can r=me, unless you want to do more changes. |
|
Oh, right, yes! (sorry your original comment is clearer now that I've woken up more...) |
|
Great, so the sanity test is going to be kept? |
|
Yeah, let's keep the tests. Let's see if CI works. @bors try |
fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries This is a direct fix for issue [132802](rust-lang#132802). Followed the outline as follows: > * give a hard error in bootstrap when using gcc to compile for wasm > * change our CI to use clang instead of gcc > * add a test that compiling a sample program for wasm32-unknown doesn't give any linker warnings The `test-various` ci job was also changed. try-job: test-various try-job: dist-various-2
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
|
How do I squash? |
|
Short answer: There are some more detailed instructions here https://rustc-dev-guide.rust-lang.org/git.html#squash-your-commits |
JayAndJef
force-pushed
the
issue-132802-fix
branch
from
faddfc1 to
e1bb12d
Compare
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries This is a direct fix for issue [132802](rust-lang#132802). Followed the outline as follows: > * give a hard error in bootstrap when using gcc to compile for wasm > * change our CI to use clang instead of gcc > * add a test that compiling a sample program for wasm32-unknown doesn't give any linker warnings The `test-various` ci job was also changed. try-job: test-various try-job: dist-various-2 try-job: x86_64-msvc-1
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#126856 (remove deprecated tool `rls`) - rust-lang#133981 (rustdoc-json: Refractor and document Id's) - rust-lang#136842 (Add libstd support for Trusty targets) - rust-lang#137355 (Implement `read_buf` and vectored read/write for SGX stdio) - rust-lang#137457 (fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries) - rust-lang#138162 (Update the standard library to Rust 2024) - rust-lang#138273 (metadata: Ignore sysroot when doing the manual native lib search in rustc) - rust-lang#138346 (naked functions: on windows emit `.endef` without the symbol name) - rust-lang#138370 (Simulate OOM for the `try_oom_error` test) r? `@ghost` `@rustbot` modify labels: rollup
|
@bors r- rollup=iffy |
bors
added
S-waiting-on-author
|
Added dist-various-1 to the try-job list of this PR :) |
|
What happened? |
|
The Probably we also use GCC for building wasm targets on |
|
got it, i thought dist various 1 didnt have wasm targets |
|
@bors try |
|
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message |
|
☔ The latest upstream changes (presumably #138523) made this pull request unmergeable. Please resolve the merge conflicts. |
Added sanity check to bootstrap to hard error on wasm builds without clang, and changed distribution image `dist-various-2` to use clang to build for official targets.
Also fixed a typo in the sanity check for bootstrap, as we are checking for clang-likeness in every wasm target.
JayAndJef
force-pushed
the
issue-132802-fix
branch
from
f75880f to
dfe4ac0
Compare
|
@bors try |
fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries This is a direct fix for issue [132802](rust-lang#132802). Followed the outline as follows: > * give a hard error in bootstrap when using gcc to compile for wasm > * change our CI to use clang instead of gcc > * add a test that compiling a sample program for wasm32-unknown doesn't give any linker warnings The `test-various` ci job was also changed. try-job: test-various try-job: dist-various-1 try-job: dist-various-2 try-job: x86_64-msvc-1
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
worked on my machine 😔. will look into later |
This is a direct fix for issue 132802.
Followed the outline as follows:
The
test-variousci job was also changed.try-job: test-various
try-job: dist-various-1
try-job: dist-various-2
try-job: x86_64-msvc-1