[PATCH] PR ld/21334: Always assign dynsym indices even in static binaries

[PATCH] PR ld/21334: Always assign dynsym indices even in static binaries

[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 move the call to `_bfd_elf_link_renumber_dynsyms' outside a 
check for dynamic sections to have been created, so that the function 
always assigns dynsym indices and initializes associated data, even for 
static binaries.

	bfd/
	PR ld/21334
	* elflink.c (bfd_elf_size_dynsym_hash_dynstr): Always call
	`_bfd_elf_link_renumber_dynsyms'.

	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.

 Calling `_bfd_elf_link_renumber_dynsyms' unconditionally has also been 
verified to some extent to be safe by Debian across their targets, as a 
side effect of `_bfd_elf_link_renumber_dynsyms' always being called when 
section GC has been enabled with `--gc-sections', as it has been the case 
with some of their packages, which according to James's statement posted 
with the PR includes KDE/Qt and some other packages.

 OK for master and 2.28?

 NB one of the test cases added relies on MIPS static GOT dump support 
in `readelf' just posted, so firstly the order of patches will have to 
be maintained and secondly I'll remove it from the 2.28 backport.

  Maciej

binutils-bfd-elf-always-dynsym-renumber.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-10 23:11:21.572119928 +0100
+++ binutils/bfd/elflink.c	2017-04-11 02:45:59.328420966 +0100
@@ -6778,6 +6778,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;
@@ -6785,24 +6787,29 @@ 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.
+
+     We always do this step even if dynamic sections have not been made,
+     because backends may need symbol accounting data produced here,
+     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).  */
+
+  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/ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-mips-elf/mips-elf.exp	2017-04-10 23:11:20.401635333 +0100
+++ binutils/ld/testsuite/ld-mips-elf/mips-elf.exp	2017-04-11 02:47:43.596728893 +0100
@@ -1130,3 +1130,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-11 02:47:43.603859887 +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-11 02:47:43.629240183 +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-11 02:47:43.646400289 +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-11 02:47:43.663515875 +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

[PING][PATCH] PR ld/21334: Always assign dynsym indices even in static binaries

[Maciej W. Rozycki]

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

> 	bfd/
> 	PR ld/21334
> 	* elflink.c (bfd_elf_size_dynsym_hash_dynstr): Always call
> 	`_bfd_elf_link_renumber_dynsyms'.
> 
> 	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.

 Can I ask for 
<https://www.sourceware.org/ml/binutils/2017-04/msg00127.html> to be 
reviewed?

  Maciej

Re: [PING][PATCH] PR ld/21334: Always assign dynsym indices even in static binaries

[Alan Modra]

On Thu, Apr 20, 2017 at 05:02:46PM +0100, Maciej W. Rozycki wrote:
>  Can I ask for 
> <https://www.sourceware.org/ml/binutils/2017-04/msg00127.html> to be 
> reviewed?

I don't even recall seeing the patch, sorry..  I suggest instead that
you renumber dynsyms for mips when !dynamic_sections_created inside a
mips elf_backend_init_index_section.  That way the great majority of
targets that do not need this for static executables will not incur
two extra passes over the symbol table.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PING][PATCH] PR ld/21334: Always assign dynsym indices even in static binaries

[Maciej W. Rozycki]

On Fri, 21 Apr 2017, Alan Modra wrote:

> >  Can I ask for 
> > <https://www.sourceware.org/ml/binutils/2017-04/msg00127.html> to be 
> > reviewed?
> 
> I don't even recall seeing the patch, sorry..

 No worries!

>  I suggest instead that
> you renumber dynsyms for mips when !dynamic_sections_created inside a
> mips elf_backend_init_index_section.  That way the great majority of
> targets that do not need this for static executables will not incur
> two extra passes over the symbol table.

 Good point about the wasted passes, thanks.

 I have looked into alternatives and concluded I could not agree about 
`elf_backend_init_index_section' being a good place to add such code to.  
While it appears right in terms of execution flow, the intent of this hook 
is I think too distant from what `_bfd_elf_link_renumber_dynsyms' does, 
and consequently placing the call there would make code hard to understand 
and maintain long-term.  I might be able to remember that myself, but 
anyone coming after me would not have that knowledge.

 So instead I looked into putting the call in `mips_before_allocation', 
right after `gld${EMULATION_NAME}_before_allocation', which is where the 
call to `bfd_elf_size_dynsym_hash_dynstr' is made from.  I thought this 
would be more appropriate.  But then there is that other call made from 
`bfd_elf_size_dynamic_sections' after section GC.  The complementing MIPS 
call for static binaries would have to be made from 
`_bfd_mips_elf_always_size_sections', again making the whole arrangement 
messy.

 So let's do it properly and have `bfd_elf_size_dynamic_sections' calls 
left in their current single places, but with an additional backend flag 
to control whether they need to be always executed.  This has the extra 
advantage of keeping the function local to `elflink.c'.  I'll be posting a 
replacement mini patch series shortly then, including a preparatory fix 
for the missing conditional in the `bfd_elf_size_dynamic_sections' call in 
section GC, as observed in the course of PR ld/21334 investigation and 
noted in Bugzilla.

 For the record, below is the alternative I considered and have discarded 
as too hackish.

  Maciej

binutils-mips-ld-always-dynsym-renumber.diff
Index: binutils/bfd/elf-bfd.h
===================================================================
--- binutils.orig/bfd/elf-bfd.h	2017-04-22 22:57:25.253629828 +0100
+++ binutils/bfd/elf-bfd.h	2017-04-22 23:32:51.062487621 +0100
@@ -2208,6 +2208,8 @@ extern bfd_boolean _bfd_elf_link_create_
   (bfd *, struct bfd_link_info *);
 extern bfd_boolean _bfd_elf_link_omit_section_dynsym
   (bfd *, struct bfd_link_info *, asection *);
+extern unsigned long _bfd_elf_link_renumber_dynsyms
+  (bfd *, struct bfd_link_info *, unsigned long *);
 extern bfd_boolean _bfd_elf_create_dynamic_sections
   (bfd *, struct bfd_link_info *);
 extern bfd_boolean _bfd_elf_create_got_section
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-04-22 23:31:17.306090155 +0100
+++ binutils/bfd/elflink.c	2017-04-22 23:32:51.090814194 +0100
@@ -907,7 +907,7 @@ _bfd_elf_link_omit_section_dynsym (bfd *
    allocated local dynamic syms, followed by the rest of the global
    symbols.  */
 
-static unsigned long
+unsigned long
 _bfd_elf_link_renumber_dynsyms (bfd *output_bfd,
 				struct bfd_link_info *info,
 				unsigned long *section_sym_count)
Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c	2017-04-22 22:57:23.000000000 +0100
+++ binutils/bfd/elfxx-mips.c	2017-04-22 23:46:58.332082902 +0100
@@ -9267,10 +9267,25 @@ _bfd_mips_elf_always_size_sections (bfd 
   asection *sect;
   struct mips_elf_link_hash_table *htab;
   struct mips_htab_traverse_info hti;
+  unsigned long section_sym_count;
 
   htab = mips_elf_hash_table (info);
   BFD_ASSERT (htab != NULL);
 
+  /* In the case of linking a static binary if symbols have been removed
+     as a result of section garbage, then reassign dynsym indices.  This
+     would have been done in our caller `bfd_elf_size_dynamic_sections',
+     however most targets do not need this step for static binaries and
+     therefore the call to `_bfd_elf_link_renumber_dynsyms' is not made
+     there in that case.
+
+     We need 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), so we do this here instead.  */
+  if (!htab->root.dynamic_sections_created && info->gc_sections)
+    _bfd_elf_link_renumber_dynsyms (output_bfd, info, &section_sym_count);
+
   /* The .reginfo section has a fixed size.  */
   sect = bfd_get_section_by_name (output_bfd, ".reginfo");
   if (sect != NULL)
Index: binutils/ld/emultempl/mipself.em
===================================================================
--- binutils.orig/ld/emultempl/mipself.em	2017-04-22 22:57:25.303526473 +0100
+++ binutils/ld/emultempl/mipself.em	2017-04-22 23:32:51.127204842 +0100
@@ -214,6 +214,7 @@ mips_create_output_section_statements (v
 static void
 mips_before_allocation (void)
 {
+  unsigned long section_sym_count;
   flagword flags;
 
   flags = elf_elfheader (link_info.output_bfd)->e_flags;
@@ -223,6 +224,22 @@ mips_before_allocation (void)
     _bfd_mips_elf_use_plts_and_copy_relocs (&link_info);
 
   gld${EMULATION_NAME}_before_allocation ();
+
+  /* Assign dynsym indices in the case of linking a static binary.
+     This would normally be done in \`bfd_elf_size_dynsym_hash_dynstr'
+     called from \`gld${EMULATION_NAME}_before_allocation' above,
+     however most targets do not need this step for static binaries
+     and therefore the call to \`_bfd_elf_link_renumber_dynsyms' is
+     not made there in that case.
+
+     We need 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), so we do this here instead.  */
+  if (is_elf_hash_table (link_info.hash)
+      && !elf_hash_table (&link_info)->dynamic_sections_created)
+    _bfd_elf_link_renumber_dynsyms (link_info.output_bfd, &link_info,
+				    &section_sym_count);
 }
 
 /* Avoid processing the fake stub_file in vercheck, stat_needed and