Re: [PATCH] PR ld/20828: Move symbol version processing ahead of GC symbol sweep

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

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;
 }
 
+/* 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));
+}
+
 /* 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;
 }