For dylib crates, warn about GNU ld <=2.28 #66839
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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
We may well not want to do this. (We might be better off just closing #61539 at this point.) |
pnkfelix
added
the
T-compiler
|
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 |
I definitely feel weird not having implemented something like this soon after opening #61539 (but I don't remember the context very well, I think it was low priority for me, someone having stumbled over it by doing very unstable proc macro manual loading). I wish there was a https://caniuse.com but for the GNU/Linux ecosystem... |
nagisa
reviewed
Nov 28, 2019
|
Could this also be sure to not do any detection unless something goes wrong? This sort of version detection can be pretty expensive and it runs a risk of being added to all compilations by default (those calling the linker). Ideally the diagnostics could be improved but not at the cost of when the diagnostic doesn't fire |
|
@alexcrichton The problem is that it goes wrong when you go to Then again, it's very unstable to |
|
@eddyb wasn't the problem originally exposed by a (maybe there's some detail I'm missing, e.g. as outlined in #60593 (comment) ...?) |
|
Ah, I overlooked this part of a comment from @eddyb from #60593:
So maybe the answer is that yes, the problem can arise for proc-macros, but not in any usage state that we officially support, ... ... and therefore we could plausibly revise this PR to only warn about |
That issue is what I'm referring to by "very unstable to Looking at the linked comment, I wrote:
So I guess there would be a scenario where rustc is handling the |
Someone at my office did point me at the whohas tool as a way to query package versions with various distributions. I tried running it to get a survey of what versions of |
|
We could use https://repology.org/project/binutils/versions: |
|
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 |
|
@eddyb ah ok. I guess I basically just wanted to point out then that this is going to add a cost to all compilations which invoke the linker, and a relatively nontrivial cost. I also suspect that almost all compilations will not actually run into this issue, so it may or may not be worth having a warning specifically for this. |
Its not meant to be run on all compilations that invoke the linker. (But maybe that was just hyperbole on your part?) As originally drafted, I meant for it to run on just compilations of Based on the conversation with @eddyb, I am inclined to further restrict it to just But I will admit it is possible that even restricting it to |
…NU ld bugs like issue 61539.
…reduce false-positive rate.
pnkfelix
force-pushed
the
issue-61539-hack-warn-prior-to-ld-version-2.29
branch
from
d8c2fb9 to
6644747
Compare
…han assuming it starts with "GNU ld version "
…e check. (The first version of the PR had this implicitly by always searching for the string literal "GNU ld version " when looking for where to start the search for the version number.)
pnkfelix
force-pushed
the
issue-61539-hack-warn-prior-to-ld-version-2.29
branch
from
6644747 to
c4a76c4
Compare
pnkfelix
changed the title
|
If this is restricted to just |
|
I'm unfortunately probably not the best reviewer for this. I personally don't think that rustc should be doing any verison checking at all, in my opinion it's far too low-level of a tool to do so. I also have basically no understanding of what this bug is or what this has to do with a linker, so I think I'm not necssarily qualified to review the intention of the PR either. @eddyb would you be willing to review this? |
Some GNU ld versions have a bug that makes them incompatible with the post-#54592 "no PLT" world. However, most things work, just not some situations where an executable loads a I'm also unsure we should land this. The benefits appear to be marginal, I don't think we've seen a bug report other than manually loading proc macros through very unstable internal implementation details. |
|
Ah unfortunately while that describes the bug at a high level I still don't really understand what all is in play here. I don't know, for example, how our usage of the PLT changed over time, how this is running afoul of a linker bug, what possible solutions would be, etc. Sounds like you do though! I'll go ahead and... r? @eddyb |
|
er oops... r? @eddyb |
|
I don't think we figured out what commit actually fixed that bug though, just that it was at some point between 2.28 and 2.29, IIRC. r? @nagisa |
|
based on above comment with data on the |
|
Okay well I made the effort but I don't think there is general interest in landing this lint at this time. . . closing PR. |
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.
This is a heuristic diagnostic to try to help prevent issues related to issue #61539.