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

[Rafael Avila de Espindola via llvm-commits]

I would suggest putting the full patch (llvm+lld) up for review.

To be clear, I think that the current version is a bit of a hack, but I
am at a loss as to what the correct fix in ThinLTO is.

The root problem is ThinLTO giving back symbols that are not prevailing.

Cheers,
Rafael

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> 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.
>
>
> 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.