This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
[committed v2] ELF/BFD: Hold the number of internal static relocs in `->reloc_count'
Correct a commit e5713223cbc1 ("MIPS/BFD: For n64 hold the number of
internal relocs in `->reloc_count'") regression and change internal
relocation handling in the generic ELF BFD linker code such that, except
in the presence of R_SPARC_OLO10 relocations, a section's `reloc_count'
holds the number of internal rather than external relocations, making
the handling more consistent between GAS, which sets `->reloc_count'
with a call to `bfd_set_reloc', and LD, which sets `->reloc_count' as it
reads input sections.
The handling of dynamic relocations remains unchanged and they continue
holding the number of external relocations in `->reloc_count'; they are
also not converted to the internal form except in `elf_link_sort_relocs'
(which does not handle the general, i.e. non-n64-MIPS case of composed
relocations correctly as per the ELF gABI, though it does not seem to
matter for the targets we currently support).
The n64 MIPS backend is the only one with `int_rels_per_ext_rel' set to
non-one, and consequently the change is trivial for all the remaining
backends and targets.
bfd/
* elf-bfd.h (RELOC_AGAINST_DISCARDED_SECTION): Subtract `count'
from `reloc_count' rather than decrementing it.
* elf.c (bfd_section_from_shdr): Multiply the adjustment to
`reloc_count' by `int_rels_per_ext_rel'.
* elf32-score.c (score_elf_final_link_relocate): Do not multiply
`reloc_count' by `int_rels_per_ext_rel' for last relocation
entry determination.
(s3_bfd_score_elf_check_relocs): Likewise.
* elf32-score7.c (score_elf_final_link_relocate): Likewise.
(s7_bfd_score_elf_relocate_section): Likewise.
(s7_bfd_score_elf_check_relocs): Likewise.
* elf64-mips.c (mips_elf64_get_reloc_upper_bound): Remove
prototype and function.
(mips_elf64_slurp_one_reloc_table): Do not update `reloc_count'.
(mips_elf64_slurp_reloc_table): Assert that `reloc_count' is
triple rather than once the sum of REL and RELA relocation entry
counts.
(bfd_elf64_get_reloc_upper_bound): Remove macro.
* elflink.c (_bfd_elf_link_read_relocs): Do not multiply
`reloc_count' by `int_rels_per_ext_rel' for internal relocation
storage allocation size determination.
(elf_link_input_bfd): Multiply `.ctors' and `.dtors' section's
size by `int_rels_per_ext_rel'. Do not multiply `reloc_count'
by `int_rels_per_ext_rel' for last relocation entry
determination.
(bfd_elf_final_link): Do not multiply `reloc_count' by
`int_rels_per_ext_rel' for internal relocation storage
allocation size determination.
(init_reloc_cookie_rels): Do not multiply `reloc_count' by
`int_rels_per_ext_rel' for last relocation entry determination.
(elf_gc_smash_unused_vtentry_relocs): Likewise.
* elfxx-mips.c (_bfd_mips_elf_check_relocs): Likewise.
(_bfd_mips_elf_relocate_section): Likewise.
---
On Fri, 2 Jun 2017, Andreas Schwab wrote:
> > I'll go with:
> >
> > size = o->reloc_count;
> > size *= sizeof (Elf_Internal_Rela);
> >
> > then if you don't mind. I find explicit casts worsen code readability.
>
> The cast makes the intent explicit though. Without it the next one
> touching this code will likely combine the expressions again.
Good point.
NB I have now got to the bottom of the issue seen by Joseph and this
observation:
On Mon, 22 May 2017, Joseph Myers wrote:
> Linking that test produces (expected) warnings as well as the segfault; I
> don't know if the presence of such warnings has anything to do with why
> this particular test causes a linker segfault.
>
> /scratch/jmyers/glibc/many8/build/glibcs/mips64-linux-gnu-n64/glibc/stdlib/test-
> canon2.o: In function `do_prepare':
> /scratch/jmyers/glibc/many8/build/glibcs/mips64-linux-gnu-n64/glibc-src/stdlib/t
> est-canon2.c:51: warning: the use of `mktemp' is dangerous, better use `mkstemp'
> or `mkdtemp'
was indeed relevant. The thing is due to the presence of the warning the
relocation table is read by LD twice, first via this sequence of calls:
load_symbols
bfd_elf_link_add_symbols
elf_link_add_object_symbols
_bfd_elf_link_check_relocs
_bfd_elf_link_read_relocs
elf_link_read_relocs_from_section
->swap_reloca_in (mips_elf64_be_swap_reloca_in)
setting section's `->used_by_bfd->relocs', and then again:
_bfd_generic_link_add_one_symbol
warning_callback
symbol_warning
warning_find_reloc (via bfd_map_over_sections)
bfd_canonicalize_reloc
_bfd_elf_canonicalize_reloc
->slurp_reloc_table (mips_elf64_slurp_reloc_table)
setting section's `->relocation'; in both cases updating `->reloc_count',
differently -- which was the actual bug.
This also means the optimisation I mentioned in the previous e-mail, i.e.
removing any R_MIPS_NONE padding from n64 MIPS relocation triplets cannot
be done without a further preparation, possibly teaching
`_bfd_elf_canonicalize_reloc' to use `->used_by_bfd->relocs' if available,
or (perhaps better yet) giving `objdump' a way to request that
`->slurp_reloc_table' does not optimise relocations. Anyway, not for the
moment.
As previously promised I have also investigated a possibility to add an
LD test case and concluded that, given the nature of the inconsistency I
have introduced, it is not feasible -- the segfault only triggered with a
combination of factors that could change based on the host, compiler flags
used, malloc(3) implementation, etc., and during my experiments I saw LD
succeed even with a GNU warning section present and consequently
`->reloc_count' set beyond the actual relocation count.
So for the record, here's the final change I have now committed, just
with an adjustment for the explicit cast requested. Thank you all for
your input in the course of making these changes.
Maciej
binutils-elf-bfd-int-rels-per-ext-rel.diff
Index: binutils/bfd/elf-bfd.h
===================================================================
--- binutils.orig/bfd/elf-bfd.h 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elf-bfd.h 2017-06-06 01:02:16.530147345 +0100
@@ -2767,7 +2767,7 @@ extern asection _bfd_elf_large_com_secti
memmove (rel, rel + count, \
(relend - rel - count) * sizeof (*rel)); \
\
- input_section->reloc_count--; \
+ input_section->reloc_count -= count; \
relend -= count; \
rel--; \
continue; \
Index: binutils/bfd/elf.c
===================================================================
--- binutils.orig/bfd/elf.c 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elf.c 2017-06-06 01:02:16.550338155 +0100
@@ -2374,7 +2374,8 @@ bfd_section_from_shdr (bfd *abfd, unsign
*hdr2 = *hdr;
*p_hdr = hdr2;
elf_elfsections (abfd)[shindex] = hdr2;
- target_sect->reloc_count += NUM_SHDR_ENTRIES (hdr);
+ target_sect->reloc_count += (NUM_SHDR_ENTRIES (hdr)
+ * bed->s->int_rels_per_ext_rel);
target_sect->flags |= SEC_RELOC;
target_sect->relocation = NULL;
target_sect->rel_filepos = hdr->sh_offset;
Index: binutils/bfd/elf32-score.c
===================================================================
--- binutils.orig/bfd/elf32-score.c 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elf32-score.c 2017-06-06 01:02:16.556557088 +0100
@@ -2038,7 +2038,7 @@ score_elf_final_link_relocate (reloc_how
bfd_vma lo_value = 0;
bed = get_elf_backend_data (output_bfd);
- relend = relocs + input_section->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = relocs + input_section->reloc_count;
lo16_rel = score_elf_next_relocation (input_bfd, R_SCORE_GOT_LO16, rel, relend);
if ((local_p) && (lo16_rel != NULL))
{
@@ -2808,7 +2808,7 @@ s3_bfd_score_elf_check_relocs (bfd *abfd
sreloc = NULL;
bed = get_elf_backend_data (abfd);
- rel_end = relocs + sec->reloc_count * bed->s->int_rels_per_ext_rel;
+ rel_end = relocs + sec->reloc_count;
for (rel = relocs; rel < rel_end; ++rel)
{
unsigned long r_symndx;
Index: binutils/bfd/elf32-score7.c
===================================================================
--- binutils.orig/bfd/elf32-score7.c 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elf32-score7.c 2017-06-06 01:02:16.577888650 +0100
@@ -1906,7 +1906,7 @@ score_elf_final_link_relocate (reloc_how
bfd_vma lo_value = 0;
bed = get_elf_backend_data (output_bfd);
- relend = relocs + input_section->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = relocs + input_section->reloc_count;
lo16_rel = score_elf_next_relocation (input_bfd, R_SCORE_GOT_LO16, rel, relend);
if ((local_p) && (lo16_rel != NULL))
{
@@ -1944,7 +1944,7 @@ score_elf_final_link_relocate (reloc_how
addend |= 0xffffc000;
bed = get_elf_backend_data (output_bfd);
- relend = relocs + input_section->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = relocs + input_section->reloc_count;
lo16_rel = score_elf_next_relocation (input_bfd, R_SCORE_GOT_LO16, rel, relend);
if ((local_p) && (lo16_rel != NULL))
{
@@ -2481,7 +2481,7 @@ s7_bfd_score_elf_relocate_section (bfd *
addend |= 0xffffc000;
bed = get_elf_backend_data (output_bfd);
- relend = relocs + input_section->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = relocs + input_section->reloc_count;
lo16_rel = score_elf_next_relocation (input_bfd, R_SCORE_GOT_LO16, rel, relend);
if (lo16_rel != NULL)
{
@@ -2617,7 +2617,7 @@ s7_bfd_score_elf_check_relocs (bfd *abfd
sreloc = NULL;
bed = get_elf_backend_data (abfd);
- rel_end = relocs + sec->reloc_count * bed->s->int_rels_per_ext_rel;
+ rel_end = relocs + sec->reloc_count;
for (rel = relocs; rel < rel_end; ++rel)
{
unsigned long r_symndx;
Index: binutils/bfd/elf64-mips.c
===================================================================
--- binutils.orig/bfd/elf64-mips.c 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elf64-mips.c 2017-06-06 01:02:16.582971692 +0100
@@ -86,8 +86,6 @@ static void mips_elf64_info_to_howto_rel
(bfd *, arelent *, Elf_Internal_Rela *);
static void mips_elf64_info_to_howto_rela
(bfd *, arelent *, Elf_Internal_Rela *);
-static long mips_elf64_get_reloc_upper_bound
- (bfd *, asection *);
static long mips_elf64_get_dynamic_reloc_upper_bound
(bfd *);
static bfd_boolean mips_elf64_slurp_one_reloc_table
@@ -3648,12 +3646,6 @@ mips_elf64_info_to_howto_rela (bfd *abfd
to three relocs, we must tell the user to allocate more space. */
static long
-mips_elf64_get_reloc_upper_bound (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
-{
- return (sec->reloc_count * 3 + 1) * sizeof (arelent *);
-}
-
-static long
mips_elf64_get_dynamic_reloc_upper_bound (bfd *abfd)
{
return _bfd_elf_get_dynamic_reloc_upper_bound (abfd) * 3;
@@ -3819,8 +3811,6 @@ mips_elf64_slurp_one_reloc_table (bfd *a
}
}
- asect->reloc_count += relent - relents;
-
if (allocated != NULL)
free (allocated);
@@ -3835,8 +3825,7 @@ mips_elf64_slurp_one_reloc_table (bfd *a
/* Read the relocations. On Irix 6, there can be two reloc sections
associated with a single data section. This is copied from
elfcode.h as well, with changes as small as accounting for 3
- internal relocs per external reloc and resetting reloc_count to
- zero before processing the relocs of a section. */
+ internal relocs per external reloc. */
static bfd_boolean
mips_elf64_slurp_reloc_table (bfd *abfd, asection *asect,
@@ -3864,7 +3853,7 @@ mips_elf64_slurp_reloc_table (bfd *abfd,
rel_hdr2 = d->rela.hdr;
reloc_count2 = (rel_hdr2 ? NUM_SHDR_ENTRIES (rel_hdr2) : 0);
- BFD_ASSERT (asect->reloc_count == reloc_count + reloc_count2);
+ BFD_ASSERT (asect->reloc_count == 3 * (reloc_count + reloc_count2));
BFD_ASSERT ((rel_hdr && asect->rel_filepos == rel_hdr->sh_offset)
|| (rel_hdr2 && asect->rel_filepos == rel_hdr2->sh_offset));
@@ -3890,9 +3879,6 @@ mips_elf64_slurp_reloc_table (bfd *abfd,
if (relents == NULL)
return FALSE;
- /* The slurp_one_reloc_table routine increments reloc_count. */
- asect->reloc_count = 0;
-
if (rel_hdr != NULL
&& ! mips_elf64_slurp_one_reloc_table (abfd, asect,
rel_hdr, reloc_count,
@@ -4437,7 +4423,6 @@ const struct elf_size_info mips_elf64_si
#define bfd_elf64_bfd_print_private_bfd_data \
_bfd_mips_elf_print_private_bfd_data
-#define bfd_elf64_get_reloc_upper_bound mips_elf64_get_reloc_upper_bound
#define bfd_elf64_get_dynamic_reloc_upper_bound mips_elf64_get_dynamic_reloc_upper_bound
#define bfd_elf64_mkobject _bfd_mips_elf_mkobject
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elflink.c 2017-06-06 01:02:16.632453703 +0100
@@ -2450,8 +2450,7 @@ _bfd_elf_link_read_relocs (bfd *abfd,
{
bfd_size_type size;
- size = o->reloc_count;
- size *= bed->s->int_rels_per_ext_rel * sizeof (Elf_Internal_Rela);
+ size = (bfd_size_type) o->reloc_count * sizeof (Elf_Internal_Rela);
if (keep_memory)
internal_relocs = alloc2 = (Elf_Internal_Rela *) bfd_alloc (abfd, size);
else
@@ -10402,7 +10401,8 @@ elf_link_input_bfd (struct elf_final_lin
".fini_array") == 0))
&& (o->name[6] == 0 || o->name[6] == '.'))
{
- if (o->size != o->reloc_count * address_size)
+ if (o->size * bed->s->int_rels_per_ext_rel
+ != o->reloc_count * address_size)
{
_bfd_error_handler
/* xgettext:c-format */
@@ -10426,7 +10426,7 @@ elf_link_input_bfd (struct elf_final_lin
relocs against removed link-once sections. */
rel = internal_relocs;
- relend = rel + o->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = rel + o->reloc_count;
for ( ; rel < relend; rel++)
{
unsigned long r_symndx = rel->r_info >> r_sym_shift;
@@ -10615,7 +10615,7 @@ elf_link_input_bfd (struct elf_final_lin
/* Adjust the reloc addresses and symbol indices. */
irela = internal_relocs;
- irelaend = irela + o->reloc_count * bed->s->int_rels_per_ext_rel;
+ irelaend = irela + o->reloc_count;
rel_hash = esdo->rel.hashes + esdo->rel.count;
/* We start processing the REL relocs, if any. When we reach
IRELAMID in the loop, we switch to the RELA relocs. */
@@ -11789,8 +11789,7 @@ bfd_elf_final_link (bfd *abfd, struct bf
if (max_internal_reloc_count != 0)
{
- amt = max_internal_reloc_count * bed->s->int_rels_per_ext_rel;
- amt *= sizeof (Elf_Internal_Rela);
+ amt = max_internal_reloc_count * sizeof (Elf_Internal_Rela);
flinfo.internal_relocs = (Elf_Internal_Rela *) bfd_malloc (amt);
if (flinfo.internal_relocs == NULL)
goto error_return;
@@ -12589,8 +12588,7 @@ init_reloc_cookie_rels (struct elf_reloc
if (cookie->rels == NULL)
return FALSE;
cookie->rel = cookie->rels;
- cookie->relend = (cookie->rels
- + sec->reloc_count * bed->s->int_rels_per_ext_rel);
+ cookie->relend = cookie->rels + sec->reloc_count;
}
cookie->rel = cookie->rels;
return TRUE;
@@ -13203,7 +13201,7 @@ elf_gc_smash_unused_vtentry_relocs (stru
bed = get_elf_backend_data (sec->owner);
log_file_align = bed->s->log_file_align;
- relend = relstart + sec->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = relstart + sec->reloc_count;
for (rel = relstart; rel < relend; ++rel)
if (rel->r_offset >= hstart && rel->r_offset < hend)
Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c 2017-06-06 01:01:56.000000000 +0100
+++ binutils/bfd/elfxx-mips.c 2017-06-06 01:02:16.644542583 +0100
@@ -8102,7 +8102,7 @@ _bfd_mips_elf_check_relocs (bfd *abfd, s
extsymoff = (elf_bad_symtab (abfd)) ? 0 : symtab_hdr->sh_info;
bed = get_elf_backend_data (abfd);
- rel_end = relocs + sec->reloc_count * bed->s->int_rels_per_ext_rel;
+ rel_end = relocs + sec->reloc_count;
/* Check for the mips16 stub sections. */
@@ -10034,7 +10034,7 @@ _bfd_mips_elf_relocate_section (bfd *out
const struct elf_backend_data *bed;
bed = get_elf_backend_data (output_bfd);
- relend = relocs + input_section->reloc_count * bed->s->int_rels_per_ext_rel;
+ relend = relocs + input_section->reloc_count;
for (rel = relocs; rel < relend; ++rel)
{
const char *name;