From 5f90947c2c27d5d4ae30ccc0e83ffc95a8597128 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Mon, 9 Jan 2017 09:52:08 -0500 Subject: [PATCH] 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 ff8a14474e5..ac182ae5606 100644 --- a/src/librustc_trans/collector.rs +++ b/src/librustc_trans/collector.rs @@ -212,7 +212,7 @@ 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 @@ fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) { .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 @@ fn can_result_in_trans_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, '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 @@ fn can_result_in_trans_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, _ => return false } - can_have_local_instance(tcx, def_id) + should_trans_locally(tcx, def_id) } } @@ -675,10 +683,27 @@ fn is_drop_in_place_intrinsic<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, '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 d93bbec7efa..c91b25ef360 100644 --- a/src/librustc_trans/partitioning.rs +++ b/src/librustc_trans/partitioning.rs @@ -133,7 +133,7 @@ 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 f6f91411225..816c3442543 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 @@ pub fn is_from_extern_crate(&self) -> bool { } } - /// 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 @@ pub fn is_generic_fn(&self) -> bool { } } - /// 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 58f904f48a1..db36b50702a 100644 --- a/src/test/codegen-units/partitioning/extern-generic.rs +++ b/src/test/codegen-units/partitioning/extern-generic.rs @@ -57,8 +57,8 @@ fn non_user() {} } // 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 2d744169d3f..70fc75c6970 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 0007eaae289..7a0217072f3 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 c1b14440faa..3cbc5593ac0 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 5c73ff0fab6..7344bdf49f6 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 097dce876af..eacba4ddb25 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::(); } + -- 2.44.0