[PATCH] D41988: [ThinLTO] - Set WeakAnyLinkage for all LinkerRedefined symbols.

[George Rimar via Phabricator via llvm-commits]

grimar created this revision.
grimar added reviewers: pcc, tejohnson.
Herald added subscribers: inglorion, mehdi_amini.

We faced an issue in one of testcases of https://reviews.llvm.org/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 https://reviews.llvm.org/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 discussion how this can be fixed in a better way.


https://reviews.llvm.org/D41988

Files:
  lib/LTO/LTO.cpp
  test/LTO/Resolution/X86/linker-redef-thin.ll


Index: test/LTO/Resolution/X86/linker-redef-thin.ll
===================================================================
--- test/LTO/Resolution/X86/linker-redef-thin.ll
+++ test/LTO/Resolution/X86/linker-redef-thin.ll
@@ -8,6 +8,11 @@
 ; 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"
 
Index: lib/LTO/LTO.cpp
===================================================================
--- lib/LTO/LTO.cpp
+++ lib/LTO/LTO.cpp
@@ -704,18 +704,17 @@
     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);
-      }
+      // 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41988.129604.patch
Type: text/x-patch
Size: 2172 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180112/91ac8823/attachment.bin>