Re: BFD internal errors in *_finish_dynamic_symbol

[Egeyar Bagcioglu <egeyar dot bagcioglu at oracle dot com>]

On 07/03/2017 04:13 PM, Egeyar Bagcioglu wrote:
> On 07/03/2017 03:24 PM, Jiong Wang wrote:
> > 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?
> Hello Jiong,
>
> I agree with your point. This assertion does not contribute to 
> correctness. It was not to skip any unforseen problems about the core 
> change of the patch we submitted. I would expect it to help detecting 
> an issue regarding the changed relocation. Instead, it detected an 
> issue with the condition to assert. Under these circumstances, I see 
> no advantage of keeping it. If a problem occurs regarding the core 
> change of our previous patches, it can easily be traced back to us via 
> git bisect.
>
> Unless someone objects by then, I'll upload the corresponding 
> mini-patch after receiving internal approval from my team.

The attached patch fixes the issue for sparc.

>From 234604426719d6696a41ef7b37837632ceb7c03e Mon Sep 17 00:00:00 2001
From: Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com>
Date: Sun, 2 Jul 2017 19:51:37 -0700
Subject: [PATCH] sparc: Remove 'abort' statement in finish_dynamic_symbol

The check is incomplete and caused issues. Maintaining it is not necessary as there is no sign of *_finish_dynamic_symbol being called unintentionally.

This patch is tested on sparc64-unknown-linux-gnu, no regressions are found.

bfd/ChangeLog:

2017-07-03  Egeyar Bagcioglu  <egeyar.bagcioglu@oracle.com>

        * elfxx-sparc.c (_bfd_sparc_elf_finish_dynamic_symbol): Remove the
        abort statement that was put for symbols that are not dynamic.
---
 bfd/elfxx-sparc.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/bfd/elfxx-sparc.c b/bfd/elfxx-sparc.c
index 1fd2141..b50d34b 100644
--- a/bfd/elfxx-sparc.c
+++ b/bfd/elfxx-sparc.c
@@ -4502,13 +4502,6 @@ _bfd_sparc_elf_finish_dynamic_symbol (bfd *output_bfd,
 
   eh = (struct _bfd_sparc_elf_link_hash_entry *) h;
 
-  /* Abort if the symbol is not dynamic in PIC */
-  if (h->dynindx == -1
-      && !h->forced_local
-      && h->root.type != bfd_link_hash_undefweak
-      && bfd_link_pic (info))
-    abort();
-
   /* We keep PLT/GOT entries without dynamic PLT/GOT relocations for
      resolved undefined weak symbols in executable so that their
      references have value 0 at run-time.  */
-- 
1.7.1