[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

Author: MaskRay

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();

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
-@foo = weak global i64 0, section "data", comdat($foo)
+@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)