-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263
Merged
Merged
rust-lld: fallback to rustc's sysroot if there's no path to the linker in the target sysroot #125263
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if a distro wants to drop Rust lld binaries and rely on system lld?
Then the corresponding
self-contained
directories either won't exist or will be empty.If we just push the
-B
options without checking then gcc will go through them and then proceed searching and finding lld on the system without errors.If we report an error here, then such distros will also need to reset self-contained flags in target specs to false, probably through env vars like the current
CFG_USE_SELF_CONTAINED_LINKER
.This will have a negative effect on portability of options like
-Clink-self-contained=+linker
between distros (if we need such portability).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'm fine with reporting a conservative error here for now, it can always be relaxed later.
At least we'll be able see what breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason why I preferred the fallback compared to the error :3, because falling back to system lld is what we currently do in the case you mention of course.
But one can also say having the self-contained linker enabled is an opt-in for the toolchain maker, it goes hand in hand with bootstrap building lld and putting in the sysroot. Not having both can be seen as a mismatch, and that situation would be properly modeled as
-Clink-self-contained=-linker -Clinker-features=+lld
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I only check for the existence of the
gcc-ld
folder, so in theory an empty directory with no lld binaries would pass this test 🤡There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One problem with the fallback is that if there is no system lld and we try to use it anyway, the error is really bad:
It doesn't even say that it was trying to find
lld
, mentioningld
instead which is not what it was looking for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds to me like they should then disable self-contained linking in their rustc builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely not going to be winning an oscar of the best error message, but also it's in gcc, a non-zero number of people are used to it, and look for the
-fuse-ld
option that is in the command just above that note. (Maybe it also depends on the gcc version and newer ones have improved it 🤔)But it's neither here nor there, the PR currently emits an error and we'll see what happens with distros. Right now, they'll be fine, and in the future they just may need to learn how to have the system lld fallback which is how you and I describe, quite sensible imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dare say most Rust users are not used to it. I certainly was entirely flabbergasted, and I know more than the average Rust programmer about how the compiler works. If an error is too obscure for me I think we can safely assume that it is useless-or-worse for the majority of our users.
That should obviously be reported against gcc, I just didn't have the time for that. But in general we try to work around weaknesses of other tools we employ when we are aware of them.
But as you say indeed for this PR this isn't relevant. I was just replying to @petrochenkov's arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could actually help with this issue in rustc by intervening into the linker error and emitting an extra note.
Like in #125417, but without retrying.