From 981c18bd80529926edc98b6b722bcdd25eab1a3a Mon Sep 17 00:00:00 2001 From: Rafael Espindola Date: Tue, 22 Mar 2016 21:35:47 +0000 Subject: [PATCH] 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 --- lib/Linker/LinkModules.cpp | 73 ++++++++++++++++++++++++++++- test/Linker/Inputs/comdat-rm-dst.ll | 5 ++ test/Linker/comdat-rm-dst.ll | 33 +++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 test/Linker/Inputs/comdat-rm-dst.ll create mode 100644 test/Linker/comdat-rm-dst.ll diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp index 2ee0b9664a3..8eeccb369df 100644 --- a/lib/Linker/LinkModules.cpp +++ b/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 &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 &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(&GV)) { + F->deleteBody(); + } else if (auto *Var = dyn_cast(&GV)) { + Var->setInitializer(nullptr); + } else { + auto &Alias = cast(GV); + Module &M = *Alias.getParent(); + PointerType &Ty = *cast(Alias.getType()); + GlobalValue *Declaration; + if (auto *FTy = dyn_cast(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 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); diff --git a/test/Linker/Inputs/comdat-rm-dst.ll b/test/Linker/Inputs/comdat-rm-dst.ll new file mode 100644 index 00000000000..41dccdf2c7c --- /dev/null +++ b/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 diff --git a/test/Linker/comdat-rm-dst.ll b/test/Linker/comdat-rm-dst.ll new file mode 100644 index 00000000000..9722f365632 --- /dev/null +++ b/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)