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

Do not pass -lc to the Emscripten linker #131885

Closed
wants to merge 1 commit into from

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Oct 18, 2024

Hack: drop -lc to work around buggy Emscripten behaviors when -lc is passed. This can be removed if/when the problems are resolved in Emscripten and we drop support for the currently existing versions of Emscripten. See
emscripten-core/emscripten#22758

Unblocks #131736

This is a repeat of #98303, but the bug we were trying to resolve then went away and this is supposed to work around a different bug.

@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 18, 2024
@hoodmane
Copy link
Contributor Author

According to Emscripten maintainer @sbc100, when linking with emcc we shouldn't pass -lc:

your best bet is to remove any/all explicit additions of -lc, since
the compiler driver (emcc) will add it if/when its needed.

emscripten-core/emscripten#17191 (comment)

Hack: drop -lc to work around buggy Emscripten behaviors when -lc
is passed. This can be removed when the problems are resolved in Emscripten and
we drop support for the currently existing versions of Emscripten.
See
emscripten-core/emscripten#22758

Unblocks
rust-lang#131736
@hoodmane hoodmane force-pushed the emscripten-drop-libc branch from b789570 to 2f6f2b2 Compare October 18, 2024 20:20
@sbc100
Copy link

sbc100 commented Oct 18, 2024

I think a better solution would be to remove the explict -lc from all platforms. Unless somebody knows of some reason why its needed/useful? Are there platforms where libc is not linked by default?

@hoodmane
Copy link
Contributor Author

I am not sure where the -lc comes from so I don't actually know how to cleanly implement that. It seems to be listed in a lib.rmeta file somewhere? What I could do is apply this hack to all 9 linkers and see if anything breaks in CI. But even if CI doesn't break, there could potentially be someone somewhere relying on this? Would be great to find out.

@workingjubilee workingjubilee added the O-emscripten Target: 50% off wasm32-unknown-musl. the savings come out of stdio.h, but hey, you get SDL! label Oct 21, 2024
@workingjubilee
Copy link
Member

I think a better solution would be to remove the explict -lc from all platforms. Unless somebody knows of some reason why its needed/useful? Are there platforms where libc is not linked by default?

There are platforms where libc is not even a thing that makes sense to talk about, yes.

@workingjubilee
Copy link
Member

workingjubilee commented Oct 21, 2024

There are platforms where libc is not even a thing that makes sense to talk about, yes.

Hm, I think the answer from the Jubilee of 30 minutes ago might have been a bit confused there.

@sbc100
Copy link

sbc100 commented Oct 22, 2024

I think a better solution would be to remove the explict -lc from all platforms. Unless somebody knows of some reason why its needed/useful? Are there platforms where libc is not linked by default?

There are platforms where libc is not even a thing that makes sense to talk about, yes.

In that case doesn't it make sense to remove the generic addition of the explicit -lc? I don't know of any platform that requires this, but I could be wrong.

@hoodmane
Copy link
Contributor Author

Personally, in interest of getting something merged, I'd prefer we'd start with this change which definitely doesn't affect anyone else. This libc problem has been around for years. It would be nice to fix it so we can enable WASM_BIGINT and other features that require Safari 15 when linking Rust.

@workingjubilee
Copy link
Member

@hoodmane Can you open an issue about "it's kind of weird that we pass -lc in the first place, isn't it...?" explaining the problem here.

@hoodmane
Copy link
Contributor Author

After a bit more investigation, it turns out that rust/libc specifically asks to pass -lc to the linker. I'm having trouble using a local override version of libc, but I think deleting these two lines may fix the problem:
https://github.com/rust-lang/libc/blob/main/src/unix/mod.rs#L392-L393

@sbc100
Copy link

sbc100 commented Oct 31, 2024

After a bit more investigation, it turns out that rust/libc specifically asks to pass -lc to the linker. I'm having trouble using a local override version of libc, but I think deleting these two lines may fix the problem: https://github.com/rust-lang/libc/blob/main/src/unix/mod.rs#L392-L393

Awesome! That looks like the correct fix! Nice find.

@workingjubilee
Copy link
Member

neat, thanks.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 1, 2024

New PR to rust-lang/libc:
rust-lang/libc#4002

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 4, 2024

Closed as superseded by rust-lang/libc#4002. Thanks @sbc100 and @workingjubilee!

@hoodmane hoodmane closed this Nov 4, 2024
@hoodmane hoodmane deleted the emscripten-drop-libc branch November 4, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-emscripten Target: 50% off wasm32-unknown-musl. the savings come out of stdio.h, but hey, you get SDL! S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants