Skip to content

Commit

Permalink
Rollup merge of #66018 - pnkfelix:issue-64872-revert-64324, r=alexcri…
Browse files Browse the repository at this point in the history
…chton

Revert PR 64324: dylibs export generics again (for now)

As discussed on PR #65781, this is a targeted attempt to undo the main semantic change from PR #64324, by putting `dylib` back in the set of crate types that export generic symbols.

The main reason to do this is that PR #64324 had unanticipated side-effects that caused bugs like #64872, and in the opinion of @alexcrichton and myself, the impact of #64872 is worse than #64319.

In other words, it is better for us, in the short term, to reopen #64319 as currently unfixed for now than to introduce new bugs like #64872.

Fix #64872

Reopen #64319
  • Loading branch information
tmandry committed Nov 1, 2019
2 parents 81505e7 + 6457914 commit d6e35d1
Show file tree
Hide file tree
Showing 12 changed files with 79 additions and 92 deletions.
8 changes: 7 additions & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1514,8 +1514,14 @@ impl<'tcx> TyCtxt<'tcx> {
CrateType::Executable |
CrateType::Staticlib |
CrateType::ProcMacro |
CrateType::Dylib |
CrateType::Cdylib => false,

// FIXME rust-lang/rust#64319, rust-lang/rust#64872:
// We want to block export of generics from dylibs,
// but we must fix rust-lang/rust#65890 before we can
// do that robustly.
CrateType::Dylib => true,

CrateType::Rlib => true,
}
})
Expand Down
19 changes: 4 additions & 15 deletions src/librustc_codegen_ssa/back/linker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use rustc::middle::dependency_format::Linkage;
use rustc::session::Session;
use rustc::session::config::{self, CrateType, OptLevel, DebugInfo,
LinkerPluginLto, Lto};
use rustc::middle::exported_symbols::ExportedSymbol;
use rustc::ty::TyCtxt;
use rustc_target::spec::{LinkerFlavor, LldFlavor};
use rustc_serialize::{json, Encoder};
Expand Down Expand Up @@ -1112,20 +1111,10 @@ fn exported_symbols(tcx: TyCtxt<'_>, crate_type: CrateType) -> Vec<String> {
continue;
}

// Do not export generic symbols from upstream crates in linked
// artifact (notably the `dylib` crate type). The main reason
// for this is that `symbol_name` is actually wrong for generic
// symbols because it guesses the name we'd give them locally
// rather than the name it has upstream (depending on
// `share_generics` settings and such).
//
// To fix that issue we just say that linked artifacts, aka
// `dylib`s, never export generic symbols and they aren't
// available to downstream crates. (the not available part is
// handled elsewhere).
if let ExportedSymbol::Generic(..) = symbol {
continue;
}
// FIXME rust-lang/rust#64319, rust-lang/rust#64872:
// We want to block export of generics from dylibs,
// but we must fix rust-lang/rust#65890 before we can
// do that robustly.

symbols.push(symbol.symbol_name(tcx).to_string());
}
Expand Down
26 changes: 5 additions & 21 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ use rustc::ty::query::QueryConfig;
use rustc::middle::cstore::{CrateSource, CrateStore, DepKind, EncodedMetadata, NativeLibraryKind};
use rustc::middle::exported_symbols::ExportedSymbol;
use rustc::middle::stability::DeprecationEntry;
use rustc::middle::dependency_format::Linkage;
use rustc::hir::def;
use rustc::hir;
use rustc::session::{CrateDisambiguator, Session};
Expand Down Expand Up @@ -235,26 +234,11 @@ provide! { <'tcx> tcx, def_id, other, cdata,
used_crate_source => { Lrc::new(cdata.source.clone()) }

exported_symbols => {
let mut syms = cdata.exported_symbols(tcx);

// When linked into a dylib crates don't export their generic symbols,
// so if that's happening then we can't load upstream monomorphizations
// 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,
}
});
if remove_generics {
syms.retain(|(sym, _threshold)| {
match sym {
ExportedSymbol::Generic(..) => false,
_ => return true,
}
});
}
let syms = cdata.exported_symbols(tcx);

// FIXME rust-lang/rust#64319, rust-lang/rust#64872: We want
// to block export of generics from dylibs, but we must fix
// rust-lang/rust#65890 before we can do that robustly.

Arc::new(syms)
}
Expand Down
39 changes: 0 additions & 39 deletions src/test/run-make-fulldeps/issue-64319/Makefile

This file was deleted.

5 changes: 0 additions & 5 deletions src/test/run-make-fulldeps/issue-64319/bar.rs

This file was deleted.

9 changes: 0 additions & 9 deletions src/test/run-make-fulldeps/issue-64319/foo.rs

This file was deleted.

4 changes: 2 additions & 2 deletions src/test/run-make-fulldeps/symbol-visibility/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,12 @@ all:
# Check that a Rust dylib exports its monomorphic functions, including generics this time
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_c_function_from_rust_dylib)" -eq "1" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_rust_function_from_rust_dylib)" -eq "1" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_generic_function_from_rust_dylib)" -eq "0" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_generic_function_from_rust_dylib)" -eq "1" ]

# Check that a Rust dylib exports the monomorphic functions from its dependencies
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_c_function_from_rlib)" -eq "1" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_rust_function_from_rlib)" -eq "1" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_generic_function_from_rlib)" -eq "0" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -c public_generic_function_from_rlib)" -eq "1" ]

# Check that an executable does not export any dynamic symbols
[ "$$($(NM) $(TMPDIR)/$(EXE_NAME) | grep -c public_c_function_from_rlib)" -eq "0" ]
Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/cross-crate/issue-64872/auxiliary/a_def_obj.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// compile-flags: -C debuginfo=2

// no-prefer-dynamic
#![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()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// compile-flags: -C debuginfo=2 -C prefer-dynamic

#![crate_type="dylib"]

extern crate a_def_obj;

pub use a_def_obj::Object;
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// no-prefer-dynamic
// compile-flags: -C debuginfo=2
#![crate_type="rlib"]

extern crate b_reexport_obj;
use b_reexport_obj::Object;

pub fn another_dyn_debug() {
let ref u = 1_u32;
let _d = &u as &dyn crate::Object;
_d.method()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// compile-flags: -C debuginfo=2 -C prefer-dynamic

#![crate_type="rlib"]

extern crate c_another_vtable_for_obj;

pub fn chain() {
c_another_vtable_for_obj::another_dyn_debug();
}
17 changes: 17 additions & 0 deletions src/test/ui/cross-crate/issue-64872/issue-64872.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// run-pass

// note that these aux-build directives must be in this order: the
// later crates depend on the earlier ones. (The particular bug that
// is being exercised here used to exhibit itself during the build of
// `chain_of_rlibs_and_dylibs.dylib`)

// aux-build:a_def_obj.rs
// aux-build:b_reexport_obj.rs
// aux-build:c_another_vtable_for_obj.rs
// aux-build:d_chain_of_rlibs_and_dylibs.rs

extern crate d_chain_of_rlibs_and_dylibs;

pub fn main() {
d_chain_of_rlibs_and_dylibs::chain();
}

0 comments on commit d6e35d1

Please sign in to comment.