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

Emscripten: Drop -lc the emcc driver will add it when it is needed #98303

Closed
wants to merge 1 commit into from

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Jun 20, 2022

According to @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.
(I don't think this is an emscripten bug BTW, but a rustc driver bug)

emscripten-core/emscripten#17191 (comment)

Resolves #98155.

According to 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.
> (I don't think this is an emscripten bug BTW, but a rustc driver bug)

emscripten-core/emscripten#17191 (comment)
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 20, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2022
@hoodmane
Copy link
Contributor Author

r? @compiler-errors

@compiler-errors
Copy link
Member

@bors r=compiler-errors,hoodmane

@bors
Copy link
Contributor

bors commented Jun 20, 2022

📌 Commit d565b20 has been approved by compiler-errors,hoodmane

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 20, 2022
@petrochenkov
Copy link
Contributor

I haven't investigated the details here, but what this PR does looks really hacky.

Why does rustc pass -lc in the first place, if it's always eliminated later?
Are there cases when -lc is indeed needed ? (non SIDE_MODULE?)
If linking of "fundamental" libraries is determined entirely by emcc, then no_default_libraries: false should be used in the target spec and libc crate shouldn't link anything.

@compiler-errors
Copy link
Member

@bors r-

@petrochenkov do you have concerns about landing this? happy to let this sit for discussion instead of just r+'ing it.

@compiler-errors

This comment was marked as outdated.

@compiler-errors

This comment was marked as outdated.

@bjorn3
Copy link
Member

bjorn3 commented Jun 20, 2022

@bors r-

@sbc100
Copy link

sbc100 commented Jun 20, 2022

I haven't investigated the details here, but what this PR does looks really hacky.

Why does rustc pass -lc in the first place, if it's always eliminated later? Are there cases when -lc is indeed needed ? (non SIDE_MODULE?) If linking of "fundamental" libraries is determined entirely by emcc, then no_default_libraries: false should be used in the target spec and libc crate shouldn't link anything.

That does sounds like a more ideal solution. Otherwise the rust driver will be forever playing catchup with the emcc logic as to which default libraries it should link.

@Mark-Simulacrum
Copy link
Member

Closing and reopening to bump bors (hopefully).

@hoodmane
Copy link
Contributor Author

Setting no_default_libraries: false doesn't fix this problem though. As far as I can tell, setting that removes -s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[] but it still passes -lc to Emscripten. I personally think that Emscripten should discard -lc but @sbc100 doesn't seem to be in support of that.

@hoodmane
Copy link
Contributor Author

@sbc100 based on your comment, could Emscripten just always ignore -lc as an argument?

@bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 20, 2022
@bors
Copy link
Contributor

bors commented Jun 20, 2022

😪 I'm awake I'm awake

@sbc100
Copy link

sbc100 commented Jun 20, 2022

@sbc100 based on your comment, could Emscripten just always ignore -lc as an argument?

It could, but fixing this on the rust side rather makes more sense to me.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 20, 2022

Could you explain why it makes more sense? I honestly have no clue. I lean in favor of modify-Rust over modify-Emscripten because we are building several hundred dynamic Emscripten libraries with ~5 different build systems and Rust is the only one that is passing -lc to the Emscripten linker. This makes me think that Rust is doing something weird rather than Emscripten.

What does native code do? I guess if I am using symbols from libc, the linker would recognize them as libc symbols and include them automatically? But surely the normal linker wouldn't cause an error if I explicitly pass -lc?

@hoodmane
Copy link
Contributor Author

Passing -Zlink-native-libraries=no to rustc seems to turn it off.

@sbc100
Copy link

sbc100 commented Jun 21, 2022

Passing -Zlink-native-libraries=no to rustc seems to turn it off.

Is that enough to solve your side module linking issue?

@hoodmane
Copy link
Contributor Author

Yup.

@petrochenkov petrochenkov self-assigned this Jun 21, 2022
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2022
@bjorn3
Copy link
Member

bjorn3 commented Jun 21, 2022

I believe rustc currently assumes that if dynamic libraries are supported, libc itself is dynamically linked. This is true for every other platform afaik. Why is it not true for emscripten?

@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 21, 2022

With no_default_libraries: true it passes the setting -sDEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[].

That's something strange, emcc is similar to gcc/clang, so no_default_libraries: true is supposed to translate to something like -nodefaultlibs and remove things like libc/libgcc/libunwind from linking.

So it looks like emscripten targets are already effectively no_default_libraries: false now.
That means the libc crate (https://crates.io/crates/libc) shouldn't link anything on those targets, but it currently does link libc.

@petrochenkov
Copy link
Contributor

fn set_output_kind with EmLinker does nothing, so apparently the actual output type setting bypasses rustc and is done using raw linker arguments like -sSIDE_MODULE=2.
That's pretty suspicious too.

How do output kinds with emscripten targets map on rustc's crate types CrateType/LinkOutputKind (executable, C-style dynamic library, etc)?
It would be preferable to set them through fn set_output_kind if possible.

no_default_libraries: false is rarely used by rustc targets, typically if the linker has some "secret knowledge" that cannot be easily reproduced in rustc and libc/libunwind crates.
But if emscripten output kinds do not map on rustc's output kinds well, then it might be the case for no_default_libraries: false.

@petrochenkov
Copy link
Contributor

Another possibility would be to turn link_dynlib into a no-op when linking a side module:

This looks like something reasonable.
Even if the issue with -lc specifically is resolved with no_default_libraries: false, it may resurface with some other crate attempts to link some different library dynamically.
It will require rustc gaining knowledge about emscripten's side modules though, apparently that's something unique without equivalents in other targets that we currently have.

What exactly should not be linked to side modules, and how can we recognize it automatically?

  • dynamic libraries (can be addressed by fn link_dylib)
  • static "fundamental" libraries like libc (can be addressed by #[link(name = "c", cfg(not(some_predicate_that_is_true_for_side_modules)))] or by no_default_libraries: false)
  • some other static libraries???

@petrochenkov petrochenkov 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 Jun 21, 2022
@hoodmane
Copy link
Contributor Author

I think I will close this PR and open an issue. Passing -Zlink-native-libraries=no makes linking side modules much more ergonomic and can be used right away while we work on a real fix.

@hoodmane hoodmane closed this Jun 21, 2022
@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 21, 2022

I will make a PR that uses set_output_kind to make Emscripten dynamic libraries when building a cdylib. I'm not sure if this is a good change, but I think it will at least help with the discussion.

@sbc100
Copy link

sbc100 commented Jun 21, 2022

With no_default_libraries: true it passes the setting -sDEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[].

That's something strange, emcc is similar to gcc/clang, so no_default_libraries: true is supposed to translate to something like -nodefaultlibs and remove things like libc/libgcc/libunwind from linking.

So it looks like emscripten targets are already effectively no_default_libraries: false now. That means the libc crate (https://crates.io/crates/libc) shouldn't link anything on those targets, but it currently does link libc.

-sDEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[] is almost certainly wrong/out-of-date. This setting defaults to the empty list anyway these days so its redundant. Also we now support -nodefaultlibs so you can use that, as with other toolchains.

@sbc100
Copy link

sbc100 commented Jun 21, 2022

I believe rustc currently assumes that if dynamic libraries are supported, libc itself is dynamically linked. This is true for every other platform afaik. Why is it not true for emscripten?

Its partly technical, partly historical, and partly based on the peculiarities of the web, but emscrpiten's dynamic linking support is based on the MAIN_MODULE/SIDE_MODULE split with all the system libraries being linked into the main module. Its not ideal but that is the way it works today.

@petrochenkov
Copy link
Contributor

Passing -Zlink-native-libraries=no makes linking side modules much more ergonomic and can be used right away while we work on a real fix.

-Zlink-native-libraries=no will also skip all static libraries though.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 23, 2022
…etrochenkov

Update no_default_libraries handling for emscripten target

`@sbc100` says:

> `-sDEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[]` is almost certainly wrong/out-of-date.   This setting defaults to the empty list anyway these days so its redundant.  Also we now support `-nodefaultlibs` so you can use that, as with other toolchains.

rust-lang#98303 (comment)
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2022
…etrochenkov

Update no_default_libraries handling for emscripten target

``@sbc100`` says:

> `-sDEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[]` is almost certainly wrong/out-of-date.   This setting defaults to the empty list anyway these days so its redundant.  Also we now support `-nodefaultlibs` so you can use that, as with other toolchains.

rust-lang#98303 (comment)
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jun 23, 2022
…etrochenkov

Update no_default_libraries handling for emscripten target

```@sbc100``` says:

> `-sDEFAULT_LIBRARY_FUNCS_TO_INCLUDE=[]` is almost certainly wrong/out-of-date.   This setting defaults to the empty list anyway these days so its redundant.  Also we now support `-nodefaultlibs` so you can use that, as with other toolchains.

rust-lang#98303 (comment)
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 16, 2022
…chenkov

Implement set_output_kind for Emscripten linker

This is on top of rust-lang#98149. See also the earlier discussion on rust-lang#98303. With this PR, a crate that specifies that it is a cdylib will compile to a working Emscripten dynamic library without adding any extra cargo, rustc, or linker flags and without fiddling with environment variables.

`@sbc100`

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 26, 2022
…chenkov

Implement set_output_kind for Emscripten linker

This is on top of rust-lang#98149. See also the earlier discussion on rust-lang#98303. With this PR, a crate that specifies that it is a cdylib will compile to a working Emscripten dynamic library without adding any extra cargo, rustc, or linker flags and without fiddling with environment variables.

`@sbc100`

r? `@petrochenkov`
@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
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-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

Emscripten dynamic linking and libc