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

["Maciej W. Rozycki" <macro at imgtec dot com>]

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