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

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

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;
 }