[ThinLTO] - Set WeakAnyLinkage for all LinkerRedefined symbols.

[grimar]

Summary

We faced an issue in one of testcases of D41987 patch (ELF/lto/defsym.ll).

There we have 2 symbols declarations:

define hidden void @bar2() {
  call void @this_is_bar2()
  ret void
}

define hidden void @bar3() {
  call void @this_is_bar3()
  ret void
}

and want to redefine one of them with -defsym=bar2=bar3 linker option.
We define bar2 on linker side before calling LTO code. With that resolution for bar2 we pass to
ThinLTO is Prevealing=0, VisibleToRegularObj=1,LinkerRedefined=1.
ThinLTO returns us strong bar2 symbol and we fail with multiple symbol definition of 'bar2'.

Patch sets WeakAnyLinkage for all LinkerRedefined symbols and not only those that are Prevealing.

Such change stops bar2 from being exported:
https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L181
and then LTO sets bar2 linkage to internal here:
https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L329

Symbol never shows up in the output anymore and that fixes the issue for us.

Though as was mentioned by Rafael in D38239 thread, "This seems to be a bit of a hack.
With it llvm sets the linkage to weak to prevent inlining and that ends up avoiding the symbol for being used,
but that seems to happen in a profitability logic, not correctness."

Posting it to discuss how this can be fixed in a better way.

[tejohnson]

The same handling occurs for regular LTO (setting LinkerRedefined to WeakAny only when prevailing). Is this a problem there? If not, why? Answering my own question perhaps, but I guess this is because in the ThinLTO case we end up importing bar2 and therefore promoting it to external linkage instead of leaving it alone which would cause it to be internalized?

It seems like the bug then is that we import a non-prevailing value. Although the best fix for that in general seems to be what this patch is doing - marking as WeakAnyLinkage.

[grimar]

In D41988#974604, @tejohnson wrote:

The same handling occurs for regular LTO (setting LinkerRedefined to WeakAny only when prevailing). Is this a problem there? If not, why?

Yeah, I saw the same code in regular LTO,
but it is not a problem for it, our defsym.ll testcase mentioned has both ThinLTO and regular LTO sub-cases and that problem occurs only for ThinLTO case.

Answering my own question perhaps, but I guess this is because in the ThinLTO case we end up importing bar2 and therefore promoting it to external linkage instead of leaving it alone which would cause it to be internalized?

That part I believe is exactly what I observed during debugging of ThinLTO.

Importing happens here:
(https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L181)

With WeakAnyLinkage isInterposableLinkage returns false and then exporting from their source module does not happen:
https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L274

What result in InternalLinkage set here
https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L337

It seems like the bug then is that we import a non-prevailing value. Although the best fix for that in general seems to be what this patch is doing - marking as WeakAnyLinkage.

[tejohnson]

In D41988#974637, @grimar wrote:

In D41988#974604, @tejohnson wrote:

The same handling occurs for regular LTO (setting LinkerRedefined to WeakAny only when prevailing). Is this a problem there? If not, why?

Yeah, I saw the same code in regular LTO,
but it is not a problem for it, our defsym.ll testcase mentioned has both ThinLTO and regular LTO sub-cases and that problem occurs only for ThinLTO case.

Answering my own question perhaps, but I guess this is because in the ThinLTO case we end up importing bar2 and therefore promoting it to external linkage instead of leaving it alone which would cause it to be internalized?

That part I believe is exactly what I observed during debugging of ThinLTO.

Normally we would have a prevailing copy that would be imported, but I guess there is none here. Thinking more about the regular LTO case, since there also isn't a prevailing copy of bar2, is that non-prevailing copy kept in the merged module, and if so, wouldn't it be incorrectly inlined because it won't be set to WeakAnyLinkage?

Importing happens here:
(https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L181)

With WeakAnyLinkage isInterposableLinkage returns false and then exporting from their source module does not happen:
https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L274

What result in InternalLinkage set here
https://github.com/llvm-mirror/llvm/blob/master/lib/LTO/LTO.cpp#L337

It seems like the bug then is that we import a non-prevailing value. Although the best fix for that in general seems to be what this patch is doing - marking as WeakAnyLinkage.

[grimar]

In D41988#974645, @tejohnson wrote:

In D41988#974637, @grimar wrote:

In D41988#974604, @tejohnson wrote:

The same handling occurs for regular LTO (setting LinkerRedefined to WeakAny only when prevailing). Is this a problem there? If not, why?

Yeah, I saw the same code in regular LTO,
but it is not a problem for it, our defsym.ll testcase mentioned has both ThinLTO and regular LTO sub-cases and that problem occurs only for ThinLTO case.

Answering my own question perhaps, but I guess this is because in the ThinLTO case we end up importing bar2 and therefore promoting it to external linkage instead of leaving it alone which would cause it to be internalized?

That part I believe is exactly what I observed during debugging of ThinLTO.

Normally we would have a prevailing copy that would be imported, but I guess there is none here. Thinking more about the regular LTO case, since there also isn't a prevailing copy of bar2, is that non-prevailing copy kept in the merged module, and if so, wouldn't it be incorrectly inlined because it won't be set to WeakAnyLinkage?

From what I see it is not inlined in output: https://github.com/llvm-mirror/lld/blob/master/test/ELF/lto/defsym.ll#L17 (regular LTO test passes and result looks expected),
but let me debug it on moday. Honestly I did not look into regular LTO code very deeply as it just worked for us.
I'll return with more details. Thanks for looking at this !

[grimar]

Chosen direction is D42107.

lib/LTO/LTO.cpp

Error LTO::addThinLTO(BitcodeModule BM, ArrayRef<InputFile::Symbol> Syms,
 
   for (const InputFile::Symbol &Sym : Syms) {
     assert(ResI != ResE);
     SymbolResolution Res = *ResI++;
 
     if (!Sym.getIRName().empty()) {
       auto GUID = GlobalValue::getGUID(GlobalValue::getGlobalIdentifier(
           Sym.getIRName(), GlobalValue::ExternalLinkage, ""));
-      if (Res.Prevailing) {
+      if (Res.Prevailing)
         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 the linker resolved the symbol to a local definition then mark it
       // as local in the summary for the module we are adding.
       if (Res.FinalDefinitionInLinkageUnit) {
         if (auto S = ThinLTO.CombinedIndex.findSummaryInModule(
                 GUID, BM.getModuleIdentifier())) {
           S->setDSOLocal(true);
         }

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.1 | FileCheck %s
 
 ; CHECK: Name: patatino
 ; CHECK-NEXT: Value:
 ; CHECK-NEXT: Size:
 ; CHECK-NEXT: Binding: Weak
 ; CHECK-NEXT: Type: Function
 
+; RUN: llvm-lto2 run -o %t1.o %t.o -r %t.o,patatino,r
+; RUN: llvm-readobj -t %t1.o.1 | FileCheck %s --check-prefix=NOSYM
+
+; NOSYM-NOT: Name: patatino
+
 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
 }