Auto merge of #32557 - dotdash:issue-32518, r=nikomatsakis

Use weak_odr linkage when reusing definitions across codegen units

When reuing a definition across codegen units, we obviously cannot use
internal linkage, but using external linkage means that we can end up
with multiple conflicting definitions of a single symbol across
multiple crates. Since the definitions should all be equal
semantically, we can use weak_odr linkage to resolve the situation.

Fixes #32518

r? @nikomatsakis

8 files changed, 152 insertions, 17 deletions

diff --git a/src/librustc_llvm/lib.rs b/src/librustc_llvm/lib.rs
index ee57812fb98..4df2da801f9 100644
--- a/src/librustc_llvm/lib.rs
+++ b/src/librustc_llvm/lib.rs
@@ -2125,6 +2125,9 @@ extern {
     pub fn LLVMRustFreeOperandBundleDef(Bundle: OperandBundleDefRef);
 
     pub fn LLVMRustPositionBuilderAtStart(B: BuilderRef, BB: BasicBlockRef);
+
+    pub fn LLVMRustSetComdat(M: ModuleRef, V: ValueRef, Name: *const c_char);
+    pub fn LLVMRustUnsetComdat(V: ValueRef);
 }
 
 // LLVM requires symbols from this library, but apparently they're not printed
@@ -2149,6 +2152,24 @@ pub fn SetLinkage(global: ValueRef, link: Linkage) {
     }
 }
 
+// Externally visible symbols that might appear in multiple translation units need to appear in
+// their own comdat section so that the duplicates can be discarded at link time. This can for
+// example happen for generics when using multiple codegen units. This function simply uses the
+// value's name as the comdat value to make sure that it is in a 1-to-1 relationship to the
+// function.
+// For more details on COMDAT sections see e.g. http://www.airs.com/blog/archives/52
+pub fn SetUniqueComdat(llmod: ModuleRef, val: ValueRef) {
+    unsafe {
+        LLVMRustSetComdat(llmod, val, LLVMGetValueName(val));
+    }
+}
+
+pub fn UnsetComdat(val: ValueRef) {
+    unsafe {
+        LLVMRustUnsetComdat(val);
+    }
+}
+
 pub fn SetDLLStorageClass(global: ValueRef, class: DLLStorageClassTypes) {
     unsafe {
         LLVMRustSetDLLStorageClass(global, class);
diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs
index 5f9997ffbb8..559ba17e1ae 100644
--- a/src/librustc_trans/base.rs
+++ b/src/librustc_trans/base.rs
@@ -2215,18 +2215,27 @@ pub fn update_linkage(ccx: &CrateContext,
         }
     }
 
-    match id {
-        Some(id) if ccx.reachable().contains(&id) => {
+    let (is_reachable, is_generic) = if let Some(id) = id {
+        (ccx.reachable().contains(&id), false)
+    } else {
+        (false, true)
+    };
+
+    // We need external linkage for items reachable from other translation units, this include
+    // other codegen units in case of parallel compilations.
+    if is_reachable || ccx.sess().opts.cg.codegen_units > 1 {
+        if is_generic {
+            // This only happens with multiple codegen units, in which case we need to use weak_odr
+            // linkage because other crates might expose the same symbol. We cannot use
+            // linkonce_odr here because the symbol might then get dropped before the other codegen
+            // units get to link it.
+            llvm::SetUniqueComdat(ccx.llmod(), llval);
+            llvm::SetLinkage(llval, llvm::WeakODRLinkage);
+        } else {
             llvm::SetLinkage(llval, llvm::ExternalLinkage);
-        },
-        _ => {
-            // `id` does not refer to an item in `ccx.reachable`.
-            if ccx.sess().opts.cg.codegen_units > 1 {
-                llvm::SetLinkage(llval, llvm::ExternalLinkage);
-            } else {
-                llvm::SetLinkage(llval, llvm::InternalLinkage);
-            }
-        },
+        }
+    } else {
+        llvm::SetLinkage(llval, llvm::InternalLinkage);
     }
 }
 
@@ -2547,8 +2556,10 @@ fn internalize_symbols(cx: &SharedCrateContext, reachable: &HashSet<&str>) {
         // then give it internal linkage.
         for ccx in cx.iter() {
             for val in iter_globals(ccx.llmod()).chain(iter_functions(ccx.llmod())) {
+                let linkage = llvm::LLVMGetLinkage(val);
                 // We only care about external definitions.
-                if !(llvm::LLVMGetLinkage(val) == llvm::ExternalLinkage as c_uint &&
+                if !((linkage == llvm::ExternalLinkage as c_uint ||
+                      linkage == llvm::WeakODRLinkage as c_uint) &&
                      llvm::LLVMIsDeclaration(val) == 0) {
                     continue;
                 }
@@ -2560,6 +2571,7 @@ fn internalize_symbols(cx: &SharedCrateContext, reachable: &HashSet<&str>) {
                    !reachable.contains(str::from_utf8(&name).unwrap()) {
                     llvm::SetLinkage(val, llvm::InternalLinkage);
                     llvm::SetDLLStorageClass(val, llvm::DefaultStorageClass);
+                    llvm::UnsetComdat(val);
                 }
             }
         }
diff --git a/src/librustc_trans/monomorphize.rs b/src/librustc_trans/monomorphize.rs
index fe3ab58761a..04a07bdf1ce 100644
--- a/src/librustc_trans/monomorphize.rs
+++ b/src/librustc_trans/monomorphize.rs
@@ -123,7 +123,6 @@ pub fn monomorphic_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
             ref attrs, node: hir::ImplItemKind::Method(
                 hir::MethodSig { ref decl, .. }, ref body), ..
         }) => {
-            base::update_linkage(ccx, lldecl, None, base::OriginalTranslation);
             attributes::from_fn_attrs(ccx, attrs, lldecl);
 
             let is_first = !ccx.available_monomorphizations().borrow()
@@ -133,12 +132,14 @@ pub fn monomorphic_fn<'a, 'tcx>(ccx: &CrateContext<'a, 'tcx>,
             }
 
             let trans_everywhere = attr::requests_inline(attrs);
-            if trans_everywhere && !is_first {
-                llvm::SetLinkage(lldecl, llvm::AvailableExternallyLinkage);
-            }
-
             if trans_everywhere || is_first {
+                let origin = if is_first { base::OriginalTranslation } else { base::InlinedCopy };
+                base::update_linkage(ccx, lldecl, None, origin);
                 trans_fn(ccx, decl, body, lldecl, psubsts, fn_node_id);
+            } else {
+                // We marked the value as using internal linkage earlier, but that is illegal for
+                // declarations, so switch back to external linkage.
+                llvm::SetLinkage(lldecl, llvm::ExternalLinkage);
             }
         }
 
diff --git a/src/rustllvm/RustWrapper.cpp b/src/rustllvm/RustWrapper.cpp
index 6ff90a8f53a..697b2d3f539 100644
--- a/src/rustllvm/RustWrapper.cpp
+++ b/src/rustllvm/RustWrapper.cpp
@@ -1189,3 +1189,16 @@ extern "C" void LLVMRustPositionBuilderAtStart(LLVMBuilderRef B, LLVMBasicBlockR
     auto point = unwrap(BB)->getFirstInsertionPt();
     unwrap(B)->SetInsertPoint(unwrap(BB), point);
 }
+
+extern "C" void LLVMRustSetComdat(LLVMModuleRef M, LLVMValueRef V, const char *Name) {
+    Triple TargetTriple(unwrap(M)->getTargetTriple());
+    GlobalObject *GV = unwrap<GlobalObject>(V);
+    if (!TargetTriple.isOSBinFormatMachO()) {
+        GV->setComdat(unwrap(M)->getOrInsertComdat(Name));
+    }
+}
+
+extern "C" void LLVMRustUnsetComdat(LLVMValueRef V) {
+    GlobalObject *GV = unwrap<GlobalObject>(V);
+    GV->setComdat(nullptr);
+}
diff --git a/src/test/auxiliary/cgu_test.rs b/src/test/auxiliary/cgu_test.rs
new file mode 100644
index 00000000000..7c88d3d37e3
--- /dev/null
+++ b/src/test/auxiliary/cgu_test.rs
@@ -0,0 +1,16 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// no-prefer-dynamic
+// compile-flags: --crate-type=lib
+
+pub fn id<T>(t: T) -> T {
+  t
+}
diff --git a/src/test/auxiliary/cgu_test_a.rs b/src/test/auxiliary/cgu_test_a.rs
new file mode 100644
index 00000000000..0f0d1cd87e1
--- /dev/null
+++ b/src/test/auxiliary/cgu_test_a.rs
@@ -0,0 +1,25 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// no-prefer-dynamic
+// compile-flags: -Ccodegen-units=2 --crate-type=lib
+
+extern crate cgu_test;
+
+pub mod a {
+    pub fn a() {
+        ::cgu_test::id(0);
+    }
+}
+pub mod b {
+    pub fn a() {
+        ::cgu_test::id(0);
+    }
+}
diff --git a/src/test/auxiliary/cgu_test_b.rs b/src/test/auxiliary/cgu_test_b.rs
new file mode 100644
index 00000000000..0f0d1cd87e1
--- /dev/null
+++ b/src/test/auxiliary/cgu_test_b.rs
@@ -0,0 +1,25 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// no-prefer-dynamic
+// compile-flags: -Ccodegen-units=2 --crate-type=lib
+
+extern crate cgu_test;
+
+pub mod a {
+    pub fn a() {
+        ::cgu_test::id(0);
+    }
+}
+pub mod b {
+    pub fn a() {
+        ::cgu_test::id(0);
+    }
+}
diff --git a/src/test/run-pass/issue-32518.rs b/src/test/run-pass/issue-32518.rs
new file mode 100644
index 00000000000..386d3e6524c
--- /dev/null
+++ b/src/test/run-pass/issue-32518.rs
@@ -0,0 +1,22 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// no-prefer-dynamic
+// aux-build:cgu_test.rs
+// aux-build:cgu_test_a.rs
+// aux-build:cgu_test_b.rs
+
+extern crate cgu_test_a;
+extern crate cgu_test_b;
+
+fn main() {
+    cgu_test_a::a::a();
+    cgu_test_b::a::a();
+}