Fix host code appearing in Wasm 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 |
|
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 |
|
@JayAndJef |
|
☔ The latest upstream changes (presumably #140772) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@JayAndJef any chance you are able to pick this back up? You probably need to install |
|
Sorry yall, I kind of completely forgot about this pr. Is anyone else willing to take this on? |
|
Well, somebody did mention interest in #132802. But this seems close enough to get over the line if you are able! Needs a rebase plus Clang should be installed, probably not too much after that. |
|
Gotchu, will work on this week. Sorry for the wait |
|
No worries! It's open source and life happens :) |
|
Sorry, what should I do so that the changes would be mergeable? |
|
The error message at the bottom of #137457 (comment) says:
which is the error message added here. You probably need to make similar change to the dist-various-1 dockerfile that you did to dist-various-2, setting the Also this needs a rebase since there is a conflict. |
| if target.contains("wasm") | ||
| && (build.config.optimized_compiler_builtins(*target) | ||
| || build.config.rust_std_features.contains("compiler-builtins-c")) | ||
| { | ||
| let is_clang = build.cc_tool(*target).is_like_clang(); | ||
| if !is_clang { | ||
| panic!("only clang supports building c code for wasm targets"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Mind adding some more guidance to the panic message so anyone who hits this doesn't have to dig too much? Maybe something like
panic!(
"Clang is required to build C code for Wasm targets, got `{}` instead\n\
this is because compiler-builtins is configured to build C source. Either \
ensure Clang is used, or adjust this configuration.",
tool.path().display()
);
tgross35
changed the title
wasm32-unknown-unknown binarieswasm32-unknown-unknown binaries
tgross35
changed the title
wasm32-unknown-unknown binaries
|
I keep getting merge conflicts while rebasing and I don't know how to solve them. I tried manually updating the cargo submodule to the latest commit in upstream, but then when I run git add and continue the rebase it complains that I haven't solved the conflict and added the changes. I'll add installing clang to the dockerfile right now and fix these after |
|
Maybe abort the rebase and do Make sure you're not changing the submodule in a commit or something (doesn't look like it from the state of the PR) |
|
I'm incredibly confused on wtf is going on with git right now because I keep running into unexpected things - I'm going to take a break and hopefully find some clarity tomorrow. Just to be clear, I was supposed to sync the repository then |
|
What are you running into? I think the Get Out of Jail Free card should look something like this: # Ensure you are on the right branch and it is up to date
git switch issue-132802-fix
git pull --rebase
# ^ if that fails for whatever reason, you can do a more extreme
# `git reset --hard dfe4ac0ba7ed`, which will blow away any chances and
# reset your branch to the last commit on this PR. Careful!
# Forget submodules
git submodule deinit --all --force
# Fetch rust-lang/rust. The latest commit on master will be FETCH_HEAD
git fetch https://github.com/rust-lang/rust.git master
# Do the actual rebase. You will need to resolve a conflict; do that, `git add .`,
# and `git rebase --continue`.
#
# Make sure you don't accidentally add a submodule here, sometimes git can be
# weird like that. If you see anything suspicious, `git submodule deinit --all --force`
git rebase FETCH_HEAD
# And update the PR
git push --force-with-lease |
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