[PATCH] Add a versioned field to elf_link_hash_entry

[PATCH] Add a versioned field to elf_link_hash_entry

[H.J. Lu]

This patch adds a versioned field to elf_link_hash_entry so that we can
avoid calling strchr and strrchr if the symbol is unversioned.

Any comments, feedbacks, objections?

H.J.
---
	* elf-bfd.h (elf_link_hash_entry): Add versioned.
	* elflink.c (_bfd_elf_merge_symbol): Don't look for symbol
	version if the symbol is unversioned.  Initialize versioned.
	(_bfd_elf_add_default_symbol): Don't look for symbol version
	if the symbol is unversioned or hidden.  Initialize versioned.
	(elf_collect_hash_codes): Don't look for symbol version if the
	symbol is unversioned.
	(elf_collect_gnu_hash_codes): Likewise.
	(bfd_elf_gc_mark_dynamic_ref_symbol): Likewise.
---
 bfd/elf-bfd.h |   6 ++++
 bfd/elflink.c | 109 ++++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 77 insertions(+), 38 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index e08b2d6..df71083 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -178,6 +178,12 @@ struct elf_link_hash_entry
   unsigned int needs_plt : 1;
   /* Symbol appears in a non-ELF input file.  */
   unsigned int non_elf : 1;
+  /* Symbol version information:
+     0: unknown
+     1: unversioned
+     2: versioned
+   */
+  unsigned int versioned : 2;
   /* Symbol should be marked as hidden in the version information.  */
   unsigned int hidden : 1;
   /* Symbol was forced to local scope due to a version script file.  */
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 832b374..ce28764 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -971,15 +971,27 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bed = get_elf_backend_data (abfd);
 
   /* NEW_VERSION is the symbol version of the new symbol.  */
-  new_version = strrchr (name, ELF_VER_CHR);
-  if (new_version)
+  if (h->versioned != 1)
     {
-      if (new_version > name && new_version[-1] != ELF_VER_CHR)
-	h->hidden = 1;
-      new_version += 1;
-      if (new_version[0] == '\0')
-	new_version = NULL;
+      /* Symbol version is unknown or versioned.  */
+      new_version = strrchr (name, ELF_VER_CHR);
+      if (new_version)
+	{
+	  if (h->versioned == 0)
+	    {
+	      h->versioned = 2;
+	      if (new_version > name && new_version[-1] != ELF_VER_CHR)
+		h->hidden = 1;
+	    }
+	  new_version += 1;
+	  if (new_version[0] == '\0')
+	    new_version = NULL;
+	}
+      else
+	h->versioned = 1;
     }
+  else
+    new_version = NULL;
 
   /* For merging, we only care about real symbols.  But we need to make
      sure that indirect symbol dynamic flags are updated.  */
@@ -1008,14 +1020,13 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	    {
 	      /* OLD_VERSION is the symbol version of the existing
 		 symbol. */
-	      char *old_version = strrchr (h->root.root.string,
-					   ELF_VER_CHR);
-	      if (old_version)
-		{
-		  old_version += 1;
-		  if (old_version[0] == '\0')
-		    old_version = NULL;
-		}
+	      char *old_version;
+
+	      if (h->versioned == 2)
+		old_version = strrchr (h->root.root.string,
+				       ELF_VER_CHR) + 1;
+	      else
+		 old_version = NULL;
 
 	      /* The new symbol matches the existing symbol if they
 		 have the same symbol version.  */
@@ -1674,13 +1685,31 @@ _bfd_elf_add_default_symbol (bfd *abfd,
   asection *tmp_sec;
   bfd_boolean matched;
 
+  if (h->versioned == 1 || h->hidden)
+    return TRUE;
+
   /* If this symbol has a version, and it is the default version, we
      create an indirect symbol from the default name to the fully
      decorated name.  This will cause external references which do not
      specify a version to be bound to this version of the symbol.  */
   p = strchr (name, ELF_VER_CHR);
-  if (p == NULL || p[1] != ELF_VER_CHR)
-    return TRUE;
+  if (h->versioned == 0)
+    {
+      if (p == NULL)
+	{
+	  h->versioned = 1;
+	  return TRUE;
+	}
+      else
+	{
+	  h->versioned = 2;
+	  if (p[1] != ELF_VER_CHR)
+	    {
+	      h->hidden = 1;
+	      return TRUE;
+	    }
+	}
+    }
 
   bed = get_elf_backend_data (abfd);
   collect = bed->collect;
@@ -5230,7 +5259,6 @@ elf_collect_hash_codes (struct elf_link_hash_entry *h, void *data)
 {
   struct hash_codes_info *inf = (struct hash_codes_info *) data;
   const char *name;
-  char *p;
   unsigned long ha;
   char *alc = NULL;
 
@@ -5239,18 +5267,21 @@ elf_collect_hash_codes (struct elf_link_hash_entry *h, void *data)
     return TRUE;
 
   name = h->root.root.string;
-  p = strchr (name, ELF_VER_CHR);
-  if (p != NULL)
+  if (h->versioned == 2)
     {
-      alc = (char *) bfd_malloc (p - name + 1);
-      if (alc == NULL)
+      char *p = strchr (name, ELF_VER_CHR);
+      if (p != NULL)
 	{
-	  inf->error = TRUE;
-	  return FALSE;
+	  alc = (char *) bfd_malloc (p - name + 1);
+	  if (alc == NULL)
+	    {
+	      inf->error = TRUE;
+	      return FALSE;
+	    }
+	  memcpy (alc, name, p - name);
+	  alc[p - name] = '\0';
+	  name = alc;
 	}
-      memcpy (alc, name, p - name);
-      alc[p - name] = '\0';
-      name = alc;
     }
 
   /* Compute the hash value.  */
@@ -5298,7 +5329,6 @@ elf_collect_gnu_hash_codes (struct elf_link_hash_entry *h, void *data)
 {
   struct collect_gnu_hash_codes *s = (struct collect_gnu_hash_codes *) data;
   const char *name;
-  char *p;
   unsigned long ha;
   char *alc = NULL;
 
@@ -5311,18 +5341,21 @@ elf_collect_gnu_hash_codes (struct elf_link_hash_entry *h, void *data)
     return TRUE;
 
   name = h->root.root.string;
-  p = strchr (name, ELF_VER_CHR);
-  if (p != NULL)
+  if (h->versioned == 2)
     {
-      alc = (char *) bfd_malloc (p - name + 1);
-      if (alc == NULL)
+      char *p = strchr (name, ELF_VER_CHR);
+      if (p != NULL)
 	{
-	  s->error = TRUE;
-	  return FALSE;
+	  alc = (char *) bfd_malloc (p - name + 1);
+	  if (alc == NULL)
+	    {
+	      s->error = TRUE;
+	      return FALSE;
+	    }
+	  memcpy (alc, name, p - name);
+	  alc[p - name] = '\0';
+	  name = alc;
 	}
-      memcpy (alc, name, p - name);
-      alc[p - name] = '\0';
-      name = alc;
     }
 
   /* Compute the hash value.  */
@@ -12541,7 +12574,7 @@ bfd_elf_gc_mark_dynamic_ref_symbol (struct elf_link_hash_entry *h, void *inf)
 		  || (h->dynamic
 		      && d != NULL
 		      && (*d->match) (&d->head, NULL, h->root.root.string)))
-	      && (strchr (h->root.root.string, ELF_VER_CHR) != NULL
+	      && (h->versioned == 2
 		  || !bfd_hide_sym_by_version (info->version_info,
 					       h->root.root.string)))))
     h->root.u.def.section->flags |= SEC_KEEP;
-- 
2.4.3

Re: [PATCH] Add a versioned field to elf_link_hash_entry

[Alan Modra]

On Fri, Aug 07, 2015 at 05:36:25AM -0700, H.J. Lu wrote:
> +  /* Symbol version information:
> +     0: unknown
> +     1: unversioned
> +     2: versioned
> +   */
> +  unsigned int versioned : 2;
>    /* Symbol should be marked as hidden in the version information.  */
>    unsigned int hidden : 1;

Seems to me it would be better to use an enum, and ENUM_BITFIELD.
Also, I believe "hidden" could be merged into the new field, giving it
four states: unknown, unversioned, versioned, versioned_hidden.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] Add a versioned field to elf_link_hash_entry

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Sun, Aug 9, 2015 at 7:56 PM, Alan Modra <amodra@gmail.com> wrote:
> On Fri, Aug 07, 2015 at 05:36:25AM -0700, H.J. Lu wrote:
>> +  /* Symbol version information:
>> +     0: unknown
>> +     1: unversioned
>> +     2: versioned
>> +   */
>> +  unsigned int versioned : 2;
>>    /* Symbol should be marked as hidden in the version information.  */
>>    unsigned int hidden : 1;
>
> Seems to me it would be better to use an enum, and ENUM_BITFIELD.
> Also, I believe "hidden" could be merged into the new field, giving it
> four states: unknown, unversioned, versioned, versioned_hidden.

This is what I checked in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Replace-hidden-with-versioned-in-elf_link_hash_entry.patch --]
[-- Type: text/x-patch, Size: 9518 bytes --]

From 99ac18a7529dbacddf14782f5fac4b53d0d5a31b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 28 Jul 2015 05:11:02 -0700
Subject: [PATCH] Replace hidden with versioned in elf_link_hash_entry

This patch replaces the "hidden" field with the "versioned" field in
elf_link_hash_entry so that we can avoid calling strchr and strrchr if
the symbol is unversioned.

	* elf-bfd.h (elf_symbol_version): New enum.
	(elf_link_hash_entry): Replace hidden with versioned.
	* elflink.c (_bfd_elf_merge_symbol): Don't look for symbol
	version if the symbol is unversioned.  Initialize versioned.
	(_bfd_elf_add_default_symbol): Don't look for symbol version
	if the symbol is unversioned or hidden.  Initialize versioned.
	(elf_collect_hash_codes): Don't look for symbol version if the
	symbol is unversioned.
	(elf_collect_gnu_hash_codes): Likewise.
	(bfd_elf_gc_mark_dynamic_ref_symbol): Likewise.
	(_bfd_elf_link_hash_copy_indirect): Check versioned instead of
	hidden.
	(elf_link_output_extsym): Likewise.
---
 bfd/ChangeLog |   2 +-
 bfd/elf-bfd.h |  13 ++++++-
 bfd/elflink.c | 123 +++++++++++++++++++++++++++++++++++++---------------------
 3 files changed, 91 insertions(+), 47 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index c319f80..dce3b4a 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -27,7 +27,7 @@
 	(elf_link_output_extsym): Bind a symbol locally when linking
 	executable if it is locally defined, hidden versioned, not
 	referenced by shared library and not exported.  Turn on
-	VERSYM_HIDDEN only if the hidden vesioned symbol is defined
+	VERSYM_HIDDEN only if the hidden versioned symbol is defined
 	locally.
 
 2015-08-05  Nick Clifton  <nickc@redhat.com>
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index e08b2d6..c92671a 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -108,6 +108,15 @@ struct elf_link_virtual_table_entry
     struct elf_link_hash_entry *parent;
   };
 
+/* ELF symbol version.  */
+enum elf_symbol_version
+  {
+    unknown = 0,
+    unversioned,
+    versioned,
+    versioned_hidden
+  };
+
 /* ELF linker hash table entries.  */
 
 struct elf_link_hash_entry
@@ -178,8 +187,8 @@ struct elf_link_hash_entry
   unsigned int needs_plt : 1;
   /* Symbol appears in a non-ELF input file.  */
   unsigned int non_elf : 1;
-  /* Symbol should be marked as hidden in the version information.  */
-  unsigned int hidden : 1;
+  /* Symbol version information.  */
+  ENUM_BITFIELD (elf_symbol_version) versioned : 2;
   /* Symbol was forced to local scope due to a version script file.  */
   unsigned int forced_local : 1;
   /* Symbol was forced to be dynamic due to a version script file.  */
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 832b374..031cffe 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -971,15 +971,28 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bed = get_elf_backend_data (abfd);
 
   /* NEW_VERSION is the symbol version of the new symbol.  */
-  new_version = strrchr (name, ELF_VER_CHR);
-  if (new_version)
+  if (h->versioned != unversioned)
     {
-      if (new_version > name && new_version[-1] != ELF_VER_CHR)
-	h->hidden = 1;
-      new_version += 1;
-      if (new_version[0] == '\0')
-	new_version = NULL;
+      /* Symbol version is unknown or versioned.  */
+      new_version = strrchr (name, ELF_VER_CHR);
+      if (new_version)
+	{
+	  if (h->versioned == unknown)
+	    {
+	      if (new_version > name && new_version[-1] != ELF_VER_CHR)
+		h->versioned = versioned_hidden;
+	      else
+		h->versioned = versioned;
+	    }
+	  new_version += 1;
+	  if (new_version[0] == '\0')
+	    new_version = NULL;
+	}
+      else
+	h->versioned = unversioned;
     }
+  else
+    new_version = NULL;
 
   /* For merging, we only care about real symbols.  But we need to make
      sure that indirect symbol dynamic flags are updated.  */
@@ -998,8 +1011,8 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	     to the symbol with the same symbol version.  NEW_HIDDEN is
 	     true if the new symbol is only visibile to the symbol with
 	     the same symbol version.  */
-	  bfd_boolean old_hidden = h->hidden;
-	  bfd_boolean new_hidden = hi->hidden;
+	  bfd_boolean old_hidden = h->versioned == versioned_hidden;
+	  bfd_boolean new_hidden = hi->versioned == versioned_hidden;
 	  if (!old_hidden && !new_hidden)
 	    /* The new symbol matches the existing symbol if both
 	       aren't hidden.  */
@@ -1008,14 +1021,13 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	    {
 	      /* OLD_VERSION is the symbol version of the existing
 		 symbol. */
-	      char *old_version = strrchr (h->root.root.string,
-					   ELF_VER_CHR);
-	      if (old_version)
-		{
-		  old_version += 1;
-		  if (old_version[0] == '\0')
-		    old_version = NULL;
-		}
+	      char *old_version;
+
+	      if (h->versioned >= versioned)
+		old_version = strrchr (h->root.root.string,
+				       ELF_VER_CHR) + 1;
+	      else
+		 old_version = NULL;
 
 	      /* The new symbol matches the existing symbol if they
 		 have the same symbol version.  */
@@ -1674,13 +1686,32 @@ _bfd_elf_add_default_symbol (bfd *abfd,
   asection *tmp_sec;
   bfd_boolean matched;
 
+  if (h->versioned == unversioned || h->versioned == versioned_hidden)
+    return TRUE;
+
   /* If this symbol has a version, and it is the default version, we
      create an indirect symbol from the default name to the fully
      decorated name.  This will cause external references which do not
      specify a version to be bound to this version of the symbol.  */
   p = strchr (name, ELF_VER_CHR);
-  if (p == NULL || p[1] != ELF_VER_CHR)
-    return TRUE;
+  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;
+	}
+    }
 
   bed = get_elf_backend_data (abfd);
   collect = bed->collect;
@@ -5230,7 +5261,6 @@ elf_collect_hash_codes (struct elf_link_hash_entry *h, void *data)
 {
   struct hash_codes_info *inf = (struct hash_codes_info *) data;
   const char *name;
-  char *p;
   unsigned long ha;
   char *alc = NULL;
 
@@ -5239,18 +5269,21 @@ elf_collect_hash_codes (struct elf_link_hash_entry *h, void *data)
     return TRUE;
 
   name = h->root.root.string;
-  p = strchr (name, ELF_VER_CHR);
-  if (p != NULL)
+  if (h->versioned >= versioned)
     {
-      alc = (char *) bfd_malloc (p - name + 1);
-      if (alc == NULL)
+      char *p = strchr (name, ELF_VER_CHR);
+      if (p != NULL)
 	{
-	  inf->error = TRUE;
-	  return FALSE;
+	  alc = (char *) bfd_malloc (p - name + 1);
+	  if (alc == NULL)
+	    {
+	      inf->error = TRUE;
+	      return FALSE;
+	    }
+	  memcpy (alc, name, p - name);
+	  alc[p - name] = '\0';
+	  name = alc;
 	}
-      memcpy (alc, name, p - name);
-      alc[p - name] = '\0';
-      name = alc;
     }
 
   /* Compute the hash value.  */
@@ -5298,7 +5331,6 @@ elf_collect_gnu_hash_codes (struct elf_link_hash_entry *h, void *data)
 {
   struct collect_gnu_hash_codes *s = (struct collect_gnu_hash_codes *) data;
   const char *name;
-  char *p;
   unsigned long ha;
   char *alc = NULL;
 
@@ -5311,18 +5343,21 @@ elf_collect_gnu_hash_codes (struct elf_link_hash_entry *h, void *data)
     return TRUE;
 
   name = h->root.root.string;
-  p = strchr (name, ELF_VER_CHR);
-  if (p != NULL)
+  if (h->versioned >= versioned)
     {
-      alc = (char *) bfd_malloc (p - name + 1);
-      if (alc == NULL)
+      char *p = strchr (name, ELF_VER_CHR);
+      if (p != NULL)
 	{
-	  s->error = TRUE;
-	  return FALSE;
+	  alc = (char *) bfd_malloc (p - name + 1);
+	  if (alc == NULL)
+	    {
+	      s->error = TRUE;
+	      return FALSE;
+	    }
+	  memcpy (alc, name, p - name);
+	  alc[p - name] = '\0';
+	  name = alc;
 	}
-      memcpy (alc, name, p - name);
-      alc[p - name] = '\0';
-      name = alc;
     }
 
   /* Compute the hash value.  */
@@ -6859,7 +6894,7 @@ _bfd_elf_link_hash_copy_indirect (struct bfd_link_info *info,
      symbol which just became indirect if DIR isn't a hidden versioned
      symbol.  */
 
-  if (!dir->hidden)
+  if (dir->versioned != versioned_hidden)
     {
       dir->ref_dynamic |= ind->ref_dynamic;
       dir->ref_regular |= ind->ref_regular;
@@ -8963,7 +8998,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
 				&& !h->dynamic
 				&& !h->ref_dynamic
 				&& h->def_regular
-				&& h->hidden));
+				&& h->versioned == versioned_hidden));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -9359,9 +9394,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
 		iversym.vs_vers++;
 	    }
 
-	  /* Turn on VERSYM_HIDDEN only if the hidden vesioned symbol is
+	  /* Turn on VERSYM_HIDDEN only if the hidden versioned symbol is
 	     defined locally.  */
-	  if (h->hidden && h->def_regular)
+	  if (h->versioned == versioned_hidden && h->def_regular)
 	    iversym.vs_vers |= VERSYM_HIDDEN;
 
 	  eversym = (Elf_External_Versym *) flinfo->symver_sec->contents;
@@ -12541,7 +12576,7 @@ bfd_elf_gc_mark_dynamic_ref_symbol (struct elf_link_hash_entry *h, void *inf)
 		  || (h->dynamic
 		      && d != NULL
 		      && (*d->match) (&d->head, NULL, h->root.root.string)))
-	      && (strchr (h->root.root.string, ELF_VER_CHR) != NULL
+	      && (h->versioned >= versioned
 		  || !bfd_hide_sym_by_version (info->version_info,
 					       h->root.root.string)))))
     h->root.u.def.section->flags |= SEC_KEEP;
-- 
2.4.3

Re: [PATCH] Add a versioned field to elf_link_hash_entry

[Andreas Krebbel]

On 08/10/2015 04:58 PM, H.J. Lu wrote:
> On Sun, Aug 9, 2015 at 7:56 PM, Alan Modra <amodra@gmail.com> wrote:
>> On Fri, Aug 07, 2015 at 05:36:25AM -0700, H.J. Lu wrote:
>>> +  /* Symbol version information:
>>> +     0: unknown
>>> +     1: unversioned
>>> +     2: versioned
>>> +   */
>>> +  unsigned int versioned : 2;
>>>    /* Symbol should be marked as hidden in the version information.  */
>>>    unsigned int hidden : 1;
>>
>> Seems to me it would be better to use an enum, and ENUM_BITFIELD.
>> Also, I believe "hidden" could be merged into the new field, giving it
>> four states: unknown, unversioned, versioned, versioned_hidden.
> 
> This is what I checked in.
> 
> Thanks.

This seem to have triggered:
https://sourceware.org/bugzilla/show_bug.cgi?id=19073

Bye,

-Andreas-