rustc: Fix mixing crates with different share_generics
#64324
Merged
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
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
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 |
There was a problem hiding this comment.
Thanks, @alexcrichton! The fix looks good. r=me with the comments addressed.
5 tasks
alexcrichton
force-pushed
the
share-generics-again
branch
from
c0547bb
to
0ca8430
Compare
|
So it turns out the previous version of this PR was also incorrect (hence the test failure on CI). The problem was that it was testing whether the defining crate was in @michaelwoerister this actually I think poses an interesting question for everything in the standard library because the standard library is basically never built in Still running the full test suite locally, but @michaelwoerister if you wouldn't mind giving another look at this I'm a bit hesitant to carry over the r+! |
alexcrichton
force-pushed
the
share-generics-again
branch
from
0ca8430
to
189805b
Compare
|
Alright now I'm actually just hurting myself in my own confusion. Standard library generics are indeed shared today, my comment above was actually about a previous version of my patch. I'm basically throwing logic at the wall right now and seeing what sticks, and this definitely isn't great. Taking a step back it seems like there's logically some duplication between this function and this deduction. I'm confusing myself now as to why symbol names need to account for Thinking about this more I think that this is sort of a fundamental problem with how the linker is requesting symbol names today. For some crate types (like dylibs) the compiler needs to generate an exhaustive symbol list to export, and for dylibs this means exported upstream symbols as well. The problem here is that we're dynamically calculating the actual name of the symbol (via the
The problem here is that these two contexts are subtly different around how they're supposed to calculate the symbol name. The main piece of this boils down to what crate instantiated the symbol, where in the case of linked symbols we've always got it but today it's always inferred and is dependent on the Note that this patch is green on CI, but fails on Windows, because for whatever reason the test case I have passes on Linux but not on macOS or Windows. |
|
I'm sorry this is giving you trouble, @alexcrichton. I remember fighting with the logic quite a bit during the initial implementation. OK, so the problem is that
Do we know if both symbols show up in the export list? Or just the upstream or downstream one? This is another problem that only exists because of Rust dylibs |
|
Somewhat related: We might want to give using |
|
One thing that might be happening:
I don't know if linkers actually work that way. But archives are treated more lazily than object files, right? |
|
OK, I was able to reproduce the problem on Windows and here is my analysis (which I think is the same as yours, @alexcrichton):
So in essence, there are two symbol names for the same instance, both of which the compiler needs in different contexts, but the |
|
Oh man believe me I'd be the first person to throw my weight behind any "remove rust dylibs once and for all" proposal, they do indeed seem to only cause endless effort and pain for not a lot of gain! In any case, putting that aside, sounds like we're definitely on the same page. Agreed with everything you've said :) I was thinking a bit about how to best solve this yesterday, but I wasn't really able to come up with something great. I'll look to see if there's a low-impact second query argument we could add or maybe a low-impact way to add a second query. |
|
👍 |
alexcrichton
force-pushed
the
share-generics-again
branch
from
189805b
to
78ede6d
Compare
|
Ok I've edited the PR description and I've also pushed up a new commit, and I believe that this should address the issue. |
|
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 |
|
... or this strategy completely breaks sharing of generics in the first place. Still working! |
alexcrichton
force-pushed
the
share-generics-again
branch
from
78ede6d
to
aae90a8
Compare
|
Ok nth time's the charm, I think this one actually works |
|
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 |
alexcrichton
force-pushed
the
share-generics-again
branch
from
aae90a8
to
c7da417
Compare
|
Am I reading this right that the changes here store all symbol names as strings in metadata? That must be a lot of data. Let's do a perf run. @bors try |
|
Thank you, @alexcrichton! This looks good. Let's do one last perf run, so there are no surprises. @bors try @rust-timer queue |
|
Awaiting bors try build completion |
bors
added a commit
that referenced
this pull request
Sep 24, 2019
rustc: Fix mixing crates with different `share_generics` This commit addresses #64319 by removing the `dylib` crate type from the list of crate type that exports generic symbols. The bug in #64319 arises because a `dylib` crate type was trying to export a symbol in an uptream crate but it miscalculated the symbol name of the uptream symbol. This isn't really necessary, though, since `dylib` crates aren't that heavily used, so we can just conservatively say that the `dylib` crate type never exports generic symbols, forcibly removing them from the exported symbol lists if were to otherwise find them. The fix here happens in two places: * First is in the `local_crate_exports_generics` method, indicating that it's now `false` for the `Dylib` crate type. Only rlibs actually export generics at this point. * Next is when we load exported symbols from upstream crate. If, for our compilation session, the crate may be included from a dynamic library, then its generic symbols are removed. When the crate was linked into a dynamic library its symbols weren't exported, so we can't consider them a candidate to link against. Overally this should avoid situations where we incorrectly calculate the upstream symbol names in the face of differnet `share_generics` options, ultimately... Closes #64319
|
☀️ Try build successful - checks-azure |
|
Queued b4ba2a3 with parent 66bf391, future comparison URL. |
|
Finished benchmarking try commit b4ba2a3, comparison URL. |
|
Alright, looks like performance isn't affected. @bors r+ |
|
📌 Commit f00c634 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit f00c634 with merge fe08eaaa1e820ca9acf191c76dd88a0a1d3ced74... |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Sep 25, 2019
…r=michaelwoerister rustc: Fix mixing crates with different `share_generics` This commit addresses rust-lang#64319 by removing the `dylib` crate type from the list of crate type that exports generic symbols. The bug in rust-lang#64319 arises because a `dylib` crate type was trying to export a symbol in an uptream crate but it miscalculated the symbol name of the uptream symbol. This isn't really necessary, though, since `dylib` crates aren't that heavily used, so we can just conservatively say that the `dylib` crate type never exports generic symbols, forcibly removing them from the exported symbol lists if were to otherwise find them. The fix here happens in two places: * First is in the `local_crate_exports_generics` method, indicating that it's now `false` for the `Dylib` crate type. Only rlibs actually export generics at this point. * Next is when we load exported symbols from upstream crate. If, for our compilation session, the crate may be included from a dynamic library, then its generic symbols are removed. When the crate was linked into a dynamic library its symbols weren't exported, so we can't consider them a candidate to link against. Overally this should avoid situations where we incorrectly calculate the upstream symbol names in the face of differnet `share_generics` options, ultimately... Closes rust-lang#64319
|
@bors retry rolled up. |
bors
added a commit
that referenced
this pull request
Sep 25, 2019
Rollup of 7 pull requests Successful merges: - #64324 (rustc: Fix mixing crates with different `share_generics`) - #64428 (Error explanation e0524) - #64481 (A more explanatory thread local storage panic message) - #64599 (Rustdoc render async function re-export) - #64743 (Update cargo) - #64746 (Remove blanket silencing of "type annotation needed" errors) - #64753 (Don't emit explain with json short messages.) Failed merges: r? @ghost
|
🎉 |
pnkfelix
added a commit
to pnkfelix/rust
that referenced
this pull request
Nov 1, 2019
…rics export). Includes the anticipated fallout to run-make-fulldeps test suite from this change. (We need to reopen issue 64319 as part of landing this.)
tmandry
added a commit
to tmandry/rust
that referenced
this pull request
Nov 1, 2019
…r=alexcrichton Revert PR 64324: dylibs export generics again (for now) As discussed on PR rust-lang#65781, this is a targeted attempt to undo the main semantic change from PR rust-lang#64324, by putting `dylib` back in the set of crate types that export generic symbols. The main reason to do this is that PR rust-lang#64324 had unanticipated side-effects that caused bugs like rust-lang#64872, and in the opinion of @alexcrichton and myself, the impact of rust-lang#64872 is worse than rust-lang#64319. In other words, it is better for us, in the short term, to reopen rust-lang#64319 as currently unfixed for now than to introduce new bugs like rust-lang#64872. Fix rust-lang#64872 Reopen rust-lang#64319
2 tasks
michaelwoerister
added a commit
to michaelwoerister/rust
that referenced
this pull request
Jan 20, 2020
The regression test is originally from rust-lang#64324 but was removed again after the fix in there turned out to break other things.
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 commit addresses #64319 by removing the
dylibcrate type from thelist of crate type that exports generic symbols. The bug in #64319
arises because a
dylibcrate type was trying to export a symbol in anuptream crate but it miscalculated the symbol name of the uptream
symbol. This isn't really necessary, though, since
dylibcrates aren'tthat heavily used, so we can just conservatively say that the
dylibcrate type never exports generic symbols, forcibly removing them from
the exported symbol lists if were to otherwise find them.
The fix here happens in two places:
First is in the
local_crate_exports_genericsmethod, indicating thatit's now
falsefor theDylibcrate type. Only rlibs actuallyexport generics at this point.
Next is when we load exported symbols from upstream crate. If, for our
compilation session, the crate may be included from a dynamic library,
then its generic symbols are removed. When the crate was linked into a
dynamic library its symbols weren't exported, so we can't consider
them a candidate to link against.
Overally this should avoid situations where we incorrectly calculate the
upstream symbol names in the face of differnet
share_genericsoptions,ultimately...
Closes #64319