[Linker] Handle comdat nodeduplicate

[MaskRay]

Summary

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.

[MaskRay]

rename variable

[phosek]

LGTM

llvm/lib/Linker/LinkModules.cpp

 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Linker/Linker.h"
 #include "llvm/Support/Error.h"
 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.
 class ModuleLinker {
   IRMover &Mover;
   std::unique_ptr<Module> SrcM;
 
   SetVector<GlobalValue *> ValuesToLink;
GlobalValue *getLinkedToGlobal(const GlobalValue *SrcGV) {
     return DGV;
   }
 
   /// Drop GV if it is a member of a comdat that we are dropping.
   /// This can happen with COFF's largest selection kind.
   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,
                std::function<void(Module &, const StringSet<> &)>
                    InternalizeCallback = {})
       : Mover(Mover), SrcM(std::move(SrcM)), Flags(Flags),
         InternalizeCallback(std::move(InternalizeCallback)) {}
 
return emitError("Linking COMDATs named '" + ComdatName +
                      "': invalid selection kinds!");
   }
 
   switch (Result) {
   case Comdat::SelectionKind::Any:
     // 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: {
     const GlobalVariable *DstGV;
     const GlobalVariable *SrcGV;
     if (getComdatLeader(DstM, ComdatName, DstGV) ||
         getComdatLeader(*SrcM, ComdatName, SrcGV))
       return true;
bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
   assert(!Src.hasExternalWeakLinkage());
   assert(!Dest.hasExternalWeakLinkage());
   assert(Dest.hasExternalLinkage() && Src.hasExternalLinkage() &&
          "Unexpected linkage type!");
   return emitError("Linking globals named '" + Src.getName() +
                    "': symbol multiply defined!");
 }
 
-bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
+bool ModuleLinker::linkIfNeeded(GlobalValue &GV,
+                                SmallVectorImpl<GlobalValue *> &GVToClone) {
   GlobalValue *DGV = getLinkedToGlobal(&GV);
 
   if (shouldLinkOnlyNeeded()) {
     // Always import variables with appending linkage.
     if (!GV.hasAppendingLinkage()) {
       // Don't import globals unless they are referenced by the destination
       // module.
       if (!DGV)
if (const Comdat *SC = GV.getComdat()) {
     std::tie(std::ignore, ComdatFrom) = ComdatsChosen[SC];
     if (ComdatFrom == LinkFrom::Dst)
       return false;
   }
 
   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;
 }
 
 void ModuleLinker::addLazyFor(GlobalValue &GV, const IRMover::ValueAdder &Add) {
   // Add these to the internalize list
   if (!GV.hasLinkOnceLinkage() && !GV.hasAvailableExternallyLinkage() &&
bool ModuleLinker::run() {
 
   for (GlobalAlias &GA : SrcM->aliases())
     if (GA.hasLinkOnceLinkage())
       if (const Comdat *SC = GA.getComdat())
         LazyComdatMembers[SC].push_back(&GA);
 
   // 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();
     if (!SC)
       continue;
     for (GlobalValue *GV2 : LazyComdatMembers[SC]) {
       GlobalValue *DGV = getLinkedToGlobal(GV2);
       bool LinkFromSrc = true;

llvm/test/Linker/comdat-nodeduplicate.ll

 ; 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
 @foo = global i64 43, comdat($foo)
 
 ;--- 1-aux.ll
 $foo = comdat nodeduplicate
 @foo = global i64 43, comdat($foo)
 
 ;--- 2.ll
 $foo = comdat nodeduplicate
 @foo = global i64 2, section "data", comdat($foo), align 8
 @bar = weak global i64 0, section "cnts", comdat($foo)
 @qux = weak_odr global i64 4, comdat($foo)
 
 ;--- 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)
 
 ;--- non-var.ll
 $foo = comdat nodeduplicate
 define weak void @foo() comdat {
   ret void
 }