public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64
  2017-05-05 12:10 [PATCH V2 0/3] Fix relocation deletion problems in SPARC and MIPS Jose E. Marchesi
@ 2017-05-05 12:10 ` Jose E. Marchesi
  2017-05-10 16:51   ` Jose E. Marchesi
  2017-05-05 12:10 ` [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64 Jose E. Marchesi
  2017-05-05 12:10 ` [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc Jose E. Marchesi
  2 siblings, 1 reply; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-05 12:10 UTC (permalink / raw)
  To: binutils

This patch fixes the deletion of relocations in BFD sections in
sparc64 targets.

A specialized `_bfd_set_reloc' function is provided that updates the
internal canon_reloc_count(sec) counter instead of sec->reloc_count.
Additionally, the `write_relocs' callback in elf64-sparc is adapted to
use the canon_reloc_count to traverse `sec->orelocation'.

Tested in sparc64-linux-gnu targets.
Fixes an existing failure in the merge-notes objcopy test.
No regressions.

bfd/ChangeLog:

2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* elf64-sparc.c (elf64_sparc_set_reloc): New function.
	(bfd_elf64_set_reloc): Define.
	(elf64_sparc_write_relocs): Use `canon_reloc_count'.
---
 bfd/ChangeLog     |  6 ++++++
 bfd/elf64-sparc.c | 24 +++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 9002dfa..d8ec8a7 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,5 +1,11 @@
 2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
 
+	* elf64-sparc.c (elf64_sparc_set_reloc): New function.
+	(bfd_elf64_set_reloc): Define.
+	(elf64_sparc_write_relocs): Use `canon_reloc_count'.
+
+2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
 	* targets.c (BFD_JUMP_TABLE_RELOCS): Add NAME##_set_reloc.
 	(struct bfd_target): New field _bfd_set_reloc.
 	* bfd.c (bfd_set_reloc): Call backend _set_bfd.
diff --git a/bfd/elf64-sparc.c b/bfd/elf64-sparc.c
index 9456b59..3e0772d 100644
--- a/bfd/elf64-sparc.c
+++ b/bfd/elf64-sparc.c
@@ -278,6 +278,18 @@ elf64_sparc_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
   return ret;
 }
 
+/* Install a new set of internal relocs.  */
+
+static void
+elf64_sparc_set_reloc (bfd *abfd ATTRIBUTE_UNUSED,
+                       asection *asect,
+                       arelent **location,
+                       unsigned int count)
+{
+  asect->orelocation = location;
+  canon_reloc_count (asect) = count;
+}
+
 /* Write out the relocs.  */
 
 static void
@@ -302,14 +314,14 @@ elf64_sparc_write_relocs (bfd *abfd, asection *sec, void * data)
      reloc_count field to zero to inhibit writing them here.  Also,
      sometimes the SEC_RELOC flag gets set even when there aren't any
      relocs.  */
-  if (sec->reloc_count == 0)
+  if (canon_reloc_count (sec) == 0)
     return;
 
   /* We can combine two relocs that refer to the same address
      into R_SPARC_OLO10 if first one is R_SPARC_LO10 and the
      latter is R_SPARC_13 with no associated symbol.  */
   count = 0;
-  for (idx = 0; idx < sec->reloc_count; idx++)
+  for (idx = 0; idx < canon_reloc_count (sec); idx++)
     {
       bfd_vma addr;
 
@@ -317,7 +329,7 @@ elf64_sparc_write_relocs (bfd *abfd, asection *sec, void * data)
 
       addr = sec->orelocation[idx]->address;
       if (sec->orelocation[idx]->howto->type == R_SPARC_LO10
-	  && idx < sec->reloc_count - 1)
+	  && idx < canon_reloc_count (sec) - 1)
 	{
 	  arelent *r = sec->orelocation[idx + 1];
 
@@ -354,7 +366,7 @@ elf64_sparc_write_relocs (bfd *abfd, asection *sec, void * data)
   outbound_relocas = (Elf64_External_Rela *) rela_hdr->contents;
   src_rela = outbound_relocas;
 
-  for (idx = 0; idx < sec->reloc_count; idx++)
+  for (idx = 0; idx < canon_reloc_count (sec); idx++)
     {
       Elf_Internal_Rela dst_rela;
       arelent *ptr;
@@ -388,7 +400,7 @@ elf64_sparc_write_relocs (bfd *abfd, asection *sec, void * data)
 	}
 
       if (ptr->howto->type == R_SPARC_LO10
-	  && idx < sec->reloc_count - 1)
+	  && idx < canon_reloc_count (sec) - 1)
 	{
 	  arelent *r = sec->orelocation[idx + 1];
 
@@ -854,6 +866,8 @@ const struct elf_size_info elf64_sparc_size_info =
   elf64_sparc_canonicalize_reloc
 #define bfd_elf64_canonicalize_dynamic_reloc \
   elf64_sparc_canonicalize_dynamic_reloc
+#define bfd_elf64_set_reloc \
+  elf64_sparc_set_reloc
 #define elf_backend_add_symbol_hook \
   elf64_sparc_add_symbol_hook
 #define elf_backend_get_symbol_type \
-- 
2.3.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64
  2017-05-05 12:10 [PATCH V2 0/3] Fix relocation deletion problems in SPARC and MIPS Jose E. Marchesi
  2017-05-05 12:10 ` [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64 Jose E. Marchesi
@ 2017-05-05 12:10 ` Jose E. Marchesi
  2017-05-17 15:29   ` Maciej W. Rozycki
  2017-05-05 12:10 ` [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc Jose E. Marchesi
  2 siblings, 1 reply; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-05 12:10 UTC (permalink / raw)
  To: binutils

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.

This patch also removes the safety check recently introduced in
objcopy, to avoid deleting relocations if the count returned by
`bfd_canonicalize_relocs' is bigger than `sec->reloc_count', as it is
no longer necessary.

bfd/ChangeLog:

    2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>

    	* elfxx-mips.h (struct _bfd_mips_elf_section_data): Moved from
    	elfxx-mips.c.  Add a new field `reloc_count'.
    	(mips_elf_section_data): Moved from elfxx-mips.c.
    	(canon_reloc_count): Define.
    	* elf64-mips.c (mips_elf64_set_reloc): New function.
    	(mips_elf64_canonicalize_reloc): Use canon_reloc_count.
    	(mips_elf64_slurp_reloc_table): Likewise.
    	(mips_elf64_slurp_one_reloc_table): Likewise.
    	(mips_elf64_write_relocs): Use `canon_reloc_count'.
    	(mips_elf64_write_rel): Likewise.
    	(mips_elf64_write_rela): Likewise.
    	(mips_elf64_canonicalize_dynamic_reloc): Likewise.
    	(bfd_elf64_set_reloc): Define.

binutils/ChangeLog:

    2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>

    	* objcopy.c (merge_gnu_build_notes): Remove special case for
    	mips64 and sparc64, as it is not needed anymore.
---
 bfd/ChangeLog      | 16 ++++++++++++++++
 bfd/elf64-mips.c   | 47 ++++++++++++++++++++++++++++++++++-------------
 bfd/elfxx-mips.c   | 12 ------------
 bfd/elfxx-mips.h   | 16 ++++++++++++++++
 binutils/ChangeLog |  5 +++++
 binutils/objcopy.c |  7 -------
 6 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index d8ec8a7..71ad6a8 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,5 +1,21 @@
 2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
 
+	* elfxx-mips.h (struct _bfd_mips_elf_section_data): Moved from
+	elfxx-mips.c.  Add a new field `reloc_count'.
+	(mips_elf_section_data): Moved from elfxx-mips.c.
+	(canon_reloc_count): Define.
+	* elf64-mips.c (mips_elf64_set_reloc): New function.
+	(mips_elf64_canonicalize_reloc): Use canon_reloc_count.
+	(mips_elf64_slurp_reloc_table): Likewise.
+	(mips_elf64_slurp_one_reloc_table): Likewise.
+	(mips_elf64_write_relocs): Use `canon_reloc_count'.
+	(mips_elf64_write_rel): Likewise.
+	(mips_elf64_write_rela): Likewise.
+	(mips_elf64_canonicalize_dynamic_reloc): Likewise.
+	(bfd_elf64_set_reloc): Define.
+
+2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
 	* elf64-sparc.c (elf64_sparc_set_reloc): New function.
 	(bfd_elf64_set_reloc): Define.
 	(elf64_sparc_write_relocs): Use `canon_reloc_count'.
diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
index a66c319..63880c6 100644
--- a/bfd/elf64-mips.c
+++ b/bfd/elf64-mips.c
@@ -90,6 +90,8 @@ static long mips_elf64_get_reloc_upper_bound
   (bfd *, asection *);
 static long mips_elf64_canonicalize_reloc
   (bfd *, asection *, arelent **, asymbol **);
+static void mips_elf64_set_reloc
+  (bfd *, asection *, arelent **, unsigned int);
 static long mips_elf64_get_dynamic_reloc_upper_bound
   (bfd *);
 static long mips_elf64_canonicalize_dynamic_reloc
@@ -3680,12 +3682,12 @@ mips_elf64_canonicalize_reloc (bfd *abfd, sec_ptr section,
     return -1;
 
   tblptr = section->relocation;
-  for (i = 0; i < section->reloc_count * 3; i++)
+  for (i = 0; i < canon_reloc_count (section); i++)
     *relptr++ = tblptr++;
 
   *relptr = NULL;
 
-  return section->reloc_count * 3;
+  return canon_reloc_count (section);
 }
 
 static long
@@ -3715,7 +3717,7 @@ mips_elf64_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
 
 	  if (! (*slurp_relocs) (abfd, s, syms, TRUE))
 	    return -1;
-	  count = s->size / elf_section_data (s)->this_hdr.sh_entsize * 3;
+	  count = canon_reloc_count (s);
 	  p = s->relocation;
 	  for (i = 0; i < count; i++)
 	    *storage++ = p++;
@@ -3728,6 +3730,22 @@ mips_elf64_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
   return ret;
 }
 
+/* A similar problem happens with set_reloc.  This version updates the
+   internal `reloc_count' field in `struct _mips_elf_section_data' in
+   order to hold the new number of internal relocations.  This avoids
+   overwriting `asect->reloc_count' that holds the number of external
+   relocations.  */
+
+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;
+}
+
 /* 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
@@ -3885,7 +3903,7 @@ mips_elf64_slurp_one_reloc_table (bfd *abfd, asection *asect,
 	}
     }
 
-  asect->reloc_count += (relent - relents) / 3;
+  canon_reloc_count (asect) += (relent - relents);
 
   if (allocated != NULL)
     free (allocated);
@@ -3956,8 +3974,9 @@ mips_elf64_slurp_reloc_table (bfd *abfd, asection *asect,
   if (relents == NULL)
     return FALSE;
 
-  /* The slurp_one_reloc_table routine increments reloc_count.  */
-  asect->reloc_count = 0;
+  /* The slurp_one_reloc_table routine increments
+     canon_reloc_count.  */
+  canon_reloc_count (asect) = 0;
 
   if (rel_hdr != NULL
       && ! mips_elf64_slurp_one_reloc_table (abfd, asect,
@@ -3972,6 +3991,7 @@ mips_elf64_slurp_reloc_table (bfd *abfd, asection *asect,
 					     symbols, dynamic))
     return FALSE;
 
+  asect->reloc_count = canon_reloc_count (asect) / 3;
   asect->relocation = relents;
   return TRUE;
 }
@@ -3997,13 +4017,13 @@ mips_elf64_write_relocs (bfd *abfd, asection *sec, void *data)
      reloc_count field to zero to inhibit writing them here.  Also,
      sometimes the SEC_RELOC flag gets set even when there aren't any
      relocs.  */
-  if (sec->reloc_count == 0)
+  if (canon_reloc_count (sec) == 0)
     return;
 
   /* We can combine up to three relocs that refer to the same address
      if the latter relocs have no associated symbol.  */
   count = 0;
-  for (idx = 0; idx < sec->reloc_count; idx++)
+  for (idx = 0; idx < canon_reloc_count (sec); idx++)
     {
       bfd_vma addr;
       unsigned int i;
@@ -4015,7 +4035,7 @@ mips_elf64_write_relocs (bfd *abfd, asection *sec, void *data)
 	{
 	  arelent *r;
 
-	  if (idx + 1 >= sec->reloc_count)
+	  if (idx + 1 >= canon_reloc_count (sec))
 	    break;
 	  r = sec->orelocation[idx + 1];
 	  if (r->address != addr
@@ -4061,7 +4081,7 @@ mips_elf64_write_rel (bfd *abfd, asection *sec,
     }
 
   ext_rel = (Elf64_Mips_External_Rel *) rel_hdr->contents;
-  for (idx = 0; idx < sec->reloc_count; idx++, ext_rel++)
+  for (idx = 0; idx < canon_reloc_count (sec); idx++, ext_rel++)
     {
       arelent *ptr;
       Elf64_Mips_Internal_Rela int_rel;
@@ -4114,7 +4134,7 @@ mips_elf64_write_rel (bfd *abfd, asection *sec,
 	{
 	  arelent *r;
 
-	  if (idx + 1 >= sec->reloc_count)
+	  if (idx + 1 >= canon_reloc_count (sec))
 	    break;
 	  r = sec->orelocation[idx + 1];
 	  if (r->address != ptr->address
@@ -4159,7 +4179,7 @@ mips_elf64_write_rela (bfd *abfd, asection *sec,
     }
 
   ext_rela = (Elf64_Mips_External_Rela *) rela_hdr->contents;
-  for (idx = 0; idx < sec->reloc_count; idx++, ext_rela++)
+  for (idx = 0; idx < canon_reloc_count (sec); idx++, ext_rela++)
     {
       arelent *ptr;
       Elf64_Mips_Internal_Rela int_rela;
@@ -4213,7 +4233,7 @@ mips_elf64_write_rela (bfd *abfd, asection *sec,
 	{
 	  arelent *r;
 
-	  if (idx + 1 >= sec->reloc_count)
+	  if (idx + 1 >= canon_reloc_count (sec))
 	    break;
 	  r = sec->orelocation[idx + 1];
 	  if (r->address != ptr->address
@@ -4505,6 +4525,7 @@ const struct elf_size_info mips_elf64_size_info =
 
 #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_set_reloc mips_elf64_set_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
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index 70c7f1c..8aa979e 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -221,18 +221,6 @@ struct mips_elf_traverse_got_arg
   int value;
 };
 
-struct _mips_elf_section_data
-{
-  struct bfd_elf_section_data elf;
-  union
-  {
-    bfd_byte *tdata;
-  } u;
-};
-
-#define mips_elf_section_data(sec) \
-  ((struct _mips_elf_section_data *) elf_section_data (sec))
-
 #define is_mips_elf(bfd)				\
   (bfd_get_flavour (bfd) == bfd_target_elf_flavour	\
    && elf_tdata (bfd) != NULL				\
diff --git a/bfd/elfxx-mips.h b/bfd/elfxx-mips.h
index 44ad789..5710c61 100644
--- a/bfd/elfxx-mips.h
+++ b/bfd/elfxx-mips.h
@@ -22,6 +22,22 @@
 #include "elf/internal.h"
 #include "elf/mips.h"
 
+struct _mips_elf_section_data
+{
+  struct bfd_elf_section_data elf;
+  union
+  {
+    bfd_byte *tdata;
+  } u;
+  unsigned int reloc_count;
+};
+
+#define mips_elf_section_data(sec) \
+  ((struct _mips_elf_section_data *) elf_section_data (sec))
+
+#define canon_reloc_count(sec) \
+  ((struct _mips_elf_section_data *) elf_section_data (sec))->reloc_count
+
 extern bfd_boolean _bfd_mips_elf_mkobject
   (bfd *);
 extern bfd_boolean _bfd_mips_elf_new_section_hook
diff --git a/binutils/ChangeLog b/binutils/ChangeLog
index b3a539a..5a924bc 100644
--- a/binutils/ChangeLog
+++ b/binutils/ChangeLog
@@ -1,3 +1,8 @@
+2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* objcopy.c (merge_gnu_build_notes): Remove special case for
+	mips64 and sparc64, as it is not needed anymore.
+
 2017-05-02  H.J. Lu  <hongjiu.lu@intel.com>
 
 	* objcopy.c (merge_gnu_build_notes): Cast relcount to unsigned
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index ccb5e12..42c7775 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2137,13 +2137,6 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
 	    relcount = 0;
 	}
 
-      /* A few targets (eg MIPS, SPARC) create multiple internal relocs to
-	 represent a single external reloc.  Unfortunately the current BFD
-	 API does not handle deleting relocs in such situations very well
-	 and so it is unsafe to proceed.  */
-      if ((unsigned long) relcount > sec->reloc_count)
-	goto done;
-
       /* Eliminate the duplicates.  */
       new = new_contents = xmalloc (size);
       for (pnote = pnotes, old = contents;
-- 
2.3.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V2 0/3] Fix relocation deletion problems in SPARC and MIPS
@ 2017-05-05 12:10 Jose E. Marchesi
  2017-05-05 12:10 ` [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64 Jose E. Marchesi
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-05 12:10 UTC (permalink / raw)
  To: binutils

[Changes from previous version:
- The patch serie has now been tested by building and checking with
  --enable-targets=all --enable-64-bit-bfd.
- As a result of the above, a number of missing targets have been
  fixed to provide the bfd_set_reloc entry point, making the first
  patch complete: coff-alpha, mach-o, vms-alpha, aout-adobe, bout,
  coff-mips, i386os9k, ieee, oasys, som, versados, coff64-rs6000.
- The `bfd_set_reloc' entry point in bfd.c has been turned into
  a macro.
- The comment describing the `mips_elf64_set_reloc' function has been
  fixed.
- I am struggling at the moment to get the regression tester machinery
  from Nick working, to run regression testing in all supported targets.
  This may take some time...]

This patch serie aims to fix the problems detailed in the mailing list
post https://sourceware.org/ml/binutils/2017-04/msg00264.html.

The first patch makes it possible for BFD targets to define their own
versions of `bfd_set_reloc', and provides a default version used by
all the existing targets.

The second patch adds a specialized version of `bfd_set_reloc' for
elf64-sparc targets, that fixes the API problem described in the URL
above, and also the regression in the merge-notes objcopy test.

The third patch adds a similar hack to elf64-mips targets.  Note that
in this case it was needed to add a new field to the
`_mips_elf_section-data' and use a similar hack than in sparc64.  This
is because the assembler adds single relocations using `bfd_set_reloc'
and thus dividing the number by 3 and setting it to `sec->reloc_count'
wouldn't work.

It would be good if a MIPS maintainer looks at this last patch, as my
MIPS knowledge is rudimentary to say the best :D

Salud!


Jose E. Marchesi (3):
  bfd: new BFD target entry point _bfd_set_reloc.
  bfd: fix the deletion of relocs in sparc64
  bfd: fix the deletion of relocs in mips64

 bfd/ChangeLog       | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 bfd/aout-adobe.c    |  1 +
 bfd/aout-target.h   |  3 +++
 bfd/aout-tic30.c    |  3 +++
 bfd/bfd-in2.h       |  5 +++++
 bfd/bfd.c           | 12 ++----------
 bfd/bout.c          |  1 +
 bfd/coff-alpha.c    |  3 +++
 bfd/coff-mips.c     |  1 +
 bfd/coff-rs6000.c   |  1 +
 bfd/coff64-rs6000.c |  2 ++
 bfd/coffcode.h      |  4 ++++
 bfd/elf64-mips.c    | 47 ++++++++++++++++++++++++++++++++-------------
 bfd/elf64-sparc.c   | 24 ++++++++++++++++++-----
 bfd/elfxx-mips.c    | 12 ------------
 bfd/elfxx-mips.h    | 16 ++++++++++++++++
 bfd/elfxx-target.h  |  3 +++
 bfd/i386msdos.c     |  1 +
 bfd/i386os9k.c      |  2 ++
 bfd/ieee.c          |  1 +
 bfd/libbfd-in.h     |  2 ++
 bfd/libbfd.c        |  9 +++++++++
 bfd/libbfd.h        |  8 ++++++++
 bfd/mach-o-target.c |  1 +
 bfd/nlm-target.h    |  1 +
 bfd/oasys.c         |  1 +
 bfd/reloc.c         | 26 +++++++++++++++++++++++++
 bfd/som.c           |  1 +
 bfd/targets.c       |  3 +++
 bfd/versados.c      |  1 +
 bfd/vms-alpha.c     |  5 +++++
 binutils/ChangeLog  |  5 +++++
 binutils/objcopy.c  |  7 -------
 33 files changed, 220 insertions(+), 47 deletions(-)

-- 
2.3.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc.
  2017-05-05 12:10 [PATCH V2 0/3] Fix relocation deletion problems in SPARC and MIPS Jose E. Marchesi
  2017-05-05 12:10 ` [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64 Jose E. Marchesi
  2017-05-05 12:10 ` [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64 Jose E. Marchesi
@ 2017-05-05 12:10 ` Jose E. Marchesi
  2017-05-07  2:53   ` Alan Modra
  2 siblings, 1 reply; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-05 12:10 UTC (permalink / raw)
  To: binutils

This patch adds a new entry point to the BFD_JUMP_TABLE_RELOCS.  The
previous common implementation `bfd_set_reloc', in bfd/bfd.c, has been
moved to bfd/reloc.c with the name `_bfd_generic_set_reloc', and all
BFD targets has been adapted to use it.

This patch doesn't introduce any change on functionality, but prepares
the ground for further work.

bfd/ChangeLog:

    2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>

    	* targets.c (BFD_JUMP_TABLE_RELOCS): Add NAME##_set_reloc.
    	(struct bfd_target): New field _bfd_set_reloc.
    	* bfd.c (bfd_set_reloc): Call backend _set_bfd.
    	* reloc.c (_bfd_generic_set_reloc): New function.
    	* coffcode.h (coff_set_reloc): Define to _bfd_generic_set_reloc.
    	* nlm-target.h (nlm_set_reloc): Likewise.
    	* coff-rs6000.c (_bfd_xcoff_set_reloc): Likewise.
    	* aout-tic30.c (MY_set_reloc): Likewise.
    	* aout-target.h (MY_set_reloc): Likewise.
    	* elfxx-target.h (bfd_elfNN_set_reloc): Likewise.
    	* coff-alpha.c (_bfd_ecoff_set_reloc): Likewise.
    	* mach-o-target.c (bfd_mach_o_set_reloc): Likewise.
    	* vms-alpha.c (alpha_vms_set_reloc): Likewise.
    	* aout-adobe.c (aout_32_set_reloc): Likewise.
    	* bout.c (b_out_set_reloc): Likewise.
    	* coff-mips.c (_bfd_ecoff_set_reloc): Likewise.
    	* i386os9k.c (aout_32_set_reloc): Likewise.
    	* ieee.c (ieee_set_reloc): Likewise.
    	* oasys.c (oasys_set_reloc): Likewise.
    	* som.c (som_set_reloc): Likewise.
    	* versados.c (versados_set_reloc): Likewise.
    	* coff64-rs6000.c (rs6000_xcoff64_vec): Add
    	_bfd_generic_set_reloc.
    	(rs6000_xcoff64_aix_vec): LIkewise.
    	* libbfd.c (_bfd_norelocs_set_reloc): New function.
    	* libbfd-in.h: Prototype for _bfd_norelocs_set_reloc.
    	* i386msdos.c (msdos_set_reloc): Define to
    	_bfd_norelocs_set_reloc.
    	* elfcode.h (elf_set_reloc): Define.
    	* bfd-in2.h: Regenerated.
---
 bfd/ChangeLog       | 33 +++++++++++++++++++++++++++++++++
 bfd/aout-adobe.c    |  1 +
 bfd/aout-target.h   |  3 +++
 bfd/aout-tic30.c    |  3 +++
 bfd/bfd-in2.h       |  5 +++++
 bfd/bfd.c           | 12 ++----------
 bfd/bout.c          |  1 +
 bfd/coff-alpha.c    |  3 +++
 bfd/coff-mips.c     |  1 +
 bfd/coff-rs6000.c   |  1 +
 bfd/coff64-rs6000.c |  2 ++
 bfd/coffcode.h      |  4 ++++
 bfd/elfxx-target.h  |  3 +++
 bfd/i386msdos.c     |  1 +
 bfd/i386os9k.c      |  2 ++
 bfd/ieee.c          |  1 +
 bfd/libbfd-in.h     |  2 ++
 bfd/libbfd.c        |  9 +++++++++
 bfd/libbfd.h        |  8 ++++++++
 bfd/mach-o-target.c |  1 +
 bfd/nlm-target.h    |  1 +
 bfd/oasys.c         |  1 +
 bfd/reloc.c         | 26 ++++++++++++++++++++++++++
 bfd/som.c           |  1 +
 bfd/targets.c       |  3 +++
 bfd/versados.c      |  1 +
 bfd/vms-alpha.c     |  5 +++++
 27 files changed, 124 insertions(+), 10 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 27ac8c3..9002dfa 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,36 @@
+2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* targets.c (BFD_JUMP_TABLE_RELOCS): Add NAME##_set_reloc.
+	(struct bfd_target): New field _bfd_set_reloc.
+	* bfd.c (bfd_set_reloc): Call backend _set_bfd.
+	* reloc.c (_bfd_generic_set_reloc): New function.
+	* coffcode.h (coff_set_reloc): Define to _bfd_generic_set_reloc.
+	* nlm-target.h (nlm_set_reloc): Likewise.
+	* coff-rs6000.c (_bfd_xcoff_set_reloc): Likewise.
+	* aout-tic30.c (MY_set_reloc): Likewise.
+	* aout-target.h (MY_set_reloc): Likewise.
+	* elfxx-target.h (bfd_elfNN_set_reloc): Likewise.
+	* coff-alpha.c (_bfd_ecoff_set_reloc): Likewise.
+	* mach-o-target.c (bfd_mach_o_set_reloc): Likewise.
+	* vms-alpha.c (alpha_vms_set_reloc): Likewise.
+	* aout-adobe.c (aout_32_set_reloc): Likewise.
+	* bout.c (b_out_set_reloc): Likewise.
+	* coff-mips.c (_bfd_ecoff_set_reloc): Likewise.
+	* i386os9k.c (aout_32_set_reloc): Likewise.
+	* ieee.c (ieee_set_reloc): Likewise.
+	* oasys.c (oasys_set_reloc): Likewise.
+	* som.c (som_set_reloc): Likewise.
+	* versados.c (versados_set_reloc): Likewise.
+	* coff64-rs6000.c (rs6000_xcoff64_vec): Add
+	_bfd_generic_set_reloc.
+	(rs6000_xcoff64_aix_vec): LIkewise.
+	* libbfd.c (_bfd_norelocs_set_reloc): New function.
+	* libbfd-in.h: Prototype for _bfd_norelocs_set_reloc.
+	* i386msdos.c (msdos_set_reloc): Define to
+	_bfd_norelocs_set_reloc.
+	* elfcode.h (elf_set_reloc): Define.
+	* bfd-in2.h: Regenerated.
+
 2017-05-01  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
 
 	PR ld/21404
diff --git a/bfd/aout-adobe.c b/bfd/aout-adobe.c
index d47dd20..a2c927d 100644
--- a/bfd/aout-adobe.c
+++ b/bfd/aout-adobe.c
@@ -474,6 +474,7 @@ aout_adobe_sizeof_headers (bfd *ignore_abfd ATTRIBUTE_UNUSED,
 #define aout_32_bfd_final_link		            _bfd_generic_final_link
 #define aout_32_bfd_link_split_section	            _bfd_generic_link_split_section
 #define aout_32_bfd_link_check_relocs               _bfd_generic_link_check_relocs
+#define aout_32_set_reloc			    _bfd_generic_set_reloc
 
 const bfd_target aout_adobe_vec =
 {
diff --git a/bfd/aout-target.h b/bfd/aout-target.h
index 9f77c95..2e98c4d 100644
--- a/bfd/aout-target.h
+++ b/bfd/aout-target.h
@@ -450,6 +450,9 @@ MY_bfd_final_link (bfd *abfd, struct bfd_link_info *info)
 #ifndef MY_canonicalize_reloc
 #define MY_canonicalize_reloc NAME (aout, canonicalize_reloc)
 #endif
+#ifndef MY_set_reloc
+#define MY_set_reloc _bfd_generic_set_reloc
+#endif
 #ifndef MY_make_empty_symbol
 #define MY_make_empty_symbol NAME (aout, make_empty_symbol)
 #endif
diff --git a/bfd/aout-tic30.c b/bfd/aout-tic30.c
index 29dad9f..5db3da7 100644
--- a/bfd/aout-tic30.c
+++ b/bfd/aout-tic30.c
@@ -905,6 +905,9 @@ tic30_aout_set_arch_mach (bfd *abfd,
 #ifndef MY_canonicalize_reloc
 #define MY_canonicalize_reloc NAME (aout, canonicalize_reloc)
 #endif
+#ifndef MY_set_reloc
+#define MY_set_reloc _bfd_generic_set_reloc
+#endif
 #ifndef MY_make_empty_symbol
 #define MY_make_empty_symbol NAME (aout, make_empty_symbol)
 #endif
diff --git a/bfd/bfd-in2.h b/bfd/bfd-in2.h
index 17a35c0..8617881 100644
--- a/bfd/bfd-in2.h
+++ b/bfd/bfd-in2.h
@@ -7075,6 +7075,8 @@ long bfd_canonicalize_reloc
 void bfd_set_reloc
    (bfd *abfd, asection *sec, arelent **rel, unsigned int count);
 
+#define bfd_set_reloc(abfd, asect, location, count) \
+     BFD_SEND (abfd, _bfd_set_reloc, (abfd, asect, location, count))
 bfd_boolean bfd_set_file_flags (bfd *abfd, flagword flags);
 
 int bfd_get_arch_size (bfd *abfd);
@@ -7542,12 +7544,15 @@ typedef struct bfd_target
 #define BFD_JUMP_TABLE_RELOCS(NAME) \
   NAME##_get_reloc_upper_bound, \
   NAME##_canonicalize_reloc, \
+  NAME##_set_reloc, \
   NAME##_bfd_reloc_type_lookup, \
   NAME##_bfd_reloc_name_lookup
 
   long        (*_get_reloc_upper_bound) (bfd *, sec_ptr);
   long        (*_bfd_canonicalize_reloc)
     (bfd *, sec_ptr, arelent **, struct bfd_symbol **);
+  void        (*_bfd_set_reloc)
+    (bfd *, sec_ptr, arelent **, unsigned int);
   /* See documentation on reloc types.  */
   reloc_howto_type *
               (*reloc_type_lookup) (bfd *, bfd_reloc_code_real_type);
diff --git a/bfd/bfd.c b/bfd/bfd.c
index 23a4350..c6fce45 100644
--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -1064,18 +1064,10 @@ DESCRIPTION
 	section @var{sec} to the values @var{rel} and @var{count}.
 	The argument @var{abfd} is ignored.
 
+.#define bfd_set_reloc(abfd, asect, location, count) \
+.     BFD_SEND (abfd, _bfd_set_reloc, (abfd, asect, location, count))
 */
 
-void
-bfd_set_reloc (bfd *ignore_abfd ATTRIBUTE_UNUSED,
-	       sec_ptr asect,
-	       arelent **location,
-	       unsigned int count)
-{
-  asect->orelocation = location;
-  asect->reloc_count = count;
-}
-
 /*
 FUNCTION
 	bfd_set_file_flags
diff --git a/bfd/bout.c b/bfd/bout.c
index 1d3bf66..0a6f8db 100644
--- a/bfd/bout.c
+++ b/bfd/bout.c
@@ -1392,6 +1392,7 @@ b_out_bfd_get_relocated_section_contents (bfd *output_bfd,
 #define b_out_bfd_define_common_symbol         bfd_generic_define_common_symbol
 #define aout_32_get_section_contents_in_window _bfd_generic_get_section_contents_in_window
 #define b_out_bfd_link_check_relocs            _bfd_generic_link_check_relocs
+#define b_out_set_reloc			       _bfd_generic_set_reloc
 
 extern const bfd_target bout_le_vec;
 
diff --git a/bfd/coff-alpha.c b/bfd/coff-alpha.c
index 9ce1975..8ecba2f 100644
--- a/bfd/coff-alpha.c
+++ b/bfd/coff-alpha.c
@@ -2342,6 +2342,9 @@ static const struct ecoff_backend_data alpha_ecoff_backend_data =
 #define _bfd_ecoff_bfd_define_common_symbol bfd_generic_define_common_symbol
 #define _bfd_ecoff_bfd_link_check_relocs    _bfd_generic_link_check_relocs
 
+/* Installing internal relocations in a section is also generic.  */
+#define _bfd_ecoff_set_reloc _bfd_generic_set_reloc
+
 const bfd_target alpha_ecoff_le_vec =
 {
   "ecoff-littlealpha",		/* name */
diff --git a/bfd/coff-mips.c b/bfd/coff-mips.c
index f872ebe..45c65f8 100644
--- a/bfd/coff-mips.c
+++ b/bfd/coff-mips.c
@@ -1356,6 +1356,7 @@ static const struct ecoff_backend_data mips_ecoff_backend_data =
 #define _bfd_ecoff_section_already_linked \
   _bfd_coff_section_already_linked
 #define _bfd_ecoff_bfd_define_common_symbol bfd_generic_define_common_symbol
+#define _bfd_ecoff_set_reloc _bfd_generic_set_reloc
 
 extern const bfd_target mips_ecoff_be_vec;
 
diff --git a/bfd/coff-rs6000.c b/bfd/coff-rs6000.c
index e2b149b..b49e393 100644
--- a/bfd/coff-rs6000.c
+++ b/bfd/coff-rs6000.c
@@ -3993,6 +3993,7 @@ const struct xcoff_dwsect_name xcoff_dwsect_names[] = {
 /* For reloc entry points.  */
 #define _bfd_xcoff_get_reloc_upper_bound coff_get_reloc_upper_bound
 #define _bfd_xcoff_canonicalize_reloc coff_canonicalize_reloc
+#define _bfd_xcoff_set_reloc _bfd_generic_set_reloc
 #define _bfd_xcoff_bfd_reloc_type_lookup _bfd_xcoff_reloc_type_lookup
 #define _bfd_xcoff_bfd_reloc_name_lookup _bfd_xcoff_reloc_name_lookup
 
diff --git a/bfd/coff64-rs6000.c b/bfd/coff64-rs6000.c
index e919dcb..56458e0 100644
--- a/bfd/coff64-rs6000.c
+++ b/bfd/coff64-rs6000.c
@@ -2720,6 +2720,7 @@ const bfd_target rs6000_xcoff64_vec =
     /* Reloc */
     coff_get_reloc_upper_bound,
     coff_canonicalize_reloc,
+    _bfd_generic_set_reloc,
     xcoff64_reloc_type_lookup,
     xcoff64_reloc_name_lookup,
 
@@ -2979,6 +2980,7 @@ const bfd_target rs6000_xcoff64_aix_vec =
     /* Reloc */
     coff_get_reloc_upper_bound,
     coff_canonicalize_reloc,
+    _bfd_generic_set_reloc,
     xcoff64_reloc_type_lookup,
     xcoff64_reloc_name_lookup,
 
diff --git a/bfd/coffcode.h b/bfd/coffcode.h
index 96a7886..f9d3978 100644
--- a/bfd/coffcode.h
+++ b/bfd/coffcode.h
@@ -5439,6 +5439,10 @@ coff_canonicalize_reloc (bfd * abfd,
   return section->reloc_count;
 }
 
+#ifndef coff_set_reloc
+#define coff_set_reloc _bfd_generic_set_reloc
+#endif
+
 #ifndef coff_reloc16_estimate
 #define coff_reloc16_estimate dummy_reloc16_estimate
 
diff --git a/bfd/elfxx-target.h b/bfd/elfxx-target.h
index 2e24e11..072efaa 100644
--- a/bfd/elfxx-target.h
+++ b/bfd/elfxx-target.h
@@ -46,6 +46,9 @@
 #ifndef bfd_elfNN_canonicalize_reloc
 #define bfd_elfNN_canonicalize_reloc	_bfd_elf_canonicalize_reloc
 #endif
+#ifndef bfd_elfNN_set_reloc
+#define bfd_elfNN_set_reloc		_bfd_generic_set_reloc
+#endif
 #ifndef bfd_elfNN_find_nearest_line
 #define bfd_elfNN_find_nearest_line	_bfd_elf_find_nearest_line
 #endif
diff --git a/bfd/i386msdos.c b/bfd/i386msdos.c
index d37d000..5062506 100644
--- a/bfd/i386msdos.c
+++ b/bfd/i386msdos.c
@@ -176,6 +176,7 @@ msdos_set_section_contents (bfd *abfd,
 #define msdos_minisymbol_to_symbol _bfd_nosymbols_minisymbol_to_symbol
 
 #define msdos_canonicalize_reloc _bfd_norelocs_canonicalize_reloc
+#define msdos_set_reloc _bfd_norelocs_set_reloc
 #define msdos_get_reloc_upper_bound _bfd_norelocs_get_reloc_upper_bound
 #define msdos_32_bfd_link_split_section  _bfd_generic_link_split_section
 
diff --git a/bfd/i386os9k.c b/bfd/i386os9k.c
index fbec96c..7f79b23 100644
--- a/bfd/i386os9k.c
+++ b/bfd/i386os9k.c
@@ -165,6 +165,8 @@ os9k_sizeof_headers (bfd *abfd ATTRIBUTE_UNUSED,
 #define aout_32_get_section_contents_in_window \
   _bfd_generic_get_section_contents_in_window
 
+#define aout_32_set_reloc _bfd_generic_set_reloc
+
 #define os9k_bfd_get_relocated_section_contents \
   bfd_generic_get_relocated_section_contents
 #define os9k_bfd_relax_section bfd_generic_relax_section
diff --git a/bfd/ieee.c b/bfd/ieee.c
index a2d3835..763c2b8 100644
--- a/bfd/ieee.c
+++ b/bfd/ieee.c
@@ -3880,6 +3880,7 @@ ieee_sizeof_headers (bfd *abfd ATTRIBUTE_UNUSED,
 #define ieee_bfd_final_link _bfd_generic_final_link
 #define ieee_bfd_link_split_section  _bfd_generic_link_split_section
 #define ieee_bfd_link_check_relocs   _bfd_generic_link_check_relocs
+#define ieee_set_reloc		     _bfd_generic_set_reloc
 
 const bfd_target ieee_vec =
 {
diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
index ad362dc..2d1bf27 100644
--- a/bfd/libbfd-in.h
+++ b/bfd/libbfd-in.h
@@ -444,6 +444,8 @@ extern bfd_boolean _bfd_vms_lib_ia64_mkarchive (bfd *abfd);
 extern long _bfd_norelocs_get_reloc_upper_bound (bfd *, asection *);
 extern long _bfd_norelocs_canonicalize_reloc (bfd *, asection *,
 					      arelent **, asymbol **);
+extern void _bfd_norelocs_set_reloc (bfd *, asection *,
+                                     arelent **, unsigned int);
 #define _bfd_norelocs_bfd_reloc_type_lookup \
   ((reloc_howto_type *(*) (bfd *, bfd_reloc_code_real_type)) bfd_nullvoidptr)
 #define _bfd_norelocs_bfd_reloc_name_lookup \
diff --git a/bfd/libbfd.c b/bfd/libbfd.c
index 46bb232..554234f 100644
--- a/bfd/libbfd.c
+++ b/bfd/libbfd.c
@@ -119,6 +119,15 @@ _bfd_norelocs_canonicalize_reloc (bfd *abfd ATTRIBUTE_UNUSED,
   return 0;
 }
 
+void
+_bfd_norelocs_set_reloc (bfd *abfd ATTRIBUTE_UNUSED,
+                         asection *sec ATTRIBUTE_UNUSED,
+                         arelent **relptr ATTRIBUTE_UNUSED,
+                         unsigned int count ATTRIBUTE_UNUSED)
+{
+  /* Do nothing.  */
+}
+
 bfd_boolean
 _bfd_nocore_core_file_matches_executable_p
   (bfd *ignore_core_bfd ATTRIBUTE_UNUSED,
diff --git a/bfd/libbfd.h b/bfd/libbfd.h
index 8bac650..7e58598 100644
--- a/bfd/libbfd.h
+++ b/bfd/libbfd.h
@@ -449,6 +449,8 @@ extern bfd_boolean _bfd_vms_lib_ia64_mkarchive (bfd *abfd);
 extern long _bfd_norelocs_get_reloc_upper_bound (bfd *, asection *);
 extern long _bfd_norelocs_canonicalize_reloc (bfd *, asection *,
 					      arelent **, asymbol **);
+extern void _bfd_norelocs_set_reloc (bfd *, asection *,
+                                     arelent **, unsigned int);
 #define _bfd_norelocs_bfd_reloc_type_lookup \
   ((reloc_howto_type *(*) (bfd *, bfd_reloc_code_real_type)) bfd_nullvoidptr)
 #define _bfd_norelocs_bfd_reloc_name_lookup \
@@ -3206,6 +3208,12 @@ bfd_byte *bfd_generic_get_relocated_section_contents
     bfd_boolean relocatable,
     asymbol **symbols);
 
+void _bfd_generic_set_reloc
+   (bfd *abfd,
+    sec_ptr section,
+    arelent **relptr,
+    unsigned int count);
+
 /* Extracted from archures.c.  */
 extern const bfd_arch_info_type bfd_default_arch_struct;
 const bfd_arch_info_type *bfd_default_compatible
diff --git a/bfd/mach-o-target.c b/bfd/mach-o-target.c
index 3a4e72f..c25a743 100644
--- a/bfd/mach-o-target.c
+++ b/bfd/mach-o-target.c
@@ -58,6 +58,7 @@
 #define bfd_mach_o_bfd_copy_private_bfd_data          _bfd_generic_bfd_copy_private_bfd_data
 #define bfd_mach_o_core_file_matches_executable_p     generic_core_file_matches_executable_p
 #define bfd_mach_o_core_file_pid                      _bfd_nocore_core_file_pid
+#define bfd_mach_o_set_reloc			      _bfd_generic_set_reloc
 
 #define bfd_mach_o_get_dynamic_symtab_upper_bound     bfd_mach_o_get_symtab_upper_bound
 #define bfd_mach_o_canonicalize_dynamic_symtab	      bfd_mach_o_canonicalize_symtab
diff --git a/bfd/nlm-target.h b/bfd/nlm-target.h
index 4f9e50f..872ee32 100644
--- a/bfd/nlm-target.h
+++ b/bfd/nlm-target.h
@@ -38,6 +38,7 @@
 
 #define nlm_get_reloc_upper_bound               nlmNAME (get_reloc_upper_bound)
 #define nlm_canonicalize_reloc                  nlmNAME (canonicalize_reloc)
+#define nlm_set_reloc				_bfd_generic_set_reloc
 #define nlm_bfd_reloc_type_lookup               bfd_default_reloc_type_lookup
 #define nlm_bfd_reloc_name_lookup         _bfd_norelocs_bfd_reloc_name_lookup
 #define nlm_set_section_contents                nlmNAME (set_section_contents)
diff --git a/bfd/oasys.c b/bfd/oasys.c
index 06491aa..defb25a 100644
--- a/bfd/oasys.c
+++ b/bfd/oasys.c
@@ -1194,6 +1194,7 @@ oasys_sizeof_headers (bfd *abfd ATTRIBUTE_UNUSED,
 #define oasys_bfd_final_link                       _bfd_generic_final_link
 #define oasys_bfd_link_split_section               _bfd_generic_link_split_section
 #define oasys_bfd_link_check_relocs                _bfd_generic_link_check_relocs
+#define oasys_set_reloc				   _bfd_generic_set_reloc
 
 const bfd_target oasys_vec =
 {
diff --git a/bfd/reloc.c b/bfd/reloc.c
index 12520d1..8dedfe8 100644
--- a/bfd/reloc.c
+++ b/bfd/reloc.c
@@ -8258,3 +8258,29 @@ error_return:
   free (reloc_vector);
   return NULL;
 }
+
+/*
+INTERNAL_FUNCTION
+	_bfd_generic_set_reloc
+
+SYNOPSIS
+	void _bfd_generic_set_reloc
+	  (bfd *abfd,
+	   sec_ptr section,
+	   arelent **relptr,
+	   unsigned int count);
+
+DESCRIPTION
+	Installs a new set of internal relocations in SECTION.
+*/
+
+
+void _bfd_generic_set_reloc
+  (bfd *abfd ATTRIBUTE_UNUSED,
+   sec_ptr section,
+   arelent **relptr,
+   unsigned int count)
+{
+  section->orelocation = relptr;
+  section->reloc_count = count;
+}
diff --git a/bfd/som.c b/bfd/som.c
index f78b651..496040c 100644
--- a/bfd/som.c
+++ b/bfd/som.c
@@ -6759,6 +6759,7 @@ som_bfd_link_split_section (bfd *abfd ATTRIBUTE_UNUSED, asection *sec)
 #define som_bfd_set_private_flags		_bfd_generic_bfd_set_private_flags
 #define som_find_inliner_info			_bfd_nosymbols_find_inliner_info
 #define som_bfd_link_check_relocs               _bfd_generic_link_check_relocs
+#define som_set_reloc				_bfd_generic_set_reloc
 
 const bfd_target hppa_som_vec =
 {
diff --git a/bfd/targets.c b/bfd/targets.c
index 5841e8d..2f3ea13 100644
--- a/bfd/targets.c
+++ b/bfd/targets.c
@@ -418,12 +418,15 @@ BFD_JUMP_TABLE macros.
 .#define BFD_JUMP_TABLE_RELOCS(NAME) \
 .  NAME##_get_reloc_upper_bound, \
 .  NAME##_canonicalize_reloc, \
+.  NAME##_set_reloc, \
 .  NAME##_bfd_reloc_type_lookup, \
 .  NAME##_bfd_reloc_name_lookup
 .
 .  long        (*_get_reloc_upper_bound) (bfd *, sec_ptr);
 .  long        (*_bfd_canonicalize_reloc)
 .    (bfd *, sec_ptr, arelent **, struct bfd_symbol **);
+.  void	       (*_bfd_set_reloc)
+.    (bfd *, sec_ptr, arelent **, unsigned int);
 .  {* See documentation on reloc types.  *}
 .  reloc_howto_type *
 .              (*reloc_type_lookup) (bfd *, bfd_reloc_code_real_type);
diff --git a/bfd/versados.c b/bfd/versados.c
index f575522..2efbcff 100644
--- a/bfd/versados.c
+++ b/bfd/versados.c
@@ -875,6 +875,7 @@ versados_canonicalize_reloc (bfd *abfd,
 #define versados_bfd_final_link                       _bfd_generic_final_link
 #define versados_bfd_link_split_section               _bfd_generic_link_split_section
 #define versados_bfd_link_check_relocs                _bfd_generic_link_check_relocs
+#define versados_set_reloc			      _bfd_generic_set_reloc
 
 const bfd_target m68k_versados_vec =
 {
diff --git a/bfd/vms-alpha.c b/bfd/vms-alpha.c
index 4a9881b..ef52120 100644
--- a/bfd/vms-alpha.c
+++ b/bfd/vms-alpha.c
@@ -5118,6 +5118,11 @@ alpha_vms_canonicalize_reloc (bfd *abfd, asection *section, arelent **relptr,
   *relptr = (arelent *) NULL;
   return section->reloc_count;
 }
+
+/* Install a new set of internal relocs.  */
+
+#define alpha_vms_set_reloc _bfd_generic_set_reloc
+
 \f
 /* This is just copied from ecoff-alpha, needs to be fixed probably.  */
 
-- 
2.3.4

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc.
  2017-05-05 12:10 ` [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc Jose E. Marchesi
@ 2017-05-07  2:53   ` Alan Modra
  2017-05-10 16:49     ` Jose E. Marchesi
  0 siblings, 1 reply; 16+ messages in thread
From: Alan Modra @ 2017-05-07  2:53 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: binutils

On Fri, May 05, 2017 at 02:10:15PM +0200, Jose E. Marchesi wrote:
> bfd/ChangeLog:
> 
>     2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
>     	* targets.c (BFD_JUMP_TABLE_RELOCS): Add NAME##_set_reloc.
>     	(struct bfd_target): New field _bfd_set_reloc.
>     	* bfd.c (bfd_set_reloc): Call backend _set_bfd.
>     	* reloc.c (_bfd_generic_set_reloc): New function.
>     	* coffcode.h (coff_set_reloc): Define to _bfd_generic_set_reloc.
>     	* nlm-target.h (nlm_set_reloc): Likewise.
>     	* coff-rs6000.c (_bfd_xcoff_set_reloc): Likewise.
>     	* aout-tic30.c (MY_set_reloc): Likewise.
>     	* aout-target.h (MY_set_reloc): Likewise.
>     	* elfxx-target.h (bfd_elfNN_set_reloc): Likewise.
>     	* coff-alpha.c (_bfd_ecoff_set_reloc): Likewise.
>     	* mach-o-target.c (bfd_mach_o_set_reloc): Likewise.
>     	* vms-alpha.c (alpha_vms_set_reloc): Likewise.
>     	* aout-adobe.c (aout_32_set_reloc): Likewise.
>     	* bout.c (b_out_set_reloc): Likewise.
>     	* coff-mips.c (_bfd_ecoff_set_reloc): Likewise.
>     	* i386os9k.c (aout_32_set_reloc): Likewise.
>     	* ieee.c (ieee_set_reloc): Likewise.
>     	* oasys.c (oasys_set_reloc): Likewise.
>     	* som.c (som_set_reloc): Likewise.
>     	* versados.c (versados_set_reloc): Likewise.
>     	* coff64-rs6000.c (rs6000_xcoff64_vec): Add
>     	_bfd_generic_set_reloc.
>     	(rs6000_xcoff64_aix_vec): LIkewise.
>     	* libbfd.c (_bfd_norelocs_set_reloc): New function.
>     	* libbfd-in.h: Prototype for _bfd_norelocs_set_reloc.
>     	* i386msdos.c (msdos_set_reloc): Define to
>     	_bfd_norelocs_set_reloc.
>     	* elfcode.h (elf_set_reloc): Define.
>     	* bfd-in2.h: Regenerated.

OK.  I'll leave approval of the MIPS patch to Maciej.

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc.
  2017-05-07  2:53   ` Alan Modra
@ 2017-05-10 16:49     ` Jose E. Marchesi
  0 siblings, 0 replies; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-10 16:49 UTC (permalink / raw)
  To: Alan Modra; +Cc: binutils


    >     2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
    > 
    >     	* targets.c (BFD_JUMP_TABLE_RELOCS): Add NAME##_set_reloc.
    >     	(struct bfd_target): New field _bfd_set_reloc.
    >     	* bfd.c (bfd_set_reloc): Call backend _set_bfd.
    >     	* reloc.c (_bfd_generic_set_reloc): New function.
    >     	* coffcode.h (coff_set_reloc): Define to _bfd_generic_set_reloc.
    >     	* nlm-target.h (nlm_set_reloc): Likewise.
    >     	* coff-rs6000.c (_bfd_xcoff_set_reloc): Likewise.
    >     	* aout-tic30.c (MY_set_reloc): Likewise.
    >     	* aout-target.h (MY_set_reloc): Likewise.
    >     	* elfxx-target.h (bfd_elfNN_set_reloc): Likewise.
    >     	* coff-alpha.c (_bfd_ecoff_set_reloc): Likewise.
    >     	* mach-o-target.c (bfd_mach_o_set_reloc): Likewise.
    >     	* vms-alpha.c (alpha_vms_set_reloc): Likewise.
    >     	* aout-adobe.c (aout_32_set_reloc): Likewise.
    >     	* bout.c (b_out_set_reloc): Likewise.
    >     	* coff-mips.c (_bfd_ecoff_set_reloc): Likewise.
    >     	* i386os9k.c (aout_32_set_reloc): Likewise.
    >     	* ieee.c (ieee_set_reloc): Likewise.
    >     	* oasys.c (oasys_set_reloc): Likewise.
    >     	* som.c (som_set_reloc): Likewise.
    >     	* versados.c (versados_set_reloc): Likewise.
    >     	* coff64-rs6000.c (rs6000_xcoff64_vec): Add
    >     	_bfd_generic_set_reloc.
    >     	(rs6000_xcoff64_aix_vec): LIkewise.
    >     	* libbfd.c (_bfd_norelocs_set_reloc): New function.
    >     	* libbfd-in.h: Prototype for _bfd_norelocs_set_reloc.
    >     	* i386msdos.c (msdos_set_reloc): Define to
    >     	_bfd_norelocs_set_reloc.
    >     	* elfcode.h (elf_set_reloc): Define.
    >     	* bfd-in2.h: Regenerated.
    
    OK.  I'll leave approval of the MIPS patch to Maciej.

I checked this in after testing for regressions in all the targets you
suggested:

aarch64-linux alpha-dec-vms alpha-linux alpha-linuxecoff alpha-netbsd
alpha-unknown-freebsd4.7 am33_2.0-linux arc-linux-uclibc arm-linuxeabi
arm-nacl arm-netbsdelf arm-nto arm-pe arm-symbianelf arm-vxworks
arm-wince-pe avr-elf bfin-elf cr16-elf cris-elf crisv32-linux crx-elf
d10v-elf d30v-elf dlx-elf epiphany-elf fr30-elf frv-elf frv-linux
ft32-elf h8300-elf hppa-linux hppa-hp-hpux10 hppa64-hp-hpux11.23
hppa64-linux i386-darwin i386-lynxos i586-linux i686-nacl i686-pc-beos
i686-pc-elf i686-pe i686-vxworks ia64-elf ia64-freebsd5 ia64-hpux
ia64-linux ia64-netbsd ia64-vms ip2k-elf iq2000-elf lm32-elf m32c-elf
m32r-elf m68hc11-elf m68hc12-elf m68k-elf m68k-linux m68k-netbsd
mcore-elf mcore-pe mep-elf metag-linux microblaze-elf mips-linux
mips-vxworks mips64-linux mipsel-linux-gnu mipsisa32el-linux
mips64-openbsd mipstx39-elf mmix mn10200-elf mn10300-elf moxie-elf
ms1-elf msp430-elf mt-elf nds32le-elf nios2-linux or1k-elf
pdp11-dec-aout pj-elf powerpc-eabisim powerpc-eabivle powerpc-linux
powerpc-nto powerpc-wrs-vxworks powerpc64-linux powerpcle-cygwin
powerpcle-elf powerpc64le-linux ppc-lynxos pru-elf riscv32-elf
riscv64-elf rl78-elf rs6000-aix4.3.3 rs6000-aix5.1 rx-elf s390-linux
s390x-linux score-elf sh-linux sh-nto sh-pe sh-rtems sh-vxworks
shl-unknown-net bsdelf sparc-aout sparc-linux sparc-vxworks
sparc64-linux spu-elf tic30-unknown-aout tic30-unknown-coff tic4x-coff
tic54x-coff tic6x-elf tilegx-linux tilepro-linux v850-elf vax-netbsdelf
visium-elf x86_64-linux x86_64-w64-mingw32 x86_64-nacl xgate-elf
xstormy16-elf xtensa-elf z8k-coff z80-coff
  
Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64
  2017-05-05 12:10 ` [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64 Jose E. Marchesi
@ 2017-05-10 16:51   ` Jose E. Marchesi
  0 siblings, 0 replies; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-10 16:51 UTC (permalink / raw)
  To: binutils


    This patch fixes the deletion of relocations in BFD sections in
    sparc64 targets.
    
    A specialized `_bfd_set_reloc' function is provided that updates the
    internal canon_reloc_count(sec) counter instead of sec->reloc_count.
    Additionally, the `write_relocs' callback in elf64-sparc is adapted to
    use the canon_reloc_count to traverse `sec->orelocation'.
    
    Tested in sparc64-linux-gnu targets.
    Fixes an existing failure in the merge-notes objcopy test.
    No regressions.
    
    bfd/ChangeLog:
    
    2017-05-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
    
    	* elf64-sparc.c (elf64_sparc_set_reloc): New function.
    	(bfd_elf64_set_reloc): Define.
    	(elf64_sparc_write_relocs): Use `canon_reloc_count'.


I just applied this patch.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64
  2017-05-05 12:10 ` [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64 Jose E. Marchesi
@ 2017-05-17 15:29   ` Maciej W. Rozycki
  2017-05-17 16:14     ` Jose E. Marchesi
  0 siblings, 1 reply; 16+ messages in thread
From: Maciej W. Rozycki @ 2017-05-17 15:29 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: Alan Modra, binutils

Jose,

 Thank you for your submission and apologies for the long RTT.

 Please post future patches with actual diffs for ChangeLog files omitted, 
so that they can be applied without a need for hand-editing.

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

 That written I have a couple of concerns I do want you to address with 
your current proposal, see below for details.

> This patch also removes the safety check recently introduced in
> objcopy, to avoid deleting relocations if the count returned by
> `bfd_canonicalize_relocs' is bigger than `sec->reloc_count', as it is
> no longer necessary.

 I think this needs to be a separate followup commit, as it's not strictly 
MIPS-specific, and both changes will then be self-contained.

> diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
> index a66c319..63880c6 100644
> --- a/bfd/elf64-mips.c
> +++ b/bfd/elf64-mips.c
> @@ -3728,6 +3730,22 @@ mips_elf64_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
>    return ret;
>  }
>  
> +/* A similar problem happens with set_reloc.  This version updates the
> +   internal `reloc_count' field in `struct _mips_elf_section_data' in
> +   order to hold the new number of internal relocations.  This avoids
> +   overwriting `asect->reloc_count' that holds the number of external
> +   relocations.  */

 What does this comment refer to, i.e. what is the problem similar to?

> +
> +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;

?

> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 70c7f1c..8aa979e 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -221,18 +221,6 @@ struct mips_elf_traverse_got_arg
>    int value;
>  };
>  
> -struct _mips_elf_section_data
> -{
> -  struct bfd_elf_section_data elf;
> -  union
> -  {
> -    bfd_byte *tdata;
> -  } u;
> -};
> -
> -#define mips_elf_section_data(sec) \
> -  ((struct _mips_elf_section_data *) elf_section_data (sec))
> -
>  #define is_mips_elf(bfd)				\
>    (bfd_get_flavour (bfd) == bfd_target_elf_flavour	\
>     && elf_tdata (bfd) != NULL				\
> diff --git a/bfd/elfxx-mips.h b/bfd/elfxx-mips.h
> index 44ad789..5710c61 100644
> --- a/bfd/elfxx-mips.h
> +++ b/bfd/elfxx-mips.h
> @@ -22,6 +22,22 @@
>  #include "elf/internal.h"
>  #include "elf/mips.h"
>  
> +struct _mips_elf_section_data
> +{
> +  struct bfd_elf_section_data elf;
> +  union
> +  {
> +    bfd_byte *tdata;
> +  } u;
> +  unsigned int reloc_count;

 Add an explanatory comment for a newly-added structure member and give it 
a more meaningful name, such as `internal_reloc_count' (assuming that it 
is needed indeed in the first place; see above).

> +};
> +
> +#define mips_elf_section_data(sec) \
> +  ((struct _mips_elf_section_data *) elf_section_data (sec))
> +
> +#define canon_reloc_count(sec) \
> +  ((struct _mips_elf_section_data *) elf_section_data (sec))->reloc_count
> +

 Please do not export this structure or the `mips_elf_section_data' 
accessor outside elfxx-mips.c.  Given that all the use is the 
`canon_reloc_count' macro, make it a function and place it in 
elfxx-mips.c.

 Also avoid namespace pollution and give it a target prefix, i.e. 
`mips_elf_'.

 Again, if indeed needed.

 I have successfully passed your change through regression-testing over my 
list of MIPS targets as follows:

mips-ecoff mips-elf mips-img-elf mips-mti-elf mips-sde-elf mips-sgi-irix5 
mips-sgi-irix6 mips-freebsd mips-linux mips-img-linux mips-mti-linux 
mips-netbsd mips-vxworks mips64-freebsd mips64-linux mips64-img-linux 
mips64-mti-linux mips64-openbsd mips64el-freebsd mips64el-linux 
mips64el-img-linux mips64el-mti-linux mips64el-openbsd mipsel-ecoff 
mipsel-elf mipsel-img-elf mipsel-mti-elf mipsel-freebsd mipsel-linux 
mipsel-img-linux mipsel-mti-linux mipsel-netbsd mipsel-vxworks 
mipsisa32-elf mipsisa32-linux mipsisa32el-elf mipsisa32el-linux 
mipsisa64-elf mipsisa64-linux mipsisa64el-elf mipsisa64el-linux tx39-elf

with these failures removed:

mips64-openbsd  -FAIL: merge notes section (64-bits)
mips64el-openbsd  -FAIL: merge notes section (64-bits)

and relocation data from the affected test case looking correct:

$ as .../binutils/testsuite/binutils-all/note-2-64.s -o tmpdir/bintest.o
$ objcopy --merge-notes tmpdir/bintest.o tmpdir/copy.o
$ readelf -r tmpdir/bintest.o

Relocation section '.rela.gnu.build.attributes' at offset 0x3e8 contains 3 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000010  000900000012 R_MIPS_64         0000000000000100 note1.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
000000000070  000a00000012 R_MIPS_64         0000000000000104 note2.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
0000000000d0  000c00000012 R_MIPS_64         0000000000000108 note3.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
$ readelf -r tmpdir/copy.o

Relocation section '.rela.gnu.build.attributes' at offset 0x390 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000010  000900000012 R_MIPS_64         0000000000000100 note1.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
000000000070  000a00000012 R_MIPS_64         0000000000000104 note2.s + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
$ 

so functionally the change looks good to me.  You could have posted these 
`readelf' dumps along your submission as a proof of correctness (well, for 
the test case at least).

 Please repost an updated change, or if you have any questions or comments 
before you start making modifications, then feel free to ask.

  Maciej

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64
  2017-05-17 15:29   ` Maciej W. Rozycki
@ 2017-05-17 16:14     ` Jose E. Marchesi
  2017-05-19 14:14       ` [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count' Maciej W. Rozycki
  0 siblings, 1 reply; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-17 16:14 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Alan Modra, binutils


    
    > 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?

     That written I have a couple of concerns I do want you to address with 
    your current proposal, see below for details.
    
    > This patch also removes the safety check recently introduced in
    > objcopy, to avoid deleting relocations if the count returned by
    > `bfd_canonicalize_relocs' is bigger than `sec->reloc_count', as it is
    > no longer necessary.
    
     I think this needs to be a separate followup commit, as it's not strictly 
    MIPS-specific, and both changes will then be self-contained.

Ok.

    > diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
    > index a66c319..63880c6 100644
    > --- a/bfd/elf64-mips.c
    > +++ b/bfd/elf64-mips.c
    > @@ -3728,6 +3730,22 @@ mips_elf64_canonicalize_dynamic_reloc (bfd *abfd, arelent **storage,
    >    return ret;
    >  }
    >  
    > +/* A similar problem happens with set_reloc.  This version updates the
    > +   internal `reloc_count' field in `struct _mips_elf_section_data' in
    > +   order to hold the new number of internal relocations.  This avoids
    > +   overwriting `asect->reloc_count' that holds the number of external
    > +   relocations.  */
    
     What does this comment refer to, i.e. what is the problem similar to?

To the comment two functions before, for
mips_elf64_canonicalize[_dynamic]_reloc:

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

    > diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
    > index 70c7f1c..8aa979e 100644
    > --- a/bfd/elfxx-mips.c
    > +++ b/bfd/elfxx-mips.c
    > @@ -221,18 +221,6 @@ struct mips_elf_traverse_got_arg
    >    int value;
    >  };
    >  
    > -struct _mips_elf_section_data
    > -{
    > -  struct bfd_elf_section_data elf;
    > -  union
    > -  {
    > -    bfd_byte *tdata;
    > -  } u;
    > -};
    > -
    > -#define mips_elf_section_data(sec) \
    > -  ((struct _mips_elf_section_data *) elf_section_data (sec))
    > -
    >  #define is_mips_elf(bfd)				\
    >    (bfd_get_flavour (bfd) == bfd_target_elf_flavour	\
    >     && elf_tdata (bfd) != NULL				\
    > diff --git a/bfd/elfxx-mips.h b/bfd/elfxx-mips.h
    > index 44ad789..5710c61 100644
    > --- a/bfd/elfxx-mips.h
    > +++ b/bfd/elfxx-mips.h
    > @@ -22,6 +22,22 @@
    >  #include "elf/internal.h"
    >  #include "elf/mips.h"
    >  
    > +struct _mips_elf_section_data
    > +{
    > +  struct bfd_elf_section_data elf;
    > +  union
    > +  {
    > +    bfd_byte *tdata;
    > +  } u;
    > +  unsigned int reloc_count;
    
     Add an explanatory comment for a newly-added structure member and give it 
    a more meaningful name, such as `internal_reloc_count' (assuming that it 
    is needed indeed in the first place; see above).

Ok.
    
    > +};
    > +
    > +#define mips_elf_section_data(sec) \
    > +  ((struct _mips_elf_section_data *) elf_section_data (sec))
    > +
    > +#define canon_reloc_count(sec) \
    > +  ((struct _mips_elf_section_data *) elf_section_data (sec))->reloc_count
    > +
    
     Please do not export this structure or the `mips_elf_section_data' 
    accessor outside elfxx-mips.c.  Given that all the use is the 
    `canon_reloc_count' macro, make it a function and place it in 
    elfxx-mips.c.
    
     Also avoid namespace pollution and give it a target prefix, i.e. 
    `mips_elf_'.

Ok.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-17 16:14     ` Jose E. Marchesi
@ 2017-05-19 14:14       ` Maciej W. Rozycki
  2017-05-19 15:01         ` Jose E. Marchesi
  2017-05-20  9:56         ` Maciej W. Rozycki
  0 siblings, 2 replies; 16+ messages in thread
From: Maciej W. Rozycki @ 2017-05-19 14:14 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: Alan Modra, binutils

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"
+}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-19 14:14       ` [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count' Maciej W. Rozycki
@ 2017-05-19 15:01         ` Jose E. Marchesi
  2017-05-19 15:47           ` Maciej W. Rozycki
  2017-05-20  9:56         ` Maciej W. Rozycki
  1 sibling, 1 reply; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-19 15:01 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Alan Modra, binutils

    
     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.

Sweet.  Very nice :)

     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.

I assume you mean `objcopy'.  Yeah, I will remove the workaround as soon
as a mips64 fix gets in.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-19 15:01         ` Jose E. Marchesi
@ 2017-05-19 15:47           ` Maciej W. Rozycki
  0 siblings, 0 replies; 16+ messages in thread
From: Maciej W. Rozycki @ 2017-05-19 15:47 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: Alan Modra, binutils

On Fri, 19 May 2017, Jose E. Marchesi wrote:

>      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.
> 
> I assume you mean `objcopy'.  Yeah, I will remove the workaround as soon
> as a mips64 fix gets in.

 Indeed `objcopy', sorry.

  Maciej

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-19 14:14       ` [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count' Maciej W. Rozycki
  2017-05-19 15:01         ` Jose E. Marchesi
@ 2017-05-20  9:56         ` Maciej W. Rozycki
  2017-05-22  9:21           ` Jose E. Marchesi
  2017-05-22 17:51           ` Joseph Myers
  1 sibling, 2 replies; 16+ messages in thread
From: Maciej W. Rozycki @ 2017-05-20  9:56 UTC (permalink / raw)
  To: Jose E. Marchesi; +Cc: Alan Modra, binutils

On Fri, 19 May 2017, Maciej W. Rozycki wrote:

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

 I have now applied this change.

  Maciej

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-20  9:56         ` Maciej W. Rozycki
@ 2017-05-22  9:21           ` Jose E. Marchesi
  2017-05-22 17:51           ` Joseph Myers
  1 sibling, 0 replies; 16+ messages in thread
From: Jose E. Marchesi @ 2017-05-22  9:21 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Alan Modra, binutils

    On Fri, 19 May 2017, Maciej W. Rozycki wrote:
    
    >  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.
    
     I have now applied this change.
    
And I just checked the patch below to remove the objcopy workaround.

I think by now the two problems described in
https://sourceware.org/ml/binutils/2017-04/msg00264.html are fixed :)

commit b4f5b984e5e771e75cee43942e56455531a02e68
Author: Jose E. Marchesi <jose.marchesi@oracle.com>
Date:   Fri May 19 23:45:29 2017 -0700

    binutils: remove sparc64/mips64 workaround in objcopy build notes merge code
    
    This patch removes a workaround recently installed in objcopy that
    avoided removing duplicated notes in targets for which the number of
    internal relocations may be bigger than the number of external
    relocations.  With the recent fixes in sparc64 and mips64, this
    workaround is no longer necessary.
    
    2017-05-19  Jose E. Marchesi  <jose.marchesi@oracle.com>
    
    	* objcopy.c (merge_gnu_build_notes): Remove workaround that
    	prevented deleting relocations in duplicated notes in mips64 and
    	sparc.

diff --git a/binutils/ChangeLog b/binutils/ChangeLog
index 59e3bb3..60a199a 100644
--- a/binutils/ChangeLog
+++ b/binutils/ChangeLog
@@ -1,3 +1,9 @@
+2017-05-19  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* objcopy.c (merge_gnu_build_notes): Remove workaround that
+	prevented deleting relocations in duplicated notes in mips64 and
+	sparc.
+
 2017-05-19  Maciej W. Rozycki  <macro@imgtec.com>
 
 	* testsuite/binutils-all/mips/mips-note-2.d: New test.
diff --git a/binutils/objcopy.c b/binutils/objcopy.c
index ccb5e12..42c7775 100644
--- a/binutils/objcopy.c
+++ b/binutils/objcopy.c
@@ -2137,13 +2137,6 @@ merge_gnu_build_notes (bfd * abfd, asection * sec, bfd_size_type size, bfd_byte
 	    relcount = 0;
 	}
 
-      /* A few targets (eg MIPS, SPARC) create multiple internal relocs to
-	 represent a single external reloc.  Unfortunately the current BFD
-	 API does not handle deleting relocs in such situations very well
-	 and so it is unsafe to proceed.  */
-      if ((unsigned long) relcount > sec->reloc_count)
-	goto done;
-
       /* Eliminate the duplicates.  */
       new = new_contents = xmalloc (size);
       for (pnote = pnotes, old = contents;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-20  9:56         ` Maciej W. Rozycki
  2017-05-22  9:21           ` Jose E. Marchesi
@ 2017-05-22 17:51           ` Joseph Myers
  2017-05-22 20:48             ` Maciej W. Rozycki
  1 sibling, 1 reply; 16+ messages in thread
From: Joseph Myers @ 2017-05-22 17:51 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Jose E. Marchesi, Alan Modra, binutils

This change breaks testing glibc for MIPS n64.  Specifically, linking the 
stdlib/test-canon2 test fails with "collect2: fatal error: ld terminated 
with signal 11 [Segmentation fault], core dumped", when using this 
binutils commit, but all tests link OK when using the previous binutils 
commit.

This is testing with GCC mainline, and glibc mainline with Zack's patch 
<https://sourceware.org/ml/libc-alpha/2017-05/msg00648.html> for 
subsequent build breakage applied, but it's quite likely it would appear 
with other GCC versions when using mainline binutils.  It first showed up 
with my glibc bot 
<https://sourceware.org/ml/libc-testresults/2017-q2/msg00208.html>.

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/test-canon2.c:51: warning: the use of `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count'
  2017-05-22 17:51           ` Joseph Myers
@ 2017-05-22 20:48             ` Maciej W. Rozycki
  0 siblings, 0 replies; 16+ messages in thread
From: Maciej W. Rozycki @ 2017-05-22 20:48 UTC (permalink / raw)
  To: Joseph Myers; +Cc: Jose E. Marchesi, Alan Modra, binutils

On Mon, 22 May 2017, Joseph Myers wrote:

> This change breaks testing glibc for MIPS n64.  Specifically, linking the 
> stdlib/test-canon2 test fails with "collect2: fatal error: ld terminated 
> with signal 11 [Segmentation fault], core dumped", when using this 
> binutils commit, but all tests link OK when using the previous binutils 
> commit.

 Thanks for your report and sorry for the breakage, I'll debug this.

  Maciej

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-05-22 18:10 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-05 12:10 [PATCH V2 0/3] Fix relocation deletion problems in SPARC and MIPS Jose E. Marchesi
2017-05-05 12:10 ` [PATCH V2 2/3] bfd: fix the deletion of relocs in sparc64 Jose E. Marchesi
2017-05-10 16:51   ` Jose E. Marchesi
2017-05-05 12:10 ` [PATCH V2 3/3] bfd: fix the deletion of relocs in mips64 Jose E. Marchesi
2017-05-17 15:29   ` Maciej W. Rozycki
2017-05-17 16:14     ` Jose E. Marchesi
2017-05-19 14:14       ` [PATCH] MIPS/BFD: For n64 hold the number of internal relocs in `->reloc_count' Maciej W. Rozycki
2017-05-19 15:01         ` Jose E. Marchesi
2017-05-19 15:47           ` Maciej W. Rozycki
2017-05-20  9:56         ` Maciej W. Rozycki
2017-05-22  9:21           ` Jose E. Marchesi
2017-05-22 17:51           ` Joseph Myers
2017-05-22 20:48             ` Maciej W. Rozycki
2017-05-05 12:10 ` [PATCH V2 1/3] bfd: new BFD target entry point _bfd_set_reloc Jose E. Marchesi
2017-05-07  2:53   ` Alan Modra
2017-05-10 16:49     ` Jose E. Marchesi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).