[ThinLTO] Import symver directives for imported symbols (PR48214)

[hans]

Summary

When importing symbols from another module, also import any corresponding symver directives.

(I'm new to this part of the code, so please give me pointers on how I can turn this into something that's fit for landing.)

[tejohnson]

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

llvm/lib/Linker/IRMover.cpp

711

This function gets called once per global value being linked in, so it would redo the below code multiple times.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

490

leftover debugging output?

[hans]

In D92335#2424693, @tejohnson wrote:

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

Yes, that seems easier. Thanks!

llvm/lib/Linker/IRMover.cpp

711

Yes, to make this work I figured we'd pull the symver metadata into a data structure that could be queried quickly here. But this approach no longer applies anyway.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

490

Oops, yes. But these changes are no longer necessary anyway.

[hans]

Pulling in the symver directives in IRLinker::run() instead. (I didn't do a separate function since it's just a few lines, but happy to do that if you think it would be better.)

Please take another look.

[hans]

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

[tejohnson]

In D92335#2425679, @hans wrote:

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

I think that's fine to have that dependence.

Largely lgtm but suggestion for refactor and test below.

llvm/lib/Linker/IRMover.cpp

1502

I think it makes sense to handle this earlier where we handle inline asm when !IsPerformingImport, since they logically go together (i.e. for regular LTO merge them, for ThinLTO just pull in the symver). Maybe refactor out the two sets of handling into a single helper method that deals with inline asm.

llvm/test/ThinLTO/X86/import-symver.ll

9

Maybe show a second case where if we don't import bar we also don't import its symver? You could add a second case where we disable importing (add -import-instr-limit=0 to an invocation of llvm-lto -thinlto-action=import) and check the results.

[hans]

Adding test case where the symver doesn't get imported.

llvm/lib/Linker/IRMover.cpp

1502

I looked at that first, but at that point the actual linking/importing into DstM hasn't happened yet (the worklist processing comes a few lines later), and so checking if symbols exist in DstM wouldn't work.

I suppose we could move the !IsPerformingImport inline asm handling down here though. I don't think there'd be any downside to doing that after worklist processing rather than before. Would that be better?

llvm/test/ThinLTO/X86/import-symver.ll

9

Done.

[tejohnson]

llvm/lib/Linker/IRMover.cpp

1502

Ah yeah I see the issue. I don't think there would be any issue with moving that earlier handling down here though, so why don't you do that so all the inline asm handling is together.

[hans]

Moving the "Append the module inline asm string" code down, and simplifying it in the process.

[hans]

llvm/lib/Linker/IRMover.cpp

1502

Done.

llvm/test/Linker/link-arm-and-thumb-module-inline-asm.ll

16 ↗

(On Diff #308725)

The previous code would output an empty module asm "" line here. My simplification removes that, and also makes the test expectations a bit more explicit.

[tejohnson]

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

[hans]

In D92335#2426457, @tejohnson wrote:

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

Landed that separately as 45d8a7843253ec68367c26114a2aa6bff2a7a4bb

llvm/lib/Linker/IRMover.cpp

else
       NewGV =
           new GlobalVariable(DstM, TypeMap.get(SGV->getValueType()),
                              /*isConstant*/ false, GlobalValue::ExternalLinkage,
                              /*init*/ nullptr, SGV->getName(),
                              /*insertbefore*/ nullptr,
                              SGV->getThreadLocalMode(), SGV->getAddressSpace());
   }
 
+  if (IsPerformingImport) {
+    // If there's a symver for the imported symbol, import it too.
+    NamedMDNode *Symvers = SGV->getParent()->getNamedMetadata("symvers");
+    if (Symvers) {
+      // FIXME: Use a faster data structure.
+      for (MDNode *Tuple : Symvers->operands()) {
+        StringRef Name =  cast<MDString>(Tuple->getOperand(0))->getString();
+        StringRef Alias = cast<MDString>(Tuple->getOperand(1))->getString();
+        if (Name == SGV->getName()) {
+          SmallString<256> S(".symver ");
+          S += Name;
+          S += ", ";
+          S += Alias;
+          DstM.appendModuleInlineAsm(S);
+        }
+      }
+    }
+  }
+
   if (ForDefinition)
     NewGV->setLinkage(SGV->getLinkage());
   else if (SGV->hasExternalWeakLinkage())
     NewGV->setLinkage(GlobalValue::ExternalWeakLinkage);
 
   if (auto *NewGO = dyn_cast<GlobalObject>(NewGV)) {
     // Metadata for global variables and function declarations is copied eagerly.
     if (isa<GlobalVariable>(SGV) || SGV->isDeclaration())
Error IRLinker::run() {
   Mapper.addFlags(RF_NullMapMissingGlobalValues);
 
   // 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.
   return linkModuleFlagsMetadata();
 }
 
 IRMover::StructTypeKeyInfo::KeyTy::KeyTy(ArrayRef<Type *> E, bool P)
     : ETypes(E), IsPacked(P) {}
 
 IRMover::StructTypeKeyInfo::KeyTy::KeyTy(const StructType *ST)
     : ETypes(ST->elements()), IsPacked(ST->isPacked()) {}
 

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

if (!ModuleId.empty()) {
       NewIndex = std::make_unique<ModuleSummaryIndex>(
           buildModuleSummaryIndex(M, nullptr, &PSI));
       Index = NewIndex.get();
     }
   }
 
   // Write it out as an unsplit ThinLTO module.
 
+  // Add symver metadata.
+  // FIXME: Move to a helper function, since splitAndWriteThinLTOBitcode() has the same code?
+  SmallVector<MDNode *, 8> Symvers;
+  ModuleSymbolTable::CollectAsmSymvers(M, [&](StringRef Name, StringRef Alias) {
+    Function *F = M.getFunction(Name);
+    if (!F || F->use_empty())
+      return;
+    auto &Ctx = M.getContext();
+    Symvers.push_back(MDTuple::get(
+        Ctx, {MDString::get(Ctx, Name), MDString::get(Ctx, Alias)}));
+  });
+  if (!Symvers.empty()) {
+    llvm::errs () << "writing symvers metadata\n";
+    NamedMDNode *NMD = M.getOrInsertNamedMetadata("symvers");
+    for (auto MD : Symvers)
+      NMD->addOperand(MD);
+  }
+
   // Save the module hash produced for the full bitcode, which will
   // be used in the backends, and use that in the minimized bitcode
   // produced for the full link.
   ModuleHash ModHash = {{0}};
   WriteBitcodeToFile(M, OS, /*ShouldPreserveUseListOrder=*/false, Index,
                      /*GenerateHash=*/true, &ModHash);
   // If a minimized bitcode module was requested for the thin link, only
   // the information that is needed by thin link will be written in the

llvm/test/ThinLTO/X86/Inputs/import-symver-foo.ll

+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm ".symver bar, bar@BAR_1.2.3"
+
+declare dso_local i32 @bar()
+
+define dso_local i32 @foo() {
+entry:
+  %call = tail call i32 @bar()
+  ret i32 %call
+}

llvm/test/ThinLTO/X86/import-symver.ll

+; RUN: opt -thinlto-bc %s -o %t1.bc
+; RUN: opt -thinlto-bc %p/Inputs/import-symver-foo.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=thinlink %t1.bc %t2.bc -o %t3.index.bc
+
+; RUN: llvm-lto -thinlto-action=import -exported-symbol=main %t1.bc -thinlto-index=%t3.index.bc
+; RUN: llvm-dis %t1.bc.thinlto.imported.bc -o - | FileCheck %s
+
+; When @bar gets imported, the symver must be imported too.
+; CHECK: module asm ".symver bar, bar@BAR_1.2.3"
+; CHECK: declare dso_local i32 @bar()
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare dso_local i32 @foo()
+
+define dso_local i32 @main() {
+entry:
+  %call = tail call i32 @foo()
+  ret i32 %call
+}

llvm/test/Transforms/ThinLTOBitcodeWriter/symver.ll

 ; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
 ; RUN: llvm-modextract -n 1 -o - %t | llvm-dis | FileCheck %s
 
+; RUN: opt -thinlto-bc %s | llvm-dis | FileCheck %s
+
 ; The target assembly parser is required to parse the symver directives
 ; REQUIRES: x86-registered-target
 
 target triple = "x86_64-unknown-linux-gnu"
 
 module asm ".symver used, used@VER"
 module asm ".symver unused, unused@VER"
 module asm ".symver variable, variable@VER"