Drop comdats from the dst module if they are not selected.

A really unfortunate design of llvm-link and related libraries is that
they operate one module at a time.

This means they can copy a GV to the destination module that should not
be there in the final result because a later bitcode file takes
precedence.

We already handled cases like a strong GV replacing a weak for example.

One case that is not currently handled is a comdat replacing another.
This doesn't happen in ELF, but with COFF largest selection kind it is
possible.

In "llvm-link a.ll b.ll" if the selected comdat was from a.ll,
everything will work and we will not copy the comdat from b.ll.

But if we run "llvm-link b.ll a.ll", we fail to delete the already
copied comdat from b.ll. This patch fixes that.

llvm-svn: 264103

llvm/lib/Linker/LinkModules.cpp

@@ -102,6 +102,11 @@ class ModuleLinker {
     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);
 
   /// Helper method to check if we are importing from the current source
@@ -449,7 +454,45 @@ void ModuleLinker::addLazyFor(GlobalValue &GV, IRMover::ValueAdder Add) {
   }
 }
 
+void ModuleLinker::dropReplacedComdat(
+    GlobalValue &GV, const DenseSet<const Comdat *> &ReplacedDstComdats) {
+  Comdat *C = GV.getComdat();
+  if (!C)
+    return;
+  if (!ReplacedDstComdats.count(C))
+    return;
+  if (GV.use_empty()) {
+    GV.eraseFromParent();
+    return;
+  }
+
+  if (auto *F = dyn_cast<Function>(&GV)) {
+    F->deleteBody();
+  } else if (auto *Var = dyn_cast<GlobalVariable>(&GV)) {
+    Var->setInitializer(nullptr);
+  } else {
+    auto &Alias = cast<GlobalAlias>(GV);
+    Module &M = *Alias.getParent();
+    PointerType &Ty = *cast<PointerType>(Alias.getType());
+    GlobalValue *Declaration;
+    if (auto *FTy = dyn_cast<FunctionType>(Alias.getValueType())) {
+      Declaration = Function::Create(FTy, GlobalValue::ExternalLinkage, "", &M);
+    } else {
+      Declaration =
+          new GlobalVariable(M, Ty.getElementType(), /*isConstant*/ false,
+                             GlobalValue::ExternalLinkage,
+                             /*Initializer*/ nullptr);
+    }
+    Declaration->takeName(&Alias);
+    Alias.replaceAllUsesWith(Declaration);
+    Alias.eraseFromParent();
+  }
+}
+
 bool ModuleLinker::run() {
+  Module &DstM = Mover.getModule();
+  DenseSet<const Comdat *> ReplacedDstComdats;
+
   for (const auto &SMEC : SrcM->getComdatSymbolTable()) {
     const Comdat &C = SMEC.getValue();
     if (ComdatsChosen.count(&C))
@@ -459,6 +502,35 @@ bool ModuleLinker::run() {
     if (getComdatResult(&C, SK, LinkFromSrc))
       return true;
     ComdatsChosen[&C] = std::make_pair(SK, LinkFromSrc);
+
+    if (!LinkFromSrc)
+      continue;
+
+    Module::ComdatSymTabType &ComdatSymTab = DstM.getComdatSymbolTable();
+    Module::ComdatSymTabType::iterator DstCI = ComdatSymTab.find(C.getName());
+    if (DstCI == ComdatSymTab.end())
+      continue;
+
+    // The source comdat is replacing the dest one.
+    const Comdat *DstC = &DstCI->second;
+    ReplacedDstComdats.insert(DstC);
+  }
+
+  // Alias have to go first, since we are not able to find their comdats
+  // otherwise.
+  for (auto I = DstM.alias_begin(), E = DstM.alias_end(); I != E;) {
+    GlobalAlias &GV = *I++;
+    dropReplacedComdat(GV, ReplacedDstComdats);
+  }
+
+  for (auto I = DstM.global_begin(), E = DstM.global_end(); I != E;) {
+    GlobalVariable &GV = *I++;
+    dropReplacedComdat(GV, ReplacedDstComdats);
+  }
+
+  for (auto I = DstM.begin(), E = DstM.end(); I != E;) {
+    Function &GV = *I++;
+    dropReplacedComdat(GV, ReplacedDstComdats);
   }
 
   for (GlobalVariable &GV : SrcM->globals())
@@ -507,7 +579,6 @@ bool ModuleLinker::run() {
                  },
                  ValIDToTempMDMap, false))
     return true;
-  Module &DstM = Mover.getModule();
   for (auto &P : Internalize) {
     GlobalValue *GV = DstM.getNamedValue(P.first());
     GV->setLinkage(GlobalValue::InternalLinkage);

llvm/test/Linker/Inputs/comdat-rm-dst.ll

@@ -0,0 +1,5 @@
+target datalayout = "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32"
+target triple = "i686-pc-windows-msvc"
+
+$foo = comdat largest
+@foo = global i64 43, comdat

llvm/test/Linker/comdat-rm-dst.ll

@@ -0,0 +1,33 @@
+; RUN: llvm-link -S -o %t %s %p/Inputs/comdat-rm-dst.ll
+; RUN: FileCheck %s < %t
+; RUN: FileCheck --check-prefix=RM %s < %t
+
+target datalayout = "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32"
+target triple = "i686-pc-windows-msvc"
+
+$foo = comdat largest
+@foo = global i32 42, comdat
+; CHECK-DAG: @foo = global i64 43, comdat
+
+; RM-NOT: @alias =
+@alias = alias i32, i32* @foo
+
+; We should arguably reject an out of comdat reference to int_alias. Given that
+; the verifier accepts it, test that we at least produce an output that passes
+; the verifier.
+; CHECK-DAG: @int_alias = external global i32
+@int_alias = internal alias i32, i32* @foo
+@bar = global i32* @int_alias
+
+@func_alias = alias void (), void ()* @func
+@zed = global void()* @func_alias
+; CHECK-DAG: @zed = global void ()* @func_alias
+; CHECK-DAG: declare void @func_alias()
+
+; RM-NOT: @func()
+define void @func() comdat($foo) {
+  ret void
+}
+
+; RM-NOT: var
+@var = global i32 42, comdat($foo)