Re: BFD internal errors in *_finish_dynamic_symbol

[Jiong Wang <jiong dot wang at foss dot arm dot com>]

On 30/06/17 15:11, Jiong Wang wrote:
> On 29/06/17 22:23, Joseph Myers wrote:
>
> > Building the glibc testsuite, test elf/ifuncmain1staticpie, I see the
> > following BFD internal errors on AArch64 and SPARC64:
> >
> > /scratch/jmyers/glibc-bot/install/compilers/aarch64-linux-gnu/lib/gcc/aarch64-glibc-linux-gnu/8.0.0/../../../../aarch64-glibc-linux-gnu/bin/ld: 
> >
> > BFD (GNU Binutils) 2.28.51.20170629 internal error, aborting at
> > /scratch/jmyers/glibc-bot/src/binutils/bfd/elfnn-aarch64.c:8900 in
> > elf64_aarch64_finish_dynamic_symbol
> >
> > /scratch/jmyers/glibc-bot/install/compilers/sparc64-linux-gnu/lib/gcc/sparc64-glibc-linux-gnu/8.0.0/../../../../sparc64-glibc-linux-gnu/bin/ld: 
> >
> > BFD (GNU Binutils) 2.28.51.20170629 internal error, aborting at
> > /scratch/jmyers/glibc-bot/src/binutils/bfd/elfxx-sparc.c:4510 in
> > _bfd_sparc_elf_finish_dynamic_symbol
> >
> > These are caused by the commits:
> >
> > commit ec1acaba1381d0196c45965a7db9942b67fbd88d
> > Author: Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com>
> > Date:   Thu Jun 29 04:28:27 2017 -0700
> >
> >      bfd: prevent all but undef weak syms from becoming dynamic in 
> > sparc.
> >
> > commit ff07562f1e369b6e37eafb2a888dc48fa2453e86
> > Author: Jiong Wang <jiong.wang@arm.com>
> > Date:   Thu Jun 29 11:47:43 2017 +0100
> >
> >      [AArch64] Only override the symbol dynamic decision on undefined 
> > weak symbol
> >
> > (as verified by: building with binutils commit
> > ec1acaba1381d0196c45965a7db9942b67fbd88d both internal errors appear;
> > building two commits earlier, commit
> > 2aff25ba76035d2f1f48ea8a6c4b7e498ee31790, neither error occurs).
> The new assertion added in *_finish_dynamic_symbol missed IFUNC type.
>
> A local defined IFUNC in an executable still requires some 
> finalization code in
> this finish_dynamic_symbol hook to setup PLT + IRELATIVE.
>
> The fix might be either follow the X86-64 approach to only assert on 
> entries
> that were exactly touched by that patch.
>
> Or to relax the check on IFUNC.  Searching the code in bfd/elflink.c, 
> there are
> similar checks done at the call site of backend 
> *_finish_dynamic_symbol already to
> make sure it's only called when necessary, so I fell this assertion 
> could also be
> removed.
I incline to remove the assertion at the start of *finish_dynamic_symbol as this
assertion brings nothing more guarantee on the correctness.  I guess it's the
same on x86 and SPARC.

The outer caller elf_link_output_extsym in elflink.c is a traverse function on
all external symbol, and it will only call *finish_dynamic_symbol if some
conditions is meet.  It is executed conditionally.

If the condition to trigger that assertion is satisified, it then won't satify
the outer check in finish_dynamic_symbol, so *finish_dynamic_symbol won't be
called that the assertion is expected to be dead code.

If elf_link_output_extsym is a traverse function that unconditionally called
on external symbols decided to be exported, then an assertion to make sure these
symbols are in sane status might make sense.

H.J,  Egeyar, what's your opinion here?