This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 2/2] BFD: Use `bfd_is_abs_symbol' to determine whether a symbol is absolute
- From: Alan Modra <amodra at gmail dot com>
- To: "Maciej W. Rozycki" <macro at mips dot com>
- Cc: Nick Clifton <nickc at redhat dot com>, Richard Earnshaw <rearnsha at arm dot com>, Marcus Shawcroft <marcus dot shawcroft at arm dot com>, Kuan-Lin Chen <kuanlinchentw at gmail dot com>, Wei-Cheng Wang <cole945 at gmail dot com>, Richard Sandiford <rdsandiford at googlemail dot com>, binutils at sourceware dot org
- Date: Mon, 16 Jul 2018 23:40:58 +0930
- Subject: Re: [PATCH 2/2] BFD: Use `bfd_is_abs_symbol' to determine whether a symbol is absolute
- References: <alpine.DEB.2.00.1807142224150.30992@tp.orcam.me.uk> <alpine.DEB.2.00.1807152358060.30992@tp.orcam.me.uk>
On Mon, Jul 16, 2018 at 06:43:06AM +0100, Maciej W. Rozycki wrote:
> I believe the `elfNN_aarch64_final_link_relocate' case may be redundant,
> as may `xcoff_write_global_symbol' as I'm fairly sure the symbol will have
> been converted by that time.
Yes, they should have been converted.
> I think it may make sense to use the macro
> there regardless, for consistency, so that uniform code people see makes
> them sure that this is The Right Way, as the `rel_from_abs' stuff is IMHO
> quite obscure.
Except in the case of xcoff_write_global_symbol you're making the
assert weaker. I think that one should stay as is.
> --- binutils.orig/bfd/elf32-nds32.c 2018-07-15 00:18:04.818001488 +0100
> +++ binutils/bfd/elf32-nds32.c 2018-07-15 00:18:29.781920994 +0100
> @@ -10567,7 +10567,7 @@ nds32_elf_relax_loadstore (struct bfd_li
>
> /* This is avoid to relax symbol address which is fixed
> relocations. Ex: _stack. */
> - if (h && bfd_is_abs_section (h->root.u.def.section))
> + if (h && bfd_is_abs_symbol (&h->root))
> return FALSE;
> }
>
> @@ -10707,7 +10707,7 @@ nds32_elf_relax_lo12 (struct bfd_link_in
> /* This is avoid to relax symbol address which is fixed
> relocations. Ex: _stack. */
> else if (N32_OP6 (insn) == N32_OP6_ORI
> - && h && bfd_is_abs_section (h->root.u.def.section))
> + && h && bfd_is_abs_symbol (&h->root))
> return;
> else
> {
Both of these places are buggy, I think. I couldn't see a guarantee
that the code is only reachable for defined or defweak symbols.
> Index: binutils/bfd/elfnn-aarch64.c
> ===================================================================
> --- binutils.orig/bfd/elfnn-aarch64.c 2018-07-15 00:18:04.828075364 +0100
> +++ binutils/bfd/elfnn-aarch64.c 2018-07-15 00:18:29.794055010 +0100
> @@ -5224,8 +5224,7 @@ elfNN_aarch64_final_link_relocate (reloc
>
> weak_undef_p = (h ? h->root.type == bfd_link_hash_undefweak
> : bfd_is_und_section (sym_sec));
> - abs_symbol_p = (h !=NULL && h->root.type == bfd_link_hash_defined
> - && bfd_is_abs_section (h->root.u.def.section));
> + abs_symbol_p = h != NULL && bfd_is_abs_symbol (&h->root);
I suspect that this code should have been testing for defweak too.
>
> /* Since STT_GNU_IFUNC symbol must go through PLT, we handle
> @@ -7363,8 +7362,7 @@ elfNN_aarch64_check_relocs (bfd *abfd, s
> if (h != NULL
> /* This is an absolute symbol. It represents a value instead
> of an address. */
> - && ((h->root.type == bfd_link_hash_defined
> - && bfd_is_abs_section (h->root.u.def.section))
> + && (bfd_is_abs_symbol (&h->root)
> /* This is an undefined symbol. */
> || h->root.type == bfd_link_hash_undefined))
> break;
Here too, now that check_relocs is run after symbols have been
resolved.
--
Alan Modra
Australia Development Lab, IBM