public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
@ 2017-02-01 19:31 Maciej W. Rozycki
  2017-02-02  3:25 ` Alan Modra
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-02-01 19:31 UTC (permalink / raw)
  To: Alan Modra, Nick Clifton, James Cowgill; +Cc: binutils

Complement commit b531344c34b0 ("PR ld/20828: Reorder the symbol sweep 
stage of section GC") and commit 81ff47b3a546 ("PR ld/20828: Fix linker 
script symbols wrongly forced local with section GC") and move symbol 
version processing ahead of the symbol sweep stage of section GC, all in 
`bfd_elf_size_dynamic_sections', so that version symbols created stay in 
the global scope and are not output as local symbols to the dynamic symbol 
table in the presence of corresponding symbol definitions pulled from a 
DSO involved in a link.

Consolidate the whole of symbol version processing into a single block 
from all parts scattered across the function and rearranging the local 
variables used as necessary, however leaving the setting of dynamic 
entries associated with the DT_VERDEF, DT_VERDEFNUM, DT_VERNEED and 
DT_VERNEEDNUM tags in the original places.

With the rearrangement of code blocks `Elf_Internal_Verneed *t' would 
shadow the previous definition of `struct bfd_elf_version_tree *t', so 
rename the former variable to `vn'.

	bfd/
	PR ld/20828
	* elflink.c (bfd_elf_size_dynamic_sections): Move symbol version 
	processing ahead of the call to `elf_gc_sweep_symbol'.

	ld/
	PR ld/20828
	* testsuite/ld-elf/pr20828-d.sd: New test.
	* testsuite/ld-elf/pr20828-e.sd: New test.
	* testsuite/ld-elf/pr20828-v.ver: New test version script.
	* testsuite/ld-elf/pr20828.ld: Add `.gnu.version' and 
	`.gnu.version_d'.
	* testsuite/ld-elf/shared.exp: Run the new tests.
---
 I had to do a little bit of thinking to figure out how to verify across 
all targets that this somewhat radical change does not cause any bad 
interaction between version processing in `bfd_elf_size_dynamic_sections' 
and the two backend methods (`->elf_backend_always_size_sections' and 
`->elf_backend_size_dynamic_sections') called midway through.  So I asked 
myself what could actually cause a problem here and eventually came to a 
conclusion that the only scenario I can think of is where one of the 
methods adds an exported symbol which currently goes through the usual 
version processing, but would not anymore after this change.

 To check if that is the case I have extracted, with the aid of a 
combination of usual *nix text processing tools, all the methods and 
looked through them for symbol creation.  And indeed a couple do, namely: 
`elf32_arm_always_size_sections', `elfNN_aarch64_always_size_sections' 
`elf_i386_always_size_sections', `elf_x86_64_always_size_sections', and 
`elf_xtensa_always_size_sections', all of which add an _TLS_MODULE_BASE_ 
symbol, which however has a local scope and is made hidden right away, so 
it does get any interesting changes applied through version script 
processing.

 And that was just it.  So it appears to me we should be safe with my 
proposed fix unless there's something particularly peculiar somewhere that 
I have missed (which would not be unheard of, as for example it was with 
the S+core target earlier on in the course of addressing this PR).  I have 
got good regression test results across my current set of 175 targets 
though, which are reassuring in that at least this change does not break 
something we have previously recognised.

 In addition to that, prompted by the `cris-elf' ld-cris/libdso-12c.d and 
ld-cris/libdso-13b.d tests I have cowardly decided to keep the order of 
dynamic entries produced unchanged, in case a third-party piece of 
software relies on it (even though it shouldn't), and therefore I have 
moved the creation of the 4 types used with symbol versioning (VERDEF, 
VERDEFNUM, VERNEED, VERNEEDNUM) away from the calculations of the 
corresponding values and down towards the end of the function where they 
are currently made.  There's no harm from doing that IMO, even if only to 
satisfy human readers who may have got used over the years to how the 
entries have been ordered in dumps.

 Unfortunately due to how `diff' decided to record the changes the patch 
looks more complicated than it really is, as the parts which I *haven't*
moved are shown rather than those I did move.  So to look what has changed 
it may be easier to visually compare the unchanged vs updated version of 
`bfd_elf_size_dynamic_sections' side by side.

 I do hope I haven't missed anything then, but I will be happy to address 
any concerns this proposal may raise.  Otherwise, OK to apply to master?

  Maciej

binutils-bfd-elf-size-dynamic-sections-version.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-01-30 22:51:09.000000000 +0000
+++ binutils/bfd/elflink.c	2017-01-31 04:04:19.446661678 +0000
@@ -5925,7 +5925,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
   size_t soname_indx;
   bfd *dynobj;
   const struct elf_backend_data *bed;
-  struct elf_info_failed asvinfo;
 
   *sinterpptr = NULL;
 
@@ -5934,188 +5933,17 @@ bfd_elf_size_dynamic_sections (bfd *outp
   if (!is_elf_hash_table (info->hash))
     return TRUE;
 
-  bed = get_elf_backend_data (output_bfd);
-
-  if (info->gc_sections && bed->can_gc_sections)
-    {
-      struct elf_gc_sweep_symbol_info sweep_info;
-      unsigned long section_sym_count;
-
-      /* Remove the symbols that were in the swept sections from the
-	 dynamic symbol table.  GCFIXME: Anyone know how to get them
-	 out of the static symbol table as well?  */
-      sweep_info.info = info;
-      sweep_info.hide_symbol = bed->elf_backend_hide_symbol;
-      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);
-    }
-
-  /* Any syms created from now on start with -1 in
-     got.refcount/offset and plt.refcount/offset.  */
-  elf_hash_table (info)->init_got_refcount
-    = elf_hash_table (info)->init_got_offset;
-  elf_hash_table (info)->init_plt_refcount
-    = elf_hash_table (info)->init_plt_offset;
-
-  if (bfd_link_relocatable (info)
-      && !_bfd_elf_size_group_sections (info))
-    return FALSE;
-
-  /* The backend may have to create some sections regardless of whether
-     we're dynamic or not.  */
-  if (bed->elf_backend_always_size_sections
-      && ! (*bed->elf_backend_always_size_sections) (output_bfd, info))
-    return FALSE;
-
-  /* Determine any GNU_STACK segment requirements, after the backend
-     has had a chance to set a default segment size.  */
-  if (info->execstack)
-    elf_stack_flags (output_bfd) = PF_R | PF_W | PF_X;
-  else if (info->noexecstack)
-    elf_stack_flags (output_bfd) = PF_R | PF_W;
-  else
-    {
-      bfd *inputobj;
-      asection *notesec = NULL;
-      int exec = 0;
-
-      for (inputobj = info->input_bfds;
-	   inputobj;
-	   inputobj = inputobj->link.next)
-	{
-	  asection *s;
-
-	  if (inputobj->flags
-	      & (DYNAMIC | EXEC_P | BFD_PLUGIN | BFD_LINKER_CREATED))
-	    continue;
-	  s = bfd_get_section_by_name (inputobj, ".note.GNU-stack");
-	  if (s)
-	    {
-	      if (s->flags & SEC_CODE)
-		exec = PF_X;
-	      notesec = s;
-	    }
-	  else if (bed->default_execstack)
-	    exec = PF_X;
-	}
-      if (notesec || info->stacksize > 0)
-	elf_stack_flags (output_bfd) = PF_R | PF_W | exec;
-      if (notesec && exec && bfd_link_relocatable (info)
-	  && notesec->output_section != bfd_abs_section_ptr)
-	notesec->output_section->flags |= SEC_CODE;
-    }
-
   dynobj = elf_hash_table (info)->dynobj;
 
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
-      struct elf_info_failed eif;
-      struct elf_link_hash_entry *h;
-      asection *dynstr;
+      struct bfd_elf_version_tree *verdefs;
+      unsigned long section_sym_count;
+      struct elf_info_failed asvinfo;
       struct bfd_elf_version_tree *t;
       struct bfd_elf_version_expr *d;
-      asection *s;
       bfd_boolean all_defined;
-
-      *sinterpptr = bfd_get_linker_section (dynobj, ".interp");
-      BFD_ASSERT (*sinterpptr != NULL || !bfd_link_executable (info) || info->nointerp);
-
-      if (soname != NULL)
-	{
-	  soname_indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
-					     soname, TRUE);
-	  if (soname_indx == (size_t) -1
-	      || !_bfd_elf_add_dynamic_entry (info, DT_SONAME, soname_indx))
-	    return FALSE;
-	}
-
-      if (info->symbolic)
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_SYMBOLIC, 0))
-	    return FALSE;
-	  info->flags |= DF_SYMBOLIC;
-	}
-
-      if (rpath != NULL)
-	{
-	  size_t indx;
-	  bfd_vma tag;
-
-	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr, rpath,
-				      TRUE);
-	  if (indx == (size_t) -1)
-	    return FALSE;
-
-	  tag = info->new_dtags ? DT_RUNPATH : DT_RPATH;
-	  if (!_bfd_elf_add_dynamic_entry (info, tag, indx))
-	    return FALSE;
-	}
-
-      if (filter_shlib != NULL)
-	{
-	  size_t indx;
-
-	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
-				      filter_shlib, TRUE);
-	  if (indx == (size_t) -1
-	      || !_bfd_elf_add_dynamic_entry (info, DT_FILTER, indx))
-	    return FALSE;
-	}
-
-      if (auxiliary_filters != NULL)
-	{
-	  const char * const *p;
-
-	  for (p = auxiliary_filters; *p != NULL; p++)
-	    {
-	      size_t indx;
-
-	      indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
-					  *p, TRUE);
-	      if (indx == (size_t) -1
-		  || !_bfd_elf_add_dynamic_entry (info, DT_AUXILIARY, indx))
-		return FALSE;
-	    }
-	}
-
-      if (audit != NULL)
-	{
-	  size_t indx;
-
-	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr, audit,
-				      TRUE);
-	  if (indx == (size_t) -1
-	      || !_bfd_elf_add_dynamic_entry (info, DT_AUDIT, indx))
-	    return FALSE;
-	}
-
-      if (depaudit != NULL)
-	{
-	  size_t indx;
-
-	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr, depaudit,
-				      TRUE);
-	  if (indx == (size_t) -1
-	      || !_bfd_elf_add_dynamic_entry (info, DT_DEPAUDIT, indx))
-	    return FALSE;
-	}
-
-      eif.info = info;
-      eif.failed = FALSE;
-
-      /* If we are supposed to export all symbols into the dynamic symbol
-	 table (this is not the normal case), then do so.  */
-      if (info->export_dynamic
-	  || (bfd_link_executable (info) && info->dynamic))
-	{
-	  elf_link_hash_traverse (elf_hash_table (info),
-				  _bfd_elf_export_symbol,
-				  &eif);
-	  if (eif.failed)
-	    return FALSE;
-	}
+      asection *s;
 
       /* Make all global versions with definition.  */
       for (t = info->version_info; t != NULL; t = t->next)
@@ -6200,129 +6028,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	    }
 	}
 
-      /* Find all symbols which were defined in a dynamic object and make
-	 the backend pick a reasonable value for them.  */
-      elf_link_hash_traverse (elf_hash_table (info),
-			      _bfd_elf_adjust_dynamic_symbol,
-			      &eif);
-      if (eif.failed)
-	return FALSE;
-
-      /* Add some entries to the .dynamic section.  We fill in some of the
-	 values later, in bfd_elf_final_link, but we must add the entries
-	 now so that we know the final size of the .dynamic section.  */
-
-      /* If there are initialization and/or finalization functions to
-	 call then add the corresponding DT_INIT/DT_FINI entries.  */
-      h = (info->init_function
-	   ? elf_link_hash_lookup (elf_hash_table (info),
-				   info->init_function, FALSE,
-				   FALSE, FALSE)
-	   : NULL);
-      if (h != NULL
-	  && (h->ref_regular
-	      || h->def_regular))
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_INIT, 0))
-	    return FALSE;
-	}
-      h = (info->fini_function
-	   ? elf_link_hash_lookup (elf_hash_table (info),
-				   info->fini_function, FALSE,
-				   FALSE, FALSE)
-	   : NULL);
-      if (h != NULL
-	  && (h->ref_regular
-	      || h->def_regular))
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_FINI, 0))
-	    return FALSE;
-	}
-
-      s = bfd_get_section_by_name (output_bfd, ".preinit_array");
-      if (s != NULL && s->linker_has_input)
-	{
-	  /* DT_PREINIT_ARRAY is not allowed in shared library.  */
-	  if (! bfd_link_executable (info))
-	    {
-	      bfd *sub;
-	      asection *o;
-
-	      for (sub = info->input_bfds; sub != NULL;
-		   sub = sub->link.next)
-		if (bfd_get_flavour (sub) == bfd_target_elf_flavour)
-		  for (o = sub->sections; o != NULL; o = o->next)
-		    if (elf_section_data (o)->this_hdr.sh_type
-			== SHT_PREINIT_ARRAY)
-		      {
-			_bfd_error_handler
-			  (_("%B: .preinit_array section is not allowed in DSO"),
-			   sub);
-			break;
-		      }
-
-	      bfd_set_error (bfd_error_nonrepresentable_section);
-	      return FALSE;
-	    }
-
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_PREINIT_ARRAY, 0)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_PREINIT_ARRAYSZ, 0))
-	    return FALSE;
-	}
-      s = bfd_get_section_by_name (output_bfd, ".init_array");
-      if (s != NULL && s->linker_has_input)
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_INIT_ARRAY, 0)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_INIT_ARRAYSZ, 0))
-	    return FALSE;
-	}
-      s = bfd_get_section_by_name (output_bfd, ".fini_array");
-      if (s != NULL && s->linker_has_input)
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_FINI_ARRAY, 0)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_FINI_ARRAYSZ, 0))
-	    return FALSE;
-	}
-
-      dynstr = bfd_get_linker_section (dynobj, ".dynstr");
-      /* If .dynstr is excluded from the link, we don't want any of
-	 these tags.  Strictly, we should be checking each section
-	 individually;  This quick check covers for the case where
-	 someone does a /DISCARD/ : { *(*) }.  */
-      if (dynstr != NULL && dynstr->output_section != bfd_abs_section_ptr)
-	{
-	  bfd_size_type strsize;
-
-	  strsize = _bfd_elf_strtab_size (elf_hash_table (info)->dynstr);
-	  if ((info->emit_hash
-	       && !_bfd_elf_add_dynamic_entry (info, DT_HASH, 0))
-	      || (info->emit_gnu_hash
-		  && !_bfd_elf_add_dynamic_entry (info, DT_GNU_HASH, 0))
-	      || !_bfd_elf_add_dynamic_entry (info, DT_STRTAB, 0)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_SYMTAB, 0)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_STRSZ, strsize)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_SYMENT,
-					      bed->s->sizeof_sym))
-	    return FALSE;
-	}
-    }
-
-  if (! _bfd_elf_maybe_strip_eh_frame_hdr (info))
-    return FALSE;
-
-  /* The backend must work out the sizes of all the other dynamic
-     sections.  */
-  if (dynobj != NULL
-      && bed->elf_backend_size_dynamic_sections != NULL
-      && ! (*bed->elf_backend_size_dynamic_sections) (output_bfd, info))
-    return FALSE;
-
-  if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
-    {
-      unsigned long section_sym_count;
-      struct bfd_elf_version_tree *verdefs;
-      asection *s;
-
       /* Set up the version definition section.  */
       s = bfd_get_linker_section (dynobj, ".gnu.version_d");
       BFD_ASSERT (s != NULL);
@@ -6341,7 +6046,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	{
 	  unsigned int cdefs;
 	  bfd_size_type size;
-	  struct bfd_elf_version_tree *t;
 	  bfd_byte *p;
 	  Elf_Internal_Verdef def;
 	  Elf_Internal_Verdaux defaux;
@@ -6557,34 +6261,9 @@ bfd_elf_size_dynamic_sections (bfd *outp
 		}
 	    }
 
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_VERDEF, 0)
-	      || !_bfd_elf_add_dynamic_entry (info, DT_VERDEFNUM, cdefs))
-	    return FALSE;
-
 	  elf_tdata (output_bfd)->cverdefs = cdefs;
 	}
 
-      if ((info->new_dtags && info->flags) || (info->flags & DF_STATIC_TLS))
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_FLAGS, info->flags))
-	    return FALSE;
-	}
-      else if (info->flags & DF_BIND_NOW)
-	{
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_BIND_NOW, 0))
-	    return FALSE;
-	}
-
-      if (info->flags_1)
-	{
-	  if (bfd_link_executable (info))
-	    info->flags_1 &= ~ (DF_1_INITFIRST
-				| DF_1_NODELETE
-				| DF_1_NOOPEN);
-	  if (!_bfd_elf_add_dynamic_entry (info, DT_FLAGS_1, info->flags_1))
-	    return FALSE;
-	}
-
       /* Work out the size of the version reference section.  */
 
       s = bfd_get_linker_section (dynobj, ".gnu.version_r");
@@ -6608,7 +6287,7 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	  s->flags |= SEC_EXCLUDE;
 	else
 	  {
-	    Elf_Internal_Verneed *t;
+	    Elf_Internal_Verneed *vn;
 	    unsigned int size;
 	    unsigned int crefs;
 	    bfd_byte *p;
@@ -6616,15 +6295,15 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	    /* Build the version dependency section.  */
 	    size = 0;
 	    crefs = 0;
-	    for (t = elf_tdata (output_bfd)->verref;
-		 t != NULL;
-		 t = t->vn_nextref)
+	    for (vn = elf_tdata (output_bfd)->verref;
+		 vn != NULL;
+		 vn = vn->vn_nextref)
 	      {
 		Elf_Internal_Vernaux *a;
 
 		size += sizeof (Elf_External_Verneed);
 		++crefs;
-		for (a = t->vn_auxptr; a != NULL; a = a->vna_nextptr)
+		for (a = vn->vn_auxptr; a != NULL; a = a->vna_nextptr)
 		  size += sizeof (Elf_External_Vernaux);
 	      }
 
@@ -6634,40 +6313,40 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	      return FALSE;
 
 	    p = s->contents;
-	    for (t = elf_tdata (output_bfd)->verref;
-		 t != NULL;
-		 t = t->vn_nextref)
+	    for (vn = elf_tdata (output_bfd)->verref;
+		 vn != NULL;
+		 vn = vn->vn_nextref)
 	      {
 		unsigned int caux;
 		Elf_Internal_Vernaux *a;
 		size_t indx;
 
 		caux = 0;
-		for (a = t->vn_auxptr; a != NULL; a = a->vna_nextptr)
+		for (a = vn->vn_auxptr; a != NULL; a = a->vna_nextptr)
 		  ++caux;
 
-		t->vn_version = VER_NEED_CURRENT;
-		t->vn_cnt = caux;
+		vn->vn_version = VER_NEED_CURRENT;
+		vn->vn_cnt = caux;
 		indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
-					    elf_dt_name (t->vn_bfd) != NULL
-					    ? elf_dt_name (t->vn_bfd)
-					    : lbasename (t->vn_bfd->filename),
+					    elf_dt_name (vn->vn_bfd) != NULL
+					    ? elf_dt_name (vn->vn_bfd)
+					    : lbasename (vn->vn_bfd->filename),
 					    FALSE);
 		if (indx == (size_t) -1)
 		  return FALSE;
-		t->vn_file = indx;
-		t->vn_aux = sizeof (Elf_External_Verneed);
-		if (t->vn_nextref == NULL)
-		  t->vn_next = 0;
+		vn->vn_file = indx;
+		vn->vn_aux = sizeof (Elf_External_Verneed);
+		if (vn->vn_nextref == NULL)
+		  vn->vn_next = 0;
 		else
-		  t->vn_next = (sizeof (Elf_External_Verneed)
+		  vn->vn_next = (sizeof (Elf_External_Verneed)
 				+ caux * sizeof (Elf_External_Vernaux));
 
-		_bfd_elf_swap_verneed_out (output_bfd, t,
+		_bfd_elf_swap_verneed_out (output_bfd, vn,
 					   (Elf_External_Verneed *) p);
 		p += sizeof (Elf_External_Verneed);
 
-		for (a = t->vn_auxptr; a != NULL; a = a->vna_nextptr)
+		for (a = vn->vn_auxptr; a != NULL; a = a->vna_nextptr)
 		  {
 		    a->vna_hash = bfd_elf_hash (a->vna_nodename);
 		    indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
@@ -6686,10 +6365,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
 		  }
 	      }
 
-	    if (!_bfd_elf_add_dynamic_entry (info, DT_VERNEED, 0)
-		|| !_bfd_elf_add_dynamic_entry (info, DT_VERNEEDNUM, crefs))
-	      return FALSE;
-
 	    elf_tdata (output_bfd)->cverrefs = crefs;
 	  }
       }
@@ -6703,6 +6378,343 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	  s->flags |= SEC_EXCLUDE;
 	}
     }
+
+  bed = get_elf_backend_data (output_bfd);
+
+  if (info->gc_sections && bed->can_gc_sections)
+    {
+      struct elf_gc_sweep_symbol_info sweep_info;
+      unsigned long section_sym_count;
+
+      /* Remove the symbols that were in the swept sections from the
+	 dynamic symbol table.  GCFIXME: Anyone know how to get them
+	 out of the static symbol table as well?  */
+      sweep_info.info = info;
+      sweep_info.hide_symbol = bed->elf_backend_hide_symbol;
+      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);
+    }
+
+  /* Any syms created from now on start with -1 in
+     got.refcount/offset and plt.refcount/offset.  */
+  elf_hash_table (info)->init_got_refcount
+    = elf_hash_table (info)->init_got_offset;
+  elf_hash_table (info)->init_plt_refcount
+    = elf_hash_table (info)->init_plt_offset;
+
+  if (bfd_link_relocatable (info)
+      && !_bfd_elf_size_group_sections (info))
+    return FALSE;
+
+  /* The backend may have to create some sections regardless of whether
+     we're dynamic or not.  */
+  if (bed->elf_backend_always_size_sections
+      && ! (*bed->elf_backend_always_size_sections) (output_bfd, info))
+    return FALSE;
+
+  /* Determine any GNU_STACK segment requirements, after the backend
+     has had a chance to set a default segment size.  */
+  if (info->execstack)
+    elf_stack_flags (output_bfd) = PF_R | PF_W | PF_X;
+  else if (info->noexecstack)
+    elf_stack_flags (output_bfd) = PF_R | PF_W;
+  else
+    {
+      bfd *inputobj;
+      asection *notesec = NULL;
+      int exec = 0;
+
+      for (inputobj = info->input_bfds;
+	   inputobj;
+	   inputobj = inputobj->link.next)
+	{
+	  asection *s;
+
+	  if (inputobj->flags
+	      & (DYNAMIC | EXEC_P | BFD_PLUGIN | BFD_LINKER_CREATED))
+	    continue;
+	  s = bfd_get_section_by_name (inputobj, ".note.GNU-stack");
+	  if (s)
+	    {
+	      if (s->flags & SEC_CODE)
+		exec = PF_X;
+	      notesec = s;
+	    }
+	  else if (bed->default_execstack)
+	    exec = PF_X;
+	}
+      if (notesec || info->stacksize > 0)
+	elf_stack_flags (output_bfd) = PF_R | PF_W | exec;
+      if (notesec && exec && bfd_link_relocatable (info)
+	  && notesec->output_section != bfd_abs_section_ptr)
+	notesec->output_section->flags |= SEC_CODE;
+    }
+
+  if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
+    {
+      struct elf_info_failed eif;
+      struct elf_link_hash_entry *h;
+      asection *dynstr;
+      asection *s;
+
+      *sinterpptr = bfd_get_linker_section (dynobj, ".interp");
+      BFD_ASSERT (*sinterpptr != NULL || !bfd_link_executable (info) || info->nointerp);
+
+      if (soname != NULL)
+	{
+	  soname_indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
+					     soname, TRUE);
+	  if (soname_indx == (size_t) -1
+	      || !_bfd_elf_add_dynamic_entry (info, DT_SONAME, soname_indx))
+	    return FALSE;
+	}
+
+      if (info->symbolic)
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_SYMBOLIC, 0))
+	    return FALSE;
+	  info->flags |= DF_SYMBOLIC;
+	}
+
+      if (rpath != NULL)
+	{
+	  size_t indx;
+	  bfd_vma tag;
+
+	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr, rpath,
+				      TRUE);
+	  if (indx == (size_t) -1)
+	    return FALSE;
+
+	  tag = info->new_dtags ? DT_RUNPATH : DT_RPATH;
+	  if (!_bfd_elf_add_dynamic_entry (info, tag, indx))
+	    return FALSE;
+	}
+
+      if (filter_shlib != NULL)
+	{
+	  size_t indx;
+
+	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
+				      filter_shlib, TRUE);
+	  if (indx == (size_t) -1
+	      || !_bfd_elf_add_dynamic_entry (info, DT_FILTER, indx))
+	    return FALSE;
+	}
+
+      if (auxiliary_filters != NULL)
+	{
+	  const char * const *p;
+
+	  for (p = auxiliary_filters; *p != NULL; p++)
+	    {
+	      size_t indx;
+
+	      indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr,
+					  *p, TRUE);
+	      if (indx == (size_t) -1
+		  || !_bfd_elf_add_dynamic_entry (info, DT_AUXILIARY, indx))
+		return FALSE;
+	    }
+	}
+
+      if (audit != NULL)
+	{
+	  size_t indx;
+
+	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr, audit,
+				      TRUE);
+	  if (indx == (size_t) -1
+	      || !_bfd_elf_add_dynamic_entry (info, DT_AUDIT, indx))
+	    return FALSE;
+	}
+
+      if (depaudit != NULL)
+	{
+	  size_t indx;
+
+	  indx = _bfd_elf_strtab_add (elf_hash_table (info)->dynstr, depaudit,
+				      TRUE);
+	  if (indx == (size_t) -1
+	      || !_bfd_elf_add_dynamic_entry (info, DT_DEPAUDIT, indx))
+	    return FALSE;
+	}
+
+      eif.info = info;
+      eif.failed = FALSE;
+
+      /* If we are supposed to export all symbols into the dynamic symbol
+	 table (this is not the normal case), then do so.  */
+      if (info->export_dynamic
+	  || (bfd_link_executable (info) && info->dynamic))
+	{
+	  elf_link_hash_traverse (elf_hash_table (info),
+				  _bfd_elf_export_symbol,
+				  &eif);
+	  if (eif.failed)
+	    return FALSE;
+	}
+
+      /* Find all symbols which were defined in a dynamic object and make
+	 the backend pick a reasonable value for them.  */
+      elf_link_hash_traverse (elf_hash_table (info),
+			      _bfd_elf_adjust_dynamic_symbol,
+			      &eif);
+      if (eif.failed)
+	return FALSE;
+
+      /* Add some entries to the .dynamic section.  We fill in some of the
+	 values later, in bfd_elf_final_link, but we must add the entries
+	 now so that we know the final size of the .dynamic section.  */
+
+      /* If there are initialization and/or finalization functions to
+	 call then add the corresponding DT_INIT/DT_FINI entries.  */
+      h = (info->init_function
+	   ? elf_link_hash_lookup (elf_hash_table (info),
+				   info->init_function, FALSE,
+				   FALSE, FALSE)
+	   : NULL);
+      if (h != NULL
+	  && (h->ref_regular
+	      || h->def_regular))
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_INIT, 0))
+	    return FALSE;
+	}
+      h = (info->fini_function
+	   ? elf_link_hash_lookup (elf_hash_table (info),
+				   info->fini_function, FALSE,
+				   FALSE, FALSE)
+	   : NULL);
+      if (h != NULL
+	  && (h->ref_regular
+	      || h->def_regular))
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_FINI, 0))
+	    return FALSE;
+	}
+
+      s = bfd_get_section_by_name (output_bfd, ".preinit_array");
+      if (s != NULL && s->linker_has_input)
+	{
+	  /* DT_PREINIT_ARRAY is not allowed in shared library.  */
+	  if (! bfd_link_executable (info))
+	    {
+	      bfd *sub;
+	      asection *o;
+
+	      for (sub = info->input_bfds; sub != NULL;
+		   sub = sub->link.next)
+		if (bfd_get_flavour (sub) == bfd_target_elf_flavour)
+		  for (o = sub->sections; o != NULL; o = o->next)
+		    if (elf_section_data (o)->this_hdr.sh_type
+			== SHT_PREINIT_ARRAY)
+		      {
+			_bfd_error_handler
+			  (_("%B: .preinit_array section is not allowed in DSO"),
+			   sub);
+			break;
+		      }
+
+	      bfd_set_error (bfd_error_nonrepresentable_section);
+	      return FALSE;
+	    }
+
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_PREINIT_ARRAY, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_PREINIT_ARRAYSZ, 0))
+	    return FALSE;
+	}
+      s = bfd_get_section_by_name (output_bfd, ".init_array");
+      if (s != NULL && s->linker_has_input)
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_INIT_ARRAY, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_INIT_ARRAYSZ, 0))
+	    return FALSE;
+	}
+      s = bfd_get_section_by_name (output_bfd, ".fini_array");
+      if (s != NULL && s->linker_has_input)
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_FINI_ARRAY, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_FINI_ARRAYSZ, 0))
+	    return FALSE;
+	}
+
+      dynstr = bfd_get_linker_section (dynobj, ".dynstr");
+      /* If .dynstr is excluded from the link, we don't want any of
+	 these tags.  Strictly, we should be checking each section
+	 individually;  This quick check covers for the case where
+	 someone does a /DISCARD/ : { *(*) }.  */
+      if (dynstr != NULL && dynstr->output_section != bfd_abs_section_ptr)
+	{
+	  bfd_size_type strsize;
+
+	  strsize = _bfd_elf_strtab_size (elf_hash_table (info)->dynstr);
+	  if ((info->emit_hash
+	       && !_bfd_elf_add_dynamic_entry (info, DT_HASH, 0))
+	      || (info->emit_gnu_hash
+		  && !_bfd_elf_add_dynamic_entry (info, DT_GNU_HASH, 0))
+	      || !_bfd_elf_add_dynamic_entry (info, DT_STRTAB, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_SYMTAB, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_STRSZ, strsize)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_SYMENT,
+					      bed->s->sizeof_sym))
+	    return FALSE;
+	}
+    }
+
+  if (! _bfd_elf_maybe_strip_eh_frame_hdr (info))
+    return FALSE;
+
+  /* The backend must work out the sizes of all the other dynamic
+     sections.  */
+  if (dynobj != NULL
+      && bed->elf_backend_size_dynamic_sections != NULL
+      && ! (*bed->elf_backend_size_dynamic_sections) (output_bfd, info))
+    return FALSE;
+
+  if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
+    {
+      if (elf_tdata (output_bfd)->cverdefs)
+	{
+	  unsigned int crefs = elf_tdata (output_bfd)->cverdefs;
+
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_VERDEF, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_VERDEFNUM, crefs))
+	    return FALSE;
+	}
+
+      if ((info->new_dtags && info->flags) || (info->flags & DF_STATIC_TLS))
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_FLAGS, info->flags))
+	    return FALSE;
+	}
+      else if (info->flags & DF_BIND_NOW)
+	{
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_BIND_NOW, 0))
+	    return FALSE;
+	}
+
+      if (info->flags_1)
+	{
+	  if (bfd_link_executable (info))
+	    info->flags_1 &= ~ (DF_1_INITFIRST
+				| DF_1_NODELETE
+				| DF_1_NOOPEN);
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_FLAGS_1, info->flags_1))
+	    return FALSE;
+	}
+
+      if (elf_tdata (output_bfd)->cverrefs)
+	{
+	  unsigned int crefs = elf_tdata (output_bfd)->cverrefs;
+
+	  if (!_bfd_elf_add_dynamic_entry (info, DT_VERNEED, 0)
+	      || !_bfd_elf_add_dynamic_entry (info, DT_VERNEEDNUM, crefs))
+	    return FALSE;
+	}
+    }
   return TRUE;
 }
 
Index: binutils/ld/testsuite/ld-elf/pr20828-d.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828-d.sd	2017-01-31 14:46:30.926958068 +0000
@@ -0,0 +1,9 @@
+# Make sure `vdata' is global rather than local in the dynamic symbol table,
+# e.g.:
+#    Num:    Value  Size Type    Bind   Vis      Ndx Name
+#      1: 00000000     0 OBJECT  GLOBAL DEFAULT  ABS vdata
+# vs:
+#      1: 00000000     0 OBJECT  LOCAL  DEFAULT  ABS vdata
+#...
+ *[0-9]+: +[0-9a-f]+ +0 +OBJECT +GLOBAL +DEFAULT +ABS +vdata
+#pass
Index: binutils/ld/testsuite/ld-elf/pr20828-e.sd
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828-e.sd	2017-01-31 15:16:59.078917392 +0000
@@ -0,0 +1,9 @@
+# Make sure `edata' is global rather than local in the dynamic symbol table,
+# e.g.:
+#    Num:    Value  Size Type    Bind   Vis      Ndx Name
+#      1: 00000000     0 NOTYPE  GLOBAL DEFAULT    1 edata@@vdata
+# vs:
+#      1: 00000000     0 NOTYPE  LOCAL  DEFAULT    1 edata@@vdata
+#...
+ *[0-9]+: +[0-9a-f]+ +0 +NOTYPE +GLOBAL +DEFAULT +[0-9]+ +edata@@vdata
+#pass
Index: binutils/ld/testsuite/ld-elf/pr20828-v.ver
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828-v.ver	2017-01-31 14:46:31.242435021 +0000
@@ -0,0 +1 @@
+vdata { global: edata; local: *; };
Index: binutils/ld/testsuite/ld-elf/pr20828.ld
===================================================================
--- binutils.orig/ld/testsuite/ld-elf/pr20828.ld	2017-01-31 14:46:26.169112837 +0000
+++ binutils/ld/testsuite/ld-elf/pr20828.ld	2017-01-31 14:46:31.411858383 +0000
@@ -10,6 +10,8 @@ SECTIONS
   .hash : { *(.hash) }
   .dynsym : { *(.dynsym) }
   .dynstr : { *(.dynstr) }
+  .gnu.version : { *(.gnu.version) }
+  .gnu.version_d : { *(.gnu.version_d) }
   .shstrtab : { *(.shstrtab) }
   .symtab : { *(.symtab) }
   .strtab : { *(.strtab) }
Index: binutils/ld/testsuite/ld-elf/shared.exp
===================================================================
--- binutils.orig/ld/testsuite/ld-elf/shared.exp	2017-01-31 14:46:26.528721361 +0000
+++ binutils/ld/testsuite/ld-elf/shared.exp	2017-01-31 15:19:08.139588324 +0000
@@ -77,7 +77,30 @@ if { [check_gc_sections_available] } {
 	    {pr20828.s} \
 	    {{readelf --dyn-syms pr20828-b.sd} \
 	     {readelf --dyn-syms pr20828-c.sd}} \
-	    "pr20828-2.so"]]
+	    "pr20828-2.so"] \
+	[list \
+	    "PR ld/20828 dynamic symbols with section GC\
+	     (versioned shared library)" \
+	    "$LFLAGS -shared --gc-sections -T pr20828.ld\
+	     --version-script=pr20828-v.ver" \
+	    "" "$AFLAGS_PIC" \
+	    {pr20828.s} \
+	    {{readelf --dyn-syms pr20828-c.sd} \
+	     {readelf --dyn-syms pr20828-d.sd} \
+	     {readelf --dyn-syms pr20828-e.sd}} \
+	    "libpr20828-v.so"] \
+	[list \
+	    "PR ld/20828 dynamic symbols with section GC (versioned)" \
+	    "$LFLAGS -shared --gc-sections -T pr20828.ld\
+	     --version-script=pr20828-v.ver" \
+	    "tmpdir/libpr20828-v.so" \
+	    "$AFLAGS_PIC" \
+	    {pr20828.s} \
+	    {{readelf --dyn-syms pr20828-c.sd} \
+	     {readelf --dyn-syms pr20828-d.sd} \
+	     {readelf --dyn-syms pr20828-e.sd}} \
+	    "pr20828-v.so"] \
+	]
 }
 
 # Check to see if the C compiler works

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-01 19:31 [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep Maciej W. Rozycki
@ 2017-02-02  3:25 ` Alan Modra
  2017-02-08 18:19   ` Maciej W. Rozycki
  0 siblings, 1 reply; 10+ messages in thread
From: Alan Modra @ 2017-02-02  3:25 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Nick Clifton, James Cowgill, binutils

I think you may be in trouble here:

      if ((elf_tdata (output_bfd)->cverrefs == 0
	   && elf_tdata (output_bfd)->cverdefs == 0)
	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
					     &section_sym_count) == 0)
	{
	  s = bfd_get_linker_section (dynobj, ".gnu.version");
	  s->flags |= SEC_EXCLUDE;
	}

The problem being that _bfd_elf_export_symbol and the backend
size_dynamic_sections might make symbols dynamic where there were
possibly none before.  Garbage collection also might change whether
there are dynamic symbols.

Another problem related to dynamic symbols is that running
_bfd_elf_link_assign_sym_version early means the tests of h->dynindx
in that function are no longer valid.

-- 
Alan Modra
Australia Development Lab, IBM

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-02  3:25 ` Alan Modra
@ 2017-02-08 18:19   ` Maciej W. Rozycki
  2017-02-09 19:49     ` Maciej W. Rozycki
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-02-08 18:19 UTC (permalink / raw)
  To: Alan Modra; +Cc: Nick Clifton, James Cowgill, binutils

Hi Alan,

 Thanks for your input!

> I think you may be in trouble here:
> 
>       if ((elf_tdata (output_bfd)->cverrefs == 0
> 	   && elf_tdata (output_bfd)->cverdefs == 0)
> 	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
> 					     &section_sym_count) == 0)
> 	{
> 	  s = bfd_get_linker_section (dynobj, ".gnu.version");
> 	  s->flags |= SEC_EXCLUDE;
> 	}
> 
> The problem being that _bfd_elf_export_symbol and the backend
> size_dynamic_sections might make symbols dynamic where there were
> possibly none before.  Garbage collection also might change whether
> there are dynamic symbols.

 Good point, and fixed easily, by leaving this piece of code in its 
original place.  A minor update is required because the declaration of `s' 
has been moved out of the scope of this block, so another one is required 
to keep it local (I've decided that setting `->flags' directly without the 
use of an auxiliary variable would impact the readability of this piece of 
code).

> Another problem related to dynamic symbols is that running
> _bfd_elf_link_assign_sym_version early means the tests of h->dynindx
> in that function are no longer valid.

 Thanks.  There are two such places AFAICT.

 First:

	      /* See if there is anything to force this symbol to
		 local scope.  */
	      if (d == NULL && t->locals.list != NULL)
		{
		  d = (*t->match) (&t->locals, NULL, alc);
		  if (d != NULL
		      && h->dynindx != -1
		      && ! info->export_dynamic)
		    (*bed->elf_backend_hide_symbol) (info, h, TRUE);
		}

I think actually is not a problem, because the symbol sweep will not give 
a symbol a new dynsym table entry if it doesn't have one already.  It can 
reassign an entry via `_bfd_elf_link_renumber_dynsyms', but this is all 
right because we do not care about the exact index value here.  It can 
also take an entry away, but this is not an issue either as in that case, 
likewise, it'll call `->elf_backend_hide_symbol', so it doesn't really 
matter which of the two happens first.  So there is no need to change 
anything here AFAICT.

 Second:

      /* If we are building an application, we need to create a
	 version node for this version.  */
      if (t == NULL && bfd_link_executable (info))
	{
	  struct bfd_elf_version_tree **pp;
	  int version_index;

	  /* If we aren't going to export this symbol, we don't need
	     to worry about it.  */
	  if (h->dynindx == -1)
	    return TRUE;

	  t = (struct bfd_elf_version_tree *) bfd_zalloc (info->output_bfd,
							  sizeof *t);
	  if (t == NULL)
	    {
	      sinfo->failed = TRUE;
	      return FALSE;
	    }

	  t->name = p;
	  t->name_indx = (unsigned int) -1;
	  t->used = TRUE;

	  version_index = 1;
	  /* Don't count anonymous version tag.  */
	  if (sinfo->info->version_info != NULL
	      && sinfo->info->version_info->vernum == 0)
	    version_index = 0;
	  for (pp = &sinfo->info->version_info;
	       *pp != NULL;
	       pp = &(*pp)->next)
	    ++version_index;
	  t->vernum = version_index;

	  *pp = t;

	  h->verinfo.vertree = t;
	}

is a bit more complex, because of the opposite condition.  With code as it 
stands a symbol may have already been swept and its dynsym table entry 
assignment removed, while with the change I have proposed the assignment 
will still be there, pending removal, and a version node will be created 
while it shouldn't.  Again the exact index value does not matter here.

 So I propose that we check at this point if the symbol is going to be 
eventually swept and do not create a version node in that case.  This 
could become a problem if `->mark' was set for a symbol after this point 
and before the symbol sweep later on, but we have no code doing so 
currently AFAICT and we can just declare it a no-no.

 Any flaws in my reasoning?

 FAOD here is an incremental update which implements all of this while 
keeping regression test results the same.  If you agree that this is the 
right direction, then I'll post v2 of the whole change.  I think I can 
invent a test suite case for the latter case as well, so I'll bundle that 
too.

  Maciej

binutils-bfd-elf-size-dynamic-sections-version-update.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-02-08 09:45:02.000000000 +0000
+++ binutils/bfd/elflink.c	2017-02-08 14:41:46.036973024 +0000
@@ -2051,6 +2051,21 @@ _bfd_elf_export_symbol (struct elf_link_
   return TRUE;
 }
 \f
+/* Return TRUE if the symbol identified by H qualifies for removal
+   in section garbage collection.  */
+
+static bfd_boolean
+elf_gc_symbol_eligible_p (struct elf_link_hash_entry *h)
+{
+  return (!h->mark
+	  && (((h->root.type == bfd_link_hash_defined
+		|| h->root.type == bfd_link_hash_defweak)
+	       && !((h->def_regular || ELF_COMMON_DEF_P (h))
+		    && h->root.u.def.section->gc_mark))
+	      || h->root.type == bfd_link_hash_undefined
+	      || h->root.type == bfd_link_hash_undefweak));
+}
+\f
 /* Look through the symbols which are defined in other shared
    libraries and referenced here.  Update the list of version
    dependencies.  This will be put into the .gnu.version_r section.
@@ -2233,7 +2248,10 @@ _bfd_elf_link_assign_sym_version (struct
 
 	  /* If we aren't going to export this symbol, we don't need
 	     to worry about it.  */
-	  if (h->dynindx == -1)
+	  if (h->dynindx == -1
+	      || (info->gc_sections
+		  && bed->can_gc_sections
+		  && elf_gc_symbol_eligible_p (h)))
 	    return TRUE;
 
 	  t = (struct bfd_elf_version_tree *) bfd_zalloc (info->output_bfd,
@@ -5886,13 +5904,7 @@ struct elf_gc_sweep_symbol_info
 static bfd_boolean
 elf_gc_sweep_symbol (struct elf_link_hash_entry *h, void *data)
 {
-  if (!h->mark
-      && (((h->root.type == bfd_link_hash_defined
-	    || h->root.type == bfd_link_hash_defweak)
-	   && !((h->def_regular || ELF_COMMON_DEF_P (h))
-		&& h->root.u.def.section->gc_mark))
-	  || h->root.type == bfd_link_hash_undefined
-	  || h->root.type == bfd_link_hash_undefweak))
+  if (elf_gc_symbol_eligible_p (h))
     {
       struct elf_gc_sweep_symbol_info *inf;
 
@@ -5938,7 +5950,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
       struct bfd_elf_version_tree *verdefs;
-      unsigned long section_sym_count;
       struct elf_info_failed asvinfo;
       struct bfd_elf_version_tree *t;
       struct bfd_elf_version_expr *d;
@@ -6368,15 +6379,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	    elf_tdata (output_bfd)->cverrefs = crefs;
 	  }
       }
-
-      if ((elf_tdata (output_bfd)->cverrefs == 0
-	   && elf_tdata (output_bfd)->cverdefs == 0)
-	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
-					     &section_sym_count) == 0)
-	{
-	  s = bfd_get_linker_section (dynobj, ".gnu.version");
-	  s->flags |= SEC_EXCLUDE;
-	}
     }
 
   bed = get_elf_backend_data (output_bfd);
@@ -6676,6 +6678,8 @@ bfd_elf_size_dynamic_sections (bfd *outp
 
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
+      unsigned long section_sym_count;
+
       if (elf_tdata (output_bfd)->cverdefs)
 	{
 	  unsigned int crefs = elf_tdata (output_bfd)->cverdefs;
@@ -6714,6 +6718,17 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	      || !_bfd_elf_add_dynamic_entry (info, DT_VERNEEDNUM, crefs))
 	    return FALSE;
 	}
+
+      if ((elf_tdata (output_bfd)->cverrefs == 0
+	   && elf_tdata (output_bfd)->cverdefs == 0)
+	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+					     &section_sym_count) == 0)
+	{
+	  asection *s;
+
+	  s = bfd_get_linker_section (dynobj, ".gnu.version");
+	  s->flags |= SEC_EXCLUDE;
+	}
     }
   return TRUE;
 }

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-08 18:19   ` Maciej W. Rozycki
@ 2017-02-09 19:49     ` Maciej W. Rozycki
  2017-02-15  1:55       ` Alan Modra
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-02-09 19:49 UTC (permalink / raw)
  To: Alan Modra; +Cc: Nick Clifton, James Cowgill, binutils

On Wed, 8 Feb 2017, Maciej W. Rozycki wrote:

>  Second:
> 
>       /* If we are building an application, we need to create a
> 	 version node for this version.  */
>       if (t == NULL && bfd_link_executable (info))
> 	{
> 	  struct bfd_elf_version_tree **pp;
> 	  int version_index;
> 
> 	  /* If we aren't going to export this symbol, we don't need
> 	     to worry about it.  */
> 	  if (h->dynindx == -1)
> 	    return TRUE;
> 
> 	  t = (struct bfd_elf_version_tree *) bfd_zalloc (info->output_bfd,
> 							  sizeof *t);
> 	  if (t == NULL)
> 	    {
> 	      sinfo->failed = TRUE;
> 	      return FALSE;
> 	    }
> 
> 	  t->name = p;
> 	  t->name_indx = (unsigned int) -1;
> 	  t->used = TRUE;
> 
> 	  version_index = 1;
> 	  /* Don't count anonymous version tag.  */
> 	  if (sinfo->info->version_info != NULL
> 	      && sinfo->info->version_info->vernum == 0)
> 	    version_index = 0;
> 	  for (pp = &sinfo->info->version_info;
> 	       *pp != NULL;
> 	       pp = &(*pp)->next)
> 	    ++version_index;
> 	  t->vernum = version_index;
> 
> 	  *pp = t;
> 
> 	  h->verinfo.vertree = t;
> 	}
> 
> is a bit more complex, because of the opposite condition.  With code as it 
> stands a symbol may have already been swept and its dynsym table entry 
> assignment removed, while with the change I have proposed the assignment 
> will still be there, pending removal, and a version node will be created 
> while it shouldn't.  Again the exact index value does not matter here.

 A quick update.  I have been trying to write a test case for this 
execution path and I seem to be unable to, making me suspect that the 
interaction between `->dynindx' and the symbol sweep does not actually 
matter here.

 The thing is this conditional only ever goes through for double-at `@@'
versioned symbols, such as with this example source:

	.globl	_adata
	.symver	_adata, _idata@@_vdata
_adata:

This is because:

1. The symbol has to be versioned in the incoming object due to:

  p = strchr (h->root.root.string, ELF_VER_CHR);
  if (p != NULL && h->verinfo.vertree == NULL)

   earlier on in `_bfd_elf_link_assign_sym_version', i.e. there has to be 
   an `@' in the name.

2. The symbol must have been defined locally, due to:

  if (!h->def_regular)
    return TRUE;

   earlier on in `_bfd_elf_link_assign_sym_version'.

3. We must be linking an executable rather than a DSO due to:

   if (t == NULL && bfd_link_executable (info))

   at the outer condition, quoted above.

4. The symbol must not be hidden for `->dynindx' to be non-minus-one:

	  if (h->dynindx == -1)
	    return TRUE;

   so `_bfd_elf_add_default_symbol' must have set `->versioned = 
   versioned' according to:

  p = strchr (name, ELF_VER_CHR);
  if (h->versioned == unknown)
    {
      if (p == NULL)
	{
	  h->versioned = unversioned;
	  return TRUE;
	}
      else
	{
	  if (p[1] != ELF_VER_CHR)
	    {
	      h->versioned = versioned_hidden;
	      return TRUE;
	    }
	  else
	    h->versioned = versioned;
	}
    }

   i.e. there has to be `@@' there or `_bfd_elf_fix_symbol_flags' called 
   earlier on in `_bfd_elf_link_assign_sym_version' would have hidden the
   symbol:

  else if (bfd_link_executable (eif->info)
	   && h->versioned == versioned_hidden
	   && !eif->info->export_dynamic
	   && !h->dynamic
	   && !h->ref_dynamic
	   && h->def_regular)
    (*bed->elf_backend_hide_symbol) (eif->info, h, TRUE);

   that sets `->dynindx = -1' unless the symbol has been forcifully 
   exported or is referenced by a DSO, neither in which case it can be 
   garbage-collected.

So it looks to me like for the order between this code and the symbol 
sweep to matter these conditions all at once have to be met:

1. Executable is being linked.

2. Symbol has been defined locally.

3. Symbol is being dynamically exported.

4. Symbol will be garbage-collected.

however #4 cannot happen together with #1-#3 at once, because meeting 
#1-#3 means the symbol cannot be garbage-collected.  IOW, if we get to 
here, then either `->dynindx == -1' and we don't care or `->dynindx != -1' 
and this won't change.  So it looks like most of my update can be dropped 
and only the part below retained.

 Have I missed anything here?

 NB version nodes seem to be always propagated to output even if their 
originating symbols have been removed, e.g. by being forced local with an 
anonymous version script, regardless of whether section garbage collection 
has been enabled or not.  So in any case it looks to me like we would be 
trying to be too smart here by making efforts not to create version nodes.  
Perhaps letting them through to output can be considered a preexisting bug 
however.

  Maciej

binutils-bfd-elf-size-dynamic-sections-version-update-2.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-02-09 05:24:05.743124850 +0000
+++ binutils/bfd/elflink.c	2017-02-09 19:36:48.153651299 +0000
@@ -5938,7 +5938,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
       struct bfd_elf_version_tree *verdefs;
-      unsigned long section_sym_count;
       struct elf_info_failed asvinfo;
       struct bfd_elf_version_tree *t;
       struct bfd_elf_version_expr *d;
@@ -6368,15 +6367,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	    elf_tdata (output_bfd)->cverrefs = crefs;
 	  }
       }
-
-      if ((elf_tdata (output_bfd)->cverrefs == 0
-	   && elf_tdata (output_bfd)->cverdefs == 0)
-	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
-					     &section_sym_count) == 0)
-	{
-	  s = bfd_get_linker_section (dynobj, ".gnu.version");
-	  s->flags |= SEC_EXCLUDE;
-	}
     }
 
   bed = get_elf_backend_data (output_bfd);
@@ -6676,6 +6666,8 @@ bfd_elf_size_dynamic_sections (bfd *outp
 
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
+      unsigned long section_sym_count;
+
       if (elf_tdata (output_bfd)->cverdefs)
 	{
 	  unsigned int crefs = elf_tdata (output_bfd)->cverdefs;
@@ -6714,6 +6706,17 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	      || !_bfd_elf_add_dynamic_entry (info, DT_VERNEEDNUM, crefs))
 	    return FALSE;
 	}
+
+      if ((elf_tdata (output_bfd)->cverrefs == 0
+	   && elf_tdata (output_bfd)->cverdefs == 0)
+	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+					     &section_sym_count) == 0)
+	{
+	  asection *s;
+
+	  s = bfd_get_linker_section (dynobj, ".gnu.version");
+	  s->flags |= SEC_EXCLUDE;
+	}
     }
   return TRUE;
 }

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-09 19:49     ` Maciej W. Rozycki
@ 2017-02-15  1:55       ` Alan Modra
  2017-02-15  3:43         ` Alan Modra
  0 siblings, 1 reply; 10+ messages in thread
From: Alan Modra @ 2017-02-15  1:55 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Nick Clifton, James Cowgill, binutils

On Thu, Feb 09, 2017 at 07:48:50PM +0000, Maciej W. Rozycki wrote:
> 1. Executable is being linked.
> 
> 2. Symbol has been defined locally.
> 
> 3. Symbol is being dynamically exported.
> 
> 4. Symbol will be garbage-collected.
> 
> however #4 cannot happen together with #1-#3 at once, because meeting 
> #1-#3 means the symbol cannot be garbage-collected.  IOW, if we get to 
> here, then either `->dynindx == -1' and we don't care or `->dynindx != -1' 
> and this won't change.  So it looks like most of my update can be dropped 
> and only the part below retained.
> 
>  Have I missed anything here?

I think you are correct, and this update makes the original patch OK
to apply.  I'm willing to approve it.

-- 
Alan Modra
Australia Development Lab, IBM

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-15  1:55       ` Alan Modra
@ 2017-02-15  3:43         ` Alan Modra
  2017-02-15 16:39           ` Maciej W. Rozycki
  0 siblings, 1 reply; 10+ messages in thread
From: Alan Modra @ 2017-02-15  3:43 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Nick Clifton, James Cowgill, binutils

On Wed, Feb 15, 2017 at 12:24:51PM +1030, Alan Modra wrote:
> On Thu, Feb 09, 2017 at 07:48:50PM +0000, Maciej W. Rozycki wrote:
> > 1. Executable is being linked.
> > 
> > 2. Symbol has been defined locally.
> > 
> > 3. Symbol is being dynamically exported.
> > 
> > 4. Symbol will be garbage-collected.
> > 
> > however #4 cannot happen together with #1-#3 at once, because meeting 
> > #1-#3 means the symbol cannot be garbage-collected.  IOW, if we get to 
> > here, then either `->dynindx == -1' and we don't care or `->dynindx != -1' 
> > and this won't change.  So it looks like most of my update can be dropped 
> > and only the part below retained.
> > 
> >  Have I missed anything here?
> 
> I think you are correct, and this update makes the original patch OK
> to apply.  I'm willing to approve it.

So I went and tested the patch, and found failures on x86_64 and other
architectures.

FAIL: Build rdynamic-1
FAIL: vers4a
FAIL: vers9

This says the patch is not OK.

-- 
Alan Modra
Australia Development Lab, IBM

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-15  3:43         ` Alan Modra
@ 2017-02-15 16:39           ` Maciej W. Rozycki
  2017-02-15 23:34             ` Alan Modra
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-02-15 16:39 UTC (permalink / raw)
  To: Alan Modra; +Cc: Nick Clifton, James Cowgill, binutils

On Wed, 15 Feb 2017, Alan Modra wrote:

> So I went and tested the patch, and found failures on x86_64 and other
> architectures.

 Umm, I didn't think about native/compiled testing, sorry.  I have got 
myself set up for x86_64-linux native testing now and...

> FAIL: Build rdynamic-1

... I can't reproduce this...

> FAIL: vers4a
> FAIL: vers9

... but I can see these, so I'll start from here and will also work on 
adding versions of these that do not require a compiler or system 
libraries, so that we have coverage for configurations that do not usually 
receive native/compiled testing.

 Thanks for verifying my change.

  Maciej

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-15 16:39           ` Maciej W. Rozycki
@ 2017-02-15 23:34             ` Alan Modra
  2017-02-20 19:26               ` Maciej W. Rozycki
  0 siblings, 1 reply; 10+ messages in thread
From: Alan Modra @ 2017-02-15 23:34 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: binutils

On Wed, Feb 15, 2017 at 04:38:46PM +0000, Maciej W. Rozycki wrote:
> ... I can't reproduce this...

valgrind reports uninitialized writes, so some luck is involved in
getting the test to fail.

If I put MALLOC_PERTURB_=1 in my environment then the test passes but
readelf -s --wide tmpdir/rdynamic-1 shows:

Symbol table '.dynsym' contains 18 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __libc_start_main@GLIBC_2.2.5 (2)
     2: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND __gmon_start__
     3: 0000000000601024     0 NOTYPE  GLOBAL DEFAULT   25 _edata
     4: 0000000000601020     0 NOTYPE  GLOBAL DEFAULT   25 __data_start
     5: fefefefefefefefe 0xfefefefefefefefe <processor specific>: 14 <processor specific>: 15 HIDDEN  [<other>: fc]  bad section index[65278] <corrupt>
     6: 0000000000601028     0 NOTYPE  GLOBAL DEFAULT   26 _end
     7: 0000000000601020     0 NOTYPE  WEAK   DEFAULT   25 data_start
     8: 00000000004007e0     4 OBJECT  GLOBAL DEFAULT   16 _IO_stdin_used
     9: 0000000000400760   101 FUNC    GLOBAL DEFAULT   14 __libc_csu_init
    10: 0000000000400650    42 FUNC    GLOBAL DEFAULT   14 _start
    11: 0000000000601024     0 NOTYPE  GLOBAL DEFAULT   26 __bss_start
    12: 0000000000400640     3 FUNC    GLOBAL DEFAULT   14 main
    13: fefefefefefefefe 0xfefefefefefefefe <processor specific>: 14 <processor specific>: 15 HIDDEN  [<other>: fc]  bad section index[65278] <corrupt>
    14: 00000000004005f0     0 FUNC    GLOBAL DEFAULT   11 _init
    15: 00000000004007d0     2 FUNC    GLOBAL DEFAULT   14 __libc_csu_fini
    16: 00000000004007d4     0 FUNC    GLOBAL DEFAULT   15 _fini
    17: 0000000000400750     2 FUNC    GLOBAL DEFAULT   14 rdynamic

-- 
Alan Modra
Australia Development Lab, IBM

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-15 23:34             ` Alan Modra
@ 2017-02-20 19:26               ` Maciej W. Rozycki
  2017-02-21 22:50                 ` Alan Modra
  0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-02-20 19:26 UTC (permalink / raw)
  To: Alan Modra; +Cc: binutils

On Wed, 15 Feb 2017, Alan Modra wrote:

> > ... I can't reproduce this...
> 
> valgrind reports uninitialized writes, so some luck is involved in
> getting the test to fail.
> 
> If I put MALLOC_PERTURB_=1 in my environment then the test passes but
> readelf -s --wide tmpdir/rdynamic-1 shows:
> 
> Symbol table '.dynsym' contains 18 entries:
>    Num:    Value          Size Type    Bind   Vis      Ndx Name
>      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
>      1: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __libc_start_main@GLIBC_2.2.5 (2)
>      2: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND __gmon_start__
>      3: 0000000000601024     0 NOTYPE  GLOBAL DEFAULT   25 _edata
>      4: 0000000000601020     0 NOTYPE  GLOBAL DEFAULT   25 __data_start
>      5: fefefefefefefefe 0xfefefefefefefefe <processor specific>: 14 <processor specific>: 15 HIDDEN  [<other>: fc]  bad section index[65278] <corrupt>
>      6: 0000000000601028     0 NOTYPE  GLOBAL DEFAULT   26 _end
>      7: 0000000000601020     0 NOTYPE  WEAK   DEFAULT   25 data_start
>      8: 00000000004007e0     4 OBJECT  GLOBAL DEFAULT   16 _IO_stdin_used
>      9: 0000000000400760   101 FUNC    GLOBAL DEFAULT   14 __libc_csu_init
>     10: 0000000000400650    42 FUNC    GLOBAL DEFAULT   14 _start
>     11: 0000000000601024     0 NOTYPE  GLOBAL DEFAULT   26 __bss_start
>     12: 0000000000400640     3 FUNC    GLOBAL DEFAULT   14 main
>     13: fefefefefefefefe 0xfefefefefefefefe <processor specific>: 14 <processor specific>: 15 HIDDEN  [<other>: fc]  bad section index[65278] <corrupt>
>     14: 00000000004005f0     0 FUNC    GLOBAL DEFAULT   11 _init
>     15: 00000000004007d0     2 FUNC    GLOBAL DEFAULT   14 __libc_csu_fini
>     16: 00000000004007d4     0 FUNC    GLOBAL DEFAULT   15 _fini
>     17: 0000000000400750     2 FUNC    GLOBAL DEFAULT   14 rdynamic

 This must be something specific to your environment (or mine), which is 
an obvious shortcoming (or, depending on how you look at it, a benefit) of 
compiled tests -- in that results will inevitably depend on external 
components such as target libc included in the build of individual test 
cases (the shortcoming is the lack of universal reproducibility, while the 
benefit is the diversity of test environments giving a higher chance to 
hit an issue if one is there).

 Or to put it short I still cannot reproduce it either way, and the dynsym 
table always looks the same here, regardless of whether my original patch 
has been applied or not, and if so, then if any of the updates has, as 
follows:

Symbol table '.dynsym' contains 16 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
     0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
     1: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND __libc_start_main@GLIBC_2.2.5 (2)
     2: 0000000000000000     0 NOTYPE  WEAK   DEFAULT  UND __gmon_start__
     3: 0000000000601024     0 NOTYPE  GLOBAL DEFAULT   25 _edata
     4: 0000000000601020     0 NOTYPE  GLOBAL DEFAULT   25 __data_start
     5: 0000000000601028     0 NOTYPE  GLOBAL DEFAULT   26 _end
     6: 0000000000601020     0 NOTYPE  WEAK   DEFAULT   25 data_start
     7: 0000000000400750     4 OBJECT  GLOBAL DEFAULT   16 _IO_stdin_used
     8: 00000000004006d0   101 FUNC    GLOBAL DEFAULT   14 __libc_csu_init
     9: 00000000004005d3     0 FUNC    GLOBAL DEFAULT   14 _start
    10: 0000000000601024     0 NOTYPE  GLOBAL DEFAULT   26 __bss_start
    11: 00000000004005d0     3 FUNC    GLOBAL DEFAULT   14 main
    12: 0000000000400580     0 FUNC    GLOBAL DEFAULT   11 _init
    13: 0000000000400740     2 FUNC    GLOBAL DEFAULT   14 __libc_csu_fini
    14: 0000000000400744     0 FUNC    GLOBAL DEFAULT   15 _fini
    15: 00000000004006c0     2 FUNC    GLOBAL DEFAULT   14 rdynamic

 That said however all 3 regressions have the use of `-export-dynamic' in 
common, so I have used the other two I could get to trigger here as 
reference tests and tracked the issue down to a piece of code in 
`bfd_elf_size_dynamic_sections' that I previously missed, which handles 
the option and symbols that would otherwise remain local to the dynamic 
symbol table.  These symbols obviously have to be passed through version 
processing, so they have to be exported before linker versioning is 
applied.

 By the same reasoning I have shown in the previous message these symbols 
will not be garbage-collected, or they wouldn't have been candidates for 
exporting in the first place -- as indicated in `_bfd_elf_export_symbol' 
they need to be either locally defined or locally referenced.  So it is 
safe AFAICT to have the symbol sweep pass executed after the forciful 
symbol export loop, and therefore the loop can be moved ahead of version 
processing and hence also the symbol sweep pass, while still satisfying 
the requirements of both.

 So here's an updated update, which removes the:

FAIL: vers4a
FAIL: vers9

regressions for me, and hopefully also your:

FAIL: Build rdynamic-1

(please verify that for me).

 For the purpose of further review and for the avoidance of doubt as to 
what my actual proposed change is right now, I'll post v2 of the original 
change with all the amendments applied.  It'll include a further test case 
reduced from `vers4a' too, which does not rely on the presence of a 
compiler (and which I yet need to actually push through my usual testing).

 Thanks for your input!

  Maciej

binutils-bfd-elf-size-dynamic-sections-version-update-3.diff
Index: binutils/bfd/elflink.c
===================================================================
--- binutils.orig/bfd/elflink.c	2017-02-17 23:27:33.275425647 +0000
+++ binutils/bfd/elflink.c	2017-02-17 23:46:30.128370517 +0000
@@ -5938,13 +5938,28 @@ bfd_elf_size_dynamic_sections (bfd *outp
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
       struct bfd_elf_version_tree *verdefs;
-      unsigned long section_sym_count;
       struct elf_info_failed asvinfo;
       struct bfd_elf_version_tree *t;
       struct bfd_elf_version_expr *d;
+      struct elf_info_failed eif;
       bfd_boolean all_defined;
       asection *s;
 
+      eif.info = info;
+      eif.failed = FALSE;
+
+      /* If we are supposed to export all symbols into the dynamic symbol
+	 table (this is not the normal case), then do so.  */
+      if (info->export_dynamic
+	  || (bfd_link_executable (info) && info->dynamic))
+	{
+	  elf_link_hash_traverse (elf_hash_table (info),
+				  _bfd_elf_export_symbol,
+				  &eif);
+	  if (eif.failed)
+	    return FALSE;
+	}
+
       /* Make all global versions with definition.  */
       for (t = info->version_info; t != NULL; t = t->next)
 	for (d = t->globals.list; d != NULL; d = d->next)
@@ -6368,15 +6383,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	    elf_tdata (output_bfd)->cverrefs = crefs;
 	  }
       }
-
-      if ((elf_tdata (output_bfd)->cverrefs == 0
-	   && elf_tdata (output_bfd)->cverdefs == 0)
-	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
-					     &section_sym_count) == 0)
-	{
-	  s = bfd_get_linker_section (dynobj, ".gnu.version");
-	  s->flags |= SEC_EXCLUDE;
-	}
     }
 
   bed = get_elf_backend_data (output_bfd);
@@ -6545,18 +6551,6 @@ bfd_elf_size_dynamic_sections (bfd *outp
       eif.info = info;
       eif.failed = FALSE;
 
-      /* If we are supposed to export all symbols into the dynamic symbol
-	 table (this is not the normal case), then do so.  */
-      if (info->export_dynamic
-	  || (bfd_link_executable (info) && info->dynamic))
-	{
-	  elf_link_hash_traverse (elf_hash_table (info),
-				  _bfd_elf_export_symbol,
-				  &eif);
-	  if (eif.failed)
-	    return FALSE;
-	}
-
       /* Find all symbols which were defined in a dynamic object and make
 	 the backend pick a reasonable value for them.  */
       elf_link_hash_traverse (elf_hash_table (info),
@@ -6676,6 +6670,8 @@ bfd_elf_size_dynamic_sections (bfd *outp
 
   if (dynobj != NULL && elf_hash_table (info)->dynamic_sections_created)
     {
+      unsigned long section_sym_count;
+
       if (elf_tdata (output_bfd)->cverdefs)
 	{
 	  unsigned int crefs = elf_tdata (output_bfd)->cverdefs;
@@ -6714,6 +6710,17 @@ bfd_elf_size_dynamic_sections (bfd *outp
 	      || !_bfd_elf_add_dynamic_entry (info, DT_VERNEEDNUM, crefs))
 	    return FALSE;
 	}
+
+      if ((elf_tdata (output_bfd)->cverrefs == 0
+	   && elf_tdata (output_bfd)->cverdefs == 0)
+	  || _bfd_elf_link_renumber_dynsyms (output_bfd, info,
+					     &section_sym_count) == 0)
+	{
+	  asection *s;
+
+	  s = bfd_get_linker_section (dynobj, ".gnu.version");
+	  s->flags |= SEC_EXCLUDE;
+	}
     }
   return TRUE;
 }

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

* Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep
  2017-02-20 19:26               ` Maciej W. Rozycki
@ 2017-02-21 22:50                 ` Alan Modra
  0 siblings, 0 replies; 10+ messages in thread
From: Alan Modra @ 2017-02-21 22:50 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: binutils

On Mon, Feb 20, 2017 at 07:26:07PM +0000, Maciej W. Rozycki wrote:
>  So here's an updated update, which removes the:
> 
> FAIL: vers4a
> FAIL: vers9
> 
> regressions for me, and hopefully also your:
> 
> FAIL: Build rdynamic-1

It did.

-- 
Alan Modra
Australia Development Lab, IBM

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

end of thread, other threads:[~2017-02-21 22:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-01 19:31 [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep Maciej W. Rozycki
2017-02-02  3:25 ` Alan Modra
2017-02-08 18:19   ` Maciej W. Rozycki
2017-02-09 19:49     ` Maciej W. Rozycki
2017-02-15  1:55       ` Alan Modra
2017-02-15  3:43         ` Alan Modra
2017-02-15 16:39           ` Maciej W. Rozycki
2017-02-15 23:34             ` Alan Modra
2017-02-20 19:26               ` Maciej W. Rozycki
2017-02-21 22:50                 ` Alan Modra

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