[PATCH v2 0/2] PR ld/21334: `_bfd_elf_link_renumber_dynsyms' call fixes

[PATCH v2 0/2] PR ld/21334: `_bfd_elf_link_renumber_dynsyms' call fixes

[Maciej W. Rozycki]

Alan,

 As promised here is a mini patch series to address issues with calling 
`_bfd_elf_link_renumber_dynsyms'.  See the individual descriptions for 
details.

 Please review.

  Maciej

[PATCH v2 1/2] ELF/BFD: Limit `_bfd_elf_link_renumber_dynsyms' call in section GC

[Maciej W. Rozycki]

Consistently call `_bfd_elf_link_renumber_dynsyms' only if linking a 
dynamic binary, complementing code in `bfd_elf_size_dynsym_hash_dynstr' 
and commit ccabcbe51e85 ("New attempt at fixing MIPS --gc-sections et 
al."), <https://sourceware.org/ml/binutils/2005-08/msg00258.html>.

	bfd/
	* elflink.c (bfd_elf_size_dynamic_sections): Only call 
	`_bfd_elf_link_renumber_dynsyms' after section GC if dynamic
	sections have been created.
---
 No regressions across the usual targets.  OK for master and 2.28?

  Maciej

binutils-bfd-elf-gc-dynsym-renumber.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-22 03:35:18.929206015 +0100
+++ binutils/bfd/elflink.c	2017-04-22 03:36:05.768357914 +0100
@@ -6460,7 +6460,8 @@ bfd_elf_size_dynamic_sections (bfd *outp
       elf_link_hash_traverse (elf_hash_table (info), elf_gc_sweep_symbol,
 			      &sweep_info);
 
-      _bfd_elf_link_renumber_dynsyms (output_bfd, info, &section_sym_count);
+      if (elf_hash_table (info)->dynamic_sections_created)
+	_bfd_elf_link_renumber_dynsyms (output_bfd, info, &section_sym_count);
     }
 
   /* Any syms created from now on start with -1 in

[PATCH v2 2/2] PR ld/21334: Always call `_bfd_elf_link_renumber_dynsyms' if required

[Maciej W. Rozycki]

Complement commit e17b0c351f0b ("MIPS/BFD: Respect the ELF gABI dynamic 
symbol table sort requirement") and correct an inconsistency in dynamic 
symbol accounting data causing an assertion failure in the MIPS backend:

ld: BFD (GNU Binutils) 2.28.51.20170330 assertion fail
../../binutils-gdb/bfd/elfxx-mips.c:3860

in the course of making a GOT entry in a static binary to satisfy a GOT 
relocation present in input, due to the local dynamic symbol count not 
having been established.

To do so let backends request `_bfd_elf_link_renumber_dynsyms' to be 
always called, rather than where a dynamic binary is linked only, and 
then make this request in the MIPS backend.

	bfd/
	PR ld/21334
	* elf-bfd.h (elf_backend_data): Add `always_renumber_dynsyms' 
	member.
	* elfxx-target.h [!elf_backend_always_renumber_dynsyms]
	(elf_backend_always_renumber_dynsyms): Define.
	(elfNN_bed): Initialize `always_renumber_dynsyms' member.
	* elfxx-mips.h (elf_backend_always_renumber_dynsyms): Define.
	* elflink.c (bfd_elf_size_dynamic_sections): Also call
	`_bfd_elf_link_renumber_dynsyms' if the backend has requested 
	it.
	(bfd_elf_size_dynsym_hash_dynstr): Likewise.

	ld/
	PR ld/21334
	* testsuite/ld-mips-elf/pr21334.dd: New test.
	* testsuite/ld-mips-elf/pr21334.gd: New test.
	* testsuite/ld-mips-elf/pr21334.ld: New test linker script.
	* testsuite/ld-mips-elf/pr21334.s: New test source.
	* testsuite/ld-mips-elf/mips-elf.exp: Run the new tests.
---
 No regressions against the usual targets.  OK for master and 2.28?

  Maciej

binutils-bfd-elf-always-dynsym-renumber.diff
Index: binutils/bfd/elf-bfd.h
===================================================================
--- binutils.orig/bfd/elf-bfd.h	2017-04-22 23:56:47.000000000 +0100
+++ binutils/bfd/elf-bfd.h	2017-04-23 00:14:37.913585456 +0100
@@ -1529,6 +1529,10 @@ struct elf_backend_data
   /* Address of protected data defined in the shared library may be
      external, i.e., due to copy relocation.   */
   unsigned extern_protected_data : 1;
+
+  /* True if `_bfd_elf_link_renumber_dynsyms' must be called even for
+     static binaries.  */
+  unsigned always_renumber_dynsyms : 1;
 };
 
 /* Information about reloc sections associated with a bfd_elf_section_data
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-22 23:56:47.773080252 +0100
+++ binutils/bfd/elflink.c	2017-04-23 01:13:04.711393021 +0100
@@ -6460,7 +6460,11 @@ bfd_elf_size_dynamic_sections (bfd *outp
       elf_link_hash_traverse (elf_hash_table (info), elf_gc_sweep_symbol,
 			      &sweep_info);
 
-      if (elf_hash_table (info)->dynamic_sections_created)
+      /* We need to reassign dynsym indices now that symbols may have
+	 been removed.  See the call in `bfd_elf_size_dynsym_hash_dynstr'
+	 for the details of the conditions used here.  */
+      if (elf_hash_table (info)->dynamic_sections_created
+	  || bed->always_renumber_dynsyms)
 	_bfd_elf_link_renumber_dynsyms (output_bfd, info, &section_sym_count);
     }
 
@@ -6828,6 +6832,8 @@ bfd_boolean
 bfd_elf_size_dynsym_hash_dynstr (bfd *output_bfd, struct bfd_link_info *info)
 {
   const struct elf_backend_data *bed;
+  unsigned long section_sym_count;
+  bfd_size_type dynsymcount;
 
   if (!is_elf_hash_table (info->hash))
     return TRUE;
@@ -6835,24 +6841,30 @@ bfd_elf_size_dynsym_hash_dynstr (bfd *ou
   bed = get_elf_backend_data (output_bfd);
   (*bed->elf_backend_init_index_section) (output_bfd, info);
 
+  /* Assign dynsym indices.  In a shared library we generate a section
+     symbol for each output section, which come first.  Next come all
+     of the back-end allocated local dynamic syms, followed by the rest
+     of the global symbols.
+
+     This is usually not needed for static binaries, however backends
+     can request to always do it, e.g. the MIPS backend uses dynamic
+     symbol counts to lay out GOT, which will be produced in the
+     presence of GOT relocations even in static binaries (holding fixed
+     data in that case, to satisfy those relocations).  */
+
+  if (elf_hash_table (info)->dynamic_sections_created
+      || bed->always_renumber_dynsyms)
+    dynsymcount = _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+						  &section_sym_count);
+
   if (elf_hash_table (info)->dynamic_sections_created)
     {
       bfd *dynobj;
       asection *s;
-      bfd_size_type dynsymcount;
-      unsigned long section_sym_count;
       unsigned int dtagcount;
 
       dynobj = elf_hash_table (info)->dynobj;
 
-      /* Assign dynsym indicies.  In a shared library we generate a
-	 section symbol for each output section, which come first.
-	 Next come all of the back-end allocated local dynamic syms,
-	 followed by the rest of the global symbols.  */
-
-      dynsymcount = _bfd_elf_link_renumber_dynsyms (output_bfd, info,
-						    &section_sym_count);
-
       /* Work out the size of the symbol version section.  */
       s = bfd_get_linker_section (dynobj, ".gnu.version");
       BFD_ASSERT (s != NULL);
Index: binutils/bfd/elfxx-mips.h
===================================================================
--- binutils.orig/bfd/elfxx-mips.h	2017-04-07 05:01:49.000000000 +0100
+++ binutils/bfd/elfxx-mips.h	2017-04-23 00:24:52.251311219 +0100
@@ -194,3 +194,4 @@ literal_reloc_p (int r_type)
 #define elf_backend_post_process_headers _bfd_mips_post_process_headers
 #define elf_backend_compact_eh_encoding _bfd_mips_elf_compact_eh_encoding
 #define elf_backend_cant_unwind_opcode _bfd_mips_elf_cant_unwind_opcode
+#define elf_backend_always_renumber_dynsyms TRUE
Index: binutils/bfd/elfxx-target.h
===================================================================
--- binutils.orig/bfd/elfxx-target.h	2017-04-05 23:25:45.000000000 +0100
+++ binutils/bfd/elfxx-target.h	2017-04-23 01:14:47.075873573 +0100
@@ -126,6 +126,9 @@
 #ifndef elf_backend_extern_protected_data
 #define elf_backend_extern_protected_data 0
 #endif
+#ifndef elf_backend_always_renumber_dynsyms
+#define elf_backend_always_renumber_dynsyms FALSE
+#endif
 #ifndef elf_backend_stack_align
 #define elf_backend_stack_align 16
 #endif
@@ -878,7 +881,8 @@ static struct elf_backend_data elfNN_bed
   elf_backend_no_page_alias,
   elf_backend_default_execstack,
   elf_backend_caches_rawsize,
-  elf_backend_extern_protected_data
+  elf_backend_extern_protected_data,
+  elf_backend_always_renumber_dynsyms
 };
 
 /* Forward declaration for use when initialising alternative_target field.  */
Index: binutils/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/mips-elf.exp	2017-04-22 23:56:47.811605508 +0100
+++ binutils/ld/testsuite/ld-mips-elf/mips-elf.exp	2017-04-22 23:58:50.694492097 +0100
@@ -1129,3 +1129,13 @@ if { $linux_gnu } {
 		  "export-$class-ref"]]
     }
 }
+
+# PR ld/21334 GOT relocation in static binary test.
+run_ld_link_tests [list \
+    [list \
+	"PR ld/21233 MIPS GOT16 relocation in static binary" \
+	"$abi_ldflags(o32) -e foo -T pr21334.ld" "" "$abi_asflags(o32)" \
+	{pr21334.s} \
+	{{objdump {-d --prefix-addresses} pr21334.dd} \
+	 {readelf -A pr21334.gd}} \
+	"pr21334"]]
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.dd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.dd	2017-04-22 23:58:50.711717195 +0100
@@ -0,0 +1,10 @@
+.*: +file format .*mips.*
+
+Disassembly of section \.text:
+[0-9a-f]+ <[^>]*> lui	gp,0x1
+[0-9a-f]+ <[^>]*> addiu	gp,gp,-32736
+[0-9a-f]+ <[^>]*> addu	gp,gp,t9
+[0-9a-f]+ <[^>]*> lw	v0,-32744\(gp\)
+[0-9a-f]+ <[^>]*> jr	ra
+[0-9a-f]+ <[^>]*> addiu	v0,v0,4
+	\.\.\.
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.gd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.gd	2017-04-22 23:58:50.731964110 +0100
@@ -0,0 +1,11 @@
+Static GOT:
+ Canonical gp value: 00008020
+
+ Reserved entries:
+   Address     Access    Value
+  00000030 -32752\(gp\) 00000000
+  00000034 -32748\(gp\) 80000000
+
+ Local entries:
+   Address     Access    Value
+  00000038 -32744\(gp\) 00000000
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.ld
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.ld	2017-04-22 23:58:50.744037369 +0100
@@ -0,0 +1,9 @@
+SECTIONS
+{
+  .text : { *(.text) }
+  HIDDEN (_gp = ALIGN (16) + 0x7ff0);
+  .got : { *(.got) }
+  .symtab : { *(.symtab) }
+  .strtab : { *(.strtab) }
+  /DISCARD/ : { *(*) }
+}
Index: binutils/ld/testsuite/ld-mips-elf/pr21334.s
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-mips-elf/pr21334.s	2017-04-22 23:58:50.746054019 +0100
@@ -0,0 +1,20 @@
+	.abicalls
+	.text
+	.set	noreorder
+	.globl	foo
+	.ent	foo
+foo:
+	.frame	$sp, 0, $31
+	.mask	0x00000000, 0
+	.fmask	0x00000000, 0
+	.cpload	$25
+	lw	$2, %got(bar)($28)
+	jr	$31
+	 addiu	$2, $2, 4
+	.end	foo
+	.weak	bar
+	.hidden	bar
+
+# Force some (non-delay-slot) zero bytes, to make 'objdump' print ...
+	.align	4, 0
+	.space	16

Re: [PATCH v2 0/2] PR ld/21334: `_bfd_elf_link_renumber_dynsyms' call fixes

[Alan Modra]

On Mon, Apr 24, 2017 at 08:12:28PM +0100, Maciej W. Rozycki wrote:
>  As promised here is a mini patch series to address issues with calling 
> `_bfd_elf_link_renumber_dynsyms'.  See the individual descriptions for 
> details.

OK for both patches, thanks.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH v2 0/2] PR ld/21334: `_bfd_elf_link_renumber_dynsyms' call fixes

[Maciej W. Rozycki]

On Wed, 26 Apr 2017, Alan Modra wrote:

> >  As promised here is a mini patch series to address issues with calling 
> > `_bfd_elf_link_renumber_dynsyms'.  See the individual descriptions for 
> > details.
> 
> OK for both patches, thanks.

 Committed, thanks.

  Maciej

[committed 2.28 1/2] ELF/BFD: Limit `_bfd_elf_link_renumber_dynsyms' call in section GC

[Maciej W. Rozycki]

Consistently call `_bfd_elf_link_renumber_dynsyms' only if linking a 
dynamic binary, complementing code in `bfd_elf_size_dynsym_hash_dynstr' 
and commit ccabcbe51e85 ("New attempt at fixing MIPS --gc-sections et 
al."), <https://sourceware.org/ml/binutils/2005-08/msg00258.html>.

	bfd/
	* elflink.c (elf_gc_sweep): Only call
	`_bfd_elf_link_renumber_dynsyms' if dynamic sections have been 
	created.

(backported from commit c46cec3a8cfe02fbe0f6c67ba53abc5369c5c659)
---
 No regressions across the usual targets.  Committed to 2.28.

  Maciej

binutils-bfd-elf-gc-dynsym-renumber-2.28.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-26 13:50:04.000000000 +0100
+++ binutils/bfd/elflink.c	2017-04-26 13:56:16.393714349 +0100
@@ -12983,7 +12983,8 @@ elf_gc_sweep (bfd *abfd, struct bfd_link
   elf_link_hash_traverse (elf_hash_table (info), elf_gc_sweep_symbol,
 			  &sweep_info);
 
-  _bfd_elf_link_renumber_dynsyms (abfd, info, &section_sym_count);
+  if (elf_hash_table (info)->dynamic_sections_created)
+    _bfd_elf_link_renumber_dynsyms (abfd, info, &section_sym_count);
   return TRUE;
 }
 

[committed 2.28 2/2] PR ld/21334: Always call `_bfd_elf_link_renumber_dynsyms' if required

[Maciej W. Rozycki]

Complement commit e17b0c351f0b ("MIPS/BFD: Respect the ELF gABI dynamic 
symbol table sort requirement") and correct an inconsistency in dynamic 
symbol accounting data causing an assertion failure in the MIPS backend:

ld: BFD (GNU Binutils) 2.28.51.20170330 assertion fail
../../binutils-gdb/bfd/elfxx-mips.c:3860

in the course of making a GOT entry in a static binary to satisfy a GOT 
relocation present in input, due to the local dynamic symbol count not 
having been established.

To do so let backends request `_bfd_elf_link_renumber_dynsyms' to be 
always called, rather than where a dynamic binary is linked only, and 
then make this request in the MIPS backend.

	bfd/
	PR ld/21334
	* elf-bfd.h (elf_backend_data): Add `always_renumber_dynsyms' 
	member.
	* elfxx-target.h [!elf_backend_always_renumber_dynsyms]
	(elf_backend_always_renumber_dynsyms): Define.
	(elfNN_bed): Initialize `always_renumber_dynsyms' member.
	* elfxx-mips.h (elf_backend_always_renumber_dynsyms): Define.
	* elflink.c (bfd_elf_size_dynsym_hash_dynstr): Also call
	`_bfd_elf_link_renumber_dynsyms' if the backend has requested 
	it.
	(elf_gc_sweep): Likewise.

(backported from commit 23ec1e32b1ab714649a7c25e49b5d721fe3bd3db)
---
 No regressions against the usual targets.  Committed to 2.28.

  Maciej

binutils-bfd-elf-always-dynsym-renumber-2.28.diff
Index: binutils/bfd/elf-bfd.h
===================================================================
--- binutils.orig/bfd/elf-bfd.h	2017-04-26 13:46:32.000000000 +0100
+++ binutils/bfd/elf-bfd.h	2017-04-26 13:59:01.977164246 +0100
@@ -1478,6 +1478,10 @@ struct elf_backend_data
   /* Address of protected data defined in the shared library may be
      external, i.e., due to copy relocation.   */
   unsigned extern_protected_data : 1;
+
+  /* True if `_bfd_elf_link_renumber_dynsyms' must be called even for
+     static binaries.  */
+  unsigned always_renumber_dynsyms : 1;
 };
 
 /* Information about reloc sections associated with a bfd_elf_section_data
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-26 13:56:16.000000000 +0100
+++ binutils/bfd/elflink.c	2017-04-26 13:59:01.995374303 +0100
@@ -6707,6 +6707,8 @@ bfd_boolean
 bfd_elf_size_dynsym_hash_dynstr (bfd *output_bfd, struct bfd_link_info *info)
 {
   const struct elf_backend_data *bed;
+  unsigned long section_sym_count;
+  bfd_size_type dynsymcount;
 
   if (!is_elf_hash_table (info->hash))
     return TRUE;
@@ -6714,24 +6716,30 @@ bfd_elf_size_dynsym_hash_dynstr (bfd *ou
   bed = get_elf_backend_data (output_bfd);
   (*bed->elf_backend_init_index_section) (output_bfd, info);
 
+  /* Assign dynsym indices.  In a shared library we generate a section
+     symbol for each output section, which come first.  Next come all
+     of the back-end allocated local dynamic syms, followed by the rest
+     of the global symbols.
+
+     This is usually not needed for static binaries, however backends
+     can request to always do it, e.g. the MIPS backend uses dynamic
+     symbol counts to lay out GOT, which will be produced in the
+     presence of GOT relocations even in static binaries (holding fixed
+     data in that case, to satisfy those relocations).  */
+
+  if (elf_hash_table (info)->dynamic_sections_created
+      || bed->always_renumber_dynsyms)
+    dynsymcount = _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+						  &section_sym_count);
+
   if (elf_hash_table (info)->dynamic_sections_created)
     {
       bfd *dynobj;
       asection *s;
-      bfd_size_type dynsymcount;
-      unsigned long section_sym_count;
       unsigned int dtagcount;
 
       dynobj = elf_hash_table (info)->dynobj;
 
-      /* Assign dynsym indicies.  In a shared library we generate a
-	 section symbol for each output section, which come first.
-	 Next come all of the back-end allocated local dynamic syms,
-	 followed by the rest of the global symbols.  */
-
-      dynsymcount = _bfd_elf_link_renumber_dynsyms (output_bfd, info,
-						    &section_sym_count);
-
       /* Work out the size of the symbol version section.  */
       s = bfd_get_linker_section (dynobj, ".gnu.version");
       BFD_ASSERT (s != NULL);
@@ -12983,7 +12991,11 @@ elf_gc_sweep (bfd *abfd, struct bfd_link
   elf_link_hash_traverse (elf_hash_table (info), elf_gc_sweep_symbol,
 			  &sweep_info);
 
-  if (elf_hash_table (info)->dynamic_sections_created)
+  /* We need to reassign dynsym indices now that symbols may have
+     been removed.  See the call in `bfd_elf_size_dynsym_hash_dynstr'
+     for the details of the conditions used here.  */
+  if (elf_hash_table (info)->dynamic_sections_created
+      || bed->always_renumber_dynsyms)
     _bfd_elf_link_renumber_dynsyms (abfd, info, &section_sym_count);
   return TRUE;
 }
Index: binutils/bfd/elfxx-mips.h
===================================================================
--- binutils.orig/bfd/elfxx-mips.h	2017-04-26 13:46:34.000000000 +0100
+++ binutils/bfd/elfxx-mips.h	2017-04-26 13:59:02.001414621 +0100
@@ -196,3 +196,4 @@ literal_reloc_p (int r_type)
 #define elf_backend_post_process_headers _bfd_mips_post_process_headers
 #define elf_backend_compact_eh_encoding _bfd_mips_elf_compact_eh_encoding
 #define elf_backend_cant_unwind_opcode _bfd_mips_elf_cant_unwind_opcode
+#define elf_backend_always_renumber_dynsyms TRUE
Index: binutils/bfd/elfxx-target.h
===================================================================
--- binutils.orig/bfd/elfxx-target.h	2017-04-26 13:46:34.000000000 +0100
+++ binutils/bfd/elfxx-target.h	2017-04-26 13:59:02.008468206 +0100
@@ -126,6 +126,9 @@
 #ifndef elf_backend_extern_protected_data
 #define elf_backend_extern_protected_data 0
 #endif
+#ifndef elf_backend_always_renumber_dynsyms
+#define elf_backend_always_renumber_dynsyms FALSE
+#endif
 #ifndef elf_backend_stack_align
 #define elf_backend_stack_align 16
 #endif
@@ -866,7 +869,8 @@ static struct elf_backend_data elfNN_bed
   elf_backend_no_page_alias,
   elf_backend_default_execstack,
   elf_backend_caches_rawsize,
-  elf_backend_extern_protected_data
+  elf_backend_extern_protected_data,
+  elf_backend_always_renumber_dynsyms
 };
 
 /* Forward declaration for use when initialising alternative_target field.  */

Re: [PATCH v2 0/2] PR ld/21334: `_bfd_elf_link_renumber_dynsyms' call fixes

[Maciej W. Rozycki]

On Wed, 26 Apr 2017, Maciej W. Rozycki wrote:

> > >  As promised here is a mini patch series to address issues with calling 
> > > `_bfd_elf_link_renumber_dynsyms'.  See the individual descriptions for 
> > > details.
> > 
> > OK for both patches, thanks.
> 
>  Committed, thanks.

 And now backported to 2.28, after an additional test cycle due to the 
changes being more than just a mechanical patch merge.  Final patches 
posted for documentation purposes, for the same reason, and PR ld/21334 
now closed.

  Maciej