Keep last redundant linker flag, not first #57018
Conversation
When a library (L1) is passed to the linker multiple times, this is
sometimes purposeful: there might be several other libraries in the
linker command (L2 and L3) that all depend on L1. You'd end up with a
(simplified) linker command that looks like:
-l2 -l1 -l3 -l1
With the previous behavior, when rustc encountered a redundant library,
it would keep the first instance, and remove the later ones, resulting
in:
-l2 -l1 -l3
This can cause a linker error, because on some platforms (e.g. Linux),
the linker will only include symbols from L1 that are needed *at the
point it's referenced in the command line*. So if L3 depends on
additional symbols from L1, which aren't needed by L2, the linker won't
know to include them, and you'll end up with "undefined symbols" errors.
A better behavior is to keep the *last* instance of the library:
-l2 -l3 -l1
This ensures that all "downstream" libraries have been included in the
linker command before the "upstream" library is referenced.
Fixes rust-lang#47989
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
ping from triage @matthewjasper wating for you to review this |
|
Thanks for the PR @dcreager! I'm a little worried about this though in that libraries can change name/link kind via CLI flags, so it seems like the 'kind' of a library could change as we process it silently after this change? I think it sounds right to me to move everything to the end of the command line, but I think we may wish to keep the original warning if the kind is being updated perhaps? |
Dylan-DPC-zz
added
S-waiting-on-author
|
Thanks for the feedback Alex! If I'm reading the old code right, we didn't print out the warning before if the kind changed; it was only printed if the second library reference was truly redundant. |
|
Hm I think perhaps? This is so far out of cache I barely remember what any of this code does. Can you gist a few examples of before/after behavior to help dig into what's changing warning-wise here? |
|
Sure thing! The original problem that this is trying to fix is some linker errors when you have a particular pattern of dependencies in some C/C++ code. I had put together minimal repro in this repo. On Mac, the linker errors never occur. On Linux, the linker error occurs before this patch, but not after. For both platforms, a "redundant linker flag" warning was printed out before, but not after. Before (Mac)Before (Linux)After (both)
I'm not familiar with changing the link type so I don't have any examples of that at my fingertips, but I'll try to put together a minimal example for that, too. |
|
Got a before/after for the "change library kind" behavior: BeforeAfter
That build script has a flag that updates the kind for the |
|
Ok thanks for the investigation! I'm realizing now I'm completely misreading the code, I thought it warned on Out of an abundance of caution around linking things I'd like to run this through crater to ensure that we don't have any unexpected breakage. Linkage is always super subtle and really hard to predict what or what won't regress lots. I think this change is right, but would be good to be safe too! @bors: try |
|
⌛ Trying commit c56f128 with merge 5ce78cbe7028b830a57eb1611aaf93851a11929e... |
|
☀️ Test successful - checks-travis |
|
@craterbot run start=master#d9a2e3b1ccf16c6d43f56f503bd80c1ad137d523 end=try#5ce78cbe7028b830a57eb1611aaf93851a11929e mode=build-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Absolutely! This will be fun to watch, I’ve never seen crater in action! 😀 |
|
🚨 Experiment 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
|
Uhhh, wut. @craterbot retry-report |
|
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
⌛ Testing commit 32d99ef with merge df536c0aabf04d36cbbf74b9c56be787914880f9... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added
S-waiting-on-review
|
@bors treeclosed- |
|
⌛ Testing commit 32d99ef with merge e961dba88db54067d7534666b440669afdcc8b14... |
|
💔 Test failed - checks-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors: retry |
bors
added
S-waiting-on-bors
Keep last redundant linker flag, not first When a library (L1) is passed to the linker multiple times, this is sometimes purposeful: there might be several other libraries in the linker command (L2 and L3) that all depend on L1. You'd end up with a (simplified) linker command that looks like: ``` -l2 -l1 -l3 -l1 ``` With the previous behavior, when rustc encountered a redundant library, it would keep the first instance, and remove the later ones, resulting in: ``` -l2 -l1 -l3 ``` This can cause a linker error, because on some platforms (e.g. Linux), the linker will only include symbols from L1 that are needed *at the point it's referenced in the command line*. So if L3 depends on additional symbols from L1, which aren't needed by L2, the linker won't know to include them, and you'll end up with "undefined symbols" errors. A better behavior is to keep the *last* instance of the library: ``` -l2 -l3 -l1 ``` This ensures that all "downstream" libraries have been included in the linker command before the "upstream" library is referenced. Fixes #47989
|
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@9c499cc. Direct link to PR: <rust-lang/rust#57018> 🎉 rls on windows: test-fail → test-pass (cc @nrc @Xanewok, @rust-lang/infra).
When a library (L1) is passed to the linker multiple times, this is sometimes purposeful: there might be several other libraries in the linker command (L2 and L3) that all depend on L1. You'd end up with a (simplified) linker command that looks like:
With the previous behavior, when rustc encountered a redundant library, it would keep the first instance, and remove the later ones, resulting in:
This can cause a linker error, because on some platforms (e.g. Linux), the linker will only include symbols from L1 that are needed at the point it's referenced in the command line. So if L3 depends on additional symbols from L1, which aren't needed by L2, the linker won't know to include them, and you'll end up with "undefined symbols" errors.
A better behavior is to keep the last instance of the library:
This ensures that all "downstream" libraries have been included in the linker command before the "upstream" library is referenced.
Fixes #47989