Restore "Move metadata linking after lazy global materialization/linking."

[tejohnson]

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).

lib/Linker/LinkModules.cpp

class ModuleLinker {
   Function *ImportFunction;
 
   /// Set to true if the given FunctionInfoIndex contains any functions
   /// from this source module, in which case we must conservatively assume
   /// that any of its functions may be imported into another module
   /// 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,
                FunctionInfoIndex *Index = nullptr,
                Function *FuncToImport = nullptr)
       : 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,
     // then this is the primary module being compiled in a ThinLTO
     // backend compilation, and we need to see if it has functions that
     // may be exported to another backend compilation.
     if (ImportIndex && !ImportFunction)
       HasExportedFunctions = ImportIndex->hasExportedFunctions(SrcM);
public:
   /// Handles cloning of a global values from the source module into
   /// the destination module, including setting the attributes and visibility.
   GlobalValue *copyGlobalValueProto(TypeMapTy &TypeMap, const GlobalValue *SGV,
                                     const GlobalValue *DGV = nullptr);
 
   /// 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);
 
   /// Helper method for setting a message and returning an error code.
   bool emitError(const Twine &Message) {
     DiagnosticHandler(LinkDiagnosticInfo(DS_Error, Message));
     return true;
GlobalValue *ModuleLinker::copyGlobalValueProto(TypeMapTy &TypeMap,
   return NewGV;
 }
 
 Value *ValueMaterializerTy::materializeValueFor(Value *V) {
   auto *SGV = dyn_cast<GlobalValue>(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()) {
     if (auto *DGO = dyn_cast<GlobalObject>(DGV)) {
       Comdat *DC = DstM->getOrInsertComdat(SC->getName());
       DGO->setComdat(DC);
     }
   }
bool ModuleLinker::run() {
 
   // Resolve all uses of aliases with aliasees.
   for (GlobalAlias &Src : SrcM->aliases()) {
     if (DoNotLinkFromSource.count(&Src))
       continue;
     linkGlobalValueBody(Src);
   }
 
-  // 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.
-  linkNamedMDNodes();
-
-  // Merge the module flags into the DstM module.
-  if (linkModuleFlagsMetadata())
-    return true;
-
   // 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);
   }
while (!LazilyLinkGlobalValues.empty()) {
     if (shouldLinkOnlyNeeded() && SGV->isDeclaration())
       continue;
 
     assert(!SGV->isDeclaration() && "users should not pass down decls");
     if (linkGlobalValueBody(*SGV))
       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.
+  linkNamedMDNodes();
+
+  // Merge the module flags into the DstM module.
+  if (linkModuleFlagsMetadata())
+    return true;
+
   return false;
 }
 
 Linker::StructTypeKeyInfo::KeyTy::KeyTy(ArrayRef<Type *> E, bool P)
     : ETypes(E), IsPacked(P) {}
 
 Linker::StructTypeKeyInfo::KeyTy::KeyTy(const StructType *ST)
     : ETypes(ST->elements()), IsPacked(ST->isPacked()) {}

test/Linker/Inputs/only-needed-named-metadata.ll

+@X = external global i32
+
+declare i32 @foo()
+
+declare void @c1_a()
+
+define void @bar() {
+  load i32, i32* @X
+  call i32 @foo()
+  call void @c1_a()
+  ret void
+}

test/Linker/distinct.ll

 ; RUN: llvm-link %s %S/Inputs/distinct.ll -o - -S | FileCheck %s
 
 ; Test that distinct nodes from other modules remain distinct.  The @global
 ; cases are the most interesting, since the operands actually need to be
 ; remapped.
 
 ; CHECK: @global = linkonce global i32 0
 @global = linkonce global i32 0
+; Add a reference to @global since linkonce are lazy linked only upon real
+; references from code.
+define void @func() {
+  load i32, i32* @global
+  ret void
+}
 
 ; 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}
 
 ; CHECK:      !0 = !{}
 ; CHECK-NEXT: !1 = !{!0}
 ; CHECK-NEXT: !2 = !{i32* @global}
 ; CHECK-NEXT: !3 = distinct !{}

test/Linker/only-needed-named-metadata.ll

+; 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
+
+; 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 (...)*)
+
+@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}