fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries #137457

JayAndJef · 2025-02-23T04:43:56Z

This is a direct fix for issue 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

…sm compiler_builtins

rustbot · 2025-02-23T04:44:05Z

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2025-02-23T04:44:08Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

jyn514 · 2025-02-23T04:49:03Z

src/bootstrap/src/core/sanity.rs

+
+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")


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

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.

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.

tgross35 · 2025-02-24T04:20:53Z

tests/run-make/wasm-builtins-no-linker-warning/rmake.rs

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.

Got it. I'll rewrite this next week - is there anything else?

Just to be clear - this should be a //@ build-pass test without an expected output, right?

That sounds correct 👍

Kobzol

Detecting GCC would be also useful for other things, but it sounds a bit tricky. Left one nit, otherwise the bootstrap changes (modulo GCC detection, which isn't working yet) look fine. I don't know about WASM though, so someone else should look at that.

Kobzol · 2025-02-25T09:33:25Z

src/bootstrap/src/core/sanity.rs

@@ -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


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?

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.

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?

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)

jieyouxu · 2025-02-25T11:49:57Z

cc @alexcrichton (target maintainer for wasm32-unknown-unknown) in case you have any concerns/feedback

tgross35 · 2025-02-25T18:17:37Z

Detecting GCC would be also useful for other things, but it sounds a bit tricky. Left one nit, otherwise the bootstrap changes (modulo GCC detection, which isn't working yet) look fine. I don't know about WASM though, so someone else should look at that.

cc exposes this functionality, maybe we could wrap that rather than recreating it here?

jieyouxu · 2025-02-25T18:43:51Z

cc exposes this functionality, maybe we could wrap that rather than recreating it here?

Do note that bootstrap cc unfortunately has to be pinned to a quite an old version, FWIW.

JayAndJef · 2025-02-25T18:58:36Z

Hey yall, just wanted everyone to be aware I'm in a power outage so I probably can't work on this today.

tgross35 · 2025-02-25T19:06:46Z

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 :)

JayAndJef · 2025-02-27T18:26:01Z

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

JayAndJef · 2025-03-03T00:46:26Z

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

Kobzol · 2025-03-03T07:56:28Z

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.

alexcrichton · 2025-03-03T15:47:08Z

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?)

Kobzol · 2025-03-03T15:57:33Z

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.

alexcrichton · 2025-03-03T16:00:31Z

Oh, right, yes! (sorry your original comment is clearer now that I've woken up more...)

JayAndJef · 2025-03-03T16:02:54Z

Great, so the sanity test is going to be kept?

Kobzol · 2025-03-03T16:21:56Z

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

bors · 2025-03-03T16:23:10Z

⌛ Trying commit 63db26f with merge ca07aa7...

bors · 2025-03-03T17:13:52Z

💔 Test failed - checks-actions

JayAndJef · 2025-03-03T17:14:31Z

oh no

JayAndJef · 2025-03-03T18:52:52Z

Does anyone know why clang wouldn't be able to find these headers

alexcrichton · 2025-03-03T19:42:02Z

The failing step there is Building test helpers for wasm32-unknown-unknown which I believe is using the C compiler itself to compile tests/auxiliary/rust_test_helpers.c. This won't work on wasm32-unknown-unknown since the C compiler has no host headers at all, so that file cannot be compiled. Using clang works as a compiler but it's not a complete toolchain as there's no sysroot for wasm32-unknown-unknown. I think this may be ok for compiler-rt which brings its own (IIRC), but I'm also not certain on this.

jieyouxu · 2025-03-03T19:52:12Z

That is used for some UI tests. I suppose, one way to go about that, is to skip building test_helpers if the target is wasm32-u-u, and then audit ui test failures to ignore those tests if target is wasm32-u-u(?)

JayAndJef · 2025-03-04T05:18:19Z

Would a workaround just be to revert to a run-make test?

JayAndJef · 2025-03-05T22:15:23Z

Does anyone see the need for wasm32-unknown-unknown UI tests sometime in the future? If not, I think reverting to a run-make test is a good move

alexcrichton · 2025-03-05T22:30:52Z

Personally I'd say no, we probably don't want to test wasm32-unknown-unknown. It's not impossible to test but the lack of debugging facilities (e.g. no println) makes it a non-starter for many workflows. In that sense I'd at least personally be ok with just a run-make test. (or rather I'd be ok with no testing whatsoever and just the bootstrap check, but that's just me)

Kobzol · 2025-03-07T06:53:00Z

@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

bors · 2025-03-07T06:54:14Z

⌛ Trying commit 9c25311 with merge 8f2e3fb...

bors · 2025-03-07T07:45:09Z

💔 Test failed - checks-actions

JayAndJef · 2025-03-07T19:17:49Z

Strange, worked when I ran it on my machine. Will look into later

edit: I can't find it, ill try again when i get home. if it doesnt work, we can just remove the test if its fine with everyone

JayAndJef · 2025-03-09T02:18:17Z

Removed the test, is that ok with everyone

Kobzol · 2025-03-09T12:15:37Z

src/bootstrap/src/core/sanity.rs

+        .arg("-x")
+        .arg("c")
+        .arg("/dev/null")
+        .run_capture_stdout(build)


It seems like GCC supports these flags, but maybe some other compilers won't (in particular MSVC?). I guess that if the command fails we should return false from the function rather than failing the build. There is a method for allowing failure on the command.

JayAndJef added 5 commits February 22, 2025 13:03

fix(bootstrap): hard error with GCC on bare WASM builds with c

f624850

fix(ci): build non-wasip wasm targets with clang

c49c586

feat(tests): add test for linker errors caused by x86 obj files in wa…

3f2acf2

…sm compiler_builtins

feat(ci): update test-various for wasm32-unknown-unknown

afe85f0

chore: format latest test case

2d2c135

rustbot assigned Kobzol Feb 23, 2025

jyn514 reviewed Feb 23, 2025

View reviewed changes

tgross35 reviewed Feb 24, 2025

View reviewed changes

JayAndJef added 2 commits February 24, 2025 16:02

feat(tests): migrate wasm-bare linker run-make to ui test

63edce1

feat(ci): update test-various to include migrated wasm linker test

6083188

Kobzol reviewed Feb 25, 2025

View reviewed changes

jieyouxu added the O-wasm Target: WASM (WebAssembly), http://webassembly.org/ label Feb 27, 2025

JayAndJef added 2 commits March 2, 2025 16:23

fix(bootstrap): change compiler detection for all wasm targets

4c129c0

fix(bootstrap): change compiler detection to clang

63db26f

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2025

fix(tests): migrate back to run-make test

9c25311

This comment has been minimized.

Sign in to view

fix(tests): remove wasm-bare test altogether

145befe

Kobzol reviewed Mar 9, 2025

View reviewed changes

fix for issue 132802: x86 code in wasm32-unknown-unknown binaries #137457

Are you sure you want to change the base?

fix for issue 132802: x86 code in wasm32-unknown-unknown binaries #137457

Conversation

JayAndJef commented Feb 23, 2025 • edited by Kobzol Loading

rustbot commented Feb 23, 2025

rustbot commented Feb 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kobzol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Feb 25, 2025

tgross35 commented Feb 25, 2025

jieyouxu commented Feb 25, 2025

JayAndJef commented Feb 25, 2025

tgross35 commented Feb 25, 2025

JayAndJef commented Feb 27, 2025

JayAndJef commented Mar 3, 2025

Kobzol commented Mar 3, 2025

alexcrichton commented Mar 3, 2025

Kobzol commented Mar 3, 2025

alexcrichton commented Mar 3, 2025

JayAndJef commented Mar 3, 2025

Kobzol commented Mar 3, 2025

bors commented Mar 3, 2025

This comment has been minimized.

bors commented Mar 3, 2025

JayAndJef commented Mar 3, 2025

JayAndJef commented Mar 3, 2025

alexcrichton commented Mar 3, 2025

jieyouxu commented Mar 3, 2025 • edited Loading

JayAndJef commented Mar 4, 2025

JayAndJef commented Mar 5, 2025

alexcrichton commented Mar 5, 2025

Kobzol commented Mar 7, 2025

bors commented Mar 7, 2025

This comment has been minimized.

bors commented Mar 7, 2025

JayAndJef commented Mar 7, 2025 • edited Loading

JayAndJef commented Mar 9, 2025

Choose a reason for hiding this comment

fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries #137457

fix for issue 132802: x86 code in `wasm32-unknown-unknown` binaries #137457

JayAndJef commented Feb 23, 2025 •

edited by Kobzol

Loading

jieyouxu commented Mar 3, 2025 •

edited

Loading

JayAndJef commented Mar 7, 2025 •

edited

Loading