Bring r254336 back:

The difference is that now we don't error on out-of-comdat access to
internal global values. We copy them instead. This seems to match the
expectation of COFF linkers (see pr25686).

Original message:

    Start deciding earlier what to link.

    A traditional linker is roughly split in symbol resolution and
"copying
    stuff".

    The two tasks are badly mixed in lib/Linker.

    This starts splitting them apart.

    With this patch there are no direct call to linkGlobalValueBody or
    linkGlobalValueProto. Everything is linked via WapValue.

    This also includes a few fixes:
    * A GV goes undefined if the comdat is dropped (comdat11.ll).
    * We error if an internal GV goes undefined (comdat13.ll).
    * We don't link an unused comdat.

    The first two match the behavior of an ELF linker. The second one is
    equivalent to running globaldce on the input.

llvm-svn: 254418

lib/Linker/LinkModules.cpp

@@ -436,6 +436,8 @@ class ModuleLinker {
   /// references.
   bool DoneLinkingBodies;
 
+  bool HasError = false;
+
 public:
   ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM,
                DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags,
@@ -483,6 +485,7 @@ private:
   /// Helper method for setting a message and returning an error code.
   bool emitError(const Twine &Message) {
     DiagnosticHandler(LinkDiagnosticInfo(DS_Error, Message));
+    HasError = true;
     return true;
   }
 
@@ -531,6 +534,7 @@ private:
   void upgradeMismatchedGlobalArray(StringRef Name);
   void upgradeMismatchedGlobals();
 
+  bool linkIfNeeded(GlobalValue &GV);
   bool linkAppendingVarProto(GlobalVariable *DstGV,
                              const GlobalVariable *SrcGV);
 
@@ -904,16 +908,12 @@ Value *ModuleLinker::materializeDeclFor(Value *V) {
   if (doneLinkingBodies())
     return nullptr;
 
-  GlobalValue *DGV = copyGlobalValueProto(TypeMap, SGV);
+  linkGlobalValueProto(SGV);
-
+  if (HasError)
-  if (Comdat *SC = SGV->getComdat()) {
+    return nullptr;
-    if (auto *DGO = dyn_cast<GlobalObject>(DGV)) {
+  Value *Ret = ValueMap[SGV];
-      Comdat *DC = DstM->getOrInsertComdat(SC->getName());
+  assert(Ret);
-      DGO->setComdat(DC);
+  return Ret;
-    }
-  }
-
-  return DGV;
 }
 
 void ValueMaterializerTy::materializeInitFor(GlobalValue *New,
@@ -922,15 +922,27 @@ void ValueMaterializerTy::materializeInitFor(GlobalValue *New,
 }
 
 void ModuleLinker::materializeInitFor(GlobalValue *New, GlobalValue *Old) {
+  if (auto *F = dyn_cast<Function>(New)) {
+    if (!F->isDeclaration())
+      return;
+  } else if (auto *V = dyn_cast<GlobalVariable>(New)) {
+    if (V->hasInitializer())
+      return;
+  } else {
+    auto *A = cast<GlobalAlias>(New);
+    if (A->getAliasee())
+      return;
+  }
+
+  if (Old->isDeclaration())
+    return;
+
   if (isPerformingImport() && !doImportAsDefinition(Old))
     return;
 
-  // Skip declarations that ValueMaterializer may have created in
+  if (!New->hasLocalLinkage() && DoNotLinkFromSource.count(Old))
-  // case we link in only some of SrcM.
-  if (shouldLinkOnlyNeeded() && Old->isDeclaration())
     return;
 
-  assert(!Old->isDeclaration() && "users should not pass down decls");
   linkGlobalValueBody(*Old);
 }
 
@@ -1405,7 +1417,8 @@ bool ModuleLinker::linkGlobalValueProto(GlobalValue *SGV) {
     std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
     C = DstM->getOrInsertComdat(SC->getName());
     C->setSelectionKind(SK);
-    ComdatMembers[SC].push_back(SGV);
+    if (SGV->hasInternalLinkage())
+      LinkFromSrc = true;
   } else if (DGV) {
     if (shouldLinkFromSource(LinkFromSrc, *DGV, *SGV))
       return true;
@@ -1425,31 +1438,12 @@ bool ModuleLinker::linkGlobalValueProto(GlobalValue *SGV) {
   if (DGV)
     HasUnnamedAddr = HasUnnamedAddr && DGV->hasUnnamedAddr();
 
-  if (!LinkFromSrc && !DGV)
-    return false;
-
   GlobalValue *NewGV;
-  if (!LinkFromSrc) {
+  if (!LinkFromSrc && DGV) {
     NewGV = DGV;
     // When linking from source we setVisibility from copyGlobalValueProto.
     setVisibility(NewGV, SGV, DGV);
   } else {
-    // If the GV is to be lazily linked, don't create it just yet.
-    // The ValueMaterializerTy will deal with creating it if it's used.
-    if (!DGV && !shouldOverrideFromSrc() && SGV != ImportFunction &&
-        (SGV->hasLocalLinkage() || SGV->hasLinkOnceLinkage() ||
-         SGV->hasAvailableExternallyLinkage())) {
-      DoNotLinkFromSource.insert(SGV);
-      return false;
-    }
-
-    // When we only want to link in unresolved dependencies, blacklist
-    // the symbol unless unless DestM has a matching declaration (DGV).
-    if (shouldLinkOnlyNeeded() && !(DGV && DGV->isDeclaration())) {
-      DoNotLinkFromSource.insert(SGV);
-      return false;
-    }
-
     NewGV = copyGlobalValueProto(TypeMap, SGV, DGV);
 
     if (isPerformingImport() && !doImportAsDefinition(SGV))
@@ -1459,7 +1453,7 @@ bool ModuleLinker::linkGlobalValueProto(GlobalValue *SGV) {
   NewGV->setUnnamedAddr(HasUnnamedAddr);
 
   if (auto *NewGO = dyn_cast<GlobalObject>(NewGV)) {
-    if (C)
+    if (C && LinkFromSrc)
       NewGO->setComdat(C);
 
     if (DGV && DGV->hasCommonLinkage() && SGV->hasCommonLinkage())
@@ -1842,6 +1836,38 @@ static std::string mergeTriples(const Triple &SrcTriple, const Triple &DstTriple
   return DstTriple.str();
 }
 
+bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
+  GlobalValue *DGV = getLinkedToGlobal(&GV);
+
+  if (shouldLinkOnlyNeeded() && !(DGV && DGV->isDeclaration()))
+    return false;
+
+  if (DGV && !GV.hasLocalLinkage()) {
+    GlobalValue::VisibilityTypes Visibility =
+        getMinVisibility(DGV->getVisibility(), GV.getVisibility());
+    DGV->setVisibility(Visibility);
+    GV.setVisibility(Visibility);
+  }
+
+  if (const Comdat *SC = GV.getComdat()) {
+    bool LinkFromSrc;
+    Comdat::SelectionKind SK;
+    std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];
+    if (!LinkFromSrc) {
+      DoNotLinkFromSource.insert(&GV);
+      return false;
+    }
+  }
+
+  if (!DGV && !shouldOverrideFromSrc() &&
+      (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
+       GV.hasAvailableExternallyLinkage())) {
+    return false;
+  }
+  MapValue(&GV, ValueMap, RF_MoveDistinctMDs, &TypeMap, &ValMaterializer);
+  return HasError;
+}
+
 bool ModuleLinker::run() {
   assert(DstM && "Null destination module");
   assert(SrcM && "Null source module");
@@ -1901,24 +1927,30 @@ bool ModuleLinker::run() {
   // Upgrade mismatched global arrays.
   upgradeMismatchedGlobals();
 
+  for (GlobalVariable &GV : SrcM->globals())
+    if (const Comdat *SC = GV.getComdat())
+      ComdatMembers[SC].push_back(&GV);
+
+  for (Function &SF : *SrcM)
+    if (const Comdat *SC = SF.getComdat())
+      ComdatMembers[SC].push_back(&SF);
+
+  for (GlobalAlias &GA : SrcM->aliases())
+    if (const Comdat *SC = GA.getComdat())
+      ComdatMembers[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).
   for (GlobalVariable &GV : SrcM->globals())
-    if (linkGlobalValueProto(&GV))
+    if (linkIfNeeded(GV))
       return true;
 
-  // Link the functions together between the two modules, without doing function
+  for (Function &SF : *SrcM)
-  // bodies... this just adds external function prototypes to the DstM
+    if (linkIfNeeded(SF))
-  // function...  We do this so that when we begin processing function bodies,
-  // all of the global values that may be referenced are available in our
-  // ValueMap.
-  for (Function &F :*SrcM)
-    if (linkGlobalValueProto(&F))
       return true;
 
-  // If there were any aliases, link them now.
   for (GlobalAlias &GA : SrcM->aliases())
-    if (linkGlobalValueProto(&GA))
+    if (linkIfNeeded(GA))
       return true;
 
   for (AppendingVarInfo &AppendingVar : AppendingVars)
@@ -1933,37 +1965,6 @@ bool ModuleLinker::run() {
       MapValue(GV, ValueMap, RF_MoveDistinctMDs, &TypeMap, &ValMaterializer);
   }
 
-  // Link in the function bodies that are defined in the source module into
-  // DstM.
-  for (Function &SF : *SrcM) {
-    // Skip if no body (function is external).
-    if (SF.isDeclaration())
-      continue;
-
-    // Skip if not linking from source.
-    if (DoNotLinkFromSource.count(&SF))
-      continue;
-
-    if (linkGlobalValueBody(SF))
-      return true;
-  }
-
-  // Resolve all uses of aliases with aliasees.
-  for (GlobalAlias &Src : SrcM->aliases()) {
-    if (DoNotLinkFromSource.count(&Src))
-      continue;
-    linkGlobalValueBody(Src);
-  }
-
-  // Update the initializers in the DstM module now that all globals that may
-  // be referenced are in DstM.
-  for (GlobalVariable &Src : SrcM->globals()) {
-    // Only process initialized GV's or ones not already in dest.
-    if (!Src.hasInitializer() || DoNotLinkFromSource.count(&Src))
-      continue;
-    linkGlobalValueBody(Src);
-  }
-
   // Note that we are done linking global value bodies. This prevents
   // metadata linking from creating new references.
   DoneLinkingBodies = true;

lib/Transforms/Utils/ValueMapper.cpp

@@ -41,9 +41,9 @@ Value *llvm::MapValue(const Value *V, ValueToValueMapTy &VM, RemapFlags Flags,
     if (Value *NewV =
             Materializer->materializeDeclFor(const_cast<Value *>(V))) {
       VM[V] = NewV;
-      if (auto *GV = dyn_cast<GlobalValue>(V))
+      if (auto *NewGV = dyn_cast<GlobalValue>(NewV))
-        Materializer->materializeInitFor(cast<GlobalValue>(NewV),
+        Materializer->materializeInitFor(
-                                         const_cast<GlobalValue *>(GV));
+            NewGV, const_cast<GlobalValue *>(cast<GlobalValue>(V)));
       return NewV;
     }
   }

test/Linker/Inputs/comdat11.ll

@@ -0,0 +1,9 @@
+$foo = comdat any
+@foo = global i8 1, comdat
+define void @zed() {
+  call void @bar()
+  ret void
+}
+define void @bar() comdat($foo) {
+  ret void
+}

test/Linker/Inputs/comdat13.ll

@@ -0,0 +1,9 @@
+$foo = comdat any
+@foo = internal global i8 1, comdat
+define i8* @zed() {
+  call void @bax()
+  ret i8* @foo
+}
+define internal void @bax() comdat($foo) {
+  ret void
+}

test/Linker/comdat11.ll

@@ -0,0 +1,13 @@
+; RUN: llvm-link -S %s %p/Inputs/comdat11.ll -o - | FileCheck %s
+
+$foo = comdat any
+@foo = global i8 0, comdat
+
+; CHECK: @foo = global i8 0, comdat
+
+; CHECK: define void @zed() {
+; CHECK:   call void @bar()
+; CHECK:   ret void
+; CHECK: }
+
+; CHECK: declare void @bar()

test/Linker/comdat12.ll

@@ -0,0 +1,8 @@
+; RUN: llvm-link %s -S -o - | FileCheck %s
+
+$foo = comdat largest
+define internal void @foo() comdat($foo) {
+  ret void
+}
+
+; CHECK-NOT: foo

test/Linker/comdat13.ll

@@ -0,0 +1,30 @@
+; RUN: llvm-link -S %s %p/Inputs/comdat13.ll -o - | FileCheck %s
+
+; In Inputs/comdat13.ll a function not in the $foo comdat (zed) references an
+; internal function in the comdat $foo.
+; The IR would be ilegal on ELF ("relocation refers to discarded section"),
+; but COFF linkers seem to just duplicate the comdat.
+
+$foo = comdat any
+@foo = internal global i8 0, comdat
+define i8* @bar() {
+       ret i8* @foo
+}
+
+; CHECK: $foo = comdat any
+
+; CHECK: @foo = internal global i8 0, comdat
+; CHECK: @foo.1 = internal global i8 1, comdat($foo)
+
+; CHECK:      define i8* @bar() {
+; CHECK-NEXT:   ret i8* @foo
+; CHECK-NEXT: }
+
+; CHECK:      define i8* @zed() {
+; CHECK-NEXT:   call void @bax()
+; CHECK-NEXT:   ret i8* @foo.1
+; CHECK-NEXT: }
+
+; CHECK:      define internal void @bax() comdat($foo) {
+; CHECK-NEXT:   ret void
+; CHECK-NEXT: }

test/Linker/comdat9.ll

@@ -14,6 +14,9 @@ $f2 = comdat largest
 define internal void @f2() comdat($f2) {
   ret void
 }
+define void @f3() comdat($f2) {
+  ret void
+}
 
 ; CHECK-DAG: $f2 = comdat largest
 ; CHECK-DAG: define internal void @f2() comdat {