Don't remove generics unless certain they come from a statically linked lib. #65781
Conversation
|
r? @zackmdavis (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors rollup=never |
|
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 |
|
Thanks for tracking this down @pnkfelix! The fix here looks good to me, but I'm wary of the test since it's relies on so many unstable features. We've historically had a lot of problems keeping tests like this running over time. I think though the test can be simplified, I played around with it for a bit and this gist shows the files/script necessary to reproduce the bug today which I presume is fixed after this PR. I think though the failing test may be legitimate because it's not actually sharing generics with the upstream crate. I believe that means that |
pnkfelix
force-pushed
the
issue-64872-linking-libtest
branch
from
d495d39 to
7ed13ce
Compare
|
@alexcrichton when you write
then that means that the fix as written isn't actually 100% good, right? Because it has injected a failure in the test suite, and you believe that the test as written is solid (i.e., is not relying on ... lets say "underspecified" behavior in the compiler and/or platform)? (And thus, I interpret your feedback as a request for me to update the PR to account for the interpretation of |
|
Oh, and also: Thank you for the narrowed test case, @alexcrichton ! 🎉 I had spent some time trying to make something similar (namely, something that did not rely on So I'm glad that you have proven this to not be the case (though this also means that this bug potentially affects a larger audience than we previously knew about) |
pnkfelix
force-pushed
the
issue-64872-linking-libtest
branch
from
7ed13ce to
767f295
Compare
|
Actually, the more I poke at this, the more it seems like @alexcrichton 's reduced test case may be exposing a new variant of this bug that isn't fixed by my PR ... 😢 |
Specifically, it seems to me like we are falling into a case that looks like this (I renamed the crates in alex's reduced test case to In particular, it seems like we are falling into a case where the dep-format says you have an upstream crate of with this dependency format: But look at the very first element of the tuple, aka the (ignored!) rust/src/librustc_metadata/cstore_impl.rs Lines 243 to 249 in 9285d40 It is a (What does the first element of the tuple, the I need to experiment more. |
I think the crate type for the current crate: |
Heh, that's "shame on me" for writing half the comment approving this, then seeing the CI failure, but then failing to back and rewrite the original words of the comment now that the failure was noticed.
I believe it means that for the crate type output |
ah. Now I see: this is how we represent the fact that the current crate may have multiple output types. So the length of |
or maybe its a variant that is not fixed by the new version of this PR that attempts to incorporate @alexcrichton's feedback (by handling |
|
In any case, my current inclination is to push forward on landing, and nominating for beta-backport, the PR in its original non-nuanced form, where all it did was a diff that was essentially this: @@ -243,17 +245,32 @@ provide! { <'tcx> tcx, def_id, other, cdata,
// from this crate.
let formats = tcx.dependency_formats(LOCAL_CRATE);
let remove_generics = formats.iter().any(|(_ty, list)| {
match list.get(def_id.krate.as_usize() - 1) {
- Some(Linkage::IncludedFromDylib) | Some(Linkage::Dynamic) => true,
- _ => false,
+ Some(Linkage::Static) => false,
+ _ => true,
}
});For the short-term, I believe that will address the end-user visible semantic linking bug here. And for the long-term, I would continue to push on the more nuanced form that fixes the aformentioned test regression that represents a failure to optimize. @alexcrichton do you have objections to that plan? |
|
Okay. I have now absolutely confirmed that the link failure still persists when I use the "nuanced" version of the change (i.e. the one where a The reason this is happening is perhaps subtle. In the test case, we have a chain of crate dependencies that looks like this: where both The problem as I currently understand it is:
In short: the current crate output type is not enough to make a decision here. The problem may be witnessed at the time when we compile postscript: And now that I've actually typed the horrifying filename |
This is the point in the code where we decide to return an empty list here: rust/src/librustc_metadata/dependency_format.rs Lines 105 to 108 in 2dd4e73
|
pnkfelix
force-pushed
the
issue-64872-linking-libtest
branch
from
767f295 to
fdd883a
Compare
|
(this is now ready for re-review) |
pnkfelix
added
beta-nominated
|
Also, I should have cc'ed @michaelwoerister on this from the start. Whoops! |
| @@ -1,6 +1,7 @@ | |||
| // ignore-tidy-linelength | |||
| // no-prefer-dynamic | |||
| // compile-flags:-Zprint-mono-items=eager -Zshare-generics=yes -Zincremental=tmp/partitioning-tests/shared-generics-exe | |||
| // FIXME: Use `-Z share-generics-for-unknown-linkage` to workaround rust-lang/rust#65890 | |||
| // compile-flags:-Zprint-mono-items=eager -Zshare-generics=yes -Zincremental=tmp/partitioning-tests/shared-generics-exe -Z share-generics-for-unknown-linkage | |||
There was a problem hiding this comment.
I'm a little worried that the new -Z flag is needed here, do you know what effect that'll have on general crate compilations? I think we still want to be sure that a debug mode compile shares as many generics as it possibly can, and release mode should be fine both before/after this change since no sharing happens.
Basically do general cargo build scenarios still share generics amongst all of their rlib crates?
There was a problem hiding this comment.
I'm pretty sure they do not, since this test itself serves as evidence of a case where we see less sharing. I take that to be further work that would need to be addressed in say #65890
they will be statically linked. In the particular case of rust-lang#64872, we get burned by an attempt to link to a monomorphization in libcore.rlib that ends up not being exported by libstd.dylib. (For that scenario, the attempt to lookup the linkage returns `None`, which led us terribly astray.) (That scenario is encoded in a test in a follow-on commit.) Also added some `log::info` output for the `None` case, because I want easy access in all of our builds to inspect what goes on in this logic. In response to review feedback, I had tried to revise the fix to be more nuanced in handling of `None` (i.e., the case which I've previously asserted to be "unknown linkage"). Alex's take on matters is that we should use the output crate type to determine what format the dependency here will have. However, further testing showed that approach to be flawed. So I added debugflag: `-Z share-generics-for-unknown-linkage`, to make it easy to recover the earlier behavior (which I nonetheless assert to be buggy in general), and use that flag to keep one of our codegen tests working.
(Many thanks to alex for 1. making this even smaller than what I had originally minimized, and 2. pointing out that there is precedent for having ui tests with crate dependency chains of length > 2, thus allowing me to avoid encoding this as a run-make test.)
pnkfelix
force-pushed
the
issue-64872-linking-libtest
branch
from
fdd883a to
806c0e5
Compare
|
nominating for discussion at the T-compiler meeting; I feel like I need to get more inputs in order to weigh the trade-offs here. |
|
Oh sorry I forgot to review those comments, thanks for writing this out though @pnkfelix! I want to write down some general thoughts about this regression before I get to the specifics later too. Anything with the The original bug report was originally spawned from rust-lang/wg-cargo-std-aware#32 which is what started this whole chain of events. The original feature for std-aware Cargo, however, is no longer necessary since rust-lang/cargo#7353 landed, since Cargo no longer ever builds a What we ended up doing was trading one bug report (#64319) for another (#64872). Both of these bug reports are basically " So all that's to say that if the fix for #64872 regresses performance or functionality with every day use of Cargo/rustc, I do not think we should merge a fix and we should revert #64324 because I don't think anyone cares about #64319 being fixed at this time. Ok so now on to the specifics! This is honestly so complicated that I think regardless of everything else here I'd probably just revert my original PR. I think you're right that maybe this is a case that necessitates calculating linkage types for rlibs. Alternatively the fix in #64324 needs to be completely different, actually exporting generic instantiations from the |
|
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 |
|
based on @alexcrichton 's comment, I am going to switch gears and focus on reverting PR #64324 (or a subset thereof; much of the changes are purely internal and can remain, to reduce churn) |
pnkfelix
removed
I-nominated
beta-nominated
So, @alexcrichton : I am totally board with a plan to focus on a revert of PR #64324 and have started working on it. However, I immediately noticed that the entirety of the code that we are discussing in this PR, namely the logic here: rust/src/librustc_metadata/cstore_impl.rs Lines 244 to 248 in 253fc0e ... was entirely added in PR #64324 , specifically here So ... on the one hand you are worried about the potential negative impact of landing this PR (#65781), but doesn't it seem like reverting PR #64324 would likely have at least the same negative impact? (Or maybe much of the effect of |
Well, it seems like this hypothesis was wrong; for some reason, PR #66018 does not suffer from the same regression to the |
|
closing in favor of PR #66018 |
…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
We can only link to monomorphizations from upstream crates if we know their linkage (and that linkage is static).
In the particular case of #64872, we get burned by an attempt to link to a monomorphization in libcore.rlib that ends up not being exported by libstd.dylib. (For that scenario, the attempt to lookup the linkage returns
None, which led us terribly astray.)Fix #64872