This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'

Revert parts of commit fee24f1c5bfe ("objdump improvements for mips 
elf64"), <>, 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.

	* 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.

	* 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.

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 

 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.


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
 #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"

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]