Re: BFD internal errors in *_finish_dynamic_symbol

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

On 03/07/17 15:46, Egeyar Bagcioglu wrote:
> On 07/03/2017 04:13 PM, Egeyar Bagcioglu wrote:
> > On 07/03/2017 03:24 PM, Jiong Wang wrote:
> > > >
> > > 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.

The attached patch is a similar partial revert on AArch64.

I am not sure if it qualifies obvious after this discussion.

I will commit it by early afternoon tomorrow if there is no objection so it can be
included into 2.29 branch

bfd/
2017-07-03  Jiong Wang  <jiong.wang@arm.com>

        * elfnn-aarch64.c (elfNN_aarch64_finish_dynamic_symbol): Remove the
        sanity check at the head of this function.


diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index 11954486e16b3a257b483e202b47ccfe1ca802f7..86bae0eaa8eb1462013f3c58e2a6ddffd43dceab 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -8892,13 +8892,6 @@ elfNN_aarch64_finish_dynamic_symbol (bfd *output_bfd,
   struct elf_aarch64_link_hash_table *htab;
   htab = elf_aarch64_hash_table (info);
 
-  /* Sanity check to make sure no unexpected symbol reaches here.  */
-    if (h->dynindx == -1
-	&& !h->forced_local
-	&& h->root.type != bfd_link_hash_undefweak
-	&& bfd_link_pic (info))
-      abort ();
-
   if (h->plt.offset != (bfd_vma) - 1)
     {
       asection *plt, *gotplt, *relplt;