This is the mail archive of the
mailing list for the binutils project.
Re: BFD internal errors in *_finish_dynamic_symbol
- From: Jiong Wang <jiong dot wang at foss dot arm dot com>
- To: egeyar dot bagcioglu at oracle dot com, "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: Joseph Myers <joseph at codesourcery dot com>, binutils at sourceware dot org
- Date: Mon, 3 Jul 2017 17:43:08 +0100
- Subject: Re: BFD internal errors in *_finish_dynamic_symbol
- Authentication-results: sourceware.org; auth=none
- References: <alpine.DEB.email@example.com> <firstname.lastname@example.org> <email@example.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?
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
2017-07-03 Jiong Wang <firstname.lastname@example.org>
* 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
@@ -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;