Skip to content
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

Don't remove generics unless certain they come from a statically linked lib. #65781

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1433,6 +1433,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"tell the linker to strip debuginfo when building without debuginfo enabled."),
share_generics: Option<bool> = (None, parse_opt_bool, [TRACKED],
"make the current crate share its generic instantiations"),
share_generics_for_unknown_linkage: bool = (false, parse_bool, [TRACKED],
"optimistically assume upstream crates with unknown linkage can share generics"),
chalk: bool = (false, parse_bool, [TRACKED],
"enable the experimental Chalk-based trait solving engine"),
no_parallel_llvm: bool = (false, parse_bool, [UNTRACKED],
Expand Down
41 changes: 39 additions & 2 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,45 @@ provide! { <'tcx> tcx, def_id, other, cdata,
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(l @ Linkage::NotLinked) |
Some(l @ Linkage::IncludedFromDylib) |
Some(l @ Linkage::Dynamic) => {
log::debug!("compiling `{B}` def_id: {D:?} cannot reuse generics \
from crate `{A}` because it has linkage {L:?}",
A=tcx.crate_name(def_id.krate),
B=tcx.crate_name(LOCAL_CRATE),
D=def_id,
L=l);
true
}

Some(Linkage::Static) => {
log::debug!("compiling `{B}` def_id: {D:?} attempt to reuse generics \
from statically-linked crate `{A}`",
A=tcx.crate_name(def_id.krate),
B=tcx.crate_name(LOCAL_CRATE),
D=def_id);
false
}

None => {
// rust-lang/rust#65890: A has unknown dependency format when compiling B.
if tcx.sess.opts.debugging_opts.share_generics_for_unknown_linkage {
log::info!("`{B}`, crate-type {O:?}: attempt to reuse generics \
from crate `{A}` despite its unknown dependency format",
A=tcx.crate_name(def_id.krate),
B=tcx.crate_name(LOCAL_CRATE),
O=_ty);
false
} else {
log::info!("`{B}`, crate-type {O:?}: cannot reuse generics \
from crate `{A}` because it has unknown dependency format",
A=tcx.crate_name(def_id.krate),
B=tcx.crate_name(LOCAL_CRATE),
O=_ty);
true
}
}
}
});
if remove_generics {
Expand Down
3 changes: 2 additions & 1 deletion src/test/codegen-units/partitioning/shared-generics.rs
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@pnkfelix pnkfelix Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#![crate_type="rlib"]

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// compile-flags: -C debuginfo=2 --crate-type=rlib

extern crate reexport_obj;
use reexport_obj::Object;

pub fn another_dyn_debug() {
let ref u = 1_u32;
let _d = &u as &dyn crate::Object;
_d.method()
}
13 changes: 13 additions & 0 deletions src/test/ui/cross-crate/issue-64872/auxiliary/def_obj.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// compile-flags: -C debuginfo=2 --crate-type=rlib

pub trait Object { fn method(&self) { } }

impl Object for u32 { }
impl Object for () { }
impl<T> Object for &T { }

pub fn unused() {
let ref u = 0_u32;
let _d = &u as &dyn crate::Object;
_d.method()
}
5 changes: 5 additions & 0 deletions src/test/ui/cross-crate/issue-64872/auxiliary/reexport_obj.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// compile-flags: -C debuginfo=2 -C prefer-dynamic --crate-type=dylib

extern crate def_obj;

pub use def_obj::Object;
12 changes: 12 additions & 0 deletions src/test/ui/cross-crate/issue-64872/chain-of-rlibs-and-dylibs.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// note that these aux-build directives must be in this order: the later crates
// depend on the earlier ones.

// aux-build:def_obj.rs
// aux-build:rexport_obj.rs
// aux-build:another_vtable_for_obj.rs

extern crate another_vtable_for_obj;

pub fn main() {
another_vtable_for_obj::another_dyn_debug();
}