Expose IRMover flag

[eschew]

Summary

In the 4.0 cycle, the NamedOnly flag in IRMover.cpp was changed from true to false. This wound up breaking my out-of-tree compiler frontend.

The attached patch threads through the NamedOnly flag from IRMover to Linker and adds a new flag enum value. Thus, the behavior of existing code remains the same, but linker clients can override the NamedOnly behavior as needed.

I'm looking into adding a test for the new flag but wanted to throw the current diff up for review first.

[tejohnson]

You could add an llvm-link based test, adding an option for controlling this behavior to that tool.

include/llvm/Linker/IRMover.h

64

How about just add the parameter to the existing constructor, but with a default of false (here and elsewhere).

[eschew]

This adds a defaulted parameter instead of multiple constructors. I also added a hidden flag to llvm-link.

So far I haven't managed to construct inputs which get llvm-link'ed differently when the flag is toggled (I verified that the OnlyNamed flag in TypeFinder.cpp does indeed change). I tried using the testcase from the bug which changed the flag, https://reviews.llvm.org/D26840 , but the linking behavior is essentially unchanged between ThinLTO mode, plain llvm-link, and llvm-link -no-unnamed-types. I also tried reducing a testcase from my frontend, but haven't managed to reproduce the difference in behavior via llvm-link.

FWIW I changed my frontend, so I'm not blocked by this patch, but I am curious whether there is any LLVM IR that links different via llvm-link under the NamedOnly flag.

[pcc]

I'm not yet convinced that we should take this. If changing the flag from true to false broke your frontend, it may mean that there is a bug in the IRMover that was exposed by your frontend. So we may be introducing a flag that means "do you want this set of bugs or that other set of bugs", which doesn't seem like a good idea to me.

Can you go into more detail as to how changing the flag broke your frontend?

[eschew]

I'm almost certain the bug was in my frontend's misuse of LLVM's linking infrastructure, not a bug in linking itself (per se).

My frontend first constructs a shell Module with declarations for the language's runtime functions, then codegens into the shell, then links the shell with the runtime. It was constructing the shell from a precompiled bitcode by iterating over the runtime's Module and calling shell->getOrInsertFunction(...) providing *a type from the runtime Module*. I'm guessing mixing types like that between modules isn't kosher. The workaround/fix is to round-trip the constructed shell through bitcode.

What I'm confused about is: what does the OnlyNamed flag (in TypeFinder.cpp) actually do, from the perspective of LLVM IR?

[tejohnson]

In D33870#774277, @eschew wrote:

I'm almost certain the bug was in my frontend's misuse of LLVM's linking infrastructure, not a bug in linking itself (per se).

My frontend first constructs a shell Module with declarations for the language's runtime functions, then codegens into the shell, then links the shell with the runtime. It was constructing the shell from a precompiled bitcode by iterating over the runtime's Module and calling shell->getOrInsertFunction(...) providing *a type from the runtime Module*. I'm guessing mixing types like that between modules isn't kosher. The workaround/fix is to round-trip the constructed shell through bitcode.

What I'm confused about is: what does the OnlyNamed flag (in TypeFinder.cpp) actually do, from the perspective of LLVM IR?

I'm not familiar with the TypeFinder, but I did a little bit of digging. As you noted earlier, the change of this flag from true to false recently was a fix by Mehdi for ThinLTO IR linking (r287453 - [ThinLTO] Fix crash when importing an opaque type). Looks like we need to eagerly map all types because of complications due to the fact that we are only linking some of the symbols. But it sounds like you couldn't reproduce the original problem with the test case in that change?

There are 2 other callers with OnlyNamed=false:

1. the AsmPrinter calls this with OnlyNamed=false because it is printing all the types.
2. StripTypeNames in IPO/StripSymbols.cpp, and that code is actually looking for named types, and skips those without names! So I am not sure why it needs to call TypeFinder with OnlyNamed=false to start with.

include/llvm/Linker/IRMover.h

public:
   public:
     void addNonOpaque(StructType *Ty);
     void switchToNonOpaque(StructType *Ty);
     void addOpaque(StructType *Ty);
     StructType *findNonOpaque(ArrayRef<Type *> ETypes, bool IsPacked);
     bool hasType(StructType *Ty);
   };
 
-  IRMover(Module &M);
+  IRMover(Module &M, bool OnlyNamedTypes = false);
 
   typedef std::function<void(GlobalValue &)> ValueAdder;
 
   /// Move in the provide values in \p ValuesToLink from \p Src.
   ///
   /// - \p AddLazyFor is a call back that the IRMover will call when a global
   ///   value is referenced by one of the ValuesToLink (transitively) but was
   ///   not present in ValuesToLink. The GlobalValue and a ValueAdder callback
   ///   are passed as an argument, and the callback is expected to be called

include/llvm/Linker/Linker.h

 class Linker {
   IRMover Mover;
 
 public:
   enum Flags {
     None = 0,
     OverrideFromSrc = (1 << 0),
     LinkOnlyNeeded = (1 << 1),
+    DontLinkUnnamedTypes = (1 << 2),
   };
 
-  Linker(Module &M);
+  Linker(Module &M, bool OnlyNamedTypes = false);
 
   /// \brief Link \p Src into the composite.
   ///
   /// Passing OverrideSymbols as true will have symbols from Src
   /// shadow those in the Dest.
   ///
   /// Passing InternalizeCallback will have the linker call the function with
   /// the new module and a list of global value names to be internalized by the

lib/Linker/IRMover.cpp

 
 bool IRMover::IdentifiedStructTypeSet::hasType(StructType *Ty) {
   if (Ty->isOpaque())
     return OpaqueStructTypes.count(Ty);
   auto I = NonOpaqueStructTypes.find(Ty);
   return I == NonOpaqueStructTypes.end() ? false : *I == Ty;
 }
 
-IRMover::IRMover(Module &M) : Composite(M) {
+IRMover::IRMover(Module &M, bool OnlyNamedTypes) : Composite(M) {
   TypeFinder StructTypes;
-  StructTypes.run(M, /* OnlyNamed */ false);
+  StructTypes.run(M, OnlyNamedTypes);
   for (StructType *Ty : StructTypes) {
     if (Ty->isOpaque())
       IdentifiedStructTypes.addOpaque(Ty);
     else
       IdentifiedStructTypes.addNonOpaque(Ty);
   }
   // Self-map metadatas in the destination module. This is needed when
   // DebugTypeODRUniquing is enabled on the LLVMContext, since metadata in the

lib/Linker/LinkModules.cpp

if (HasErrors)
     return true;
 
   if (InternalizeCallback)
     InternalizeCallback(DstM, Internalize);
 
   return false;
 }
 
-Linker::Linker(Module &M) : Mover(M) {}
+Linker::Linker(Module &M, bool OnlyNamedTypes) :
+  Mover(M, OnlyNamedTypes) {}
 
 bool Linker::linkInModule(
     std::unique_ptr<Module> Src, unsigned Flags,
     std::function<void(Module &, const StringSet<> &)> InternalizeCallback) {
   ModuleLinker ModLinker(Mover, std::move(Src), Flags,
                          std::move(InternalizeCallback));
   return ModLinker.run();
 }
 
 //===----------------------------------------------------------------------===//
 // LinkModules entrypoint.
 //===----------------------------------------------------------------------===//
 
 /// This function links two modules together, with the resulting Dest module
 /// modified to be the composite of the two input modules. If an error occurs,
 /// true is returned and ErrorMsg (if not null) is set to indicate the problem.
 /// Upon failure, the Dest module could be in a modified state, and shouldn't be
 /// relied on to be consistent.
 bool Linker::linkModules(
     Module &Dest, std::unique_ptr<Module> Src, unsigned Flags,
     std::function<void(Module &, const StringSet<> &)> InternalizeCallback) {
-  Linker L(Dest);
+  bool OnlyNamedTypes = (Flags & DontLinkUnnamedTypes) != 0;
+  Linker L(Dest, OnlyNamedTypes);
   return L.linkInModule(std::move(Src), Flags, std::move(InternalizeCallback));
 }
 
 //===----------------------------------------------------------------------===//
 // C API.
 //===----------------------------------------------------------------------===//
 
 LLVMBool LLVMLinkModules2(LLVMModuleRef Dest, LLVMModuleRef Src) {
   Module *D = unwrap(Dest);
   std::unique_ptr<Module> M(unwrap(Src));
   return Linker::linkModules(*D, std::move(M));
 }

tools/llvm-link/llvm-link.cpp

 
 static cl::opt<bool>
 Verbose("v", cl::desc("Print information about actions taken"));
 
 static cl::opt<bool>
 DumpAsm("d", cl::desc("Print assembly as linked"), cl::Hidden);
 
 static cl::opt<bool>
+NoLinkUnnamedTypes("no-link-unnamed", cl::desc("Don't link unnamed types"),
+                   cl::Hidden);
+
+static cl::opt<bool>
 SuppressWarnings("suppress-warnings", cl::desc("Suppress all linking warnings"),
                  cl::init(false));
 
 static cl::opt<bool> PreserveBitcodeUseListOrder(
     "preserve-bc-uselistorder",
     cl::desc("Preserve use-list order when writing LLVM bitcode."),
     cl::init(true), cl::Hidden);
 
if (!DisableDITypeMap)
     Context.enableDebugTypeODRUniquing();
 
   auto Composite = make_unique<Module>("llvm-link", Context);
   Linker L(*Composite);
 
   unsigned Flags = Linker::Flags::None;
   if (OnlyNeeded)
     Flags |= Linker::Flags::LinkOnlyNeeded;
+  if (NoLinkUnnamedTypes)
+    Flags |= Linker::Flags::DontLinkUnnamedTypes;
 
   // First add all the regular input files
   if (!linkFiles(argv[0], Context, L, InputFilenames, Flags))
     return 1;
 
   // Next the -override ones.
   if (!linkFiles(argv[0], Context, L, OverridingInputs,
                  Flags | Linker::Flags::OverrideFromSrc))