[ThinLTO] Don't link module level assembly when importing

Module inline asm was always being linked/concatenated
when running the IRLinker. This is correct for full LTO but not when
we are importing for ThinLTO, as it can result in multiply defined
symbols when the module asm defines a global symbol.

In order to test with llvm-lto2, I had to work around PR30396,
where a symbol that is defined in module assembly but defined in the
LLVM IR appears twice. Added workaround to llvm-lto2 with a FIXME.

Fixes PR30610.

Reviewers: mehdi_amini

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D25359

llvm-svn: 284030

llvm/include/llvm/Linker/IRMover.h

@@ -71,8 +71,13 @@ class IRMover {
   ///   not present in ValuesToLink. The GlobalValue and a ValueAdder callback
   ///   are passed as an argument, and the callback is expected to be called
   ///   if the GlobalValue needs to be added to the \p ValuesToLink and linked.
+  /// - \p LinkModuleInlineAsm is true if the ModuleInlineAsm string in Src
+  ///   should be linked with (concatenated into) the ModuleInlineAsm string
+  ///   for the destination module. It should be true for full LTO, but not
+  ///   when importing for ThinLTO, otherwise we can have duplicate symbols.
   Error move(std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
-             std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor);
+             std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor,
+             bool LinkModuleInlineAsm);
   Module &getModule() { return Composite; }
 
 private:

llvm/lib/LTO/LTO.cpp

@@ -367,7 +367,8 @@ Error LTO::addRegularLTO(std::unique_ptr<InputFile> Input,
   assert(ResI == Res.end());
 
   return RegularLTO.Mover->move(Obj->takeModule(), Keep,
-                                [](GlobalValue &, IRMover::ValueAdder) {});
+                                [](GlobalValue &, IRMover::ValueAdder) {},
+                                /* LinkModuleInlineAsm */ true);
 }
 
 // Add a ThinLTO object to the link.

llvm/lib/Linker/IRMover.cpp

@@ -397,6 +397,12 @@ class IRLinker {
       Worklist.push_back(GV);
   }
 
+  /// Flag whether the ModuleInlineAsm string in Src should be linked with
+  /// (concatenated into) the ModuleInlineAsm string for the destination
+  /// module. It should be true for full LTO, but not when importing for
+  /// ThinLTO, otherwise we can have duplicate symbols.
+  bool LinkModuleInlineAsm;
+
   /// Set to true when all global value body linking is complete (including
   /// lazy linking). Used to prevent metadata linking from creating new
   /// references.
@@ -482,10 +488,11 @@ class IRLinker {
   IRLinker(Module &DstM, MDMapT &SharedMDs,
            IRMover::IdentifiedStructTypeSet &Set, std::unique_ptr<Module> SrcM,
            ArrayRef<GlobalValue *> ValuesToLink,
-           std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor)
+           std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor,
+           bool LinkModuleInlineAsm)
       : DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)),
         TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this),
-        SharedMDs(SharedMDs),
+        SharedMDs(SharedMDs), LinkModuleInlineAsm(LinkModuleInlineAsm),
         Mapper(ValueMap, RF_MoveDistinctMDs | RF_IgnoreMissingLocals, &TypeMap,
                &GValMaterializer),
         AliasMCID(Mapper.registerAlternateMappingContext(AliasValueMap,
@@ -1216,7 +1223,7 @@ Error IRLinker::run() {
   DstM.setTargetTriple(mergeTriples(SrcTriple, DstTriple));
 
   // Append the module inline asm string.
-  if (!SrcM->getModuleInlineAsm().empty()) {
+  if (LinkModuleInlineAsm && !SrcM->getModuleInlineAsm().empty()) {
     if (DstM.getModuleInlineAsm().empty())
       DstM.setModuleInlineAsm(SrcM->getModuleInlineAsm());
     else
@@ -1354,9 +1361,11 @@ IRMover::IRMover(Module &M) : Composite(M) {
 
 Error IRMover::move(
     std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
-    std::function<void(GlobalValue &, ValueAdder Add)> AddLazyFor) {
+    std::function<void(GlobalValue &, ValueAdder Add)> AddLazyFor,
+    bool LinkModuleInlineAsm) {
   IRLinker TheIRLinker(Composite, SharedMDs, IdentifiedStructTypes,
-                       std::move(Src), ValuesToLink, std::move(AddLazyFor));
+                       std::move(Src), ValuesToLink, std::move(AddLazyFor),
+                       LinkModuleInlineAsm);
   Error E = TheIRLinker.run();
   Composite.dropTriviallyDeadConstantArrays();
   return E;

llvm/lib/Linker/LinkModules.cpp

@@ -582,7 +582,8 @@ bool ModuleLinker::run() {
   if (Error E = Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
                            [this](GlobalValue &GV, IRMover::ValueAdder Add) {
                              addLazyFor(GV, Add);
-                           })) {
+                           },
+                           !isPerformingImport())) {
     handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
       DstM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, EIB.message()));
       HasErrors = true;

llvm/test/ThinLTO/X86/Inputs/module_asm.ll

@@ -0,0 +1,8 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main({ i64, { i64, i8* }* } %unnamed) #0 {
+  %1 = call i32 @_simplefunction() #1
+  ret i32 %1
+}
+declare i32 @_simplefunction() #1

llvm/test/ThinLTO/X86/module_asm_glob.ll

@@ -0,0 +1,35 @@
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/module_asm.ll -o %t2.bc
+
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=main %t1.bc %t2.bc
+; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck  %s --check-prefix=NM0
+; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck  %s --check-prefix=NM1
+
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:     -r=%t1.bc,foo,plx \
+; RUN:     -r=%t1.bc,_simplefunction,pl \
+; RUN:     -r=%t2.bc,main,plx \
+; RUN:     -r=%t2.bc,_simplefunction,l
+; RUN: llvm-nm %t.o.0 | FileCheck  %s --check-prefix=NM0
+; RUN: llvm-nm %t.o.1 | FileCheck  %s --check-prefix=NM1
+
+; NM0: T foo
+; NM1-NOT: foo
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm "\09.text"
+module asm "\09.globl\09foo"
+module asm "\09.align\0916, 0x90"
+module asm "\09.type\09foo,@function"
+module asm "foo:"
+module asm "\09ret "
+module asm "\09.size\09foo, .-foo"
+module asm ""
+
+declare zeroext i16 @foo() #0
+
+define i32 @_simplefunction() #1 {
+  ret i32 1
+}

llvm/tools/llvm-lto2/llvm-lto2.cpp

@@ -158,6 +158,12 @@ int main(int argc, char **argv) {
         check(InputFile::create(MB->getMemBufferRef()), F);
 
     std::vector<SymbolResolution> Res;
+    // FIXME: Workaround PR30396 which means that a symbol can appear
+    // more than once if it is defined in module-level assembly and
+    // has a GV declaration. Keep track of the resolutions found in this
+    // file and remove them from the CommandLineResolutions map afterwards,
+    // so that we don't flag the second one as missing.
+    std::map<std::string, SymbolResolution> CurrentFileSymResolutions;
     for (const InputFile::Symbol &Sym : Input->symbols()) {
       auto I = CommandLineResolutions.find({F, Sym.getName()});
       if (I == CommandLineResolutions.end()) {
@@ -166,9 +172,13 @@ int main(int argc, char **argv) {
         HasErrors = true;
       } else {
         Res.push_back(I->second);
-        CommandLineResolutions.erase(I);
+        CurrentFileSymResolutions[Sym.getName()] = I->second;
       }
     }
+    for (auto I : CurrentFileSymResolutions) {
+      auto NumErased = CommandLineResolutions.erase({F, I.first});
+      assert(NumErased > 0);
+    }
 
     if (HasErrors)
       continue;