Skip to content

Commit

Permalink
Auto merge of #68277 - michaelwoerister:re-export-dylib-instances, r=…
Browse files Browse the repository at this point in the history
…alexcrichton

Make sure that all upstream generics get re-exported from Rust dylibs.

This PR contains a fix for #67276. Rust dylibs would not re-export all generic instances when compiling with `-Zshare-generics=on` (=default for debug builds) which could lead to situations where the compiler expected certain generic instances to be available but then the linker would not find them.

### TODO
- [x] Write a regression test based on the description [here](#67276 (comment)).
- [x] Find out if this also fixes other issues related to #64319.

r? @alexcrichton ~~(once the TODOs are done)~~
cc @pnkfelix @alexkornitzer
  • Loading branch information
bors committed Jan 20, 2020
2 parents 900811e + 0a9bcb0 commit 66b0c97
Show file tree
Hide file tree
Showing 16 changed files with 267 additions and 48 deletions.
21 changes: 18 additions & 3 deletions src/librustc/middle/exported_symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ pub enum ExportedSymbol<'tcx> {
}

impl<'tcx> ExportedSymbol<'tcx> {
pub fn symbol_name(&self, tcx: TyCtxt<'tcx>) -> ty::SymbolName {
/// This is the symbol name of an instance if it is instantiated in the
/// local crate.
pub fn symbol_name_for_local_instance(&self, tcx: TyCtxt<'tcx>) -> ty::SymbolName {
match *self {
ExportedSymbol::NonGeneric(def_id) => tcx.symbol_name(ty::Instance::mono(tcx, def_id)),
ExportedSymbol::Generic(def_id, substs) => {
Expand All @@ -50,9 +52,22 @@ impl<'tcx> ExportedSymbol<'tcx> {
}
ExportedSymbol::Generic(..) | ExportedSymbol::NoDefId(_) => cmp::Ordering::Less,
},
ExportedSymbol::Generic(..) => match *other {
ExportedSymbol::Generic(self_def_id, self_substs) => match *other {
ExportedSymbol::NonGeneric(_) => cmp::Ordering::Greater,
ExportedSymbol::Generic(..) => self.symbol_name(tcx).cmp(&other.symbol_name(tcx)),
ExportedSymbol::Generic(other_def_id, other_substs) => {
// We compare the symbol names because they are cached as query
// results which makes them relatively cheap to access repeatedly.
//
// It might be even faster to build a local cache of stable IDs
// for sorting. Exported symbols are really only sorted once
// in order to make the `exported_symbols` query result stable.
let self_symbol_name =
tcx.symbol_name(ty::Instance::new(self_def_id, self_substs));
let other_symbol_name =
tcx.symbol_name(ty::Instance::new(other_def_id, other_substs));

self_symbol_name.cmp(&other_symbol_name)
}
ExportedSymbol::NoDefId(_) => cmp::Ordering::Less,
},
ExportedSymbol::NoDefId(self_symbol_name) => match *other {
Expand Down
8 changes: 8 additions & 0 deletions src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,9 @@ rustc_queries! {
desc { |tcx| "generating MIR shim for `{}`", tcx.def_path_str(key.def_id()) }
}

/// The `symbol_name` query provides the symbol name for calling a
/// given instance from the local crate. In particular, it will also
/// look up the correct symbol name of instances from upstream crates.
query symbol_name(key: ty::Instance<'tcx>) -> ty::SymbolName {
no_force
desc { "computing the symbol for `{}`", key }
Expand Down Expand Up @@ -971,6 +974,11 @@ rustc_queries! {
}

Linking {
/// The list of symbols exported from the given crate.
///
/// - All names contained in `exported_symbols(cnum)` are guaranteed to
/// correspond to a publicly visible symbol in `cnum` machine code.
/// - The `exported_symbols` sets of different crates do not intersect.
query exported_symbols(_: CrateNum)
-> Arc<Vec<(ExportedSymbol<'tcx>, SymbolExportLevel)>> {
desc { "exported_symbols" }
Expand Down
13 changes: 6 additions & 7 deletions src/librustc_codegen_ssa/back/linker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,11 @@ fn exported_symbols(tcx: TyCtxt<'_>, crate_type: CrateType) -> Vec<String> {
let export_threshold = symbol_export::crates_export_threshold(&[crate_type]);
for &(symbol, level) in tcx.exported_symbols(LOCAL_CRATE).iter() {
if level.is_below_threshold(export_threshold) {
symbols.push(symbol.symbol_name(tcx).to_string());
symbols.push(symbol_export::symbol_name_for_instance_in_crate(
tcx,
symbol,
LOCAL_CRATE,
));
}
}

Expand All @@ -1124,12 +1128,7 @@ fn exported_symbols(tcx: TyCtxt<'_>, crate_type: CrateType) -> Vec<String> {
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());
symbols.push(symbol_export::symbol_name_for_instance_in_crate(tcx, symbol, cnum));
}
}
}
Expand Down
30 changes: 30 additions & 0 deletions src/librustc_codegen_ssa/back/symbol_export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc::ty::query::Providers;
use rustc::ty::subst::SubstsRef;
use rustc::ty::Instance;
use rustc::ty::{SymbolName, TyCtxt};
use rustc_codegen_utils::symbol_names;
use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::fx::FxHashMap;
use rustc_hir as hir;
Expand Down Expand Up @@ -358,3 +359,32 @@ fn symbol_export_level(tcx: TyCtxt<'_>, sym_def_id: DefId) -> SymbolExportLevel
SymbolExportLevel::Rust
}
}

/// This is the symbol name of the given instance instantiated in a specific crate.
pub fn symbol_name_for_instance_in_crate<'tcx>(
tcx: TyCtxt<'tcx>,
symbol: ExportedSymbol<'tcx>,
instantiating_crate: CrateNum,
) -> String {
// If this is something instantiated in the local crate then we might
// already have cached the name as a query result.
if instantiating_crate == LOCAL_CRATE {
return symbol.symbol_name_for_local_instance(tcx).to_string();
}

// This is something instantiated in an upstream crate, so we have to use
// the slower (because uncached) version of computing the symbol name.
match symbol {
ExportedSymbol::NonGeneric(def_id) => symbol_names::symbol_name_for_instance_in_crate(
tcx,
Instance::mono(tcx, def_id),
instantiating_crate,
),
ExportedSymbol::Generic(def_id, substs) => symbol_names::symbol_name_for_instance_in_crate(
tcx,
Instance::new(def_id, substs),
instantiating_crate,
),
ExportedSymbol::NoDefId(symbol_name) => symbol_name.to_string(),
}
}
4 changes: 2 additions & 2 deletions src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use super::command::Command;
use super::link::{self, get_linker, remove};
use super::linker::LinkerInfo;
use super::lto::{self, SerializedModule};
use super::symbol_export::ExportedSymbols;
use super::symbol_export::{symbol_name_for_instance_in_crate, ExportedSymbols};
use crate::{
CachedModuleCodegen, CodegenResults, CompiledModule, CrateInfo, ModuleCodegen, ModuleKind,
RLIB_BYTECODE_EXTENSION,
Expand Down Expand Up @@ -956,7 +956,7 @@ fn start_executing_work<B: ExtraBackendMethods>(
let symbols = tcx
.exported_symbols(cnum)
.iter()
.map(|&(s, lvl)| (s.symbol_name(tcx).to_string(), lvl))
.map(|&(s, lvl)| (symbol_name_for_instance_in_crate(tcx, s, cnum), lvl))
.collect();
Arc::new(symbols)
};
Expand Down
112 changes: 76 additions & 36 deletions src/librustc_codegen_utils/symbol_names.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,9 @@ use rustc::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc::mir::mono::{InstantiationMode, MonoItem};
use rustc::session::config::SymbolManglingVersion;
use rustc::ty::query::Providers;
use rustc::ty::subst::SubstsRef;
use rustc::ty::{self, Instance, TyCtxt};
use rustc_hir::def_id::LOCAL_CRATE;
use rustc_hir::def_id::{CrateNum, LOCAL_CRATE};
use rustc_hir::Node;

use rustc_span::symbol::Symbol;
Expand All @@ -102,15 +103,70 @@ use log::debug;
mod legacy;
mod v0;

/// This function computes the symbol name for the given `instance` and the
/// given instantiating crate. That is, if you know that instance X is
/// instantiated in crate Y, this is the symbol name this instance would have.
pub fn symbol_name_for_instance_in_crate(
tcx: TyCtxt<'tcx>,
instance: Instance<'tcx>,
instantiating_crate: CrateNum,
) -> String {
compute_symbol_name(tcx, instance, || instantiating_crate)
}

pub fn provide(providers: &mut Providers<'_>) {
*providers = Providers {
symbol_name: |tcx, instance| ty::SymbolName { name: symbol_name(tcx, instance) },
*providers = Providers { symbol_name: symbol_name_provider, ..*providers };
}

..*providers
};
// The `symbol_name` query provides the symbol name for calling a given
// instance from the local crate. In particular, it will also look up the
// correct symbol name of instances from upstream crates.
fn symbol_name_provider(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> ty::SymbolName {
let symbol_name = compute_symbol_name(tcx, instance, || {
// This closure determines the instantiating crate for instances that
// need an instantiating-crate-suffix for their symbol name, in order
// to differentiate between local copies.
//
// For generics we might find re-usable upstream instances. For anything
// else we rely on their being a local copy available.

if is_generic(instance.substs) {
let def_id = instance.def_id();

if !def_id.is_local() && tcx.sess.opts.share_generics() {
// If we are re-using a monomorphization from another crate,
// we have to compute the symbol hash accordingly.
let upstream_monomorphizations = tcx.upstream_monomorphizations_for(def_id);

upstream_monomorphizations
.and_then(|monos| monos.get(&instance.substs).cloned())
// If there is no instance available upstream, there'll be
// one in the current crate.
.unwrap_or(LOCAL_CRATE)
} else {
// For generic functions defined in the current crate, there
// can be no upstream instances. Also, if we don't share
// generics, we'll instantiate a local copy too.
LOCAL_CRATE
}
} else {
// For non-generic things that need to avoid naming conflicts, we
// always instantiate a copy in the local crate.
LOCAL_CRATE
}
});

ty::SymbolName { name: Symbol::intern(&symbol_name) }
}

fn symbol_name(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Symbol {
/// Computes the symbol name for the given instance. This function will call
/// `compute_instantiating_crate` if it needs to factor the instantiating crate
/// into the symbol name.
fn compute_symbol_name(
tcx: TyCtxt<'tcx>,
instance: Instance<'tcx>,
compute_instantiating_crate: impl FnOnce() -> CrateNum,
) -> String {
let def_id = instance.def_id();
let substs = instance.substs;

Expand All @@ -121,11 +177,11 @@ fn symbol_name(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Symbol {
if def_id.is_local() {
if tcx.plugin_registrar_fn(LOCAL_CRATE) == Some(def_id) {
let disambiguator = tcx.sess.local_crate_disambiguator();
return Symbol::intern(&tcx.sess.generate_plugin_registrar_symbol(disambiguator));
return tcx.sess.generate_plugin_registrar_symbol(disambiguator);
}
if tcx.proc_macro_decls_static(LOCAL_CRATE) == Some(def_id) {
let disambiguator = tcx.sess.local_crate_disambiguator();
return Symbol::intern(&tcx.sess.generate_proc_macro_decls_symbol(disambiguator));
return tcx.sess.generate_proc_macro_decls_symbol(disambiguator);
}
}

Expand Down Expand Up @@ -162,29 +218,28 @@ fn symbol_name(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Symbol {
|| !tcx.wasm_import_module_map(def_id.krate).contains_key(&def_id)
{
if let Some(name) = attrs.link_name {
return name;
return name.to_string();
}
return tcx.item_name(def_id);
return tcx.item_name(def_id).to_string();
}
}

if let Some(name) = attrs.export_name {
// Use provided name
return name;
return name.to_string();
}

if attrs.flags.contains(CodegenFnAttrFlags::NO_MANGLE) {
// Don't mangle
return tcx.item_name(def_id);
return tcx.item_name(def_id).to_string();
}

let is_generic = substs.non_erasable_generics().next().is_some();
let avoid_cross_crate_conflicts =
// If this is an instance of a generic function, we also hash in
// the ID of the instantiating crate. This avoids symbol conflicts
// in case the same instances is emitted in two crates of the same
// project.
is_generic ||
is_generic(substs) ||

// If we're dealing with an instance of a function that's inlined from
// another crate but we're marking it as globally shared to our
Expand All @@ -197,25 +252,8 @@ fn symbol_name(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Symbol {
_ => false,
};

let instantiating_crate = if avoid_cross_crate_conflicts {
Some(if is_generic {
if !def_id.is_local() && tcx.sess.opts.share_generics() {
// If we are re-using a monomorphization from another crate,
// we have to compute the symbol hash accordingly.
let upstream_monomorphizations = tcx.upstream_monomorphizations_for(def_id);

upstream_monomorphizations
.and_then(|monos| monos.get(&substs).cloned())
.unwrap_or(LOCAL_CRATE)
} else {
LOCAL_CRATE
}
} else {
LOCAL_CRATE
})
} else {
None
};
let instantiating_crate =
if avoid_cross_crate_conflicts { Some(compute_instantiating_crate()) } else { None };

// Pick the crate responsible for the symbol mangling version, which has to:
// 1. be stable for each instance, whether it's being defined or imported
Expand All @@ -232,10 +270,12 @@ fn symbol_name(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Symbol {
tcx.symbol_mangling_version(mangling_version_crate)
};

let mangled = match mangling_version {
match mangling_version {
SymbolManglingVersion::Legacy => legacy::mangle(tcx, instance, instantiating_crate),
SymbolManglingVersion::V0 => v0::mangle(tcx, instance, instantiating_crate),
};
}
}

Symbol::intern(&mangled)
fn is_generic(substs: SubstsRef<'_>) -> bool {
substs.non_erasable_generics().next().is_some()
}
39 changes: 39 additions & 0 deletions src/test/run-make-fulldeps/issue64319/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
-include ../../run-make-fulldeps/tools.mk

# Different optimization levels imply different values for `-Zshare-generics`,
# so try out a whole bunch of combinations to make sure everything is compatible
all:
# First up, try some defaults
$(RUSTC) --crate-type rlib foo.rs
$(RUSTC) --crate-type dylib bar.rs -C opt-level=3

# Next try mixing up some things explicitly
$(RUSTC) --crate-type rlib foo.rs -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -Z share-generics=no
$(RUSTC) --crate-type rlib foo.rs -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -Z share-generics=yes
$(RUSTC) --crate-type rlib foo.rs -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -Z share-generics=no
$(RUSTC) --crate-type rlib foo.rs -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -Z share-generics=yes

# Now combine a whole bunch of options together
$(RUSTC) --crate-type rlib foo.rs
$(RUSTC) --crate-type dylib bar.rs
$(RUSTC) --crate-type dylib bar.rs -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -C opt-level=1
$(RUSTC) --crate-type dylib bar.rs -C opt-level=1 -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -C opt-level=1 -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -C opt-level=2
$(RUSTC) --crate-type dylib bar.rs -C opt-level=2 -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -C opt-level=2 -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -C opt-level=3
$(RUSTC) --crate-type dylib bar.rs -C opt-level=3 -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -C opt-level=3 -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -C opt-level=s
$(RUSTC) --crate-type dylib bar.rs -C opt-level=s -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -C opt-level=s -Z share-generics=yes
$(RUSTC) --crate-type dylib bar.rs -C opt-level=z
$(RUSTC) --crate-type dylib bar.rs -C opt-level=z -Z share-generics=no
$(RUSTC) --crate-type dylib bar.rs -C opt-level=z -Z share-generics=yes
5 changes: 5 additions & 0 deletions src/test/run-make-fulldeps/issue64319/bar.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
extern crate foo;

pub fn bar() {
foo::foo();
}
9 changes: 9 additions & 0 deletions src/test/run-make-fulldeps/issue64319/foo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
pub fn foo() {
bar::<usize>();
}

pub fn bar<T>() {
baz();
}

fn baz() {}
Loading

0 comments on commit 66b0c97

Please sign in to comment.