From 22f458758652d309b7c65fa904d44f090214456c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Steinbrink?= Date: Mon, 28 Mar 2016 17:57:31 +0200 Subject: [PATCH] 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 --- src/librustc_llvm/lib.rs | 21 +++++++++++++++++ src/librustc_trans/base.rs | 36 ++++++++++++++++++++---------- src/librustc_trans/monomorphize.rs | 11 ++++----- src/rustllvm/RustWrapper.cpp | 13 +++++++++++ src/test/auxiliary/cgu_test.rs | 16 +++++++++++++ src/test/auxiliary/cgu_test_a.rs | 25 +++++++++++++++++++++ src/test/auxiliary/cgu_test_b.rs | 25 +++++++++++++++++++++ src/test/run-pass/issue-32518.rs | 22 ++++++++++++++++++ 8 files changed, 152 insertions(+), 17 deletions(-) create mode 100644 src/test/auxiliary/cgu_test.rs create mode 100644 src/test/auxiliary/cgu_test_a.rs create mode 100644 src/test/auxiliary/cgu_test_b.rs create mode 100644 src/test/run-pass/issue-32518.rs 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(V); + if (!TargetTriple.isOSBinFormatMachO()) { + GV->setComdat(unwrap(M)->getOrInsertComdat(Name)); + } +} + +extern "C" void LLVMRustUnsetComdat(LLVMValueRef V) { + GlobalObject *GV = unwrap(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 or the MIT license +// , 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 +} 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 or the MIT license +// , 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 or the MIT license +// , 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 or the MIT license +// , 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(); +}