[llvm] 0115262 - [Linker] Handle comdat nodeduplicate
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 31 22:32:25 PDT 2021
Author: Fangrui Song
Date: 2021-08-31T22:32:20-07:00
New Revision: 01152626ab87c6a9e76207a4a77b86a8a4ce6bbd
URL: https://github.com/llvm/llvm-project/commit/01152626ab87c6a9e76207a4a77b86a8a4ce6bbd
DIFF: https://github.com/llvm/llvm-project/commit/01152626ab87c6a9e76207a4a77b86a8a4ce6bbd.diff
LOG: [Linker] Handle comdat nodeduplicate
For a variable in a comdat nodeduplicate, its initializer may be significant.
E.g. its content may be implicitly referenced by another comdat member (or
required to parallel to another comdat member by the runtime when explicit
section is used). We can clone it into an unnamed private linkage variable to
preserve its content.
This partially fixes PR51394 (Sony's proprietary linker using LTO): no error
will be reported. This is partial because we do not guarantee the global
variable order if the runtime has parallel section requirement.
---
There is a similar issue for regular LTO, but unrelated to PR51394:
with lib/LTO (using either ld.lld or LLVMgold.so), linking two modules
with a weak function of the same name, can leave one weak profc and two
private profd, due to lib/LTO's current deficiency that it mixes the two
concepts together: comdat selection and symbol resolution. If the issue
is considered important, we should suppress private profd for the weak+
regular LTO case.
Reviewed By: phosek
Differential Revision: https://reviews.llvm.org/D108879
Added:
Modified:
llvm/lib/Linker/LinkModules.cpp
llvm/test/Linker/comdat-nodeduplicate.ll
Removed:
################################################################################
diff --git a/llvm/lib/Linker/LinkModules.cpp b/llvm/lib/Linker/LinkModules.cpp
index 2a44800e3d1f9..a4c70a629ebd4 100644
--- a/llvm/lib/Linker/LinkModules.cpp
+++ b/llvm/lib/Linker/LinkModules.cpp
@@ -24,7 +24,7 @@ using namespace llvm;
namespace {
-enum class LinkFrom { Dst, Src };
+enum class LinkFrom { Dst, Src, Both };
/// This is an implementation class for the LinkModules function, which is the
/// entrypoint for this file.
@@ -105,7 +105,7 @@ class ModuleLinker {
void dropReplacedComdat(GlobalValue &GV,
const DenseSet<const Comdat *> &ReplacedDstComdats);
- bool linkIfNeeded(GlobalValue &GV);
+ bool linkIfNeeded(GlobalValue &GV, SmallVectorImpl<GlobalValue *> &GVToClone);
public:
ModuleLinker(IRMover &Mover, std::unique_ptr<Module> SrcM, unsigned Flags,
@@ -179,25 +179,9 @@ bool ModuleLinker::computeResultingSelectionKind(StringRef ComdatName,
// Go with Dst.
From = LinkFrom::Dst;
break;
- case Comdat::SelectionKind::NoDeduplicate: {
- const GlobalVariable *DstGV;
- const GlobalVariable *SrcGV;
- if (getComdatLeader(DstM, ComdatName, DstGV) ||
- getComdatLeader(*SrcM, ComdatName, SrcGV))
- return true;
-
- if (SrcGV->isWeakForLinker()) {
- // Go with Dst.
- From = LinkFrom::Dst;
- } else if (DstGV->isWeakForLinker()) {
- // Go with Src.
- From = LinkFrom::Src;
- } else {
- return emitError("Linking COMDATs named '" + ComdatName +
- "': nodeduplicate has been violated!");
- }
+ case Comdat::SelectionKind::NoDeduplicate:
+ From = LinkFrom::Both;
break;
- }
case Comdat::SelectionKind::ExactMatch:
case Comdat::SelectionKind::Largest:
case Comdat::SelectionKind::SameSize: {
@@ -342,7 +326,8 @@ bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
"': symbol multiply defined!");
}
-bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
+bool ModuleLinker::linkIfNeeded(GlobalValue &GV,
+ SmallVectorImpl<GlobalValue *> &GVToClone) {
GlobalValue *DGV = getLinkedToGlobal(&GV);
if (shouldLinkOnlyNeeded()) {
@@ -404,6 +389,8 @@ bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
bool LinkFromSrc = true;
if (DGV && shouldLinkFromSource(LinkFromSrc, *DGV, GV))
return true;
+ if (DGV && ComdatFrom == LinkFrom::Both)
+ GVToClone.push_back(LinkFromSrc ? DGV : &GV);
if (LinkFromSrc)
ValuesToLink.insert(&GV);
return false;
@@ -530,22 +517,45 @@ bool ModuleLinker::run() {
// Insert all of the globals in src into the DstM module... without linking
// initializers (which could refer to functions not yet mapped over).
+ SmallVector<GlobalValue *, 0> GVToClone;
for (GlobalVariable &GV : SrcM->globals())
- if (linkIfNeeded(GV))
+ if (linkIfNeeded(GV, GVToClone))
return true;
for (Function &SF : *SrcM)
- if (linkIfNeeded(SF))
+ if (linkIfNeeded(SF, GVToClone))
return true;
for (GlobalAlias &GA : SrcM->aliases())
- if (linkIfNeeded(GA))
+ if (linkIfNeeded(GA, GVToClone))
return true;
for (GlobalIFunc &GI : SrcM->ifuncs())
- if (linkIfNeeded(GI))
+ if (linkIfNeeded(GI, GVToClone))
return true;
+ // For a variable in a comdat nodeduplicate, its initializer should be
+ // preserved (its content may be implicitly used by other members) even if
+ // symbol resolution does not pick it. Clone it into an unnamed private
+ // variable.
+ for (GlobalValue *GV : GVToClone) {
+ if (auto *Var = dyn_cast<GlobalVariable>(GV)) {
+ auto *NewVar = new GlobalVariable(*Var->getParent(), Var->getValueType(),
+ Var->isConstant(), Var->getLinkage(),
+ Var->getInitializer());
+ NewVar->copyAttributesFrom(Var);
+ NewVar->setVisibility(GlobalValue::DefaultVisibility);
+ NewVar->setLinkage(GlobalValue::PrivateLinkage);
+ NewVar->setDSOLocal(true);
+ NewVar->setComdat(Var->getComdat());
+ if (Var->getParent() != &Mover.getModule())
+ ValuesToLink.insert(NewVar);
+ } else {
+ emitError("linking '" + GV->getName() +
+ "': non-variables in comdat nodeduplicate are not handled");
+ }
+ }
+
for (unsigned I = 0; I < ValuesToLink.size(); ++I) {
GlobalValue *GV = ValuesToLink[I];
const Comdat *SC = GV->getComdat();
diff --git a/llvm/test/Linker/comdat-nodeduplicate.ll b/llvm/test/Linker/comdat-nodeduplicate.ll
index 17721696f2699..c31ece65c07d0 100644
--- a/llvm/test/Linker/comdat-nodeduplicate.ll
+++ b/llvm/test/Linker/comdat-nodeduplicate.ll
@@ -1,18 +1,21 @@
; RUN: rm -rf %t && split-file %s %t
; RUN: not llvm-link -S %t/1.ll %t/1-aux.ll 2>&1 | FileCheck %s
-; CHECK: error: Linking COMDATs named 'foo': nodeduplicate has been violated!
+; CHECK: Linking globals named 'foo': symbol multiply defined!
; RUN: llvm-link -S %t/2.ll %t/2-aux.ll | FileCheck %s --check-prefix=CHECK2
; RUN: llvm-link -S %t/2-aux.ll %t/2.ll | FileCheck %s --check-prefix=CHECK2
-; CHECK2-DAG: @foo = global i64 2, section "data", comdat, align 8
-; CHECK2-DAG: @bar = weak global i64 0, section "cnts", comdat($foo)
+; CHECK2-DAG: @[[#]] = private global i64 0, section "data", comdat($foo)
+; CHECK2-DAG: @[[#]] = private global i64 0, section "cnts", comdat($foo)
+; CHECK2-DAG: @foo = hidden global i64 2, section "data", comdat, align 8
+; CHECK2-DAG: @bar = dso_local global i64 3, section "cnts", comdat($foo), align 16
; CHECK2-DAG: @qux = weak_odr global i64 4, comdat($foo)
+; CHECK2-DAG: @fred = linkonce global i64 5, comdat($foo)
-; RUN: not llvm-link -S %t/non-var.ll %t/non-var.ll 2>&1 | FileCheck %s --check-prefix=NONVAR
+; RUN: llvm-link -S %t/non-var.ll %t/non-var.ll 2>&1 | FileCheck %s --check-prefix=NONVAR
-; NONVAR: error: Linking COMDATs named 'foo': GlobalVariable required for data dependent selection!
+; NONVAR: linking 'foo': non-variables in comdat nodeduplicate are not handled
;--- 1.ll
$foo = comdat nodeduplicate
@@ -30,7 +33,7 @@ $foo = comdat nodeduplicate
;--- 2-aux.ll
$foo = comdat nodeduplicate
- at foo = weak global i64 0, section "data", comdat($foo)
+ at foo = weak hidden global i64 0, section "data", comdat($foo)
@bar = dso_local global i64 3, section "cnts", comdat($foo), align 16
@fred = linkonce global i64 5, comdat($foo)
More information about the llvm-commits
mailing list