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

Implement set_output_kind for Emscripten linker #98358

Closed
wants to merge 2 commits into from

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Jun 21, 2022

This is on top of #98149. See also the earlier discussion on #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

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 21, 2022
@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

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

@hoodmane
Could you answer the question from #98303 (comment) in some more detail?
What output kinds does Emscripten have?
How do they map to Rust crate and output kinds?
What does -sMAIN_MODULE=0/1/2/? and -sSIDE_MODULE=0/1/2/? mean?
@rustbot author

@rustbot rustbot 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 22, 2022
@hoodmane
Copy link
Contributor Author

Copying over the most relevant comments so we can have the context all in one place.

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.

Originally posted by @petrochenkov in #98303 (comment)

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

Originally posted by @petrochenkov in #98303 (comment)

@bors

This comment was marked as resolved.

@hoodmane
Copy link
Contributor Author

hoodmane commented Jun 24, 2022

How do output kinds with emscripten targets map on rustc's crate types CrateType/LinkOutputKind (executable, C-style dynamic library, etc)?

I think the output kinds map perfectly reasonably. Emscripten has the following possible outputs:

  • an executable: a pair of a .js file and a .wasm file and possibly a .data file, can be run with node
  • a library libwhatever.a: works like a normal static library
  • a dynamic library libwhatever.so: works similar to a normal dynamic library but with some additional restrictions on when/how you can load them since if the library is larger than some threshold loading is asynchronous.

The thing that is causing the trouble is that Emscripten has decided that in many cases when native programs would use dynamic linking it is preferable to use static linking. In interest of porting these dynamic builds to Emscripten static libs without changing their build system, if you pass -shared Emscripten prints a warning and emits a static library. Emscripten has the -sSIDE_MODULE flag to indicate that you actually want to make a dynamic library.

The problem right now is that both staticlib and cdylib crates emit static libraries and so there is no remaining crate type for "for real I want a dynamic library". In my opinion the most reasonable fix is for staticlib to make a static library and for cdylib to make a dynamic library. If people actually want an Emscripten static library, they can switch the crate type easily enough.

@hoodmane
Copy link
Contributor Author

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

I think the problem here is libraries that have both a static and a dynamic version. Emscripten doesn't have the same logic that normal compilers do for picking between them. In particular, it is not a problem if you pass libwhatever.so explicitly. I think the problem here isn't as bad as it seems and the biggest issue may really be libc. Of course it would be better to link a static library into the main executable once rather than several times into different shared libraries, but that isn't the end of the world.

It would be helpful to have more data on this though. I should try to build more crates that explicitly link non-rust libraries.

@hoodmane
Copy link
Contributor Author

@sbc100 anything more you have to add here? It would simplify things on the Rust side a lot if linking issues could be fixed on the Emscripten side...

@sbc100
Copy link

sbc100 commented Jun 24, 2022

I would say that executable comes in two flavors, normal and MAIN_MODULE. You can think of them like -fno-pie and -fpie. Only the MAIN_MODULE / -fpie form can link against dynamic library. Normal executables are fully static.

@hoodmane
Copy link
Contributor Author

Well there are four executable LinkOutputKinds: DynamicNoPicExe, DynamicPicExe, StaticNoPicExe, and StaticPicExe. So it seems like we could pass -sMAIN_MODULE=1 (or 2?) for DynamicPicExe and StaticPicExe.

@petrochenkov
Copy link
Contributor

Well there are four executable LinkOutputKinds

Technically it's 5, there's also LinkOutputKind::WasiReactorExe, if there are any analogues in Emscripten, then it can also be potentially reused.

@hoodmane
Copy link
Contributor Author

I think linking executables with dynamic linking enabled cannot work right now without fixes from Emscripten. We can't use -sMAIN_MODULE=1 because that passes -Wl,--whole-archive which breaks on Rust libraries because they include the lib.rmeta file. And we can't use -sMAIN_MODULE=2 because that requires us to calculate what exports are needed but Rust has no way to calculate this without extra help.

@bjorn3
Copy link
Member

bjorn3 commented Jun 24, 2022

So it seems like we could pass -sMAIN_MODULE=1 (or 2?) for DynamicPicExe and StaticPicExe.

I don't think passing it for the *Pic* variants makes sense. A non-pic executable can still be dynamically linked and use dlopen. I think it should rather be done for the Dynamic* variants. Then toggling -sMAIN_MODULE=1 would be a matter of -Ctarget-feature=+/-crt-static which is also how non-emscripten targets effectively determine if dynamic linking and dlopen are supported.

@hoodmane
Copy link
Contributor Author

Yes that sounds right.

@hoodmane
Copy link
Contributor Author

@sbc100 how does it look to you?

@hoodmane hoodmane changed the title Add basic support for generating Emscripten side modules from cdylib crates Implement set_output_kind for Emscripten linker Jun 24, 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 25, 2022
@rust-log-analyzer

This comment has been minimized.

@hoodmane hoodmane force-pushed the emscripten-dynlib branch 4 times, most recently from 60b7b21 to 48678de Compare June 26, 2022 04:10
@hoodmane
Copy link
Contributor Author

hoodmane commented Jul 6, 2022

@petrochenkov @compiler-errors could someone review this? Or if you don't have time, do you have any recommendations for other people I might ask?

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`
@JohnTitor
Copy link
Member

Failed in rollup: #99305 (comment)
@bors r-

@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 Jul 16, 2022
@hoodmane
Copy link
Contributor Author

@petrochenkov I cherry-picked the commit from #99243 to this branch because tests won't pass on this branch without it. Should I squash this too?

@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 Jul 23, 2022
@petrochenkov
Copy link
Contributor

No, it's fine as is.
@bors r+

@bors
Copy link
Contributor

bors commented Jul 24, 2022

📌 Commit b0da01f has been approved by petrochenkov

It is now in the queue for this repository.

@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 Jul 24, 2022
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`
@ehuss
Copy link
Contributor

ehuss commented Jul 27, 2022

@bors r-

Failed in rollup: #99776 (comment)

@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 Jul 27, 2022
@JohnCSimon
Copy link
Member

ping from triage:
@hoodmane can you post your the status of this PR?
Thanks

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Comment on lines +1057 to +1061
// Without -sWASM_BIGINT there are issues with dynamic Rust libraries. There
// are no plans to fix this in Emscripten AFAIK. See
// https://github.com/emscripten-core/emscripten/pull/16693 This could also
// be fixed with panic=abort.
self.cmd.arg("-sWASM_BIGINT");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This problem is fixed now:
emscripten-core/emscripten#17328
so I should remove it.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 6, 2022

I think there's still a test failure that I don't understand. I will try to look into it again soon.

@JohnCSimon
Copy link
Member

@hoodmane
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you do please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Jan 1, 2023
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. 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.

None yet