trans: Treat generics like regular functions, not like #[inline] functions during CGU partitioning.

michaelwoerister · michaelwoerister · commit 5f90947c2c27 · 2017-01-09T10:06:58.000-05:00
diff --git 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
@@ -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
@@ -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<llvm::Linkage> {
         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
@@ -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
@@ -16,10 +16,10 @@
 #![allow(dead_code)]
 #![crate_type="lib"]
 
-//~ TRANS_ITEM fn local_generic::generic[0]<u32> @@ local_generic[Internal]
-//~ TRANS_ITEM fn local_generic::generic[0]<u64> @@ local_generic-mod1[Internal]
-//~ TRANS_ITEM fn local_generic::generic[0]<char> @@ local_generic-mod1-mod1[Internal]
-//~ TRANS_ITEM fn local_generic::generic[0]<&str> @@ local_generic-mod2[Internal]
+//~ TRANS_ITEM fn local_generic::generic[0]<u32> @@ local_generic.volatile[External]
+//~ TRANS_ITEM fn local_generic::generic[0]<u64> @@ local_generic.volatile[External]
+//~ TRANS_ITEM fn local_generic::generic[0]<char> @@ local_generic.volatile[External]
+//~ TRANS_ITEM fn local_generic::generic[0]<&str> @@ local_generic.volatile[External]
 pub fn generic<T>(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
@@ -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]<u32> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something_else[0]<u32> @@ 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]<u32> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::Trait1[0]::do_something[0]<u32> @@ 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]<u8> @@ vtable_through_const[Internal]
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something_else[0]<u8> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something[0]<u8> @@ vtable_through_const-mod1.volatile[External]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::{{impl}}[1]::do_something_else[0]<u8> @@ vtable_through_const-mod1.volatile[External]
     mod1::TRAIT1_GEN_REF.do_something(0u8);
 
-    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::id[0]<char> @@ vtable_through_const[Internal]
+    //~ TRANS_ITEM fn vtable_through_const::mod1[0]::id[0]<char> @@ 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
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
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
