From 4dca459e86b85a87e1c5d19c97b8936c842bab05 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Fri, 6 Jan 2017 10:51:37 -0500 Subject: [PATCH 1/5] trans: Disambiguate generic instance symbol names by instantiating crate. Two crates will often instantiate the same generic functions. Since we don't make any attempt to re-use these instances cross-crate, we would run into symbol conflicts for anything with external linkage. In order to avoid this, this commit makes the compiler incorporate the ID of the instantiating crate into the symbol hash. This way equal generic instances will have different symbols names when used in different crates. --- src/librustc_trans/back/symbol_names.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/librustc_trans/back/symbol_names.rs b/src/librustc_trans/back/symbol_names.rs index 938848054fee2..8f8c48a06b2c0 100644 --- a/src/librustc_trans/back/symbol_names.rs +++ b/src/librustc_trans/back/symbol_names.rs @@ -152,6 +152,15 @@ fn get_symbol_hash<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>, assert!(!substs.has_erasable_regions()); assert!(!substs.needs_subst()); substs.visit_with(&mut hasher); + + // 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. + if substs.types().next().is_some() { + hasher.hash(scx.tcx().crate_name.as_str()); + hasher.hash(scx.sess().local_crate_disambiguator().as_str()); + } } }); From 02c7b117dadcfea4416f53b4bd99828bf750871f Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 9 Jan 2017 09:46:24 -0500 Subject: [PATCH 2/5] metadata: Add is_exported_symbol() method to CrateStore. --- src/librustc/middle/cstore.rs | 2 ++ src/librustc_metadata/creader.rs | 3 +++ src/librustc_metadata/cstore.rs | 2 ++ src/librustc_metadata/cstore_impl.rs | 12 ++++++++++-- src/librustc_metadata/decoder.rs | 2 +- 5 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/librustc/middle/cstore.rs b/src/librustc/middle/cstore.rs index 7151e5226cab0..19dafcd887ea8 100644 --- a/src/librustc/middle/cstore.rs +++ b/src/librustc/middle/cstore.rs @@ -211,6 +211,7 @@ pub trait CrateStore<'tcx> { fn is_foreign_item(&self, did: DefId) -> bool; fn is_dllimport_foreign_item(&self, def: DefId) -> bool; fn is_statically_included_foreign_item(&self, def_id: DefId) -> bool; + fn is_exported_symbol(&self, def_id: DefId) -> bool; // crate metadata fn dylib_dependency_formats(&self, cnum: CrateNum) @@ -368,6 +369,7 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore { fn is_foreign_item(&self, did: DefId) -> bool { bug!("is_foreign_item") } fn is_dllimport_foreign_item(&self, id: DefId) -> bool { false } fn is_statically_included_foreign_item(&self, def_id: DefId) -> bool { false } + fn is_exported_symbol(&self, def_id: DefId) -> bool { false } // crate metadata fn dylib_dependency_formats(&self, cnum: CrateNum) diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs index 724c164b3b41a..8f7b9c24cbf8a 100644 --- a/src/librustc_metadata/creader.rs +++ b/src/librustc_metadata/creader.rs @@ -302,10 +302,13 @@ impl<'a> CrateLoader<'a> { crate_root.def_path_table.decode(&metadata) }); + let exported_symbols = crate_root.exported_symbols.decode(&metadata).collect(); + let mut cmeta = cstore::CrateMetadata { name: name, extern_crate: Cell::new(None), def_path_table: def_path_table, + exported_symbols: exported_symbols, proc_macros: crate_root.macro_derive_registrar.map(|_| { self.load_derive_macros(&crate_root, dylib.clone().map(|p| p.0), span) }), diff --git a/src/librustc_metadata/cstore.rs b/src/librustc_metadata/cstore.rs index efc19abb33e1b..761041ad7198a 100644 --- a/src/librustc_metadata/cstore.rs +++ b/src/librustc_metadata/cstore.rs @@ -80,6 +80,8 @@ pub struct CrateMetadata { /// compilation support. pub def_path_table: DefPathTable, + pub exported_symbols: FxHashSet, + pub dep_kind: Cell, pub source: CrateSource, diff --git a/src/librustc_metadata/cstore_impl.rs b/src/librustc_metadata/cstore_impl.rs index 3d025e984b040..1dcb1689d49e1 100644 --- a/src/librustc_metadata/cstore_impl.rs +++ b/src/librustc_metadata/cstore_impl.rs @@ -226,6 +226,10 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore { self.do_is_statically_included_foreign_item(def_id) } + fn is_exported_symbol(&self, def_id: DefId) -> bool { + self.get_crate_data(def_id.krate).exported_symbols.contains(&def_id.index) + } + fn is_dllimport_foreign_item(&self, def_id: DefId) -> bool { if def_id.krate == LOCAL_CRATE { self.dllimport_foreign_items.borrow().contains(&def_id.index) @@ -467,8 +471,12 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore { } fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool { - self.dep_graph.read(DepNode::MetaData(def)); - def.is_local() || self.get_crate_data(def.krate).can_have_local_instance(tcx, def.index) + if def.is_local() { + true + } else { + self.dep_graph.read(DepNode::MetaData(def)); + self.get_crate_data(def.krate).can_have_local_instance(tcx, def.index) + } } fn crates(&self) -> Vec diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs index f3a673898b25c..3add92191df97 100644 --- a/src/librustc_metadata/decoder.rs +++ b/src/librustc_metadata/decoder.rs @@ -1031,7 +1031,7 @@ impl<'a, 'tcx> CrateMetadata { } pub fn get_exported_symbols(&self) -> Vec { - self.root.exported_symbols.decode(self).map(|index| self.local_def_id(index)).collect() + self.exported_symbols.iter().map(|&index| self.local_def_id(index)).collect() } pub fn get_macro(&self, id: DefIndex) -> (ast::Name, MacroDef) { From 5f90947c2c27d5d4ae30ccc0e83ffc95a8597128 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 9 Jan 2017 09:52:08 -0500 Subject: [PATCH 3/5] trans: Treat generics like regular functions, not like #[inline] functions during CGU partitioning. --- src/librustc_trans/collector.rs | 80 ++++++++++++------- src/librustc_trans/partitioning.rs | 55 +++---------- src/librustc_trans/trans_item.rs | 49 ++++++------ .../partitioning/extern-generic.rs | 6 +- .../partitioning/local-generic.rs | 8 +- .../partitioning/vtable-through-const.rs | 10 +-- .../run-make/stable-symbol-names/Makefile | 28 +++++-- .../stable-symbol-names1.rs | 21 +++-- .../stable-symbol-names2.rs | 12 +-- 9 files changed, 145 insertions(+), 124 deletions(-) diff --git a/src/librustc_trans/collector.rs b/src/librustc_trans/collector.rs index ff8a14474e57d..ac182ae5606bc 100644 --- a/src/librustc_trans/collector.rs +++ b/src/librustc_trans/collector.rs @@ -212,7 +212,7 @@ use glue::{self, DropGlueKind}; use monomorphize::{self, Instance}; use util::nodemap::{FxHashSet, FxHashMap, DefIdMap}; -use trans_item::{TransItem, DefPathBasedNames}; +use trans_item::{TransItem, DefPathBasedNames, InstantiationMode}; use std::iter; @@ -337,6 +337,10 @@ fn collect_items_rec<'a, 'tcx: 'a>(scx: &SharedCrateContext<'a, 'tcx>, } TransItem::Static(node_id) => { let def_id = scx.tcx().map.local_def_id(node_id); + + // Sanity check whether this ended up being collected accidentally + debug_assert!(should_trans_locally(scx.tcx(), def_id)); + let ty = scx.tcx().item_type(def_id); let ty = glue::get_drop_glue_type(scx, ty); neighbors.push(TransItem::DropGlue(DropGlueKind::Ty(ty))); @@ -346,6 +350,9 @@ fn collect_items_rec<'a, 'tcx: 'a>(scx: &SharedCrateContext<'a, 'tcx>, collect_neighbours(scx, Instance::mono(scx, def_id), &mut neighbors); } TransItem::Fn(instance) => { + // Sanity check whether this ended up being collected accidentally + debug_assert!(should_trans_locally(scx.tcx(), instance.def)); + // Keep track of the monomorphization recursion depth recursion_depth_reset = Some(check_recursion_limit(scx.tcx(), instance, @@ -374,7 +381,7 @@ fn record_inlining_canditates<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, callees: &[TransItem<'tcx>], inlining_map: &mut InliningMap<'tcx>) { let is_inlining_candidate = |trans_item: &TransItem<'tcx>| { - trans_item.needs_local_copy(tcx) + trans_item.instantiation_mode(tcx) == InstantiationMode::LocalCopy }; let inlining_candidates = callees.into_iter() @@ -490,15 +497,16 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { .require(ExchangeMallocFnLangItem) .unwrap_or_else(|e| self.scx.sess().fatal(&e)); - assert!(can_have_local_instance(self.scx.tcx(), exchange_malloc_fn_def_id)); - let empty_substs = self.scx.empty_substs_for_def_id(exchange_malloc_fn_def_id); - let exchange_malloc_fn_trans_item = - create_fn_trans_item(self.scx, - exchange_malloc_fn_def_id, - empty_substs, - self.param_substs); + if should_trans_locally(self.scx.tcx(), exchange_malloc_fn_def_id) { + let empty_substs = self.scx.empty_substs_for_def_id(exchange_malloc_fn_def_id); + let exchange_malloc_fn_trans_item = + create_fn_trans_item(self.scx, + exchange_malloc_fn_def_id, + empty_substs, + self.param_substs); - self.output.push(exchange_malloc_fn_trans_item); + self.output.push(exchange_malloc_fn_trans_item); + } } _ => { /* not interesting */ } } @@ -609,7 +617,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { match tcx.item_type(def_id).sty { ty::TyFnDef(def_id, _, f) => { // Some constructors also have type TyFnDef but they are - // always instantiated inline and don't result in + // always instantiated inline and don't result in a // translation item. Same for FFI functions. if let Some(hir_map::NodeForeignItem(_)) = tcx.map.get_if_local(def_id) { return false; @@ -625,7 +633,7 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { _ => return false } - can_have_local_instance(tcx, def_id) + should_trans_locally(tcx, def_id) } } @@ -675,10 +683,27 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> { } } -fn can_have_local_instance<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, - def_id: DefId) - -> bool { - tcx.sess.cstore.can_have_local_instance(tcx, def_id) +// Returns true if we should translate an instance in the local crate. +// Returns false if we can just link to the upstream crate and therefore don't +// need a translation item. +fn should_trans_locally<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, + def_id: DefId) + -> bool { + if def_id.is_local() { + true + } else { + if tcx.sess.cstore.is_exported_symbol(def_id) || + tcx.sess.cstore.is_foreign_item(def_id) { + // We can link to the item in question, no instance needed in this + // crate + false + } else { + if !tcx.sess.cstore.can_have_local_instance(tcx, def_id) { + bug!("Cannot create local trans-item for {:?}", def_id) + } + true + } + } } fn find_drop_glue_neighbors<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>, @@ -698,14 +723,15 @@ fn find_drop_glue_neighbors<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>, // Make sure the BoxFreeFn lang-item gets translated if there is a boxed value. if let ty::TyBox(content_type) = ty.sty { let def_id = scx.tcx().require_lang_item(BoxFreeFnLangItem); - assert!(can_have_local_instance(scx.tcx(), def_id)); - let box_free_fn_trans_item = - create_fn_trans_item(scx, - def_id, - scx.tcx().mk_substs(iter::once(Kind::from(content_type))), - scx.tcx().intern_substs(&[])); - - output.push(box_free_fn_trans_item); + + if should_trans_locally(scx.tcx(), def_id) { + let box_free_fn_trans_item = + create_fn_trans_item(scx, + def_id, + scx.tcx().mk_substs(iter::once(Kind::from(content_type))), + scx.tcx().intern_substs(&[])); + output.push(box_free_fn_trans_item); + } } // If the type implements Drop, also add a translation item for the @@ -735,7 +761,7 @@ fn find_drop_glue_neighbors<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>, _ => bug!() }; - if can_have_local_instance(scx.tcx(), destructor_did) { + if should_trans_locally(scx.tcx(), destructor_did) { let trans_item = create_fn_trans_item(scx, destructor_did, substs, @@ -1080,7 +1106,7 @@ fn create_trans_items_for_vtable_methods<'a, 'tcx>(scx: &SharedCrateContext<'a, None } }) - .filter(|&(def_id, _)| can_have_local_instance(scx.tcx(), def_id)) + .filter(|&(def_id, _)| should_trans_locally(scx.tcx(), def_id)) .map(|(def_id, substs)| create_fn_trans_item(scx, def_id, substs, param_substs)); output.extend(methods); } @@ -1255,7 +1281,7 @@ fn create_trans_items_for_default_impls<'a, 'tcx>(scx: &SharedCrateContext<'a, ' continue; } - if can_have_local_instance(tcx, method.def_id) { + if should_trans_locally(tcx, method.def_id) { let item = create_fn_trans_item(scx, method.def_id, callee_substs, diff --git a/src/librustc_trans/partitioning.rs b/src/librustc_trans/partitioning.rs index d93bbec7efa41..c91b25ef360cd 100644 --- a/src/librustc_trans/partitioning.rs +++ b/src/librustc_trans/partitioning.rs @@ -133,7 +133,7 @@ use std::sync::Arc; use symbol_map::SymbolMap; use syntax::ast::NodeId; use syntax::symbol::{Symbol, InternedString}; -use trans_item::TransItem; +use trans_item::{TransItem, InstantiationMode}; use util::nodemap::{FxHashMap, FxHashSet}; pub enum PartitioningStrategy { @@ -326,13 +326,15 @@ fn place_root_translation_items<'a, 'tcx, I>(scx: &SharedCrateContext<'a, 'tcx>, let tcx = scx.tcx(); let mut roots = FxHashSet(); let mut codegen_units = FxHashMap(); + let is_incremental_build = tcx.sess.opts.incremental.is_some(); for trans_item in trans_items { - let is_root = !trans_item.is_instantiated_only_on_demand(tcx); + let is_root = trans_item.instantiation_mode(tcx) == InstantiationMode::GloballyShared; if is_root { let characteristic_def_id = characteristic_def_id_of_trans_item(scx, trans_item); - let is_volatile = trans_item.is_generic_fn(); + let is_volatile = is_incremental_build && + trans_item.is_generic_fn(); let codegen_unit_name = match characteristic_def_id { Some(def_id) => compute_codegen_unit_name(tcx, def_id, is_volatile), @@ -350,25 +352,9 @@ fn place_root_translation_items<'a, 'tcx, I>(scx: &SharedCrateContext<'a, 'tcx>, Some(explicit_linkage) => explicit_linkage, None => { match trans_item { + TransItem::Fn(..) | TransItem::Static(..) => llvm::ExternalLinkage, TransItem::DropGlue(..) => unreachable!(), - // Is there any benefit to using ExternalLinkage?: - TransItem::Fn(ref instance) => { - if instance.substs.types().next().is_none() { - // This is a non-generic functions, we always - // make it visible externally on the chance that - // it might be used in another codegen unit. - // Later on base::internalize_symbols() will - // assign "internal" linkage to those symbols - // that are not referenced from other codegen - // units (and are not publicly visible). - llvm::ExternalLinkage - } else { - // In the current setup, generic functions cannot - // be roots. - unreachable!() - } - } } } }; @@ -448,29 +434,14 @@ fn place_inlined_translation_items<'tcx>(initial_partitioning: PreInliningPartit if let Some(linkage) = codegen_unit.items.get(&trans_item) { // This is a root, just copy it over new_codegen_unit.items.insert(trans_item, *linkage); - } else if initial_partitioning.roots.contains(&trans_item) { - // This item will be instantiated in some other codegen unit, - // so we just add it here with AvailableExternallyLinkage - // FIXME(mw): I have not seen it happening yet but having - // available_externally here could potentially lead - // to the same problem with exception handling tables - // as in the case below. - new_codegen_unit.items.insert(trans_item, - llvm::AvailableExternallyLinkage); - } else if trans_item.is_from_extern_crate() && !trans_item.is_generic_fn() { - // FIXME(mw): It would be nice if we could mark these as - // `AvailableExternallyLinkage`, since they should have - // been instantiated in the extern crate. But this - // sometimes leads to crashes on Windows because LLVM - // does not handle exception handling table instantiation - // reliably in that case. - new_codegen_unit.items.insert(trans_item, llvm::InternalLinkage); } else { - // We can't be sure if this will also be instantiated - // somewhere else, so we add an instance here with - // InternalLinkage so we don't get any conflicts. - new_codegen_unit.items.insert(trans_item, - llvm::InternalLinkage); + if initial_partitioning.roots.contains(&trans_item) { + bug!("GloballyShared trans-item inlined into other CGU: \ + {:?}", trans_item); + } + + // This is a cgu-private copy + new_codegen_unit.items.insert(trans_item, llvm::InternalLinkage); } } diff --git a/src/librustc_trans/trans_item.rs b/src/librustc_trans/trans_item.rs index f6f91411225d9..816c344254371 100644 --- a/src/librustc_trans/trans_item.rs +++ b/src/librustc_trans/trans_item.rs @@ -45,6 +45,18 @@ pub enum TransItem<'tcx> { Static(NodeId) } +/// Describes how a translation item will be instantiated in object files. +#[derive(PartialEq, Eq, Clone, Copy, Debug, Hash)] +pub enum InstantiationMode { + /// There will be exactly one instance of the given TransItem. It will have + /// external linkage so that it can be linked to from other codegen units. + GloballyShared, + + /// Each codegen unit containing a reference to the given TransItem will + /// have its own private copy of the function (with internal linkage). + LocalCopy, +} + impl<'a, 'tcx> TransItem<'tcx> { pub fn define(&self, ccx: &CrateContext<'a, 'tcx>) { @@ -231,22 +243,21 @@ impl<'a, 'tcx> TransItem<'tcx> { } } - /// True if the translation item should only be translated to LLVM IR if - /// it is referenced somewhere (like inline functions, for example). - pub fn is_instantiated_only_on_demand(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> bool { - if self.explicit_linkage(tcx).is_some() { - return false; - } - + pub fn instantiation_mode(&self, + tcx: TyCtxt<'a, 'tcx, 'tcx>) + -> InstantiationMode { match *self { TransItem::Fn(ref instance) => { - !instance.def.is_local() || - instance.substs.types().next().is_some() || - common::is_closure(tcx, instance.def) || - attr::requests_inline(&tcx.get_attrs(instance.def)[..]) + if self.explicit_linkage(tcx).is_none() && + (common::is_closure(tcx, instance.def) || + attr::requests_inline(&tcx.get_attrs(instance.def)[..])) { + InstantiationMode::LocalCopy + } else { + InstantiationMode::GloballyShared + } } - TransItem::DropGlue(..) => true, - TransItem::Static(..) => false, + TransItem::DropGlue(..) => InstantiationMode::LocalCopy, + TransItem::Static(..) => InstantiationMode::GloballyShared, } } @@ -260,18 +271,6 @@ impl<'a, 'tcx> TransItem<'tcx> { } } - /// Returns true if there has to be a local copy of this TransItem in every - /// codegen unit that references it (as with inlined functions, for example) - pub fn needs_local_copy(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> bool { - // Currently everything that is instantiated only on demand is done so - // with "internal" linkage, so we need a copy to be present in every - // codegen unit. - // This is coincidental: We could also instantiate something only if it - // is referenced (e.g. a regular, private function) but place it in its - // own codegen unit with "external" linkage. - self.is_instantiated_only_on_demand(tcx) - } - pub fn explicit_linkage(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> Option { let def_id = match *self { TransItem::Fn(ref instance) => instance.def, diff --git a/src/test/codegen-units/partitioning/extern-generic.rs b/src/test/codegen-units/partitioning/extern-generic.rs index 58f904f48a17d..db36b50702a43 100644 --- a/src/test/codegen-units/partitioning/extern-generic.rs +++ b/src/test/codegen-units/partitioning/extern-generic.rs @@ -57,8 +57,8 @@ mod mod3 { } // Make sure the two generic functions from the extern crate get instantiated -// privately in every module they are use in. -//~ TRANS_ITEM fn cgu_generic_function::foo[0]<&str> @@ extern_generic[Internal] extern_generic-mod1[Internal] extern_generic-mod2[Internal] extern_generic-mod1-mod1[Internal] -//~ TRANS_ITEM fn cgu_generic_function::bar[0]<&str> @@ extern_generic[Internal] extern_generic-mod1[Internal] extern_generic-mod2[Internal] extern_generic-mod1-mod1[Internal] +// once for the current crate +//~ TRANS_ITEM fn cgu_generic_function::foo[0]<&str> @@ cgu_generic_function.volatile[External] +//~ TRANS_ITEM fn cgu_generic_function::bar[0]<&str> @@ cgu_generic_function.volatile[External] //~ TRANS_ITEM drop-glue i8 diff --git a/src/test/codegen-units/partitioning/local-generic.rs b/src/test/codegen-units/partitioning/local-generic.rs index 2d744169d3f8e..70fc75c6970b7 100644 --- a/src/test/codegen-units/partitioning/local-generic.rs +++ b/src/test/codegen-units/partitioning/local-generic.rs @@ -16,10 +16,10 @@ #![allow(dead_code)] #![crate_type="lib"] -//~ TRANS_ITEM fn local_generic::generic[0] @@ local_generic[Internal] -//~ TRANS_ITEM fn local_generic::generic[0] @@ local_generic-mod1[Internal] -//~ TRANS_ITEM fn local_generic::generic[0] @@ local_generic-mod1-mod1[Internal] -//~ TRANS_ITEM fn local_generic::generic[0]<&str> @@ local_generic-mod2[Internal] +//~ TRANS_ITEM fn local_generic::generic[0] @@ local_generic.volatile[External] +//~ TRANS_ITEM fn local_generic::generic[0] @@ local_generic.volatile[External] +//~ TRANS_ITEM fn local_generic::generic[0] @@ local_generic.volatile[External] +//~ TRANS_ITEM fn local_generic::generic[0]<&str> @@ local_generic.volatile[External] pub fn generic(x: T) -> T { x } //~ TRANS_ITEM fn local_generic::user[0] @@ local_generic[External] diff --git a/src/test/codegen-units/partitioning/vtable-through-const.rs b/src/test/codegen-units/partitioning/vtable-through-const.rs index 0007eaae28971..7a0217072f32c 100644 --- a/src/test/codegen-units/partitioning/vtable-through-const.rs +++ b/src/test/codegen-units/partitioning/vtable-through-const.rs @@ -74,19 +74,19 @@ fn main() { // Since Trait1::do_something() is instantiated via its default implementation, // it is considered a generic and is instantiated here only because it is // referenced in this module. - //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something_else[0] @@ vtable_through_const[Internal] + //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something_else[0] @@ vtable_through_const-mod1.volatile[External] // Although it is never used, Trait1::do_something_else() has to be // instantiated locally here too, otherwise the <&u32 as &Trait1> vtable // could not be fully constructed. - //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something[0] @@ vtable_through_const[Internal] + //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something[0] @@ vtable_through_const-mod1.volatile[External] mod1::TRAIT1_REF.do_something(); // Same as above - //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something[0] @@ vtable_through_const[Internal] - //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something_else[0] @@ vtable_through_const[Internal] + //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something[0] @@ vtable_through_const-mod1.volatile[External] + //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something_else[0] @@ vtable_through_const-mod1.volatile[External] mod1::TRAIT1_GEN_REF.do_something(0u8); - //~ TRANS_ITEM fn vtable_through_const::mod1[0]::id[0] @@ vtable_through_const[Internal] + //~ TRANS_ITEM fn vtable_through_const::mod1[0]::id[0] @@ vtable_through_const-mod1.volatile[External] mod1::ID_CHAR('x'); } diff --git a/src/test/run-make/stable-symbol-names/Makefile b/src/test/run-make/stable-symbol-names/Makefile index c1b14440faab6..3cbc5593ac0a2 100644 --- a/src/test/run-make/stable-symbol-names/Makefile +++ b/src/test/run-make/stable-symbol-names/Makefile @@ -9,14 +9,32 @@ # 5. write the result into a file dump-symbols = nm "$(TMPDIR)/lib$(1).rlib" \ - | grep -E "some_test_function|Bar|bar" \ + | grep -E "$(2)" \ | sed "s/.*\(_ZN.*E\).*/\1/" \ | sort \ - > "$(TMPDIR)/$(1).nm" + > "$(TMPDIR)/$(1)$(3).nm" + +# This test +# - compiles each of the two crates 2 times and makes sure each time we get +# exactly the same symbol names +# - makes sure that both crates agree on the same symbol names for monomorphic +# functions all: $(RUSTC) stable-symbol-names1.rs + $(call dump-symbols,stable_symbol_names1,generic_|mono_,_v1) + rm $(TMPDIR)/libstable_symbol_names1.rlib + $(RUSTC) stable-symbol-names1.rs + $(call dump-symbols,stable_symbol_names1,generic_|mono_,_v2) + cmp "$(TMPDIR)/stable_symbol_names1_v1.nm" "$(TMPDIR)/stable_symbol_names1_v2.nm" + $(RUSTC) stable-symbol-names2.rs - $(call dump-symbols,stable_symbol_names1) - $(call dump-symbols,stable_symbol_names2) - cmp "$(TMPDIR)/stable_symbol_names1.nm" "$(TMPDIR)/stable_symbol_names2.nm" + $(call dump-symbols,stable_symbol_names2,generic_|mono_,_v1) + rm $(TMPDIR)/libstable_symbol_names2.rlib + $(RUSTC) stable-symbol-names2.rs + $(call dump-symbols,stable_symbol_names2,generic_|mono_,_v2) + cmp "$(TMPDIR)/stable_symbol_names2_v1.nm" "$(TMPDIR)/stable_symbol_names2_v2.nm" + + $(call dump-symbols,stable_symbol_names1,mono_,_cross) + $(call dump-symbols,stable_symbol_names2,mono_,_cross) + cmp "$(TMPDIR)/stable_symbol_names1_cross.nm" "$(TMPDIR)/stable_symbol_names2_cross.nm" diff --git a/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs b/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs index 5c73ff0fab6d0..7344bdf49f6f2 100644 --- a/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs +++ b/src/test/run-make/stable-symbol-names/stable-symbol-names1.rs @@ -11,26 +11,31 @@ #![crate_type="rlib"] pub trait Foo { - fn foo(); + fn generic_method(); } pub struct Bar; impl Foo for Bar { - fn foo() {} + fn generic_method() {} } -pub fn bar() { - Bar::foo::(); +pub fn mono_function() { + Bar::generic_method::(); } -pub fn some_test_function(t: T) -> T { +pub fn mono_function_lifetime<'a>(x: &'a u64) -> u64 { + *x +} + +pub fn generic_function(t: T) -> T { t } pub fn user() { - some_test_function(0u32); - some_test_function("abc"); + generic_function(0u32); + generic_function("abc"); let x = 2u64; - some_test_function(&x); + generic_function(&x); + let _ = mono_function_lifetime(&x); } diff --git a/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs b/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs index 097dce876affc..eacba4ddb25c0 100644 --- a/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs +++ b/src/test/run-make/stable-symbol-names/stable-symbol-names2.rs @@ -13,14 +13,16 @@ extern crate stable_symbol_names1; pub fn user() { - stable_symbol_names1::some_test_function(1u32); - stable_symbol_names1::some_test_function("def"); + stable_symbol_names1::generic_function(1u32); + stable_symbol_names1::generic_function("def"); let x = 2u64; - stable_symbol_names1::some_test_function(&x); + stable_symbol_names1::generic_function(&x); + stable_symbol_names1::mono_function(); + stable_symbol_names1::mono_function_lifetime(&0); } pub fn trait_impl_test_function() { use stable_symbol_names1::*; - Bar::foo::(); - bar(); + Bar::generic_method::(); } + From 622730ca10321549d0bc0f4653d8ba143bc7951b Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 9 Jan 2017 09:54:54 -0500 Subject: [PATCH 4/5] Remove some out-dated comments from CGU partitioning docs. --- src/librustc_trans/partitioning.rs | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/librustc_trans/partitioning.rs b/src/librustc_trans/partitioning.rs index c91b25ef360cd..d1d167306c1fd 100644 --- a/src/librustc_trans/partitioning.rs +++ b/src/librustc_trans/partitioning.rs @@ -53,8 +53,6 @@ //! - One for "stable", that is non-generic, code //! - One for more "volatile" code, i.e. monomorphized instances of functions //! defined in that module -//! - Code for monomorphized instances of functions from external crates gets -//! placed into every codegen unit that uses that instance. //! //! In order to see why this heuristic makes sense, let's take a look at when a //! codegen unit can get invalidated: @@ -82,17 +80,6 @@ //! side-effect of references a little by at least not touching the non-generic //! code of the module. //! -//! As another optimization, monomorphized functions from external crates get -//! some special handling. Since we assume that the definition of such a -//! function changes rather infrequently compared to local items, we can just -//! instantiate external functions in every codegen unit where it is referenced -//! -- without having to fear that doing this will cause a lot of unnecessary -//! re-compilations. If such a reference is added or removed, the codegen unit -//! has to be re-translated anyway. -//! (Note that this only makes sense if external crates actually don't change -//! frequently. For certain multi-crate projects this might not be a valid -//! assumption). -//! //! A Note on Inlining //! ------------------ //! As briefly mentioned above, in order for LLVM to be able to inline a @@ -107,10 +94,9 @@ //! inlined, so it can distribute function instantiations accordingly. Since //! there is no way of knowing for sure which functions LLVM will decide to //! inline in the end, we apply a heuristic here: Only functions marked with -//! #[inline] and (as stated above) functions from external crates are -//! considered for inlining by the partitioner. The current implementation -//! will not try to determine if a function is likely to be inlined by looking -//! at the functions definition. +//! #[inline] are considered for inlining by the partitioner. The current +//! implementation will not try to determine if a function is likely to be +//! inlined by looking at the functions definition. //! //! Note though that as a side-effect of creating a codegen units per //! source-level module, functions from the same module will be available for From fc9dfcacf87eb5bb24271cdbb3863345fd27d751 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 9 Jan 2017 11:27:20 -0500 Subject: [PATCH 5/5] trans/metadata: Remove obsolete CrateStore::can_have_local_instance() --- src/librustc/middle/cstore.rs | 8 ------ src/librustc_metadata/cstore_impl.rs | 9 ------- src/librustc_metadata/decoder.rs | 38 +++++----------------------- src/librustc_trans/collector.rs | 2 +- 4 files changed, 7 insertions(+), 50 deletions(-) diff --git a/src/librustc/middle/cstore.rs b/src/librustc/middle/cstore.rs index 19dafcd887ea8..496a3d4a49847 100644 --- a/src/librustc/middle/cstore.rs +++ b/src/librustc/middle/cstore.rs @@ -259,11 +259,6 @@ pub trait CrateStore<'tcx> { fn get_item_mir<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> Mir<'tcx>; fn is_item_mir_available(&self, def: DefId) -> bool; - /// Take a look if we need to inline or monomorphize this. If so, we - /// will emit code for this item in the local crate, and thus - /// create a translation item for it. - fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool; - // This is basically a 1-based range of ints, which is a little // silly - I may fix that. fn crates(&self) -> Vec; @@ -438,9 +433,6 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore { fn is_item_mir_available(&self, def: DefId) -> bool { bug!("is_item_mir_available") } - fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool { - bug!("can_have_local_instance") - } // This is basically a 1-based range of ints, which is a little // silly - I may fix that. diff --git a/src/librustc_metadata/cstore_impl.rs b/src/librustc_metadata/cstore_impl.rs index 1dcb1689d49e1..7cd26df0246ea 100644 --- a/src/librustc_metadata/cstore_impl.rs +++ b/src/librustc_metadata/cstore_impl.rs @@ -470,15 +470,6 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore { self.get_crate_data(def.krate).is_item_mir_available(def.index) } - fn can_have_local_instance<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId) -> bool { - if def.is_local() { - true - } else { - self.dep_graph.read(DepNode::MetaData(def)); - self.get_crate_data(def.krate).can_have_local_instance(tcx, def.index) - } - } - fn crates(&self) -> Vec { let mut result = vec![]; diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs index 3add92191df97..4abdee345c298 100644 --- a/src/librustc_metadata/decoder.rs +++ b/src/librustc_metadata/decoder.rs @@ -445,14 +445,6 @@ impl<'tcx> EntryKind<'tcx> { EntryKind::Closure(_) => return None, }) } - fn is_const_fn(&self, meta: &CrateMetadata) -> bool { - let constness = match *self { - EntryKind::Method(data) => data.decode(meta).fn_data.constness, - EntryKind::Fn(data) => data.decode(meta).constness, - _ => hir::Constness::NotConst, - }; - constness == hir::Constness::Const - } } impl<'a, 'tcx> CrateMetadata { @@ -804,29 +796,6 @@ impl<'a, 'tcx> CrateMetadata { self.maybe_entry(id).and_then(|item| item.decode(self).mir).is_some() } - pub fn can_have_local_instance(&self, - tcx: TyCtxt<'a, 'tcx, 'tcx>, - id: DefIndex) -> bool { - self.maybe_entry(id).map_or(false, |item| { - let item = item.decode(self); - // if we don't have a MIR, then this item was never meant to be locally instantiated - // or we have a bug in the metadata serialization - item.mir.is_some() && ( - // items with generics always can have local instances if monomorphized - item.generics.map_or(false, |generics| { - let generics = generics.decode((self, tcx)); - generics.parent_types != 0 || !generics.types.is_empty() - }) || - match item.kind { - EntryKind::Closure(_) => true, - _ => false, - } || - item.kind.is_const_fn(self) || - attr::requests_inline(&self.get_attributes(&item)) - ) - }) - } - pub fn maybe_get_item_mir(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, id: DefIndex) @@ -1043,7 +1012,12 @@ impl<'a, 'tcx> CrateMetadata { } pub fn is_const_fn(&self, id: DefIndex) -> bool { - self.entry(id).kind.is_const_fn(self) + let constness = match self.entry(id).kind { + EntryKind::Method(data) => data.decode(self).fn_data.constness, + EntryKind::Fn(data) => data.decode(self).constness, + _ => hir::Constness::NotConst, + }; + constness == hir::Constness::Const } pub fn is_foreign_item(&self, id: DefIndex) -> bool { diff --git a/src/librustc_trans/collector.rs b/src/librustc_trans/collector.rs index ac182ae5606bc..5e409a2aa5520 100644 --- a/src/librustc_trans/collector.rs +++ b/src/librustc_trans/collector.rs @@ -698,7 +698,7 @@ fn should_trans_locally<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, // crate false } else { - if !tcx.sess.cstore.can_have_local_instance(tcx, def_id) { + if !tcx.sess.cstore.is_item_mir_available(def_id) { bug!("Cannot create local trans-item for {:?}", def_id) } true