[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

diff --git a/llvm/include/llvm/Linker/IRMover.h b/llvm/include/llvm/Linker/IRMover.h
index 578940e..ed263f7 100644 (file)
--- a/llvm/include/llvm/Linker/IRMover.h
+++ b/llvm/include/llvm/Linker/IRMover.h
@@ -71,8 +71,13 @@ public:
   ///   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:

diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 8d23f53..65bcef6 100644 (file)
--- a/llvm/lib/LTO/LTO.cpp
+++ b/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.

diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 5a00aae..3a16050 100644 (file)
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/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 @@ public:
   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;

diff --git a/llvm/lib/Linker/LinkModules.cpp b/llvm/lib/Linker/LinkModules.cpp
index fae9c95..cf4826f 100644 (file)
--- a/llvm/lib/Linker/LinkModules.cpp
+++ b/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;

diff --git a/llvm/test/ThinLTO/X86/Inputs/module_asm.ll b/llvm/test/ThinLTO/X86/Inputs/module_asm.ll
new file mode 100644 (file)
index 0000000..337e861
--- /dev/null
+++ b/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

diff --git a/llvm/test/ThinLTO/X86/module_asm_glob.ll b/llvm/test/ThinLTO/X86/module_asm_glob.ll
new file mode 100644 (file)
index 0000000..33588fa
--- /dev/null
+++ b/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
+}

diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp
index 2b72615..0813f47 100644 (file)
--- a/llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ b/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;