[PATCH] D54125: [LTO] Drop non-prevailing definitions only if linkage is not local or appending

[Bill Wendling via Phabricator via llvm-commits]

void added a comment.

In D54125#1314825 <https://reviews.llvm.org/D54125#1314825>, @tejohnson wrote:

> In D54125#1314824 <https://reviews.llvm.org/D54125#1314824>, @tejohnson wrote:
>
> > In D54125#1314823 <https://reviews.llvm.org/D54125#1314823>, @tejohnson wrote:
> >
> > > In D54125#1314702 <https://reviews.llvm.org/D54125#1314702>, @tejohnson wrote:
> > >
> > > > In D54125#1314634 <https://reviews.llvm.org/D54125#1314634>, @tejohnson wrote:
> > > >
> > > > > In D54125#1313868 <https://reviews.llvm.org/D54125#1313868>, @void wrote:
> > > > >
> > > > > > To replicate the failure, do this:
> > > > > >
> > > > > >   $ llvm-ar rcsTD test-archive.o softirq.o irqdesc.o
> > > > > >   $ ld.gold -plugin LLVMgold.so -plugin-opt=thinlto -plugin-opt=-code-model=kernel -plugin-opt=jobs=6 -plugin-opt=-stack-alignment=8 -m elf_x86_64 -r -o test-object.o --whole-archive test-archive.o
> > > > > >   $ nm test-object.o  | grep W
> > > > > >   0000000000001370 W arch_dynirq_lower_bound
> > > > > >   00000000000001b0 W arch_early_irq_init
> > > > > >   00000000000001a0 W arch_probe_nr_irqs
> > > > > >   0000000000000190 W early_irq_init
> > > > > >
> > > > > >
> > > > > > Note that `early_irq_init` is still weak. It should have resolved to a concrete function.
> > > > >
> > > > >
> > > > > I tried the reproducer and via -plugin-opts=save-temps I can see that the linker is telling LTO that the copy of early_irq_init in irqdesc.o is non-prevailing and that the softirq.o copy is prevailing. Therefore, with this patch the copy in irqdesc.o which is non-weak is dropped, and the weak copy in softirq.o is kept. So LTO seems to be doing what it should be based on what gold tells it.
> > > > >
> > > > > The question is why is gold picking the weak symbol as the prevailing copy and not the strong one? Note that if the order of the object files is reversed in the llvm-ar invocation, the irqdesc.o copy of that symbol is the prevailing one as per the linker, and we keep the strong symbol instead.
> > > >
> > > >
> > > > Interestingly, lld behaves differently. Even with the softirq.o being put first in the archive as you have in your repro, it says that the version of early_irq_init in irqdesc is the prevailing copy, and the strong symbol is kept. Bug in gold?
> > >
> > >
> > > I confirmed that the the llvm gold-plugin is telling gold that softirq.o:early_irq_init is a hidden weak def and that irqdesc.o:early_irq_init is a hidden strong def, and that gold is subsequently coming back and providing the following resolutions to the plugin for LTO:
> > >  softirq.o: early_irq_init: PREVAILING_DEF_REG
> > >  irqdesc.o: early_irq_init: PREEMPTED_IR
> > >
> > > Interestingly, if I compile the .o files down to native objects, then go through the same llvm-ar and gold link sequence with them, gold does what you want: it keeps the strong def. So this seems to be a bug specific to gold's plugin handling. I'm not sure how to proceed, as the patch fixes a bug and is apparently just exposing a gold linker plugin handling bug.
> >
> >
> > ah - looks like this patch exposed an issue you already discovered for this same symbol and regular LTO:
> >
> >   http://lists.llvm.org/pipermail/llvm-dev/2018-October/127051.html
> >
> > Was this ever reported to binutils/gold?
>
>
> Actually - as Peter suggested there, this is in fact fixed by a more recent version of gold. The gold I use with llvm regression tests (GNU gold (GNU Binutils 2.30.51.20180214) 1.16) fixes this issue:
>
> softirq.o: early_irq_init: PREEMPTED_IR
>  irqdesc.o: early_irq_init: PREVAILING_DEF_REG
>
> $ nm test-object.o  | grep W
>  0000000000001370 W arch_dynirq_lower_bound
>  00000000000001a0 W arch_early_irq_init
>  0000000000000190 W arch_probe_nr_irqs
>  $
>
> My installed version of gold (1.15) has the bug. So please do use a more recent version of gold to fix this issue.


That's the one I'm using. Let me see if it's possible to move to 1.16 (or maybe even lld). Thanks for your help!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54125/new/

https://reviews.llvm.org/D54125