<div dir="ltr">Rafael,<div><br></div><div>I believe this brakes sanitizer-windows build:</div><div><br></div><div><a href="http://lab.llvm.org:8011/builders/sanitizer-windows/builds/13583" class="cremed">http://lab.llvm.org:8011/builders/sanitizer-windows/builds/13583</a></div><div><br></div><div><a href="http://lab.llvm.org:8011/builders/sanitizer-windows/builds/13583/steps/run%20tests/logs/stdio" class="cremed">http://lab.llvm.org:8011/builders/sanitizer-windows/builds/13583/steps/run%20tests/logs/stdio</a></div><div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 30, 2015 at 2:04 PM Rafael Espindola via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rafael<br>
Date: Mon Nov 30 16:01:43 2015<br>
New Revision: 254336<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=254336&view=rev" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=254336&view=rev</a><br>
Log:<br>
Start deciding earlier what to link.<br>
<br>
A traditional linker is roughly split in symbol resolution and "copying<br>
stuff".<br>
<br>
The two tasks are badly mixed in lib/Linker.<br>
<br>
This starts splitting them apart.<br>
<br>
With this patch there are no direct call to linkGlobalValueBody or<br>
linkGlobalValueProto. Everything is linked via WapValue.<br>
<br>
This also includes a few fixes:<br>
* A GV goes undefined if the comdat is dropped (comdat11.ll).<br>
* We error if an internal GV goes undefined (comdat13.ll).<br>
* We don't link an unused comdat.<br>
<br>
The first two match the behavior of an ELF linker. The second one is<br>
equivalent to running globaldce on the input.<br>
<br>
Added:<br>
llvm/trunk/test/Linker/Inputs/comdat11.ll<br>
llvm/trunk/test/Linker/Inputs/comdat13.ll<br>
llvm/trunk/test/Linker/comdat11.ll<br>
llvm/trunk/test/Linker/comdat12.ll<br>
llvm/trunk/test/Linker/comdat13.ll<br>
Modified:<br>
llvm/trunk/lib/Linker/LinkModules.cpp<br>
llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>
llvm/trunk/test/Linker/comdat9.ll<br>
<br>
Modified: llvm/trunk/lib/Linker/LinkModules.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=254336&r1=254335&r2=254336&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=254336&r1=254335&r2=254336&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Linker/LinkModules.cpp (original)<br>
+++ llvm/trunk/lib/Linker/LinkModules.cpp Mon Nov 30 16:01:43 2015<br>
@@ -436,6 +436,8 @@ class ModuleLinker {<br>
/// references.<br>
bool DoneLinkingBodies;<br>
<br>
+ bool HasError = false;<br>
+<br>
public:<br>
ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM,<br>
DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags,<br>
@@ -483,6 +485,7 @@ private:<br>
/// Helper method for setting a message and returning an error code.<br>
bool emitError(const Twine &Message) {<br>
DiagnosticHandler(LinkDiagnosticInfo(DS_Error, Message));<br>
+ HasError = true;<br>
return true;<br>
}<br>
<br>
@@ -531,6 +534,7 @@ private:<br>
void upgradeMismatchedGlobalArray(StringRef Name);<br>
void upgradeMismatchedGlobals();<br>
<br>
+ bool linkIfNeeded(GlobalValue &GV);<br>
bool linkAppendingVarProto(GlobalVariable *DstGV,<br>
const GlobalVariable *SrcGV);<br>
<br>
@@ -904,16 +908,12 @@ Value *ModuleLinker::materializeDeclFor(<br>
if (doneLinkingBodies())<br>
return nullptr;<br>
<br>
- GlobalValue *DGV = copyGlobalValueProto(TypeMap, SGV);<br>
-<br>
- if (Comdat *SC = SGV->getComdat()) {<br>
- if (auto *DGO = dyn_cast<GlobalObject>(DGV)) {<br>
- Comdat *DC = DstM->getOrInsertComdat(SC->getName());<br>
- DGO->setComdat(DC);<br>
- }<br>
- }<br>
-<br>
- return DGV;<br>
+ linkGlobalValueProto(SGV);<br>
+ if (HasError)<br>
+ return nullptr;<br>
+ Value *Ret = ValueMap[SGV];<br>
+ assert(Ret);<br>
+ return Ret;<br>
}<br>
<br>
void ValueMaterializerTy::materializeInitFor(GlobalValue *New,<br>
@@ -922,15 +922,31 @@ void ValueMaterializerTy::materializeIni<br>
}<br>
<br>
void ModuleLinker::materializeInitFor(GlobalValue *New, GlobalValue *Old) {<br>
+ if (auto *F = dyn_cast<Function>(New)) {<br>
+ if (!F->isDeclaration())<br>
+ return;<br>
+ } else if (auto *V = dyn_cast<GlobalVariable>(New)) {<br>
+ if (V->hasInitializer())<br>
+ return;<br>
+ } else {<br>
+ auto *A = cast<GlobalAlias>(New);<br>
+ if (A->getAliasee())<br>
+ return;<br>
+ }<br>
+<br>
+ if (Old->isDeclaration())<br>
+ return;<br>
+<br>
if (isPerformingImport() && !doImportAsDefinition(Old))<br>
return;<br>
<br>
- // Skip declarations that ValueMaterializer may have created in<br>
- // case we link in only some of SrcM.<br>
- if (shouldLinkOnlyNeeded() && Old->isDeclaration())<br>
+ if (DoNotLinkFromSource.count(Old)) {<br>
+ if (!New->hasExternalLinkage() && !New->hasExternalWeakLinkage() &&<br>
+ !New->hasAppendingLinkage())<br>
+ emitError("Declaration points to discarded value");<br>
return;<br>
+ }<br>
<br>
- assert(!Old->isDeclaration() && "users should not pass down decls");<br>
linkGlobalValueBody(*Old);<br>
}<br>
<br>
@@ -1405,7 +1421,6 @@ bool ModuleLinker::linkGlobalValueProto(<br>
std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];<br>
C = DstM->getOrInsertComdat(SC->getName());<br>
C->setSelectionKind(SK);<br>
- ComdatMembers[SC].push_back(SGV);<br>
} else if (DGV) {<br>
if (shouldLinkFromSource(LinkFromSrc, *DGV, *SGV))<br>
return true;<br>
@@ -1425,31 +1440,12 @@ bool ModuleLinker::linkGlobalValueProto(<br>
if (DGV)<br>
HasUnnamedAddr = HasUnnamedAddr && DGV->hasUnnamedAddr();<br>
<br>
- if (!LinkFromSrc && !DGV)<br>
- return false;<br>
-<br>
GlobalValue *NewGV;<br>
- if (!LinkFromSrc) {<br>
+ if (!LinkFromSrc && DGV) {<br>
NewGV = DGV;<br>
// When linking from source we setVisibility from copyGlobalValueProto.<br>
setVisibility(NewGV, SGV, DGV);<br>
} else {<br>
- // If the GV is to be lazily linked, don't create it just yet.<br>
- // The ValueMaterializerTy will deal with creating it if it's used.<br>
- if (!DGV && !shouldOverrideFromSrc() && SGV != ImportFunction &&<br>
- (SGV->hasLocalLinkage() || SGV->hasLinkOnceLinkage() ||<br>
- SGV->hasAvailableExternallyLinkage())) {<br>
- DoNotLinkFromSource.insert(SGV);<br>
- return false;<br>
- }<br>
-<br>
- // When we only want to link in unresolved dependencies, blacklist<br>
- // the symbol unless unless DestM has a matching declaration (DGV).<br>
- if (shouldLinkOnlyNeeded() && !(DGV && DGV->isDeclaration())) {<br>
- DoNotLinkFromSource.insert(SGV);<br>
- return false;<br>
- }<br>
-<br>
NewGV = copyGlobalValueProto(TypeMap, SGV, DGV);<br>
<br>
if (isPerformingImport() && !doImportAsDefinition(SGV))<br>
@@ -1459,7 +1455,7 @@ bool ModuleLinker::linkGlobalValueProto(<br>
NewGV->setUnnamedAddr(HasUnnamedAddr);<br>
<br>
if (auto *NewGO = dyn_cast<GlobalObject>(NewGV)) {<br>
- if (C)<br>
+ if (C && LinkFromSrc)<br>
NewGO->setComdat(C);<br>
<br>
if (DGV && DGV->hasCommonLinkage() && SGV->hasCommonLinkage())<br>
@@ -1842,6 +1838,38 @@ static std::string mergeTriples(const Tr<br>
return DstTriple.str();<br>
}<br>
<br>
+bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {<br>
+ GlobalValue *DGV = getLinkedToGlobal(&GV);<br>
+<br>
+ if (shouldLinkOnlyNeeded() && !(DGV && DGV->isDeclaration()))<br>
+ return false;<br>
+<br>
+ if (DGV && !GV.hasLocalLinkage()) {<br>
+ GlobalValue::VisibilityTypes Visibility =<br>
+ getMinVisibility(DGV->getVisibility(), GV.getVisibility());<br>
+ DGV->setVisibility(Visibility);<br>
+ GV.setVisibility(Visibility);<br>
+ }<br>
+<br>
+ if (const Comdat *SC = GV.getComdat()) {<br>
+ bool LinkFromSrc;<br>
+ Comdat::SelectionKind SK;<br>
+ std::tie(SK, LinkFromSrc) = ComdatsChosen[SC];<br>
+ if (!LinkFromSrc) {<br>
+ DoNotLinkFromSource.insert(&GV);<br>
+ return false;<br>
+ }<br>
+ }<br>
+<br>
+ if (!DGV && !shouldOverrideFromSrc() &&<br>
+ (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||<br>
+ GV.hasAvailableExternallyLinkage())) {<br>
+ return false;<br>
+ }<br>
+ MapValue(&GV, ValueMap, RF_MoveDistinctMDs, &TypeMap, &ValMaterializer);<br>
+ return HasError;<br>
+}<br>
+<br>
bool ModuleLinker::run() {<br>
assert(DstM && "Null destination module");<br>
assert(SrcM && "Null source module");<br>
@@ -1901,24 +1929,30 @@ bool ModuleLinker::run() {<br>
// Upgrade mismatched global arrays.<br>
upgradeMismatchedGlobals();<br>
<br>
+ for (GlobalVariable &GV : SrcM->globals())<br>
+ if (const Comdat *SC = GV.getComdat())<br>
+ ComdatMembers[SC].push_back(&GV);<br>
+<br>
+ for (Function &SF : *SrcM)<br>
+ if (const Comdat *SC = SF.getComdat())<br>
+ ComdatMembers[SC].push_back(&SF);<br>
+<br>
+ for (GlobalAlias &GA : SrcM->aliases())<br>
+ if (const Comdat *SC = GA.getComdat())<br>
+ ComdatMembers[SC].push_back(&GA);<br>
+<br>
// Insert all of the globals in src into the DstM module... without linking<br>
// initializers (which could refer to functions not yet mapped over).<br>
for (GlobalVariable &GV : SrcM->globals())<br>
- if (linkGlobalValueProto(&GV))<br>
+ if (linkIfNeeded(GV))<br>
return true;<br>
<br>
- // Link the functions together between the two modules, without doing function<br>
- // bodies... this just adds external function prototypes to the DstM<br>
- // function... We do this so that when we begin processing function bodies,<br>
- // all of the global values that may be referenced are available in our<br>
- // ValueMap.<br>
- for (Function &F :*SrcM)<br>
- if (linkGlobalValueProto(&F))<br>
+ for (Function &SF : *SrcM)<br>
+ if (linkIfNeeded(SF))<br>
return true;<br>
<br>
- // If there were any aliases, link them now.<br>
for (GlobalAlias &GA : SrcM->aliases())<br>
- if (linkGlobalValueProto(&GA))<br>
+ if (linkIfNeeded(GA))<br>
return true;<br>
<br>
for (AppendingVarInfo &AppendingVar : AppendingVars)<br>
@@ -1933,37 +1967,6 @@ bool ModuleLinker::run() {<br>
MapValue(GV, ValueMap, RF_MoveDistinctMDs, &TypeMap, &ValMaterializer);<br>
}<br>
<br>
- // Link in the function bodies that are defined in the source module into<br>
- // DstM.<br>
- for (Function &SF : *SrcM) {<br>
- // Skip if no body (function is external).<br>
- if (SF.isDeclaration())<br>
- continue;<br>
-<br>
- // Skip if not linking from source.<br>
- if (DoNotLinkFromSource.count(&SF))<br>
- continue;<br>
-<br>
- if (linkGlobalValueBody(SF))<br>
- return true;<br>
- }<br>
-<br>
- // Resolve all uses of aliases with aliasees.<br>
- for (GlobalAlias &Src : SrcM->aliases()) {<br>
- if (DoNotLinkFromSource.count(&Src))<br>
- continue;<br>
- linkGlobalValueBody(Src);<br>
- }<br>
-<br>
- // Update the initializers in the DstM module now that all globals that may<br>
- // be referenced are in DstM.<br>
- for (GlobalVariable &Src : SrcM->globals()) {<br>
- // Only process initialized GV's or ones not already in dest.<br>
- if (!Src.hasInitializer() || DoNotLinkFromSource.count(&Src))<br>
- continue;<br>
- linkGlobalValueBody(Src);<br>
- }<br>
-<br>
// Note that we are done linking global value bodies. This prevents<br>
// metadata linking from creating new references.<br>
DoneLinkingBodies = true;<br>
<br>
Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=254336&r1=254335&r2=254336&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=254336&r1=254335&r2=254336&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Mon Nov 30 16:01:43 2015<br>
@@ -41,9 +41,9 @@ Value *llvm::MapValue(const Value *V, Va<br>
if (Value *NewV =<br>
Materializer->materializeDeclFor(const_cast<Value *>(V))) {<br>
VM[V] = NewV;<br>
- if (auto *GV = dyn_cast<GlobalValue>(V))<br>
- Materializer->materializeInitFor(cast<GlobalValue>(NewV),<br>
- const_cast<GlobalValue *>(GV));<br>
+ if (auto *NewGV = dyn_cast<GlobalValue>(NewV))<br>
+ Materializer->materializeInitFor(<br>
+ NewGV, const_cast<GlobalValue *>(cast<GlobalValue>(V)));<br>
return NewV;<br>
}<br>
}<br>
<br>
Added: llvm/trunk/test/Linker/Inputs/comdat11.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/comdat11.ll?rev=254336&view=auto" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/comdat11.ll?rev=254336&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Linker/Inputs/comdat11.ll (added)<br>
+++ llvm/trunk/test/Linker/Inputs/comdat11.ll Mon Nov 30 16:01:43 2015<br>
@@ -0,0 +1,9 @@<br>
+$foo = comdat any<br>
+@foo = global i8 1, comdat<br>
+define void @zed() {<br>
+ call void @bar()<br>
+ ret void<br>
+}<br>
+define void @bar() comdat($foo) {<br>
+ ret void<br>
+}<br>
<br>
Added: llvm/trunk/test/Linker/Inputs/comdat13.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/comdat13.ll?rev=254336&view=auto" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/comdat13.ll?rev=254336&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Linker/Inputs/comdat13.ll (added)<br>
+++ llvm/trunk/test/Linker/Inputs/comdat13.ll Mon Nov 30 16:01:43 2015<br>
@@ -0,0 +1,9 @@<br>
+$foo = comdat any<br>
+@foo = global i8 1, comdat<br>
+define void @zed() {<br>
+ call void @bar()<br>
+ ret void<br>
+}<br>
+define internal void @bar() comdat($foo) {<br>
+ ret void<br>
+}<br>
<br>
Added: llvm/trunk/test/Linker/comdat11.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat11.ll?rev=254336&view=auto" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat11.ll?rev=254336&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Linker/comdat11.ll (added)<br>
+++ llvm/trunk/test/Linker/comdat11.ll Mon Nov 30 16:01:43 2015<br>
@@ -0,0 +1,13 @@<br>
+; RUN: llvm-link -S %s %p/Inputs/comdat11.ll -o - | FileCheck %s<br>
+<br>
+$foo = comdat any<br>
+@foo = global i8 0, comdat<br>
+<br>
+; CHECK: @foo = global i8 0, comdat<br>
+<br>
+; CHECK: define void @zed() {<br>
+; CHECK: call void @bar()<br>
+; CHECK: ret void<br>
+; CHECK: }<br>
+<br>
+; CHECK: declare void @bar()<br>
<br>
Added: llvm/trunk/test/Linker/comdat12.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat12.ll?rev=254336&view=auto" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat12.ll?rev=254336&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Linker/comdat12.ll (added)<br>
+++ llvm/trunk/test/Linker/comdat12.ll Mon Nov 30 16:01:43 2015<br>
@@ -0,0 +1,8 @@<br>
+; RUN: llvm-link %s -S -o - | FileCheck %s<br>
+<br>
+$foo = comdat largest<br>
+define internal void @foo() comdat($foo) {<br>
+ ret void<br>
+}<br>
+<br>
+; CHECK-NOT: foo<br>
<br>
Added: llvm/trunk/test/Linker/comdat13.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat13.ll?rev=254336&view=auto" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat13.ll?rev=254336&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Linker/comdat13.ll (added)<br>
+++ llvm/trunk/test/Linker/comdat13.ll Mon Nov 30 16:01:43 2015<br>
@@ -0,0 +1,13 @@<br>
+; RUN: not llvm-link -S %s %p/Inputs/comdat13.ll -o %t.ll 2>&1 | FileCheck %s<br>
+<br>
+; In Inputs/comdat13.ll a function not in the $foo comdat (zed) references an<br>
+; internal function in the comdat $foo.<br>
+; We might want to have the verifier reject that, but for now we at least check<br>
+; that the linker produces an error.<br>
+; This is the IR equivalent of the "relocation refers to discarded section" in<br>
+; an ELF linker.<br>
+<br>
+; CHECK: Declaration points to discarded value<br>
+<br>
+$foo = comdat any<br>
+@foo = global i8 0, comdat<br>
<br>
Modified: llvm/trunk/test/Linker/comdat9.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat9.ll?rev=254336&r1=254335&r2=254336&view=diff" rel="noreferrer" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/comdat9.ll?rev=254336&r1=254335&r2=254336&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Linker/comdat9.ll (original)<br>
+++ llvm/trunk/test/Linker/comdat9.ll Mon Nov 30 16:01:43 2015<br>
@@ -14,6 +14,9 @@ $f2 = comdat largest<br>
define internal void @f2() comdat($f2) {<br>
ret void<br>
}<br>
+define void @f3() comdat($f2) {<br>
+ ret void<br>
+}<br>
<br>
; CHECK-DAG: $f2 = comdat largest<br>
; CHECK-DAG: define internal void @f2() comdat {<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="cremed">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="cremed">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div>