[PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
Maciej W. Rozycki
macro@imgtec.com
Fri May 19 14:14:00 GMT 2017
Revert parts of commit fee24f1c5bfe ("objdump improvements for mips
elf64"), <https://sourceware.org/ml/binutils/2003-03/msg00108.html>, and
make the `->reloc_count' member of `struct bfd_section' hold the actual
number of internal relocations stored in its `->relocation' vector. To
do so adjust `mips_elf64_slurp_one_reloc_table' to set `->reloc_count'
to the actual number of internal relocations retrieved and discard
`mips_elf64_canonicalize_reloc', `mips_elf64_canonicalize_dynamic_reloc'
and their corresponding target macros. Contrary to the description of
`mips_elf64_slurp_one_reloc_table', adjusted appropriately, this makes
generic relocation processing code happy and satisfies the "merge notes
section" binutils test case.
Add extra binutils test cases to expand the coverage of the generic
"merge notes section" test case, now passing with the n64 ABI, across
the MIPS o32, n32 and n64 ABIs regardless of the default ABI selected in
target configuration, and also to verify correctness of the relocations
produced. Conversely, do not provide any additional test cases for the
original issue addressed with the commit referred:
- objdump would display only 1/3 of the total number of relocations,
because it used the external relocation count, but each external
relocation is brought in as 3 internal relocations.
as n64 ABI relocation processing with `objdump -r' and `objdump -R' is
already widely covered across the GAS and LD test suites.
bfd/
* elf64-mips.c (mips_elf64_canonicalize_reloc): Remove prototype
and function.
(mips_elf64_canonicalize_dynamic_reloc): Likewise.
(mips_elf64_slurp_one_reloc_table): Set `reloc_count' to the
actual number of internal relocations retrieved. Adjust
function description.
(bfd_elf64_canonicalize_reloc): Remove macro.
(bfd_elf64_canonicalize_dynamic_reloc): Likewise.
binutils/
* testsuite/binutils-all/mips/mips-note-2.d: New test.
* testsuite/binutils-all/mips/mips-note-2r.d: New test.
* testsuite/binutils-all/mips/mips-note-2-n32.d: New test.
* testsuite/binutils-all/mips/mips-note-2-n64.d: New test.
* testsuite/binutils-all/mips/mips-note-2r-n32.d: New test.
* testsuite/binutils-all/mips/mips-note-2r-n64.d: New test.
* testsuite/binutils-all/mips/mips.exp: Define `has_newabi'.
Run the new tests.
---
Jose,
On Wed, 17 May 2017, Jose E. Marchesi wrote:
> > This patch fixes the deletion of relocations in BFD sections in mips64
> > targets.
> >
> > A new field `reloc_count' is added to the `_bfd_mips_elf_section_data'
> > in order to hold the number of internal relocations that the section
> > contains. A specialized `_bfd_set_reloc' function is provided that
> > updates this internal field, and the logic in the relocation-related
> > functions in elf64-mips.c is adapted to use this new field.
>
> Offhand, have you investigated reusing RELOC_AGAINST_DISCARDED_SECTION
> infrastructure here for the deletion of discarded relocations?
>
> It looks to me like the infrastructure could be used with little effort,
> e.g. `info' is only needed for `->relocatable', so the flag could be
> passed by itself rather than the whole link info structure. The MIPS n64
> case is already handled by `mips_reloc_against_discarded_section', which
> could be one handler, beside a generic one, and a SPARC64 one (which would
> have to be added), exported as a BFD method.
>
> I might be missing a detail here or there, so any actual implementation
> might come out a tad more complicated, but my high-level conclusion is I
> don't really like the duplication of the same mechanism across different
> pieces of code; being easy to miss this is really hard to maintain
> long-term, as this case has also shown.
>
> I've looked through your change as it is, on the basis that Alan has
> already approved the other parts, so please do not consider my observation
> above a request to you for further work unless you really want to look
> into it. An imperfect solution that works is certainly better short-term
> than an ideal one that yet has to be written by someone.
>
> Nope, I haven't looked at RELOC_AGAINST_DISCARDED_SECTION. How would it
> be used to make bfd_canonicalize_reloc -> bfd_set_reloc ->
> bfd_canonicalize_reloc sequences to work?
I'm not sure if it is related to the sequence in the first place -- what
I have observed is that `objcopy' appears to do the same what linker
section GC does WRT discarded relocations, however using different code.
That appears to me like unnecessary duplication.
> > +
> > +static void
> > +mips_elf64_set_reloc (bfd *abfd ATTRIBUTE_UNUSED,
> > + asection *asect,
> > + arelent **location,
> > + unsigned int count)
> > +{
> > + asect->orelocation = location;
> > + canon_reloc_count (asect) = count;
> > +}
>
> And why do you need to keep track of the number of internal relocations
> separately, given that the mapping between the internal and the external
> count is fixed at all times. Is it not enough to:
>
> BFD_ASSERT (count % 3 == 0);
> asect->reloc_count = count / 3;
>
> ?
>
> That was indeed my first approach :) But GAS seems to be setting mips64
> relocations one by one (for whatever reason) using bfd_set_reloc so the
> assert is triggered in the GAS tests.
GAS only calls `bfd_set_reloc' once per section, once all relocations
have been installed within, so it's surely not calling `bfd_set_reloc'
repeatedly for each relocation processed.
Your observation has prompted me to investigate what is going on here
though, and it looks to me like some of the backend code involved behaves
oddly. The thing is the MIPS port of GAS indeed does not install
relocation triplets, it only emits those relocations actually requested,
setting `->reloc_count' to the actual count of internal relocations
produced, regardless of whether any of them are composed or not.
Then by the time a relocation section is being produced, either of
`mips_elf64_write_rel' and `mips_elf64_write_rela' collects internal
relocations and converts them into external triplets, grouping any
composed relocations that fit and padding any unused entries with
R_MIPS_NONE relocations.
When a section is read however, such as in LD or `objdump',
`->reloc_count' is first set by `bfd_section_from_shdr' to the count of
external entries as per the associated relocation section's `->sh_size'
and `->sh_entsize', which for n64 MIPS means the count of triplets.
Later on if relocations are actually read,
`mips_elf64_slurp_one_reloc_table' converts each triplet into three
internal relocations, regardless of whether they are indeed are composed
relocations or only R_MIPS_NONE padding has been used, and finally keeps
`->reloc_count' at the third of the actual count of relocations processed,
with: "We must unfortunately set reloc_count to the number of external
relocations, because a lot of generic code seems to depend on this" being
the justification, and then `mips_elf64_canonicalize_reloc' and
`mips_elf64_canonicalize_dynamic_reloc' handling the discrepancy between
the internal relocation and the external triplet count.
That justification does not appear to stand AFAICT.
First, when GAS calls `bfd_set_reloc' `->reloc_count' is set to the
internal relocation count, which in the presence of composed relocations
(such as ones produced with `%hi(%neg(%gp_rel(...)))') will be higher than
the ultimate triplet count, and does not necessarily have to be triple the
triplet count either. So generic code in principle is prepared for
`->reloc_count' not to reflect the triplet count at least in some cases.
Second, when `mips_elf64_slurp_one_reloc_table' reads relocations back,
then it converts each triplet into three composed relocations, which makes
the situation a bit different, but as it turns out it could well discard
any R_MIPS_NONE entries used for padding and set `->reloc_count' to the
actual count of internal relocations retrieved, in which case the internal
relocations retrieved would be exactly the same as if produced by GAS.
Of course `mips_elf64_canonicalize_reloc' and
`mips_elf64_canonicalize_dynamic_reloc' would have to be adjusted
accordingly.
I have experimented with this approach and the only drawback was `objdump
-r' did not show these R_MIPS_NONE entries anymore, which is contrary to
what everyone has learnt to expect. So I have used a hack to keep these
entries where called from `objdump' only and then no regressions triggered
across any of our test suites, meaning the approach would be safe for all
other tools.
Finally, for cases where relocations have not yet been read and their
count has to be estimated like for space allocation,
`bfd_get_reloc_upper_bound' is used, which for n64 MIPS triples the
initial `->reloc_count', and has to continue to do so. However I'd expect
any code used once relocations have been read to use the actual count
rather than an estimate.
So what I have ended up with is this change, which makes
`mips_elf64_slurp_one_reloc_table' set `->reloc_count' to the actual count
of internal relocations retrieved, discards
`mips_elf64_canonicalize_reloc', `mips_elf64_canonicalize_dynamic_reloc',
fixes the "merge notes section" test case and yet does not cause any test
suite regressions. It also does not cause any differences in MIPS n64
glibc binaries built using binutils without and with the patch applied,
which I think is enough of a proof that things generally work with the
change in place.
I am going to commit this change then, as soon as the `run_dump_test'
update it depends on, and I have just posted, has been approved, which I
expect to be a formality. Any possible fallout can be handled if and as
it actually happens; this clean-up is I think well worth it.
You may want to review your other changes already committed and see if
there's anything that has become unneeded now that your MIPS change is no
longer required. That'll depend on what the SPARC part needs. Of course
your `objdump' update will still be needed for the said part.
Maciej
---
binutils-mips64-bfd-reloc-count.diff
Index: binutils/bfd/elf64-mips.c
===================================================================
--- binutils.orig/bfd/elf64-mips.c 2017-05-18 21:34:50.772042306 +0100
+++ binutils/bfd/elf64-mips.c 2017-05-18 21:36:25.887907109 +0100
@@ -88,12 +88,8 @@ static void mips_elf64_info_to_howto_rel
(bfd *, arelent *, Elf_Internal_Rela *);
static long mips_elf64_get_reloc_upper_bound
(bfd *, asection *);
-static long mips_elf64_canonicalize_reloc
- (bfd *, asection *, arelent **, asymbol **);
static long mips_elf64_get_dynamic_reloc_upper_bound
(bfd *);
-static long mips_elf64_canonicalize_dynamic_reloc
- (bfd *, arelent **, asymbol **);
static bfd_boolean mips_elf64_slurp_one_reloc_table
(bfd *, asection *, Elf_Internal_Shdr *, bfd_size_type, arelent *,
asymbol **, bfd_boolean);
@@ -3663,76 +3659,14 @@ mips_elf64_get_dynamic_reloc_upper_bound
return _bfd_elf_get_dynamic_reloc_upper_bound (abfd) * 3;
}
-/* We must also copy more relocations than the corresponding functions
- in elf.c would, so the two following functions are slightly
- modified from elf.c, that multiply the external relocation count by
- 3 to obtain the internal relocation count. */
-
-static long
-mips_elf64_canonicalize_reloc (bfd *abfd, sec_ptr section,
- arelent **relptr, asymbol **symbols)
-{
- arelent *tblptr;
- unsigned int i;
- const struct elf_backend_data *bed = get_elf_backend_data (abfd);
-
- if (! bed->s->slurp_reloc_table (abfd, section, symbols, FALSE))
- return -1;
-
- tblptr = section->relocation;
- for (i = 0; i < section->reloc_count * 3; i++)
- *relptr++ = tblptr++;
-
- *relptr = NULL;
-
- return section->reloc_count * 3;
-}
-
-static long
-mips_elf64_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
- asymbol **syms)
-{
- bfd_boolean (*slurp_relocs) (bfd *, asection *, asymbol **, bfd_boolean);
- asection *s;
- long ret;
-
- if (elf_dynsymtab (abfd) == 0)
- {
- bfd_set_error (bfd_error_invalid_operation);
- return -1;
- }
-
- slurp_relocs = get_elf_backend_data (abfd)->s->slurp_reloc_table;
- ret = 0;
- for (s = abfd->sections; s != NULL; s = s->next)
- {
- if (elf_section_data (s)->this_hdr.sh_link == elf_dynsymtab (abfd)
- && (elf_section_data (s)->this_hdr.sh_type == SHT_REL
- || elf_section_data (s)->this_hdr.sh_type == SHT_RELA))
- {
- arelent *p;
- long count, i;
-
- if (! (*slurp_relocs) (abfd, s, syms, TRUE))
- return -1;
- count = s->size / elf_section_data (s)->this_hdr.sh_entsize * 3;
- p = s->relocation;
- for (i = 0; i < count; i++)
- *storage++ = p++;
- ret += count;
- }
- }
-
- *storage = NULL;
-
- return ret;
-}
-
/* Read the relocations from one reloc section. This is mostly copied
from elfcode.h, except for the changes to expand one external
- relocation to 3 internal ones. We must unfortunately set
- reloc_count to the number of external relocations, because a lot of
- generic code seems to depend on this. */
+ relocation to 3 internal ones. To reduce processing effort we
+ could discard those R_MIPS_NONE relocations that occupy the second
+ and the third entry of a triplet, as `mips_elf64_write_rel' and
+ `mips_elf64_write_rela' recreate them in output automagically,
+ however that would also remove them from `objdump -r' output,
+ breaking a long-established tradition and likely confusing people. */
static bfd_boolean
mips_elf64_slurp_one_reloc_table (bfd *abfd, asection *asect,
@@ -3885,7 +3819,7 @@ mips_elf64_slurp_one_reloc_table (bfd *a
}
}
- asect->reloc_count += (relent - relents) / 3;
+ asect->reloc_count += relent - relents;
if (allocated != NULL)
free (allocated);
@@ -4504,9 +4438,7 @@ const struct elf_size_info mips_elf64_si
_bfd_mips_elf_print_private_bfd_data
#define bfd_elf64_get_reloc_upper_bound mips_elf64_get_reloc_upper_bound
-#define bfd_elf64_canonicalize_reloc mips_elf64_canonicalize_reloc
#define bfd_elf64_get_dynamic_reloc_upper_bound mips_elf64_get_dynamic_reloc_upper_bound
-#define bfd_elf64_canonicalize_dynamic_reloc mips_elf64_canonicalize_dynamic_reloc
#define bfd_elf64_mkobject _bfd_mips_elf_mkobject
/* The SGI style (n)64 NewABI. */
Index: binutils/binutils/testsuite/binutils-all/mips/mips-note-2-n32.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/binutils/testsuite/binutils-all/mips/mips-note-2-n32.d 2017-05-18 21:36:37.284550989 +0100
@@ -0,0 +1,7 @@
+#PROG: objcopy
+#readelf: --notes --wide
+#objcopy: --merge-notes
+#name: MIPS merge notes section (n32)
+#as: -n32 -mips3
+#source: ../note-2-32.s
+#dump: ../note-2-32.d
Index: binutils/binutils/testsuite/binutils-all/mips/mips-note-2-n64.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/binutils/testsuite/binutils-all/mips/mips-note-2-n64.d 2017-05-18 21:36:37.312048942 +0100
@@ -0,0 +1,7 @@
+#PROG: objcopy
+#readelf: --notes --wide
+#objcopy: --merge-notes
+#name: MIPS merge notes section (n64)
+#as: -64 -mips3
+#source: ../note-2-64.s
+#dump: ../note-2-64.d
Index: binutils/binutils/testsuite/binutils-all/mips/mips-note-2.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/binutils/testsuite/binutils-all/mips/mips-note-2.d 2017-05-18 21:36:37.324255567 +0100
@@ -0,0 +1,7 @@
+#PROG: objcopy
+#readelf: --notes --wide
+#objcopy: --merge-notes
+#name: MIPS merge notes section (o32)
+#as: -32
+#source: ../note-2-32.s
+#dump: ../note-2-32.d
Index: binutils/binutils/testsuite/binutils-all/mips/mips-note-2r-n32.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/binutils/testsuite/binutils-all/mips/mips-note-2r-n32.d 2017-05-18 21:36:37.338533531 +0100
@@ -0,0 +1,11 @@
+#PROG: objcopy
+#readelf: --relocs
+#objcopy: --merge-notes
+#name: MIPS merge notes section relocations (n32)
+#as: -n32 -mips3
+#source: ../note-2-32.s
+
+Relocation section '\.rela\.gnu\.build\.attributes' at offset .* contains 2 entries:
+ Offset Info Type Sym\.Value Sym\. Name \+ Addend
+00000010 ......02 R_MIPS_32 00000100 note1\.s \+ 0
+0000006c ......02 R_MIPS_32 00000104 note2\.s \+ 0
Index: binutils/binutils/testsuite/binutils-all/mips/mips-note-2r-n64.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/binutils/testsuite/binutils-all/mips/mips-note-2r-n64.d 2017-05-18 21:36:37.402349074 +0100
@@ -0,0 +1,15 @@
+#PROG: objcopy
+#readelf: --relocs
+#objcopy: --merge-notes
+#name: MIPS merge notes section relocations (n64)
+#as: -64 -mips3
+#source: ../note-2-64.s
+
+Relocation section '\.rela\.gnu\.build\.attributes' at offset .* contains 2 entries:
+ Offset Info Type Sym\. Value Sym\. Name \+ Addend
+000000000010 ....00000012 R_MIPS_64 0000000000000100 note1\.s \+ 0
+ Type2: R_MIPS_NONE
+ Type3: R_MIPS_NONE
+000000000070 ....00000012 R_MIPS_64 0000000000000104 note2\.s \+ 0
+ Type2: R_MIPS_NONE
+ Type3: R_MIPS_NONE
Index: binutils/binutils/testsuite/binutils-all/mips/mips-note-2r.d
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ binutils/binutils/testsuite/binutils-all/mips/mips-note-2r.d 2017-05-18 21:36:37.445059528 +0100
@@ -0,0 +1,11 @@
+#PROG: objcopy
+#readelf: --relocs
+#objcopy: --merge-notes
+#name: MIPS merge notes section relocations (o32)
+#as: -32
+#source: ../note-2-32.s
+
+Relocation section '\.rel\.gnu\.build\.attributes' at offset .* contains 2 entries:
+ Offset Info Type Sym\.Value Sym\. Name
+00000010 ......02 R_MIPS_32 00000100 note1\.s
+0000006c ......02 R_MIPS_32 00000104 note2\.s
Index: binutils/binutils/testsuite/binutils-all/mips/mips.exp
===================================================================
--- binutils.orig/binutils/testsuite/binutils-all/mips/mips.exp 2017-05-18 21:36:18.354373967 +0100
+++ binutils/binutils/testsuite/binutils-all/mips/mips.exp 2017-05-18 21:36:37.454209624 +0100
@@ -27,6 +27,12 @@ if [is_remote host] {
set copyfile tmpdir/copy
}
+set has_newabi [expr [istarget *-*-irix6*] \
+ || [istarget mips*-*-linux*] \
+ || [istarget mips*-sde-elf*] \
+ || [istarget mips*-mti-elf*] \
+ || [istarget mips*-img-elf*]]
+
run_dump_test "mips-ase-1"
run_dump_test "mips-ase-2"
run_dump_test "mips-ase-3"
@@ -41,3 +47,12 @@ run_dump_test "mips16-extend-insn"
run_dump_test "mips16e2-extend-insn"
run_dump_test "mips16-alias"
run_dump_test "mips16-noalias"
+
+run_dump_test "mips-note-2"
+run_dump_test "mips-note-2r"
+if $has_newabi {
+ run_dump_test "mips-note-2-n32"
+ run_dump_test "mips-note-2-n64"
+ run_dump_test "mips-note-2r-n32"
+ run_dump_test "mips-note-2r-n64"
+}
More information about the Binutils
mailing list