From 5cb3a1fa7267f9602fbc7a5e50123cccf2fdbbbc Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Fri, 6 Nov 2015 17:50:53 +0000 Subject: [PATCH] Restore "Move metadata linking after lazy global materialization/linking." Summary: This reverts commit r251965. Restore "Move metadata linking after lazy global materialization/linking." This restores commit r251926, with fixes for the LTO bootstrapping bot failure. The bot failure was caused by references from debug metadata to otherwise unreferenced globals. Previously, this caused the lazy linking to link in their defs, which is unnecessary. With this patch, because lazy linking is complete when we encounter the metadata reference, the materializer created a declaration. For definitions such as aliases and comdats, it is illegal to have a declaration. Furthermore, metadata linking should not change code generation. Therefore, when linking of global value bodies is complete, the materializer will simply return nullptr as the new reference for the linked metadata. This change required fixing a different test to ensure there was a real reference to a linkonce global that was only being reference from metadata. Note that the new changes to the only-needed-named-metadata.ll test illustrate an issue with llvm-link -only-needed handling of comdat groups, whereby it may result in an incomplete comdat group. I note this in the test comments, but the issue is orthogonal to this patch (it can be reproduced without any metadata at head). Reviewers: dexonsmith, rafael, tra Subscribers: tobiasvk, joker.eph, llvm-commits Differential Revision: http://reviews.llvm.org/D14447 llvm-svn: 252320 --- lib/Linker/LinkModules.cpp | 21 +++++- .../Inputs/only-needed-named-metadata.ll | 3 + test/Linker/distinct.ll | 2 + test/Linker/only-needed-named-metadata.ll | 69 +++++++++++++++++-- 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp index 74ca197ddb3..010c5611949 100644 --- a/lib/Linker/LinkModules.cpp +++ b/lib/Linker/LinkModules.cpp @@ -440,6 +440,11 @@ class ModuleLinker { /// as part of a different backend compilation process. bool HasExportedFunctions; + /// Set to true when all global value body linking is complete (including + /// lazy linking). Used to prevent metadata linking from creating new + /// references. + bool DoneLinkingBodies; + public: ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM, DiagnosticHandlerFunction DiagnosticHandler, unsigned Flags, @@ -448,7 +453,8 @@ public: : DstM(dstM), SrcM(srcM), TypeMap(Set), ValMaterializer(TypeMap, DstM, LazilyLinkGlobalValues, this), DiagnosticHandler(DiagnosticHandler), Flags(Flags), ImportIndex(Index), - ImportFunction(FuncToImport), HasExportedFunctions(false) { + ImportFunction(FuncToImport), HasExportedFunctions(false), + DoneLinkingBodies(false) { assert((ImportIndex || !ImportFunction) && "Expect a FunctionInfoIndex when importing"); // If we have a FunctionInfoIndex but no function to import, @@ -475,6 +481,9 @@ public: /// Check if we should promote the given local value to global scope. bool doPromoteLocalToGlobal(const GlobalValue *SGV); + /// Check if all global value body linking is complete. + bool doneLinkingBodies() { return DoneLinkingBodies; } + private: bool shouldLinkFromSource(bool &LinkFromSrc, const GlobalValue &Dest, const GlobalValue &Src); @@ -888,6 +897,12 @@ Value *ValueMaterializerTy::materializeValueFor(Value *V) { if (!SGV) return nullptr; + // If we are done linking global value bodies (i.e. we are performing + // metadata linking), don't link in the global value due to this + // reference, simply map it to null. + if (ModLinker->doneLinkingBodies()) + return nullptr; + GlobalValue *DGV = ModLinker->copyGlobalValueProto(TypeMap, SGV); if (Comdat *SC = SGV->getComdat()) { @@ -1918,6 +1933,10 @@ bool ModuleLinker::run() { return true; } + // Note that we are done linking global value bodies. This prevents + // metadata linking from creating new references. + DoneLinkingBodies = true; + // Remap all of the named MDNodes in Src into the DstM module. We do this // after linking GlobalValues so that MDNodes that reference GlobalValues // are properly remapped. diff --git a/test/Linker/Inputs/only-needed-named-metadata.ll b/test/Linker/Inputs/only-needed-named-metadata.ll index fa7bc2e3cc8..95151692d3b 100644 --- a/test/Linker/Inputs/only-needed-named-metadata.ll +++ b/test/Linker/Inputs/only-needed-named-metadata.ll @@ -2,8 +2,11 @@ declare i32 @foo() +declare void @c1_a() + define void @bar() { load i32, i32* @X call i32 @foo() + call void @c1_a() ret void } diff --git a/test/Linker/distinct.ll b/test/Linker/distinct.ll index c8e5c89eb09..d88d8ae16d9 100644 --- a/test/Linker/distinct.ll +++ b/test/Linker/distinct.ll @@ -6,6 +6,8 @@ ; CHECK: @global = linkonce global i32 0 @global = linkonce global i32 0 +; Add an external reference to @global so that it gets linked in. +@alias = alias i32, i32* @global ; CHECK: !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !0, !1, !2, !9, !10, !11, !12, !13, !14} !named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8} diff --git a/test/Linker/only-needed-named-metadata.ll b/test/Linker/only-needed-named-metadata.ll index f64637d3c75..36b42590628 100644 --- a/test/Linker/only-needed-named-metadata.ll +++ b/test/Linker/only-needed-named-metadata.ll @@ -1,16 +1,75 @@ ; RUN: llvm-as %S/only-needed-named-metadata.ll -o %t.bc ; RUN: llvm-as %S/Inputs/only-needed-named-metadata.ll -o %t2.bc -; RUN: llvm-link -S -only-needed %t2.bc %t.bc | FileCheck %s -; RUN: llvm-link -S -internalize -only-needed %t2.bc %t.bc | FileCheck %s -; CHECK: @U = external global i32 -; CHECK: declare i32 @unused() +; Without -only-needed we should lazy link linkonce globals, and the +; metadata reference should not cause them to be linked. +; RUN: llvm-link -S %t2.bc %t.bc | FileCheck %s +; CHECK-NOT:@U_linkonce +; CHECK-NOT:@unused_linkonce() + +; With -only-needed the metadata references should not cause any of the +; otherwise unreferenced globals to be linked. This also ensures that the +; metadata references don't provoke the module linker to create declarations, +; which are illegal for aliases and globals in comdats. +; Note that doing -only-needed with the comdat shown below leads to a only +; part of the comdat group being linked, which is not technically correct. +; RUN: llvm-link -S -only-needed %t2.bc %t.bc | FileCheck %s -check-prefix=ONLYNEEDED +; RUN: llvm-link -S -internalize -only-needed %t2.bc %t.bc | FileCheck %s -check-prefix=ONLYNEEDED +; ONLYNEEDED-NOT:@U +; ONLYNEEDED-NOT:@U_linkonce +; ONLYNEEDED-NOT:@unused() +; ONLYNEEDED-NOT:@unused_linkonce() +; ONLYNEEDED-NOT:@linkoncealias +; ONLYNEEDED-NOT:@linkoncefunc2() +; ONLYNEEDED-NOT:@weakalias +; ONLYNEEDED-NOT:@globalfunc1() +; ONLYNEEDED-NOT:@analias +; ONLYNEEDED-NOT:@globalfunc2() +; ONLYNEEDED-NOT:@c1_c +; ONLYNEEDED-NOT:@c1() + +$c1 = comdat any +@c1_c = global i32 0, comdat($c1) +define void @c1() comdat { + ret void +} +define void @c1_a() comdat($c1) { + ret void +} @X = global i32 5 @U = global i32 6 +@U_linkonce = linkonce_odr hidden global i32 6 define i32 @foo() { ret i32 7 } define i32 @unused() { ret i32 8 } +define linkonce_odr hidden i32 @unused_linkonce() { ret i32 8 } +@linkoncealias = alias void (...), bitcast (void ()* @linkoncefunc2 to void (...)*) -!llvm.named = !{!0, !1} +@weakalias = weak alias void (...), bitcast (void ()* @globalfunc1 to void (...)*) +@analias = alias void (...), bitcast (void ()* @globalfunc2 to void (...)*) + +define void @globalfunc1() #0 { +entry: + ret void +} + +define void @globalfunc2() #0 { +entry: + ret void +} + +$linkoncefunc2 = comdat any +define linkonce_odr void @linkoncefunc2() #0 comdat { +entry: + ret void +} + +!llvm.named = !{!0, !1, !2, !3, !4, !5, !6, !7} !0 = !{i32 ()* @unused} !1 = !{i32* @U} +!2 = !{i32 ()* @unused_linkonce} +!3 = !{i32* @U_linkonce} +!4 = !{void (...)* @weakalias} +!5 = !{void (...)* @analias} +!6 = !{void (...)* @linkoncealias} +!7 = !{void ()* @c1}