[lib/LTO] Fix the interaction between linker redefined symbols and ThinLTO

[davide]

Summary

This is the same of https://reviews.llvm.org/rL304719 but for ThinLTO.
The substantial difference is that in this case we don't have whole visibility, just the summary.
In the LTO case, when we got the resolution for the input file we could just see if the linker told us whether a symbol was linker redefined (using --wrap or --defsym) and switch the linkage directly for the GV.

Here, we have the summary. So, we record that the linkage changed from <whatever it was> to $weakany to prevent IPOs across this symbol boundaries and actually just switch the linkage at FunctionImport time.

This patch should also fix the lld bits (as all the scaffolding for communicating if a symbol is linker redefined should be there & should be the same), but I'll make sure to add some tests there as well.

Fixes https://bugs.llvm.org/show_bug.cgi?id=33192

[tejohnson]

lib/Transforms/IPO/FunctionImport.cpp

544 ↗

(On Diff #105448)

The comment doesn't seem to match the check. And don't we want to be able to convert renamed externally visible symbols to WeakAny (like in the attached test case)?

560 ↗

(On Diff #105448)

Will this do the wrong thing (i.e. would we potentially convert something to a declaration when we wanted to convert it to WeakAny (the routine was previously only used to convert to available_externally).

[davide]

This has a known problem, I'll upload a new version in a bit.

[davide]

Actually, I now think I understand this slightly better.
As this was handling only the update of linkage to available_externally, I think the easiest solution is that of moving the early return later and in case the original and the linkage in the summary don't match, and the one in the summary is weak, just switch to it.
I think this is safe, but maybe we might want to do the switch only for some kind of (original) linkages, maybe.

[tejohnson]

lib/LTO/LTO.cpp

673 ↗

(On Diff #105477)

Suggest doing this in thinLTOResolveWeakForLinkerGUID/thinLTOResolveWeakForLinkerInIndex for 2 reasons:

1. avoids extra combined index hash lookup to find the summary
2. since we are doing linkage update in the corresponding LTO backend routine hinLTOResolveWeakForLinkerModule

The names of all 3 of these routines should probably be changed to reflect the additional scope (not just handling linker resolution of weak symbols anymore). Not sure of a good name offhand though...

lib/Transforms/IPO/FunctionImport.cpp

555 ↗

(On Diff #105477)

Actually we may be switching to either available_externally (for the non-prevailing copies) or converting linkonce->weak for the prevailing copy (sorry, my earlier comment was misleading). So probably change comment to reflect that we are doing weak for linker resolution below.

[davide]

lib/LTO/LTO.cpp

673 ↗

(On Diff #105477)

In order to find which symbols are LinkerRedefined we need to access the resolution vector, so if we move this logic to thinLTOResolveWeakForLinkerGUID we need to add an extra symbol walk for all the input files, I guess, which I'm not entirely sure it will be faster than having extra summary lookups.

I'll leave the call to you, but this is something I wanted to point out anyway :)

lib/Transforms/IPO/FunctionImport.cpp

555 ↗

(On Diff #105477)

I'll remove this.

[pcc]

lib/LTO/LTO.cpp

673 ↗

(On Diff #105477)

You could also reduce the number of summary lookups by changing the code to look like this:

if (Res.LinkerRedefined)
  if (auto S = ...)
    S->setLinkage(...);

[tejohnson]

lib/LTO/LTO.cpp

673 ↗

(On Diff #105477)

You're right, forgot we don't have that flag handy later. Maybe ok for now then, but we may want to reevaluate at some point - I've seen the index lookups get hot before for really large apps. I guess we could stash in the PrevailingModuleForGUID map (make the result a tuple), and then change isPrevailing to return this flag. But perhaps not critical for now.

If it is left as is here, then in thinLTOResolveWeakForLinkerModule it should probably be documented where the new linkage is set up in the summary (since not in the corresponding "InIndex" version of that routine).

[tejohnson]

lib/LTO/LTO.cpp

673 ↗

(On Diff #105477)

Good point Peter. The LinkerRedefined should be uncommon, so let's just reorder the checks as you suggested.

[davide]

Thanks for the comments, Peter & Teresa. New version available.

[tejohnson]

Lgtm with one request below.

lib/Transforms/IPO/FunctionImport.cpp

549 ↗

(On Diff #105501)

Please add comment here indicating where this is set on the summary, since it isn't in the corresponding InIndex version of this function.

llvm/trunk/lib/LTO/LTO.cpp

for (const InputFile::Symbol &Sym : Syms) {
     assert(ResI != ResE);
     SymbolResolution Res = *ResI++;
 
     if (Res.Prevailing) {
       if (!Sym.getIRName().empty()) {
         auto GUID = GlobalValue::getGUID(GlobalValue::getGlobalIdentifier(
             Sym.getIRName(), GlobalValue::ExternalLinkage, ""));
         ThinLTO.PrevailingModuleForGUID[GUID] = BM.getModuleIdentifier();
+
+        // For linker redefined symbols (via --wrap or --defsym) we want to
+        // switch the linkage to `weak` to prevent IPOs from happening.
+        // Find the summary in the module for this very GV and record the new
+        // linkage so that we can switch it when we import the GV.
+        if (Res.LinkerRedefined)
+          if (auto S = ThinLTO.CombinedIndex.findSummaryInModule(
+                  GUID, BM.getModuleIdentifier()))
+            S->setLinkage(GlobalValue::WeakAnyLinkage);
       }
     }
   }
 
   if (!ThinLTO.ModuleMap.insert({BM.getModuleIdentifier(), BM}).second)
     return make_error<StringError>(
         "Expected at most one ThinLTO module per bitcode file",
         inconvertibleErrorCode());

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

if (Function *F = dyn_cast<Function>(&GV)) {
       // For now we don't resolve or drop aliases. Once we do we'll
       // need to add support here for creating either a function or
       // variable declaration, and return the new GlobalValue* for
       // the caller to use.
       llvm_unreachable("Expected function or variable");
   };
 
   auto updateLinkage = [&](GlobalValue &GV) {
-    if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
-      return;
     // See if the global summary analysis computed a new resolved linkage.
     const auto &GS = DefinedGlobals.find(GV.getGUID());
     if (GS == DefinedGlobals.end())
       return;
     auto NewLinkage = GS->second->linkage();
     if (NewLinkage == GV.getLinkage())
       return;
+
+    // Switch the linkage to weakany if asked for, e.g. we do this for
+    // linker redefined symbols (via --wrap or --defsym).
+    if (NewLinkage == GlobalValue::WeakAnyLinkage) {
+      GV.setLinkage(NewLinkage);
+      return;
+    }
+
+    if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
+      return;
     // Check for a non-prevailing def that has interposable linkage
     // (e.g. non-odr weak or linkonce). In that case we can't simply
     // convert to available_externally, since it would lose the
     // interposable property and possibly get inlined. Simply drop
     // the definition in that case.
     if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
         GlobalValue::isInterposableLinkage(GV.getLinkage()))
       ConvertToDeclaration(GV);

llvm/trunk/test/LTO/Resolution/X86/linker-redef-thin.ll

+; RUN: opt -module-summary %s -o %t.o
+; RUN: llvm-lto2 run -o %t1.o %t.o -r %t.o,patatino,pr
+; RUN: llvm-readobj -t %t1.o.0 | FileCheck %s
+
+; CHECK: Name: patatino
+; CHECK-NEXT: Value:
+; CHECK-NEXT: Size:
+; CHECK-NEXT: Binding: Weak
+; CHECK-NEXT: Type: Function
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @patatino() {
+  ret void
+}