For dylib crates, warn about GNU ld <=2.28#66839
Closed
pnkfelix wants to merge 4 commits intorust-lang:masterfrom
Closed
For dylib crates, warn about GNU ld <=2.28#66839pnkfelix wants to merge 4 commits intorust-lang:masterfrom
pnkfelix wants to merge 4 commits intorust-lang:masterfrom
Conversation
Contributor
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
|
We may well not want to do this. (We might be better off just closing #61539 at this point.) |
Contributor
Author
pnkfelix
added
the
T-compiler
Contributor
|
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 |
Member
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
Member
|
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 |
Member
|
@alexcrichton The problem is that it goes wrong when you go to Then again, it's very unstable to |
Contributor
Author
|
@eddyb wasn't the problem originally exposed by a (maybe there's some detail I'm missing, e.g. as outlined in #60593 (comment) ...?) |
Contributor
Author
|
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 |
Member
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 |
Contributor
Author
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 |
Contributor
|
We could use https://repology.org/project/binutils/versions: |
Contributor
|
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 |
Member
|
@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. |
Contributor
Author
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
Member
|
If this is restricted to just |
Contributor
Member
|
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? |
Member
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. |
Member
|
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 |
Member
|
er oops... r? @eddyb |
Member
|
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 |
Contributor
Author
|
based on above comment with data on the |
Contributor
Author
|
Okay well I made the effort but I don't think there is general interest in landing this lint at this time. . . closing PR. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.