From 8228ccf6d53d349ae9bf79244afe58de8b65f0ec Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 25 Oct 2019 00:42:51 +0200 Subject: [PATCH 1/2] We can only link to monomorphizations from upstream crates if we know they will be statically linked. In the particular case of rust-lang/rust#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. --- src/librustc/session/config.rs | 2 + src/librustc_metadata/cstore_impl.rs | 41 ++++++++++++++++++- .../partitioning/shared-generics.rs | 3 +- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 33b9ddaa62230..628e268030fb6 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1433,6 +1433,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "tell the linker to strip debuginfo when building without debuginfo enabled."), share_generics: Option = (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], diff --git a/src/librustc_metadata/cstore_impl.rs b/src/librustc_metadata/cstore_impl.rs index d942a19194a14..ba1a31de78df8 100644 --- a/src/librustc_metadata/cstore_impl.rs +++ b/src/librustc_metadata/cstore_impl.rs @@ -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 { diff --git a/src/test/codegen-units/partitioning/shared-generics.rs b/src/test/codegen-units/partitioning/shared-generics.rs index 58e485be00321..344cbf2c4826a 100644 --- a/src/test/codegen-units/partitioning/shared-generics.rs +++ b/src/test/codegen-units/partitioning/shared-generics.rs @@ -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 #![crate_type="rlib"] From 806c0e55731d2ad8da92545a976139f4ccbc85ef Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 30 Oct 2019 16:32:40 +0100 Subject: [PATCH 2/2] ui test formulation of regression test for issue 64872. (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.) --- .../issue-64872/auxiliary/another_vtable_for_obj.rs | 10 ++++++++++ .../ui/cross-crate/issue-64872/auxiliary/def_obj.rs | 13 +++++++++++++ .../issue-64872/auxiliary/reexport_obj.rs | 5 +++++ .../issue-64872/chain-of-rlibs-and-dylibs.rs | 12 ++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 src/test/ui/cross-crate/issue-64872/auxiliary/another_vtable_for_obj.rs create mode 100644 src/test/ui/cross-crate/issue-64872/auxiliary/def_obj.rs create mode 100644 src/test/ui/cross-crate/issue-64872/auxiliary/reexport_obj.rs create mode 100644 src/test/ui/cross-crate/issue-64872/chain-of-rlibs-and-dylibs.rs diff --git a/src/test/ui/cross-crate/issue-64872/auxiliary/another_vtable_for_obj.rs b/src/test/ui/cross-crate/issue-64872/auxiliary/another_vtable_for_obj.rs new file mode 100644 index 0000000000000..a28eb8265b637 --- /dev/null +++ b/src/test/ui/cross-crate/issue-64872/auxiliary/another_vtable_for_obj.rs @@ -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() +} diff --git a/src/test/ui/cross-crate/issue-64872/auxiliary/def_obj.rs b/src/test/ui/cross-crate/issue-64872/auxiliary/def_obj.rs new file mode 100644 index 0000000000000..8854f4e2772d8 --- /dev/null +++ b/src/test/ui/cross-crate/issue-64872/auxiliary/def_obj.rs @@ -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 Object for &T { } + +pub fn unused() { + let ref u = 0_u32; + let _d = &u as &dyn crate::Object; + _d.method() +} diff --git a/src/test/ui/cross-crate/issue-64872/auxiliary/reexport_obj.rs b/src/test/ui/cross-crate/issue-64872/auxiliary/reexport_obj.rs new file mode 100644 index 0000000000000..85fe3a9aeca9e --- /dev/null +++ b/src/test/ui/cross-crate/issue-64872/auxiliary/reexport_obj.rs @@ -0,0 +1,5 @@ +// compile-flags: -C debuginfo=2 -C prefer-dynamic --crate-type=dylib + +extern crate def_obj; + +pub use def_obj::Object; diff --git a/src/test/ui/cross-crate/issue-64872/chain-of-rlibs-and-dylibs.rs b/src/test/ui/cross-crate/issue-64872/chain-of-rlibs-and-dylibs.rs new file mode 100644 index 0000000000000..ef0032eb62596 --- /dev/null +++ b/src/test/ui/cross-crate/issue-64872/chain-of-rlibs-and-dylibs.rs @@ -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(); +}