Linker: Move special casing for available_externally in IRMover to clients. NFCI.

[pcc]

Summary

The goal is to simplify the semantic model for clients of IRMover.

[mehdi_amini]

llvm/lib/LTO/LTO.cpp

481

Do we have a test that shows that:

1. if we first import an available_externally, then a later linkonce_odr (and/or strong) would overwrite?
2. if we first import a linkonce_odr (and/or strong), a later available_externally wouldn't overwrite?

llvm/lib/Linker/IRMover.cpp

875

No test changed, that's annoying that we're not covering this case (independently of the new LTO API). Any idea how to test this? (along with the removal below as well)

[mehdi_amini]

LGTM otherwise.

[tejohnson]

llvm/lib/Linker/LinkModules.cpp

398

I guess removing this is required to make it NFC. Is it required for correctness too? I don't think so but want to make sure my understanding is right. Is it better to continue not mapping these in here (I think they get mapped later if referenced, right?) and not bother making this NFC?

[pcc]

• Add test
• A change is required in a different place to make it NFC

[pcc]

llvm/lib/LTO/LTO.cpp

481

I've added tests for both of those things, as well as a standalone test for the available_externally functionality (previously only a gold plugin test was covering this).

llvm/lib/Linker/IRMover.cpp

875

The only direct users of IRMover are LinkModules and LTO. I guess we could write a unit test for IRMover perhaps? Doesn't seem worth it for this one change though.

llvm/lib/Linker/LinkModules.cpp

398

Actually this change was needed for us to link available_externally at all (with only the change to shouldLink we would hit the 'return false;' here and never link any available_externally globals). But taking a closer look I found that we were linking too much, even unreferenced available_externally globals (which was a functional change). I've uploaded a new change that causes available_externally to be handled like linkonce and only linked if referenced.

[mehdi_amini]

Thanks for the tests! Still LGTM.

llvm/lib/Linker/IRMover.cpp

875

I wouldn't suggest it :)

[mehdi_amini]

llvm/test/Linker/available_externally_a.ll

7 ↗

(On Diff #86763)

Actually why isn't llvm-link including the available_externally?

[pcc]

llvm/test/Linker/available_externally_a.ll

7 ↗

(On Diff #86763)

Because it is unreferenced, see discussion with Teresa in LinkModules.cpp.

[mehdi_amini]

llvm/test/Linker/available_externally_a.ll

7 ↗

(On Diff #86763)

Oh right :)

llvm/lib/LTO/LTO.cpp

for (const InputFile::Symbol &Sym :
        make_range(InputFile::symbol_iterator(SymTab.symbols().begin(), SymTab,
                                              nullptr),
                   InputFile::symbol_iterator(SymTab.symbols().end(), SymTab,
                                              nullptr))) {
     assert(ResI != ResE);
     SymbolResolution Res = *ResI++;
     addSymbolToGlobalRes(Used, Sym, Res, 0);
 
+    if (Sym.isGV()) {
+      GlobalValue *GV = Sym.getGV();
+      if (Res.Prevailing) {
         if (Sym.getFlags() & object::BasicSymbolRef::SF_Undefined)
           continue;
-    if (Res.Prevailing && Sym.isGV()) {
-      GlobalValue *GV = Sym.getGV();
         Keep.push_back(GV);
         switch (GV->getLinkage()) {
         default:
           break;
         case GlobalValue::LinkOnceAnyLinkage:
           GV->setLinkage(GlobalValue::WeakAnyLinkage);
           break;
         case GlobalValue::LinkOnceODRLinkage:
           GV->setLinkage(GlobalValue::WeakODRLinkage);
           break;
         }
+      } else if (GV->hasAvailableExternallyLinkage()) {
+        // We can link available_externally symbols even if they are
+        // non-prevailing.
+        GlobalValue *CombinedGV =
+            RegularLTO.CombinedModule->getNamedValue(GV->getName());
+        if (!CombinedGV || CombinedGV->isDeclaration())
+          Keep.push_back(GV);
+      }
     }
     // Common resolution: collect the maximum size/alignment over all commons.
     // We also record if we see an instance of a common as prevailing, so that
     // if none is prevailing we can ignore it later.
     if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       // FIXME: We should figure out what to do about commons defined by asm.
       // For now they aren't reported correctly by ModuleSymbolTable.
       auto &CommonRes = RegularLTO.Commons[Sym.getGV()->getName()];

llvm/lib/Linker/IRMover.cpp

 
 bool IRLinker::shouldLink(GlobalValue *DGV, GlobalValue &SGV) {
   if (ValuesToLink.count(&SGV) || SGV.hasLocalLinkage())
     return true;
 
   if (DGV && !DGV->isDeclarationForLinker())
     return false;
 
-  if (SGV.hasAvailableExternallyLinkage())
-    return true;
-
   if (SGV.isDeclaration() || DoneLinkingBodies)
     return false;
 
   // Callback to the client to give a chance to lazily add the Global to the
   // list of value to link.
   bool LazilyAdded = false;
   AddLazyFor(SGV, [this, &LazilyAdded](GlobalValue &GV) {
     maybeAdd(&GV);

llvm/lib/Linker/LinkModules.cpp

bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
   // double frees).
   if (GV.hasAppendingLinkage() && isPerformingImport())
     return false;
 
   if (isPerformingImport()) {
     if (!doImportAsDefinition(&GV))
       return false;
   } else if (!DGV && !shouldOverrideFromSrc() &&
-             (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage() ||
-              GV.hasAvailableExternallyLinkage()))
+             (GV.hasLocalLinkage() || GV.hasLinkOnceLinkage()))
     return false;
 
   if (GV.isDeclaration())
     return false;
 
   if (const Comdat *SC = GV.getComdat()) {
     bool LinkFromSrc;
     Comdat::SelectionKind SK;