Force rlibs to be linked in whole for cdylib and bin #93791
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-compiler
|
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Would it make sense to only do this when |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Wow! It's great that #50007 is finally seeing a fix, after all this time! Just to make sure, we still need #93718 in addition to this, right?
@nico-abram I could be wrong, but think it might be a little more complex than this, to handle the case where a |
|
From the test, it seems that the current behaviour is deliberately introduced in cc719d2 by @alexcrichton. |
|
I was planning to make an exactly opposite change in #85144, but it requires stabilizing the |
|
On a closer look, this PR doesn't touch native libs, only rlibs, so the linking modifiers that I've mentioned are somewhat orthogonal to this (but we can support similar linking modifiers for rlibs too). However, including all symbols is still a huge overkill for fixing #50007 and #47384, those issues need to be addressed in some more selective way. |
petrochenkov
added
S-waiting-on-author
What's your suggested alternative? In #85144 you said (for dylib) we are using symbol export list for this, but this is not sufficient: the exported symbol list is passed to LD using version script, but this wouldn't affect the symbol resolution. E.g. |
|
In my mental model, the artifact of each crate, being a compilation unit on its own, should be closer to a .o file than .a file. So I also think link rlibs as a whole should be the expected behaviour. |
Somehow mark only the |
|
Let's do a perf run to see how "overkill" is this change. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit e760877 with merge f80eb827f9222fa2d5893442f376349408e73200... |
|
☀️ Try build successful - checks-actions |
|
Queued f80eb827f9222fa2d5893442f376349408e73200 with parent e7aca89, future comparison URL. |
|
Finished benchmarking commit (f80eb827f9222fa2d5893442f376349408e73200): comparison url. Summary: This benchmark run shows 45 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never |
rustbot
added
perf-regression
petrochenkov
removed
the
S-waiting-on-review
|
ping from triage: FYI: when a PR is ready for review, post a message containing |
|
Currently there is a disagreement about whether this should be done or not. |
This was referenced Mar 27, 2022
|
Closing in favor of #95604. |
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 10, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking Fix rust-lang#50007 and rust-lang#47384 This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed. Related rust-lang#93791, rust-lang#95363 r? `@petrochenkov` cc `@carbotaniuman`
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 11, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking Fix rust-lang#50007 and rust-lang#47384 This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed. Related rust-lang#93791, rust-lang#95363 r? ``@petrochenkov`` cc ``@carbotaniuman``
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 11, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking Fix rust-lang#50007 and rust-lang#47384 This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed. Related rust-lang#93791, rust-lang#95363 r? ```@petrochenkov``` cc ```@carbotaniuman```
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 17, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking Fix rust-lang#50007 and rust-lang#47384 This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed. Related rust-lang#93791, rust-lang#95363 r? `@petrochenkov` cc `@carbotaniuman`
Dylan-DPC
added a commit
to Dylan-DPC/rust
that referenced
this pull request
Apr 18, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking Fix rust-lang#50007 and rust-lang#47384 This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed. Related rust-lang#93791, rust-lang#95363 r? `@petrochenkov` cc `@carbotaniuman`
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 25, 2022
Generate synthetic object file to ensure all exported and used symbols participate in the linking Fix rust-lang#50007 and rust-lang#47384 This is the synthetic object file approach that I described in rust-lang#95363 (comment), allowing all exported and used symbols to be linked while still allowing them to be GCed. Related rust-lang#93791, rust-lang#95363 r? `@petrochenkov` cc `@carbotaniuman`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #50007 and #47384
These issues are caused by the fact that linker will perform archive member selection. By default, an object file in an archive won't be selected unless it is needed to satisfy an undefined reference. This step precedes any linker script and section GC. To address this issue, we can just the whole rlibs to be linked (this is
--whole-archivefor ELF targets).