Skip to content

rustc: Always handle exported symbols on the wasm target #60526

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

Merged
merged 1 commit into from
May 7, 2019

Conversation

alexcrichton
Copy link
Member

Currently when linking an artifact rustc will only conditionally call
the Linker::export_symbols function, but this causes issues on some
targets, like WebAssembly, where it means that executable outputs will
not have the same symbols exported that cdylib outputs have. This commit
sinks the conditional call to export_symbols inside the various
implementations of the function that still need it, and otherwise the
wasm linker is configured to always pass through symbol visibility
lists.

Currently when linking an artifact rustc will only conditionally call
the `Linker::export_symbols` function, but this causes issues on some
targets, like WebAssembly, where it means that executable outputs will
not have the same symbols exported that cdylib outputs have. This commit
sinks the conditional call to `export_symbols` inside the various
implementations of the function that still need it, and otherwise the
wasm linker is configured to always pass through symbol visibility
lists.
@rust-highfive
Copy link
Contributor

r? @oli-obk

(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 May 3, 2019
@oli-obk
Copy link
Contributor

oli-obk commented May 5, 2019

If the early return is forgotten for a linker, are the any adverse side effects? And if so, how hard is it to figure out from the side effects that the linker export_symbols method is missing an early return?

@alexcrichton
Copy link
Member Author

I haven't tested it out but I suspect that link errors would crop up here and there. In theory we should always be passing the symbol whitelist to the linker, but that's likely to be a breaking change (even though it's not supposed to be) for weird esoteric linker reasons. This doesn't really change that often though so I'm not too too worried about forgotten returns.

@oli-obk
Copy link
Contributor

oli-obk commented May 6, 2019

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented May 6, 2019

📌 Commit 884632c has been approved by oli-obk

@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 May 6, 2019
@bors
Copy link
Collaborator

bors commented May 6, 2019

⌛ Testing commit 884632c with merge f64ed09...

bors added a commit that referenced this pull request May 6, 2019
rustc: Always handle exported symbols on the wasm target

Currently when linking an artifact rustc will only conditionally call
the `Linker::export_symbols` function, but this causes issues on some
targets, like WebAssembly, where it means that executable outputs will
not have the same symbols exported that cdylib outputs have. This commit
sinks the conditional call to `export_symbols` inside the various
implementations of the function that still need it, and otherwise the
wasm linker is configured to always pass through symbol visibility
lists.
@bors
Copy link
Collaborator

bors commented May 7, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing f64ed09 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 7, 2019
@bors bors merged commit 884632c into rust-lang:master May 7, 2019
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #60526!

Tested on commit f64ed09.
Direct link to PR: #60526

🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request May 7, 2019
Tested on commit rust-lang/rust@f64ed09.
Direct link to PR: <rust-lang/rust#60526>

🎉 rls on windows: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
@bors bors mentioned this pull request May 7, 2019
@alexcrichton alexcrichton deleted the wasm-main-symbols branch July 8, 2019 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants