[PATCH] PR ld/18720: Properly merge non-default versioned symbol

[PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

The non-default versioned symbol can only be merged with the versioned
symbol with the same symbol version.  _bfd_elf_merge_symbol should
check the symbol version before merging the new non-default versioned
symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
copy any references to the non-default versioned symbol.   We need to
bind a symbol locally when linking executable if it is locally defined,
non-default versioned, not referenced by shared library and not exported.

OK for master?

Thanks.

H.J.
---
bfd/

	PR ld/18720
	* elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
	* elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
	if the new symbol matches the existing one.  The new non-default
	versioned symbol symbol matches the existing symbol if they have
	the same symbol version. Update the existing symbol only if they
	match.
	(_bfd_elf_add_default_symbol): Update call to
	_bfd_elf_merge_symbol.
	(elf_link_add_object_symbols): Override a definition only if the
	new symbol matches the existing one.
	(_bfd_elf_link_hash_copy_indirect): Don't copy any references to
	the non-default versioned symbol.
	(elf_link_output_extsym): Bind a symbol locally when linking
	executable if it is locally defined, non-default versioned, not
	referenced by shared library and not exported.

ld/testsuite/

	PR ld/18720
	* ld-elf/indirect.exp: Run tests for PR ld/18720.
	* ld-elf/pr18720.out: New file.
	* ld-elf/pr18720a.c: Likewise.
	* ld-elf/pr18720b.c: Likewise.
	* ld-elf/pr18720c.c: Likewise.
---
 bfd/elf-bfd.h                    |   2 +
 bfd/elflink.c                    | 108 +++++++++++++++++++++++++++++++++------
 ld/testsuite/ld-elf/indirect.exp |  19 ++++++-
 ld/testsuite/ld-elf/pr18720.out  |   2 +
 ld/testsuite/ld-elf/pr18720a.c   |  18 +++++++
 ld/testsuite/ld-elf/pr18720b.c   |   9 ++++
 ld/testsuite/ld-elf/pr18720c.c   |  15 ++++++
 7 files changed, 155 insertions(+), 18 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr18720.out
 create mode 100644 ld/testsuite/ld-elf/pr18720a.c
 create mode 100644 ld/testsuite/ld-elf/pr18720b.c
 create mode 100644 ld/testsuite/ld-elf/pr18720c.c

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index e08b2d6..aeba83e 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -203,6 +203,8 @@ struct elf_link_hash_entry
   /* Symbol is defined by a shared library with non-default visibility
      in a read/write section.  */
   unsigned int protected_def : 1;
+  /* Symbol is non-default versioned.  */
+  unsigned int nondeflt_version : 1;
 
   /* String table index in .dynstr if this is a dynamic symbol.  */
   unsigned long dynstr_index;
diff --git a/bfd/elflink.c b/bfd/elflink.c
index ccb7ba2..e91d786 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -939,7 +939,8 @@ _bfd_elf_merge_symbol (bfd *abfd,
 		       bfd_boolean *skip,
 		       bfd_boolean *override,
 		       bfd_boolean *type_change_ok,
-		       bfd_boolean *size_change_ok)
+		       bfd_boolean *size_change_ok,
+		       bfd_boolean *matched)
 {
   asection *sec, *oldsec;
   struct elf_link_hash_entry *h;
@@ -950,6 +951,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bfd_boolean newdyn, olddyn, olddef, newdef, newdyncommon, olddyncommon;
   bfd_boolean newweak, oldweak, newfunc, oldfunc;
   const struct elf_backend_data *bed;
+  char *new_version;
 
   *skip = FALSE;
   *override = FALSE;
@@ -968,6 +970,17 @@ _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 && new_version[1] != '\0')
+    {
+      if (new_version > name && new_version[-1] != ELF_VER_CHR)
+	h->nondeflt_version = 1;
+      new_version += 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.  */
   hi = h;
@@ -975,6 +988,42 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	 || h->root.type == bfd_link_hash_warning)
     h = (struct elf_link_hash_entry *) h->root.u.i.link;
 
+  if (!*matched)
+    {
+      if (hi == h || h->root.type == bfd_link_hash_new)
+	*matched = TRUE;
+      else
+	{
+	  /* OLD_HIDDEN is true if the existing symbol is only visibile
+	     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->nondeflt_version;
+	  bfd_boolean new_hidden = hi->nondeflt_version;
+	  if (!old_hidden && !new_hidden)
+	    /* The new symbol matches the existing symbol if both
+	       aren't hidden.  */
+	    *matched = TRUE;
+	  else
+	    {
+	      /* 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] != '\0')
+		old_version += 1;
+	      else
+		old_version = NULL;
+
+	      /* The new symbol matches the existing symbol if they
+		 have the same symbol version.  */
+	      *matched = (old_version != NULL
+			  && new_version != NULL
+			  && strcmp (old_version, new_version) == 0);
+	    }
+	}
+    }
+
   /* OLDBFD and OLDSEC are a BFD and an ASECTION associated with the
      existing symbol.  */
 
@@ -1047,7 +1096,9 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	}
       else
 	{
-	  h->dynamic_def = 1;
+	  /* Update the existing symbol only if they match. */
+	  if (*matched)
+	    h->dynamic_def = 1;
 	  hi->dynamic_def = 1;
 	}
     }
@@ -1618,6 +1669,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
   char *p;
   size_t len, shortlen;
   asection *tmp_sec;
+  bfd_boolean matched;
 
   /* If this symbol has a version, and it is the default version, we
      create an indirect symbol from the default name to the fully
@@ -1644,10 +1696,11 @@ _bfd_elf_add_default_symbol (bfd *abfd,
      actually going to define an indirect symbol.  */
   type_change_ok = FALSE;
   size_change_ok = FALSE;
+  matched = TRUE;
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -1762,7 +1815,7 @@ nondefault:
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -3880,6 +3933,7 @@ error_free_dyn:
       bfd_boolean common;
       unsigned int old_alignment;
       bfd *old_bfd;
+      bfd_boolean matched;
 
       override = FALSE;
 
@@ -4014,6 +4068,7 @@ error_free_dyn:
       size_change_ok = FALSE;
       type_change_ok = bed->type_change_ok;
       old_weak = FALSE;
+      matched = FALSE;
       old_alignment = 0;
       old_bfd = NULL;
       new_sec = sec;
@@ -4143,13 +4198,16 @@ error_free_dyn:
 	  if (!_bfd_elf_merge_symbol (abfd, info, name, isym, &sec, &value,
 				      sym_hash, &old_bfd, &old_weak,
 				      &old_alignment, &skip, &override,
-				      &type_change_ok, &size_change_ok))
+				      &type_change_ok, &size_change_ok,
+				      &matched))
 	    goto error_free_vers;
 
 	  if (skip)
 	    continue;
 
-	  if (override)
+	  /* Override a definition only if the new symbol matches the
+	     existing one.  */
+	  if (override && matched)
 	    definition = FALSE;
 
 	  h = *sym_hash;
@@ -6804,14 +6862,18 @@ _bfd_elf_link_hash_copy_indirect (struct bfd_link_info *info,
   struct elf_link_hash_table *htab;
 
   /* Copy down any references that we may have already seen to the
-     symbol which just became indirect.  */
+     symbol which just became indirect if DIR isn't a non-default
+     versioned symbol.  */
 
-  dir->ref_dynamic |= ind->ref_dynamic;
-  dir->ref_regular |= ind->ref_regular;
-  dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
-  dir->non_got_ref |= ind->non_got_ref;
-  dir->needs_plt |= ind->needs_plt;
-  dir->pointer_equality_needed |= ind->pointer_equality_needed;
+  if (!dir->nondeflt_version)
+    {
+      dir->ref_dynamic |= ind->ref_dynamic;
+      dir->ref_regular |= ind->ref_regular;
+      dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
+      dir->non_got_ref |= ind->non_got_ref;
+      dir->needs_plt |= ind->needs_plt;
+      dir->pointer_equality_needed |= ind->pointer_equality_needed;
+    }
 
   if (ind->root.type != bfd_link_hash_indirect)
     return;
@@ -8898,6 +8960,16 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   const struct elf_backend_data *bed;
   long indx;
   int ret;
+  /* A symbol is bound locally if it is forced local or it is locally
+     defined, non-default versioned, not referenced by shared library
+     and not exported when linking executable.  */
+  bfd_boolean local_bind = (h->forced_local
+			    || (flinfo->info->executable
+				&& !flinfo->info->export_dynamic
+				&& !h->dynamic
+				&& !h->ref_dynamic
+				&& h->def_regular
+				&& h->nondeflt_version));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -8909,12 +8981,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* Decide whether to output this symbol in this pass.  */
   if (eoinfo->localsyms)
     {
-      if (!h->forced_local)
+      if (!local_bind)
 	return TRUE;
     }
   else
     {
-      if (h->forced_local)
+      if (local_bind)
 	return TRUE;
     }
 
@@ -9036,7 +9108,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   sym.st_value = 0;
   sym.st_size = h->size;
   sym.st_other = h->other;
-  if (h->forced_local)
+  if (local_bind)
     {
       sym.st_info = ELF_ST_INFO (STB_LOCAL, h->type);
       /* Turn off visibility on local symbol.  */
@@ -9218,8 +9290,10 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
       /* Since there is no version information in the dynamic string,
 	 if there is no version info in symbol version section, we will
 	 have a run-time problem if not linking executable, referenced
-	 by shared library, or not locally defined.  */
+	 by shared library, not locally defined, or not bound locally.
+      */
       if (h->verinfo.verdef == NULL
+	  && !local_bind
 	  && (!flinfo->info->executable
 	      || h->ref_dynamic
 	      || !h->def_regular))
diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
index 468ef2b..1677cb6 100644
--- a/ld/testsuite/ld-elf/indirect.exp
+++ b/ld/testsuite/ld-elf/indirect.exp
@@ -64,7 +64,9 @@ if { ![ld_compile $CC $srcdir/$subdir/indirect1a.c tmpdir/indirect1a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3a.c tmpdir/indirect3a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3b.c tmpdir/indirect3b.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect4a.c tmpdir/indirect4a.o]
-     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o] } {
+     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o]
+     || ![ld_compile "$CC -O2 -fPIC" $srcdir/$subdir/pr18720a.c tmpdir/pr18720a.o]
+     || ![ld_compile $CC $srcdir/$subdir/pr18720b.c tmpdir/pr18720b.o] } {
     unresolved "Indirect symbol tests"
     return
 }
@@ -79,6 +81,9 @@ set build_tests {
   {"Build libindirect4c.so"
    "-shared" "-fPIC"
    {indirect4c.c} {} "libindirect4c.so"}
+  {"Build libpr18720c.so"
+   "-shared" "-fPIC"
+   {pr18720c.c} {} "libpr18720c.so"}
 }
 
 run_cc_link_tests $build_tests
@@ -132,6 +137,18 @@ set run_tests {
     {"Run with libindirect4c.so 4"
      "tmpdir/libindirect4c.so tmpdir/indirect4b.o tmpdir/indirect4a.o" ""
      {dummy.c} "indirect4d" "indirect4.out"}
+    {"Run with libpr18720c.so 1"
+     "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
+     {dummy.c} "pr18720a" "pr18720.out"}
+    {"Run with libpr18720c.so 2"
+     "tmpdir/pr18720a.o tmpdir/libpr18720c.so tmpdir/pr18720b.o" ""
+     {dummy.c} "pr18720b" "pr18720.out"}
+    {"Run with libpr18720c.so 3"
+     "tmpdir/pr18720b.o tmpdir/libpr18720c.so tmpdir/pr18720a.o" ""
+     {dummy.c} "pr18720c" "pr18720.out"}
+    {"Run with libpr18720c.so 4"
+     "tmpdir/libpr18720c.so tmpdir/pr18720b.o tmpdir/pr18720a.o" ""
+     {dummy.c} "pr18720d" "pr18720.out"}
 }
 
 run_ld_link_exec_tests [] $run_tests
diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
new file mode 100644
index 0000000..482e981
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720.out
@@ -0,0 +1,2 @@
+MAIN
+DSO
diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
new file mode 100644
index 0000000..39bbcb1
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720a.c
@@ -0,0 +1,18 @@
+extern void bar (void);
+extern void foo (void);
+
+__attribute__ ((noinline, noclone))
+int
+foo_p (void)
+{
+  return (long) &foo == 0x12345678 ? 1 : 0;
+}
+
+int
+main (void)
+{
+  foo ();
+  foo_p ();
+  bar ();
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr18720b.c b/ld/testsuite/ld-elf/pr18720b.c
new file mode 100644
index 0000000..dbb37c3
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720b.c
@@ -0,0 +1,9 @@
+#include <stdio.h>
+
+void
+foo (void)
+{
+  printf ("MAIN\n");
+}
+
+asm (".symver foo,foo@FOO");
diff --git a/ld/testsuite/ld-elf/pr18720c.c b/ld/testsuite/ld-elf/pr18720c.c
new file mode 100644
index 0000000..b52cb95
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720c.c
@@ -0,0 +1,15 @@
+#include <stdio.h>
+
+extern void foo (void);
+
+void
+foo (void)
+{
+  printf ("DSO\n");
+}
+
+void
+bar (void)
+{
+  foo ();
+}
-- 
2.4.3

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[Cary Coutant]

> diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
> new file mode 100644
> index 0000000..482e981
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr18720.out
> @@ -0,0 +1,2 @@
> +MAIN
> +DSO

Why should the direct call to foo from the main program bind to the
non-default versioned symbol? It seems to me that both calls should
bind to the version in the DSO.

> diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
> new file mode 100644
> index 0000000..39bbcb1
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr18720a.c
> @@ -0,0 +1,18 @@
> +extern void bar (void);
> +extern void foo (void);
> +
> +__attribute__ ((noinline, noclone))
> +int
> +foo_p (void)
> +{
> +  return (long) &foo == 0x12345678 ? 1 : 0;
> +}

You don't have any checks to see which version the indirect reference
actually binds to.

-cary

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

On Mon, Jul 27, 2015 at 9:11 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>> diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
>> new file mode 100644
>> index 0000000..482e981
>> --- /dev/null
>> +++ b/ld/testsuite/ld-elf/pr18720.out
>> @@ -0,0 +1,2 @@
>> +MAIN
>> +DSO
>
> Why should the direct call to foo from the main program bind to the
> non-default versioned symbol? It seems to me that both calls should
> bind to the version in the DSO.

It is because foo is defined in main.

>> diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
>> new file mode 100644
>> index 0000000..39bbcb1
>> --- /dev/null
>> +++ b/ld/testsuite/ld-elf/pr18720a.c
>> @@ -0,0 +1,18 @@
>> +extern void bar (void);
>> +extern void foo (void);
>> +
>> +__attribute__ ((noinline, noclone))
>> +int
>> +foo_p (void)
>> +{
>> +  return (long) &foo == 0x12345678 ? 1 : 0;
>> +}
>
> You don't have any checks to see which version the indirect reference
> actually binds to.

You are right.  I will update the testcase.

Thanks.


-- 
H.J.

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[Cary Coutant]

>> Why should the direct call to foo from the main program bind to the
>> non-default versioned symbol? It seems to me that both calls should
>> bind to the version in the DSO.
>
> It is because foo is defined in main.

But it's not -- only foo@FOO is defined in main. If you think it's
right that main should also have an unversioned definition, then why
should the call from bar() to foo() bind to the one in the DSO? And
which foo() are you expecting the indirect reference to bind to?

-cary

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

On Mon, Jul 27, 2015 at 9:47 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>>> Why should the direct call to foo from the main program bind to the
>>> non-default versioned symbol? It seems to me that both calls should
>>> bind to the version in the DSO.
>>
>> It is because foo is defined in main.
>
> But it's not -- only foo@FOO is defined in main. If you think it's
> right that main should also have an unversioned definition, then why
> should the call from bar() to foo() bind to the one in the DSO? And
> which foo() are you expecting the indirect reference to bind to?
>

Here is our disagreement on this code:

+#include <stdio.h>
+
+void
+foo (void)
+{
+  printf ("MAIN\n");
+}
+
+asm (".symver foo,foo@FOO");

To me, this defines foo and foo@FOO.  Both points to the same
address.  This is how gas and ld work on symbol versioning for
a long time.  I don't think we should change it just because gold
behaves differently.

-- 
H.J.

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[Cary Coutant]

>> But it's not -- only foo@FOO is defined in main. If you think it's
>> right that main should also have an unversioned definition, then why
>> should the call from bar() to foo() bind to the one in the DSO? And
>> which foo() are you expecting the indirect reference to bind to?
>
> Here is our disagreement on this code:
>
> +#include <stdio.h>
> +
> +void
> +foo (void)
> +{
> +  printf ("MAIN\n");
> +}
> +
> +asm (".symver foo,foo@FOO");
>
> To me, this defines foo and foo@FOO.  Both points to the same
> address.  This is how gas and ld work on symbol versioning for
> a long time.  I don't think we should change it just because gold
> behaves differently.

OK, if you think that's correct behavior, you didn't answer why you
still expect the call from bar() to bind to the foo() in the DSO.
Without the .symver, both linkers bind to the foo() in MAIN.

I don't think we should change it *just because gold behaves
differently* -- I'm working on a fix now, but it's a bit messy. It's
just that I *don't* think it should define both foo and foo@FOO.

In the gas documentation, it's clear that .symver should be used with
different names. It always uses "name" and "name2":

        .symver name, name2@nodename

There's nothing explaining what should happen when "name" and "name2"
are the same.

The ld documentation also never shows any examples where the two are
not different symbols.

Only in the testsuites can you find examples where they are the same
symbol, but I don't think the testsuites should be the authority when
it comes to undefined behavior.

To me, if you're going to allow the two names to be the same, it
should define only one symbol. That would allow a library developer to
maintain multiple versions of a function in separate source files,
like this:

foo-orig.c:
int foo() { return 0; }
asm (".symver foo,foo@VER0");

foo-ver1.c:
int foo() { return 1; }
asm (".symver foo,foo@VER1");

foo-ver2.c:
int foo() { return 2; }
asm (".symver foo,foo@VER2");

As an alternative to this (as suggested in the ld manual):

foo.c:
int foo_orig() { return 0; }
asm (".symver foo_orig, foo@VER0");
int foo_ver1() { return 0; }
asm (".symver foo_ver1, foo@VER1");
int foo_ver2() { return 0; }
asm (".symver foo_ver2, foo@VER2");

-cary

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

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

On Mon, Jul 27, 2015 at 10:15 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>>> But it's not -- only foo@FOO is defined in main. If you think it's
>>> right that main should also have an unversioned definition, then why
>>> should the call from bar() to foo() bind to the one in the DSO? And
>>> which foo() are you expecting the indirect reference to bind to?
>>
>> Here is our disagreement on this code:
>>
>> +#include <stdio.h>
>> +
>> +void
>> +foo (void)
>> +{
>> +  printf ("MAIN\n");
>> +}
>> +
>> +asm (".symver foo,foo@FOO");
>>
>> To me, this defines foo and foo@FOO.  Both points to the same
>> address.  This is how gas and ld work on symbol versioning for
>> a long time.  I don't think we should change it just because gold
>> behaves differently.
>
> OK, if you think that's correct behavior, you didn't answer why you
> still expect the call from bar() to bind to the foo() in the DSO.

It is because foo@FOO in main is hidden from DSO.  It
will only be used by ld.so to resolve references to foo@FOO,
not the naked "foo".

> Without the .symver, both linkers bind to the foo() in MAIN.
>
> I don't think we should change it *just because gold behaves
> differently* -- I'm working on a fix now, but it's a bit messy. It's
> just that I *don't* think it should define both foo and foo@FOO.
>
> In the gas documentation, it's clear that .symver should be used with
> different names. It always uses "name" and "name2":
>
>         .symver name, name2@nodename
>
> There's nothing explaining what should happen when "name" and "name2"
> are the same.
>
> The ld documentation also never shows any examples where the two are
> not different symbols.

Improvements to ld and gas manuals are more than welcome.

> Only in the testsuites can you find examples where they are the same
> symbol, but I don't think the testsuites should be the authority when
> it comes to undefined behavior.

To me, the GNU ld testsuite is the authority of GNU linker behavior.
The undefined behaviors are those not covered by the GNU ld testsuite.

> To me, if you're going to allow the two names to be the same, it
> should define only one symbol. That would allow a library developer to
> maintain multiple versions of a function in separate source files,
> like this:
>

The current gas/linker provide flexibility to support both ways.

Here is the updated patch to check the address of foo in
main.

-- 
H.J.

[-- Attachment #2: 0001-Properly-merge-non-default-versioned-symbol.patch --]
[-- Type: text/x-patch, Size: 14083 bytes --]

From 29bb5f00b15a4a07dde3d45e03747be3c6eba3f3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 26 Jul 2015 07:59:57 -0700
Subject: [PATCH] Properly merge non-default versioned symbol

The non-default versioned symbol can only be merged with the versioned
symbol with the same symbol version.  _bfd_elf_merge_symbol should
check the symbol version before merging the new non-default versioned
symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
copy any references to the non-default versioned symbol.   We need to
bind a symbol locally when linking executable if it is locally defined,
non-default versioned, not referenced by shared library and not exported.

bfd/

	PR ld/18720
	* elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
	* elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
	if the new symbol matches the existing one.  The new non-default
	versioned symbol symbol matches the existing symbol if they have
	the same symbol version. Update the existing symbol only if they
	match.
	(_bfd_elf_add_default_symbol): Update call to
	_bfd_elf_merge_symbol.
	(elf_link_add_object_symbols): Override a definition only if the
	new symbol matches the existing one.
	(_bfd_elf_link_hash_copy_indirect): Don't copy any references to
	the non-default versioned symbol.
	(elf_link_output_extsym): Bind a symbol locally when linking
	executable if it is locally defined, non-default versioned, not
	referenced by shared library and not exported.

ld/testsuite/

	PR ld/18720
	* ld-elf/indirect.exp: Run tests for PR ld/18720.
	* ld-elf/pr18720.out: New file.
	* ld-elf/pr18720a.c: Likewise.
	* ld-elf/pr18720b.c: Likewise.
	* ld-elf/pr18720c.c: Likewise.

check address
---
 bfd/elf-bfd.h                    |   2 +
 bfd/elflink.c                    | 108 +++++++++++++++++++++++++++++++++------
 ld/testsuite/ld-elf/indirect.exp |  19 ++++++-
 ld/testsuite/ld-elf/pr18720.out  |   2 +
 ld/testsuite/ld-elf/pr18720a.c   |  23 +++++++++
 ld/testsuite/ld-elf/pr18720b.c   |  11 ++++
 ld/testsuite/ld-elf/pr18720c.c   |  15 ++++++
 7 files changed, 162 insertions(+), 18 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr18720.out
 create mode 100644 ld/testsuite/ld-elf/pr18720a.c
 create mode 100644 ld/testsuite/ld-elf/pr18720b.c
 create mode 100644 ld/testsuite/ld-elf/pr18720c.c

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index e08b2d6..aeba83e 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -203,6 +203,8 @@ struct elf_link_hash_entry
   /* Symbol is defined by a shared library with non-default visibility
      in a read/write section.  */
   unsigned int protected_def : 1;
+  /* Symbol is non-default versioned.  */
+  unsigned int nondeflt_version : 1;
 
   /* String table index in .dynstr if this is a dynamic symbol.  */
   unsigned long dynstr_index;
diff --git a/bfd/elflink.c b/bfd/elflink.c
index ccb7ba2..75b8641 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -939,7 +939,8 @@ _bfd_elf_merge_symbol (bfd *abfd,
 		       bfd_boolean *skip,
 		       bfd_boolean *override,
 		       bfd_boolean *type_change_ok,
-		       bfd_boolean *size_change_ok)
+		       bfd_boolean *size_change_ok,
+		       bfd_boolean *matched)
 {
   asection *sec, *oldsec;
   struct elf_link_hash_entry *h;
@@ -950,6 +951,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bfd_boolean newdyn, olddyn, olddef, newdef, newdyncommon, olddyncommon;
   bfd_boolean newweak, oldweak, newfunc, oldfunc;
   const struct elf_backend_data *bed;
+  char *new_version;
 
   *skip = FALSE;
   *override = FALSE;
@@ -968,6 +970,17 @@ _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 && new_version[1] != '\0')
+    {
+      if (new_version > name && new_version[-1] != ELF_VER_CHR)
+	h->nondeflt_version = 1;
+      new_version += 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.  */
   hi = h;
@@ -975,6 +988,42 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	 || h->root.type == bfd_link_hash_warning)
     h = (struct elf_link_hash_entry *) h->root.u.i.link;
 
+  if (!*matched)
+    {
+      if (hi == h || h->root.type == bfd_link_hash_new)
+	*matched = TRUE;
+      else
+	{
+	  /* OLD_HIDDEN is true if the existing symbol is only visibile
+	     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->nondeflt_version;
+	  bfd_boolean new_hidden = hi->nondeflt_version;
+	  if (!old_hidden && !new_hidden)
+	    /* The new symbol matches the existing symbol if both
+	       aren't hidden.  */
+	    *matched = TRUE;
+	  else
+	    {
+	      /* 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] != '\0')
+		old_version += 1;
+	      else
+		old_version = NULL;
+
+	      /* The new symbol matches the existing symbol if they
+		 have the same symbol version.  */
+	      *matched = (old_version != NULL
+			  && new_version != NULL
+			  && strcmp (old_version, new_version) == 0);
+	    }
+	}
+    }
+
   /* OLDBFD and OLDSEC are a BFD and an ASECTION associated with the
      existing symbol.  */
 
@@ -1047,7 +1096,9 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	}
       else
 	{
-	  h->dynamic_def = 1;
+	  /* Update the existing symbol only if they match. */
+	  if (*matched)
+	    h->dynamic_def = 1;
 	  hi->dynamic_def = 1;
 	}
     }
@@ -1618,6 +1669,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
   char *p;
   size_t len, shortlen;
   asection *tmp_sec;
+  bfd_boolean matched;
 
   /* If this symbol has a version, and it is the default version, we
      create an indirect symbol from the default name to the fully
@@ -1644,10 +1696,11 @@ _bfd_elf_add_default_symbol (bfd *abfd,
      actually going to define an indirect symbol.  */
   type_change_ok = FALSE;
   size_change_ok = FALSE;
+  matched = TRUE;
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -1762,7 +1815,7 @@ nondefault:
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -3880,6 +3933,7 @@ error_free_dyn:
       bfd_boolean common;
       unsigned int old_alignment;
       bfd *old_bfd;
+      bfd_boolean matched;
 
       override = FALSE;
 
@@ -4014,6 +4068,7 @@ error_free_dyn:
       size_change_ok = FALSE;
       type_change_ok = bed->type_change_ok;
       old_weak = FALSE;
+      matched = FALSE;
       old_alignment = 0;
       old_bfd = NULL;
       new_sec = sec;
@@ -4143,13 +4198,16 @@ error_free_dyn:
 	  if (!_bfd_elf_merge_symbol (abfd, info, name, isym, &sec, &value,
 				      sym_hash, &old_bfd, &old_weak,
 				      &old_alignment, &skip, &override,
-				      &type_change_ok, &size_change_ok))
+				      &type_change_ok, &size_change_ok,
+				      &matched))
 	    goto error_free_vers;
 
 	  if (skip)
 	    continue;
 
-	  if (override)
+	  /* Override a definition only if the new symbol matches the
+	     existing one.  */
+	  if (override && matched)
 	    definition = FALSE;
 
 	  h = *sym_hash;
@@ -6804,14 +6862,18 @@ _bfd_elf_link_hash_copy_indirect (struct bfd_link_info *info,
   struct elf_link_hash_table *htab;
 
   /* Copy down any references that we may have already seen to the
-     symbol which just became indirect.  */
+     symbol which just became indirect if DIR isn't a non-default
+     versioned symbol.  */
 
-  dir->ref_dynamic |= ind->ref_dynamic;
-  dir->ref_regular |= ind->ref_regular;
-  dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
-  dir->non_got_ref |= ind->non_got_ref;
-  dir->needs_plt |= ind->needs_plt;
-  dir->pointer_equality_needed |= ind->pointer_equality_needed;
+  if (!dir->nondeflt_version)
+    {
+      dir->ref_dynamic |= ind->ref_dynamic;
+      dir->ref_regular |= ind->ref_regular;
+      dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
+      dir->non_got_ref |= ind->non_got_ref;
+      dir->needs_plt |= ind->needs_plt;
+      dir->pointer_equality_needed |= ind->pointer_equality_needed;
+    }
 
   if (ind->root.type != bfd_link_hash_indirect)
     return;
@@ -8898,6 +8960,16 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   const struct elf_backend_data *bed;
   long indx;
   int ret;
+  /* A symbol is bound locally if it is forced local or it is locally
+     defined, non-default versioned, not referenced by shared library
+     and not exported when linking executable.  */
+  bfd_boolean local_bind = (h->forced_local
+			    || (flinfo->info->executable
+				&& !flinfo->info->export_dynamic
+				&& !h->dynamic
+				&& !h->ref_dynamic
+				&& h->def_regular
+				&& h->nondeflt_version));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -8909,12 +8981,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* Decide whether to output this symbol in this pass.  */
   if (eoinfo->localsyms)
     {
-      if (!h->forced_local)
+      if (!local_bind)
 	return TRUE;
     }
   else
     {
-      if (h->forced_local)
+      if (local_bind)
 	return TRUE;
     }
 
@@ -9036,7 +9108,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   sym.st_value = 0;
   sym.st_size = h->size;
   sym.st_other = h->other;
-  if (h->forced_local)
+  if (local_bind)
     {
       sym.st_info = ELF_ST_INFO (STB_LOCAL, h->type);
       /* Turn off visibility on local symbol.  */
@@ -9218,8 +9290,10 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
       /* Since there is no version information in the dynamic string,
 	 if there is no version info in symbol version section, we will
 	 have a run-time problem if not linking executable, referenced
-	 by shared library, or not locally defined.  */
+	 by shared library, not locally defined, or not bound locally.
+      */
       if (h->verinfo.verdef == NULL
+	  && !local_bind
 	  && (!flinfo->info->executable
 	      || h->ref_dynamic
 	      || !h->def_regular))
diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
index 468ef2b..539112a 100644
--- a/ld/testsuite/ld-elf/indirect.exp
+++ b/ld/testsuite/ld-elf/indirect.exp
@@ -64,7 +64,9 @@ if { ![ld_compile $CC $srcdir/$subdir/indirect1a.c tmpdir/indirect1a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3a.c tmpdir/indirect3a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3b.c tmpdir/indirect3b.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect4a.c tmpdir/indirect4a.o]
-     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o] } {
+     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o]
+     || ![ld_compile "$CC -O2 -fPIC -I../bfd" $srcdir/$subdir/pr18720a.c tmpdir/pr18720a.o]
+     || ![ld_compile $CC $srcdir/$subdir/pr18720b.c tmpdir/pr18720b.o] } {
     unresolved "Indirect symbol tests"
     return
 }
@@ -79,6 +81,9 @@ set build_tests {
   {"Build libindirect4c.so"
    "-shared" "-fPIC"
    {indirect4c.c} {} "libindirect4c.so"}
+  {"Build libpr18720c.so"
+   "-shared" "-fPIC"
+   {pr18720c.c} {} "libpr18720c.so"}
 }
 
 run_cc_link_tests $build_tests
@@ -132,6 +137,18 @@ set run_tests {
     {"Run with libindirect4c.so 4"
      "tmpdir/libindirect4c.so tmpdir/indirect4b.o tmpdir/indirect4a.o" ""
      {dummy.c} "indirect4d" "indirect4.out"}
+    {"Run with libpr18720c.so 1"
+     "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
+     {check-ptr-eq.c} "pr18720a" "pr18720.out"}
+    {"Run with libpr18720c.so 2"
+     "tmpdir/pr18720a.o tmpdir/libpr18720c.so tmpdir/pr18720b.o" ""
+     {check-ptr-eq.c} "pr18720b" "pr18720.out"}
+    {"Run with libpr18720c.so 3"
+     "tmpdir/pr18720b.o tmpdir/libpr18720c.so tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720c" "pr18720.out"}
+    {"Run with libpr18720c.so 4"
+     "tmpdir/libpr18720c.so tmpdir/pr18720b.o tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720d" "pr18720.out"}
 }
 
 run_ld_link_exec_tests [] $run_tests
diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
new file mode 100644
index 0000000..482e981
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720.out
@@ -0,0 +1,2 @@
+MAIN
+DSO
diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
new file mode 100644
index 0000000..dbbac5e
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720a.c
@@ -0,0 +1,23 @@
+#include <bfd_stdint.h>
+
+extern void bar (void);
+extern void foo (void);
+extern void foo_alias (void);
+extern void check_ptr_eq (void *, void *);
+
+__attribute__ ((noinline, noclone))
+int
+foo_p (void)
+{
+  return (intptr_t) &foo == 0x12345678 ? 1 : 0;
+}
+
+int
+main (void)
+{
+  foo ();
+  foo_p ();
+  bar ();
+  check_ptr_eq (&foo, &foo_alias);
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr18720b.c b/ld/testsuite/ld-elf/pr18720b.c
new file mode 100644
index 0000000..90d376b
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720b.c
@@ -0,0 +1,11 @@
+#include <stdio.h>
+
+void
+foo (void)
+{
+  printf ("MAIN\n");
+}
+
+asm (".symver foo,foo@FOO");
+asm (".set foo_alias,foo");
+asm (".global foo_alias");
diff --git a/ld/testsuite/ld-elf/pr18720c.c b/ld/testsuite/ld-elf/pr18720c.c
new file mode 100644
index 0000000..b52cb95
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720c.c
@@ -0,0 +1,15 @@
+#include <stdio.h>
+
+extern void foo (void);
+
+void
+foo (void)
+{
+  printf ("DSO\n");
+}
+
+void
+bar (void)
+{
+  foo ();
+}
-- 
2.4.3

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[Cary Coutant]

>>> To me, this defines foo and foo@FOO.  Both points to the same
>>> address.  This is how gas and ld work on symbol versioning for
>>> a long time.  I don't think we should change it just because gold
>>> behaves differently.
>>
>> OK, if you think that's correct behavior, you didn't answer why you
>> still expect the call from bar() to bind to the foo() in the DSO.
>
> It is because foo@FOO in main is hidden from DSO.  It
> will only be used by ld.so to resolve references to foo@FOO,
> not the naked "foo".

But you just said "this defines foo and foo@FOO"!

This whole business of combining Sun-style versioning with Gnu-style
versioning is a mess. The de facto behavior as implemented by gas and
gnu ld don't conform to any rational model that I can imagine.

-cary

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

On Mon, Jul 27, 2015 at 10:50 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>>>> To me, this defines foo and foo@FOO.  Both points to the same
>>>> address.  This is how gas and ld work on symbol versioning for
>>>> a long time.  I don't think we should change it just because gold
>>>> behaves differently.
>>>
>>> OK, if you think that's correct behavior, you didn't answer why you
>>> still expect the call from bar() to bind to the foo() in the DSO.
>>
>> It is because foo@FOO in main is hidden from DSO.  It
>> will only be used by ld.so to resolve references to foo@FOO,
>> not the naked "foo".
>
> But you just said "this defines foo and foo@FOO"!

Yes, only for link-time, not for run-time.


-- 
H.J.

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[Cary Coutant]

>> To me, if you're going to allow the two names to be the same, it
>> should define only one symbol. That would allow a library developer to
>> maintain multiple versions of a function in separate source files,
>> like this:
>
> The current gas/linker provide flexibility to support both ways.

No, it doesn't. If you try to link foo-orig.o, foo-ver1.o, and
foo-ver2.o, you'll get multiply-defined symbol errors for "foo".

-cary

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

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

On Mon, Jul 27, 2015 at 10:53 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 27, 2015 at 10:50 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>>>>> To me, this defines foo and foo@FOO.  Both points to the same
>>>>> address.  This is how gas and ld work on symbol versioning for
>>>>> a long time.  I don't think we should change it just because gold
>>>>> behaves differently.
>>>>
>>>> OK, if you think that's correct behavior, you didn't answer why you
>>>> still expect the call from bar() to bind to the foo() in the DSO.
>>>
>>> It is because foo@FOO in main is hidden from DSO.  It
>>> will only be used by ld.so to resolve references to foo@FOO,
>>> not the naked "foo".
>>
>> But you just said "this defines foo and foo@FOO"!
>
> Yes, only for link-time, not for run-time.
>

It turns out that there is a hidden field for non-default versioned
symbol already.  Here is the updated patch to use it instead of
adding a new field.

It is true that the GNU symbol version is very poorly documented.
We ran into all kinds of problems when it was first introduced.  We
added a testcase to the ld testsuite for each problem we fixed.


-- 
H.J.

[-- Attachment #2: 0001-Properly-merge-hidden-versioned-symbol.patch --]
[-- Type: text/x-patch, Size: 14964 bytes --]

From 459c6d6b593adf0ddc339a4c1e8ee796f1aa9e40 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 26 Jul 2015 07:59:57 -0700
Subject: [PATCH] Properly merge hidden versioned symbol

The hidden versioned symbol can only be merged with the versioned
symbol with the same symbol version.  _bfd_elf_merge_symbol should
check the symbol version before merging the new hidden versioned
symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
copy any references to the hidden versioned symbol.   We need to
bind a symbol locally when linking executable if it is locally defined,
hidden versioned, not referenced by shared library and not exported.

bfd/

	PR ld/18720
	* elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
	if the new symbol matches the existing one.  The new hidden
	versioned symbol matches the existing symbol if they have the
	same symbol version. Update the existing symbol only if they
	match.
	(_bfd_elf_add_default_symbol): Update call to
	_bfd_elf_merge_symbol.
	(_bfd_elf_link_assign_sym_version): Don't set the hidden field
	here.
	(elf_link_add_object_symbols): Override a definition only if the
	new symbol matches the existing one.
	(_bfd_elf_link_hash_copy_indirect): Don't copy any references to
	the hidden versioned symbol.
	(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
	locally.

ld/testsuite/

	PR ld/18720
	* ld-elf/indirect.exp: Run tests for PR ld/18720.
	* ld-elf/pr18720.out: New file.
	* ld-elf/pr18720a.c: Likewise.
	* ld-elf/pr18720b.c: Likewise.
	* ld-elf/pr18720c.c: Likewise.
---
 bfd/elflink.c                    | 134 +++++++++++++++++++++++++++++----------
 ld/testsuite/ld-elf/indirect.exp |  19 +++++-
 ld/testsuite/ld-elf/pr18720.out  |   2 +
 ld/testsuite/ld-elf/pr18720a.c   |  23 +++++++
 ld/testsuite/ld-elf/pr18720b.c   |  11 ++++
 ld/testsuite/ld-elf/pr18720c.c   |  15 +++++
 6 files changed, 168 insertions(+), 36 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr18720.out
 create mode 100644 ld/testsuite/ld-elf/pr18720a.c
 create mode 100644 ld/testsuite/ld-elf/pr18720b.c
 create mode 100644 ld/testsuite/ld-elf/pr18720c.c

diff --git a/bfd/elflink.c b/bfd/elflink.c
index ccb7ba2..220a722 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -939,7 +939,8 @@ _bfd_elf_merge_symbol (bfd *abfd,
 		       bfd_boolean *skip,
 		       bfd_boolean *override,
 		       bfd_boolean *type_change_ok,
-		       bfd_boolean *size_change_ok)
+		       bfd_boolean *size_change_ok,
+		       bfd_boolean *matched)
 {
   asection *sec, *oldsec;
   struct elf_link_hash_entry *h;
@@ -950,6 +951,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bfd_boolean newdyn, olddyn, olddef, newdef, newdyncommon, olddyncommon;
   bfd_boolean newweak, oldweak, newfunc, oldfunc;
   const struct elf_backend_data *bed;
+  char *new_version;
 
   *skip = FALSE;
   *override = FALSE;
@@ -968,6 +970,17 @@ _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 (new_version > name && new_version[-1] != ELF_VER_CHR)
+	h->hidden = 1;
+      new_version += 1;
+      if (new_version[0] == '\0')
+	new_version = NULL;
+    }
+
   /* For merging, we only care about real symbols.  But we need to make
      sure that indirect symbol dynamic flags are updated.  */
   hi = h;
@@ -975,6 +988,45 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	 || h->root.type == bfd_link_hash_warning)
     h = (struct elf_link_hash_entry *) h->root.u.i.link;
 
+  if (!*matched)
+    {
+      if (hi == h || h->root.type == bfd_link_hash_new)
+	*matched = TRUE;
+      else
+	{
+	  /* OLD_HIDDEN is true if the existing symbol is only visibile
+	     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;
+	  if (!old_hidden && !new_hidden)
+	    /* The new symbol matches the existing symbol if both
+	       aren't hidden.  */
+	    *matched = TRUE;
+	  else
+	    {
+	      /* 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;
+		}
+
+	      /* The new symbol matches the existing symbol if they
+		 have the same symbol version.  */
+	      *matched = (old_version == new_version
+			  || (old_version != NULL
+			      && new_version != NULL
+			      && strcmp (old_version, new_version) == 0));
+	    }
+	}
+    }
+
   /* OLDBFD and OLDSEC are a BFD and an ASECTION associated with the
      existing symbol.  */
 
@@ -1047,7 +1099,9 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	}
       else
 	{
-	  h->dynamic_def = 1;
+	  /* Update the existing symbol only if they match. */
+	  if (*matched)
+	    h->dynamic_def = 1;
 	  hi->dynamic_def = 1;
 	}
     }
@@ -1618,6 +1672,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
   char *p;
   size_t len, shortlen;
   asection *tmp_sec;
+  bfd_boolean matched;
 
   /* If this symbol has a version, and it is the default version, we
      create an indirect symbol from the default name to the fully
@@ -1644,10 +1699,11 @@ _bfd_elf_add_default_symbol (bfd *abfd,
      actually going to define an indirect symbol.  */
   type_change_ok = FALSE;
   size_change_ok = FALSE;
+  matched = TRUE;
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -1762,7 +1818,7 @@ nondefault:
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -1972,26 +2028,14 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
   if (p != NULL && h->verinfo.vertree == NULL)
     {
       struct bfd_elf_version_tree *t;
-      bfd_boolean hidden;
 
-      hidden = TRUE;
-
-      /* There are two consecutive ELF_VER_CHR characters if this is
-	 not a hidden symbol.  */
       ++p;
       if (*p == ELF_VER_CHR)
-	{
-	  hidden = FALSE;
-	  ++p;
-	}
+	++p;
 
       /* If there is no version string, we can just return out.  */
       if (*p == '\0')
-	{
-	  if (hidden)
-	    h->hidden = 1;
-	  return TRUE;
-	}
+	return TRUE;
 
       /* Look for the version.  If we find it, it is no longer weak.  */
       for (t = sinfo->info->version_info; t != NULL; t = t->next)
@@ -2087,9 +2131,6 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
 	  sinfo->failed = TRUE;
 	  return FALSE;
 	}
-
-      if (hidden)
-	h->hidden = 1;
     }
 
   /* If we don't have a version for this symbol, see if we can find
@@ -3880,6 +3921,7 @@ error_free_dyn:
       bfd_boolean common;
       unsigned int old_alignment;
       bfd *old_bfd;
+      bfd_boolean matched;
 
       override = FALSE;
 
@@ -4014,6 +4056,7 @@ error_free_dyn:
       size_change_ok = FALSE;
       type_change_ok = bed->type_change_ok;
       old_weak = FALSE;
+      matched = FALSE;
       old_alignment = 0;
       old_bfd = NULL;
       new_sec = sec;
@@ -4143,13 +4186,16 @@ error_free_dyn:
 	  if (!_bfd_elf_merge_symbol (abfd, info, name, isym, &sec, &value,
 				      sym_hash, &old_bfd, &old_weak,
 				      &old_alignment, &skip, &override,
-				      &type_change_ok, &size_change_ok))
+				      &type_change_ok, &size_change_ok,
+				      &matched))
 	    goto error_free_vers;
 
 	  if (skip)
 	    continue;
 
-	  if (override)
+	  /* Override a definition only if the new symbol matches the
+	     existing one.  */
+	  if (override && matched)
 	    definition = FALSE;
 
 	  h = *sym_hash;
@@ -6804,14 +6850,18 @@ _bfd_elf_link_hash_copy_indirect (struct bfd_link_info *info,
   struct elf_link_hash_table *htab;
 
   /* Copy down any references that we may have already seen to the
-     symbol which just became indirect.  */
+     symbol which just became indirect if DIR isn't a hidden versioned
+     symbol.  */
 
-  dir->ref_dynamic |= ind->ref_dynamic;
-  dir->ref_regular |= ind->ref_regular;
-  dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
-  dir->non_got_ref |= ind->non_got_ref;
-  dir->needs_plt |= ind->needs_plt;
-  dir->pointer_equality_needed |= ind->pointer_equality_needed;
+  if (!dir->hidden)
+    {
+      dir->ref_dynamic |= ind->ref_dynamic;
+      dir->ref_regular |= ind->ref_regular;
+      dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
+      dir->non_got_ref |= ind->non_got_ref;
+      dir->needs_plt |= ind->needs_plt;
+      dir->pointer_equality_needed |= ind->pointer_equality_needed;
+    }
 
   if (ind->root.type != bfd_link_hash_indirect)
     return;
@@ -8898,6 +8948,16 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   const struct elf_backend_data *bed;
   long indx;
   int ret;
+  /* A symbol is bound locally if it is forced local or it is locally
+     defined, hidden versioned, not referenced by shared library and
+     not exported when linking executable.  */
+  bfd_boolean local_bind = (h->forced_local
+			    || (flinfo->info->executable
+				&& !flinfo->info->export_dynamic
+				&& !h->dynamic
+				&& !h->ref_dynamic
+				&& h->def_regular
+				&& h->hidden));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -8909,12 +8969,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* Decide whether to output this symbol in this pass.  */
   if (eoinfo->localsyms)
     {
-      if (!h->forced_local)
+      if (!local_bind)
 	return TRUE;
     }
   else
     {
-      if (h->forced_local)
+      if (local_bind)
 	return TRUE;
     }
 
@@ -9036,7 +9096,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   sym.st_value = 0;
   sym.st_size = h->size;
   sym.st_other = h->other;
-  if (h->forced_local)
+  if (local_bind)
     {
       sym.st_info = ELF_ST_INFO (STB_LOCAL, h->type);
       /* Turn off visibility on local symbol.  */
@@ -9218,8 +9278,10 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
       /* Since there is no version information in the dynamic string,
 	 if there is no version info in symbol version section, we will
 	 have a run-time problem if not linking executable, referenced
-	 by shared library, or not locally defined.  */
+	 by shared library, not locally defined, or not bound locally.
+      */
       if (h->verinfo.verdef == NULL
+	  && !local_bind
 	  && (!flinfo->info->executable
 	      || h->ref_dynamic
 	      || !h->def_regular))
@@ -9292,7 +9354,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
 		iversym.vs_vers++;
 	    }
 
-	  if (h->hidden)
+	  /* Turn on VERSYM_HIDDEN only if the hidden vesioned symbol is
+	     defined locally.  */
+	  if (h->hidden && h->def_regular)
 	    iversym.vs_vers |= VERSYM_HIDDEN;
 
 	  eversym = (Elf_External_Versym *) flinfo->symver_sec->contents;
diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
index 468ef2b..539112a 100644
--- a/ld/testsuite/ld-elf/indirect.exp
+++ b/ld/testsuite/ld-elf/indirect.exp
@@ -64,7 +64,9 @@ if { ![ld_compile $CC $srcdir/$subdir/indirect1a.c tmpdir/indirect1a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3a.c tmpdir/indirect3a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3b.c tmpdir/indirect3b.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect4a.c tmpdir/indirect4a.o]
-     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o] } {
+     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o]
+     || ![ld_compile "$CC -O2 -fPIC -I../bfd" $srcdir/$subdir/pr18720a.c tmpdir/pr18720a.o]
+     || ![ld_compile $CC $srcdir/$subdir/pr18720b.c tmpdir/pr18720b.o] } {
     unresolved "Indirect symbol tests"
     return
 }
@@ -79,6 +81,9 @@ set build_tests {
   {"Build libindirect4c.so"
    "-shared" "-fPIC"
    {indirect4c.c} {} "libindirect4c.so"}
+  {"Build libpr18720c.so"
+   "-shared" "-fPIC"
+   {pr18720c.c} {} "libpr18720c.so"}
 }
 
 run_cc_link_tests $build_tests
@@ -132,6 +137,18 @@ set run_tests {
     {"Run with libindirect4c.so 4"
      "tmpdir/libindirect4c.so tmpdir/indirect4b.o tmpdir/indirect4a.o" ""
      {dummy.c} "indirect4d" "indirect4.out"}
+    {"Run with libpr18720c.so 1"
+     "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
+     {check-ptr-eq.c} "pr18720a" "pr18720.out"}
+    {"Run with libpr18720c.so 2"
+     "tmpdir/pr18720a.o tmpdir/libpr18720c.so tmpdir/pr18720b.o" ""
+     {check-ptr-eq.c} "pr18720b" "pr18720.out"}
+    {"Run with libpr18720c.so 3"
+     "tmpdir/pr18720b.o tmpdir/libpr18720c.so tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720c" "pr18720.out"}
+    {"Run with libpr18720c.so 4"
+     "tmpdir/libpr18720c.so tmpdir/pr18720b.o tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720d" "pr18720.out"}
 }
 
 run_ld_link_exec_tests [] $run_tests
diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
new file mode 100644
index 0000000..482e981
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720.out
@@ -0,0 +1,2 @@
+MAIN
+DSO
diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
new file mode 100644
index 0000000..dbbac5e
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720a.c
@@ -0,0 +1,23 @@
+#include <bfd_stdint.h>
+
+extern void bar (void);
+extern void foo (void);
+extern void foo_alias (void);
+extern void check_ptr_eq (void *, void *);
+
+__attribute__ ((noinline, noclone))
+int
+foo_p (void)
+{
+  return (intptr_t) &foo == 0x12345678 ? 1 : 0;
+}
+
+int
+main (void)
+{
+  foo ();
+  foo_p ();
+  bar ();
+  check_ptr_eq (&foo, &foo_alias);
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr18720b.c b/ld/testsuite/ld-elf/pr18720b.c
new file mode 100644
index 0000000..90d376b
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720b.c
@@ -0,0 +1,11 @@
+#include <stdio.h>
+
+void
+foo (void)
+{
+  printf ("MAIN\n");
+}
+
+asm (".symver foo,foo@FOO");
+asm (".set foo_alias,foo");
+asm (".global foo_alias");
diff --git a/ld/testsuite/ld-elf/pr18720c.c b/ld/testsuite/ld-elf/pr18720c.c
new file mode 100644
index 0000000..b52cb95
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720c.c
@@ -0,0 +1,15 @@
+#include <stdio.h>
+
+extern void foo (void);
+
+void
+foo (void)
+{
+  printf ("DSO\n");
+}
+
+void
+bar (void)
+{
+  foo ();
+}
-- 
2.4.3

Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol

[H.J. Lu]

On Mon, Jul 27, 2015 at 12:27 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>>> To me, if you're going to allow the two names to be the same, it
>>> should define only one symbol. That would allow a library developer to
>>> maintain multiple versions of a function in separate source files,
>>> like this:
>>
>> The current gas/linker provide flexibility to support both ways.
>
> No, it doesn't. If you try to link foo-orig.o, foo-ver1.o, and
> foo-ver2.o, you'll get multiply-defined symbol errors for "foo".
>

That is how GNU symbol version works.  You can improve it only without
breaking ld testsuite.

-- 
H.J.

[PATCH] PR ld/18720: Properly merge hidden versioned symbol

[H.J. Lu]

On Sun, Jul 26, 2015 at 03:15:50PM -0700, H.J. Lu wrote:
> The non-default versioned symbol can only be merged with the versioned
> symbol with the same symbol version.  _bfd_elf_merge_symbol should
> check the symbol version before merging the new non-default versioned
> symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
> copy any references to the non-default versioned symbol.   We need to
> bind a symbol locally when linking executable if it is locally defined,
> non-default versioned, not referenced by shared library and not exported.
> 
> OK for master?
> 
> Thanks.
> 
> H.J.
> ---
> bfd/
> 
> 	PR ld/18720
> 	* elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
> 	* elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
> 	if the new symbol matches the existing one.  The new non-default
> 	versioned symbol symbol matches the existing symbol if they have
> 	the same symbol version. Update the existing symbol only if they
> 	match.
> 	(_bfd_elf_add_default_symbol): Update call to
> 	_bfd_elf_merge_symbol.
> 	(elf_link_add_object_symbols): Override a definition only if the
> 	new symbol matches the existing one.
> 	(_bfd_elf_link_hash_copy_indirect): Don't copy any references to
> 	the non-default versioned symbol.
> 	(elf_link_output_extsym): Bind a symbol locally when linking
> 	executable if it is locally defined, non-default versioned, not
> 	referenced by shared library and not exported.
> 

Here is the upated patch which uses the existing "hidden" field in
elf_link_hash_entry.  Any objections, comments?

Thanks.

H.J.
--
bfd/

	PR ld/18720
	* elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
	if the new symbol matches the existing one.  The new hidden
	versioned symbol matches the existing symbol if they have the
	same symbol version. Update the existing symbol only if they
	match.
	(_bfd_elf_add_default_symbol): Update call to
	_bfd_elf_merge_symbol.
	(_bfd_elf_link_assign_sym_version): Don't set the hidden field
	here.
	(elf_link_add_object_symbols): Override a definition only if the
	new symbol matches the existing one.
	(_bfd_elf_link_hash_copy_indirect): Don't copy any references to
	the hidden versioned symbol.
	(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
	locally.

ld/testsuite/

	PR ld/18720
	* ld-elf/indirect.exp: Run tests for PR ld/18720.
	* ld-elf/pr18720.out: New file.
	* ld-elf/pr18720a.c: Likewise.
	* ld-elf/pr18720b.c: Likewise.
	* ld-elf/pr18720c.c: Likewise.
---
 bfd/elflink.c                    | 134 +++++++++++++++++++++++++++++----------
 ld/testsuite/ld-elf/indirect.exp |  25 +++++++-
 ld/testsuite/ld-elf/pr18720.out  |   2 +
 ld/testsuite/ld-elf/pr18720a.c   |  27 ++++++++
 ld/testsuite/ld-elf/pr18720b.c   |  11 ++++
 ld/testsuite/ld-elf/pr18720c.c   |  15 +++++
 6 files changed, 178 insertions(+), 36 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr18720.out
 create mode 100644 ld/testsuite/ld-elf/pr18720a.c
 create mode 100644 ld/testsuite/ld-elf/pr18720b.c
 create mode 100644 ld/testsuite/ld-elf/pr18720c.c

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 846f35e..832b374 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -939,7 +939,8 @@ _bfd_elf_merge_symbol (bfd *abfd,
 		       bfd_boolean *skip,
 		       bfd_boolean *override,
 		       bfd_boolean *type_change_ok,
-		       bfd_boolean *size_change_ok)
+		       bfd_boolean *size_change_ok,
+		       bfd_boolean *matched)
 {
   asection *sec, *oldsec;
   struct elf_link_hash_entry *h;
@@ -950,6 +951,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
   bfd_boolean newdyn, olddyn, olddef, newdef, newdyncommon, olddyncommon;
   bfd_boolean newweak, oldweak, newfunc, oldfunc;
   const struct elf_backend_data *bed;
+  char *new_version;
 
   *skip = FALSE;
   *override = FALSE;
@@ -968,6 +970,17 @@ _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 (new_version > name && new_version[-1] != ELF_VER_CHR)
+	h->hidden = 1;
+      new_version += 1;
+      if (new_version[0] == '\0')
+	new_version = NULL;
+    }
+
   /* For merging, we only care about real symbols.  But we need to make
      sure that indirect symbol dynamic flags are updated.  */
   hi = h;
@@ -975,6 +988,45 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	 || h->root.type == bfd_link_hash_warning)
     h = (struct elf_link_hash_entry *) h->root.u.i.link;
 
+  if (!*matched)
+    {
+      if (hi == h || h->root.type == bfd_link_hash_new)
+	*matched = TRUE;
+      else
+	{
+	  /* OLD_HIDDEN is true if the existing symbol is only visibile
+	     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;
+	  if (!old_hidden && !new_hidden)
+	    /* The new symbol matches the existing symbol if both
+	       aren't hidden.  */
+	    *matched = TRUE;
+	  else
+	    {
+	      /* 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;
+		}
+
+	      /* The new symbol matches the existing symbol if they
+		 have the same symbol version.  */
+	      *matched = (old_version == new_version
+			  || (old_version != NULL
+			      && new_version != NULL
+			      && strcmp (old_version, new_version) == 0));
+	    }
+	}
+    }
+
   /* OLDBFD and OLDSEC are a BFD and an ASECTION associated with the
      existing symbol.  */
 
@@ -1047,7 +1099,9 @@ _bfd_elf_merge_symbol (bfd *abfd,
 	}
       else
 	{
-	  h->dynamic_def = 1;
+	  /* Update the existing symbol only if they match. */
+	  if (*matched)
+	    h->dynamic_def = 1;
 	  hi->dynamic_def = 1;
 	}
     }
@@ -1618,6 +1672,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
   char *p;
   size_t len, shortlen;
   asection *tmp_sec;
+  bfd_boolean matched;
 
   /* If this symbol has a version, and it is the default version, we
      create an indirect symbol from the default name to the fully
@@ -1644,10 +1699,11 @@ _bfd_elf_add_default_symbol (bfd *abfd,
      actually going to define an indirect symbol.  */
   type_change_ok = FALSE;
   size_change_ok = FALSE;
+  matched = TRUE;
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -1767,7 +1823,7 @@ nondefault:
   tmp_sec = sec;
   if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
 			      &hi, poldbfd, NULL, NULL, &skip, &override,
-			      &type_change_ok, &size_change_ok))
+			      &type_change_ok, &size_change_ok, &matched))
     return FALSE;
 
   if (skip)
@@ -1977,26 +2033,14 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
   if (p != NULL && h->verinfo.vertree == NULL)
     {
       struct bfd_elf_version_tree *t;
-      bfd_boolean hidden;
 
-      hidden = TRUE;
-
-      /* There are two consecutive ELF_VER_CHR characters if this is
-	 not a hidden symbol.  */
       ++p;
       if (*p == ELF_VER_CHR)
-	{
-	  hidden = FALSE;
-	  ++p;
-	}
+	++p;
 
       /* If there is no version string, we can just return out.  */
       if (*p == '\0')
-	{
-	  if (hidden)
-	    h->hidden = 1;
-	  return TRUE;
-	}
+	return TRUE;
 
       /* Look for the version.  If we find it, it is no longer weak.  */
       for (t = sinfo->info->version_info; t != NULL; t = t->next)
@@ -2092,9 +2136,6 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
 	  sinfo->failed = TRUE;
 	  return FALSE;
 	}
-
-      if (hidden)
-	h->hidden = 1;
     }
 
   /* If we don't have a version for this symbol, see if we can find
@@ -3885,6 +3926,7 @@ error_free_dyn:
       bfd_boolean common;
       unsigned int old_alignment;
       bfd *old_bfd;
+      bfd_boolean matched;
 
       override = FALSE;
 
@@ -4019,6 +4061,7 @@ error_free_dyn:
       size_change_ok = FALSE;
       type_change_ok = bed->type_change_ok;
       old_weak = FALSE;
+      matched = FALSE;
       old_alignment = 0;
       old_bfd = NULL;
       new_sec = sec;
@@ -4148,13 +4191,16 @@ error_free_dyn:
 	  if (!_bfd_elf_merge_symbol (abfd, info, name, isym, &sec, &value,
 				      sym_hash, &old_bfd, &old_weak,
 				      &old_alignment, &skip, &override,
-				      &type_change_ok, &size_change_ok))
+				      &type_change_ok, &size_change_ok,
+				      &matched))
 	    goto error_free_vers;
 
 	  if (skip)
 	    continue;
 
-	  if (override)
+	  /* Override a definition only if the new symbol matches the
+	     existing one.  */
+	  if (override && matched)
 	    definition = FALSE;
 
 	  h = *sym_hash;
@@ -6810,14 +6856,18 @@ _bfd_elf_link_hash_copy_indirect (struct bfd_link_info *info,
   struct elf_link_hash_table *htab;
 
   /* Copy down any references that we may have already seen to the
-     symbol which just became indirect.  */
+     symbol which just became indirect if DIR isn't a hidden versioned
+     symbol.  */
 
-  dir->ref_dynamic |= ind->ref_dynamic;
-  dir->ref_regular |= ind->ref_regular;
-  dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
-  dir->non_got_ref |= ind->non_got_ref;
-  dir->needs_plt |= ind->needs_plt;
-  dir->pointer_equality_needed |= ind->pointer_equality_needed;
+  if (!dir->hidden)
+    {
+      dir->ref_dynamic |= ind->ref_dynamic;
+      dir->ref_regular |= ind->ref_regular;
+      dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
+      dir->non_got_ref |= ind->non_got_ref;
+      dir->needs_plt |= ind->needs_plt;
+      dir->pointer_equality_needed |= ind->pointer_equality_needed;
+    }
 
   if (ind->root.type != bfd_link_hash_indirect)
     return;
@@ -8904,6 +8954,16 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   const struct elf_backend_data *bed;
   long indx;
   int ret;
+  /* A symbol is bound locally if it is forced local or it is locally
+     defined, hidden versioned, not referenced by shared library and
+     not exported when linking executable.  */
+  bfd_boolean local_bind = (h->forced_local
+			    || (flinfo->info->executable
+				&& !flinfo->info->export_dynamic
+				&& !h->dynamic
+				&& !h->ref_dynamic
+				&& h->def_regular
+				&& h->hidden));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -8915,12 +8975,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* Decide whether to output this symbol in this pass.  */
   if (eoinfo->localsyms)
     {
-      if (!h->forced_local)
+      if (!local_bind)
 	return TRUE;
     }
   else
     {
-      if (h->forced_local)
+      if (local_bind)
 	return TRUE;
     }
 
@@ -9041,7 +9101,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   sym.st_value = 0;
   sym.st_size = h->size;
   sym.st_other = h->other;
-  if (h->forced_local)
+  if (local_bind)
     {
       sym.st_info = ELF_ST_INFO (STB_LOCAL, h->type);
       /* Turn off visibility on local symbol.  */
@@ -9223,8 +9283,10 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
       /* Since there is no version information in the dynamic string,
 	 if there is no version info in symbol version section, we will
 	 have a run-time problem if not linking executable, referenced
-	 by shared library, or not locally defined.  */
+	 by shared library, not locally defined, or not bound locally.
+      */
       if (h->verinfo.verdef == NULL
+	  && !local_bind
 	  && (!flinfo->info->executable
 	      || h->ref_dynamic
 	      || !h->def_regular))
@@ -9297,7 +9359,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
 		iversym.vs_vers++;
 	    }
 
-	  if (h->hidden)
+	  /* Turn on VERSYM_HIDDEN only if the hidden vesioned symbol is
+	     defined locally.  */
+	  if (h->hidden && h->def_regular)
 	    iversym.vs_vers |= VERSYM_HIDDEN;
 
 	  eversym = (Elf_External_Versym *) flinfo->symver_sec->contents;
diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
index 468ef2b..e8ac1ae 100644
--- a/ld/testsuite/ld-elf/indirect.exp
+++ b/ld/testsuite/ld-elf/indirect.exp
@@ -64,7 +64,9 @@ if { ![ld_compile $CC $srcdir/$subdir/indirect1a.c tmpdir/indirect1a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3a.c tmpdir/indirect3a.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect3b.c tmpdir/indirect3b.o]
      || ![ld_compile $CC $srcdir/$subdir/indirect4a.c tmpdir/indirect4a.o]
-     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o] } {
+     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o]
+     || ![ld_compile "$CC -O2 -fPIC -I../bfd" $srcdir/$subdir/pr18720a.c tmpdir/pr18720a.o]
+     || ![ld_compile $CC $srcdir/$subdir/pr18720b.c tmpdir/pr18720b.o] } {
     unresolved "Indirect symbol tests"
     return
 }
@@ -79,6 +81,12 @@ set build_tests {
   {"Build libindirect4c.so"
    "-shared" "-fPIC"
    {indirect4c.c} {} "libindirect4c.so"}
+  {"Build libpr18720c.so"
+   "-shared" "-fPIC"
+   {pr18720c.c} {} "libpr18720c.so"}
+  {"Build pr18720b1.o"
+   "-r -nostdlib tmpdir/pr18720b.o" ""
+   {dummy.c} {} "pr18720b1.o"}
 }
 
 run_cc_link_tests $build_tests
@@ -132,6 +140,21 @@ set run_tests {
     {"Run with libindirect4c.so 4"
      "tmpdir/libindirect4c.so tmpdir/indirect4b.o tmpdir/indirect4a.o" ""
      {dummy.c} "indirect4d" "indirect4.out"}
+    {"Run with libpr18720c.so 1"
+     "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
+     {check-ptr-eq.c} "pr18720a" "pr18720.out"}
+    {"Run with libpr18720c.so 2"
+     "tmpdir/pr18720a.o tmpdir/libpr18720c.so tmpdir/pr18720b.o" ""
+     {check-ptr-eq.c} "pr18720b" "pr18720.out"}
+    {"Run with libpr18720c.so 3"
+     "tmpdir/pr18720b.o tmpdir/libpr18720c.so tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720c" "pr18720.out"}
+    {"Run with libpr18720c.so 4"
+     "tmpdir/libpr18720c.so tmpdir/pr18720b.o tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720d" "pr18720.out"}
+    {"Run with libpr18720c.so 5"
+     "tmpdir/libpr18720c.so tmpdir/pr18720b1.o tmpdir/pr18720a.o" ""
+     {check-ptr-eq.c} "pr18720d" "pr18720.out"}
 }
 
 run_ld_link_exec_tests [] $run_tests
diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
new file mode 100644
index 0000000..482e981
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720.out
@@ -0,0 +1,2 @@
+MAIN
+DSO
diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
new file mode 100644
index 0000000..752623b
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720a.c
@@ -0,0 +1,27 @@
+#include <bfd_stdint.h>
+
+extern void bar (void);
+extern void foo (void);
+extern void foo_alias (void);
+extern void check_ptr_eq (void *, void *);
+
+#if defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4005
+__attribute__ ((noinline, noclone))
+#else
+__attribute__ ((noinline))
+#endif
+int
+foo_p (void)
+{
+  return (intptr_t) &foo == 0x12345678 ? 1 : 0;
+}
+
+int
+main (void)
+{
+  foo ();
+  foo_p ();
+  bar ();
+  check_ptr_eq (&foo, &foo_alias);
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr18720b.c b/ld/testsuite/ld-elf/pr18720b.c
new file mode 100644
index 0000000..90d376b
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720b.c
@@ -0,0 +1,11 @@
+#include <stdio.h>
+
+void
+foo (void)
+{
+  printf ("MAIN\n");
+}
+
+asm (".symver foo,foo@FOO");
+asm (".set foo_alias,foo");
+asm (".global foo_alias");
diff --git a/ld/testsuite/ld-elf/pr18720c.c b/ld/testsuite/ld-elf/pr18720c.c
new file mode 100644
index 0000000..b52cb95
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720c.c
@@ -0,0 +1,15 @@
+#include <stdio.h>
+
+extern void foo (void);
+
+void
+foo (void)
+{
+  printf ("DSO\n");
+}
+
+void
+bar (void)
+{
+  foo ();
+}
-- 
2.4.3

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[H.J. Lu]

I will check it in this Friday unless there is an objection.


H.J.

On Sat, Aug 1, 2015 at 7:07 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Jul 26, 2015 at 03:15:50PM -0700, H.J. Lu wrote:
>> The non-default versioned symbol can only be merged with the versioned
>> symbol with the same symbol version.  _bfd_elf_merge_symbol should
>> check the symbol version before merging the new non-default versioned
>> symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
>> copy any references to the non-default versioned symbol.   We need to
>> bind a symbol locally when linking executable if it is locally defined,
>> non-default versioned, not referenced by shared library and not exported.
>>
>> OK for master?
>>
>> Thanks.
>>
>> H.J.
>> ---
>> bfd/
>>
>>       PR ld/18720
>>       * elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
>>       * elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
>>       if the new symbol matches the existing one.  The new non-default
>>       versioned symbol symbol matches the existing symbol if they have
>>       the same symbol version. Update the existing symbol only if they
>>       match.
>>       (_bfd_elf_add_default_symbol): Update call to
>>       _bfd_elf_merge_symbol.
>>       (elf_link_add_object_symbols): Override a definition only if the
>>       new symbol matches the existing one.
>>       (_bfd_elf_link_hash_copy_indirect): Don't copy any references to
>>       the non-default versioned symbol.
>>       (elf_link_output_extsym): Bind a symbol locally when linking
>>       executable if it is locally defined, non-default versioned, not
>>       referenced by shared library and not exported.
>>
>
> Here is the upated patch which uses the existing "hidden" field in
> elf_link_hash_entry.  Any objections, comments?
>
> Thanks.
>
> H.J.
> --
> bfd/
>
>         PR ld/18720
>         * elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
>         if the new symbol matches the existing one.  The new hidden
>         versioned symbol matches the existing symbol if they have the
>         same symbol version. Update the existing symbol only if they
>         match.
>         (_bfd_elf_add_default_symbol): Update call to
>         _bfd_elf_merge_symbol.
>         (_bfd_elf_link_assign_sym_version): Don't set the hidden field
>         here.
>         (elf_link_add_object_symbols): Override a definition only if the
>         new symbol matches the existing one.
>         (_bfd_elf_link_hash_copy_indirect): Don't copy any references to
>         the hidden versioned symbol.
>         (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
>         locally.
>
> ld/testsuite/
>
>         PR ld/18720
>         * ld-elf/indirect.exp: Run tests for PR ld/18720.
>         * ld-elf/pr18720.out: New file.
>         * ld-elf/pr18720a.c: Likewise.
>         * ld-elf/pr18720b.c: Likewise.
>         * ld-elf/pr18720c.c: Likewise.
> ---
>  bfd/elflink.c                    | 134 +++++++++++++++++++++++++++++----------
>  ld/testsuite/ld-elf/indirect.exp |  25 +++++++-
>  ld/testsuite/ld-elf/pr18720.out  |   2 +
>  ld/testsuite/ld-elf/pr18720a.c   |  27 ++++++++
>  ld/testsuite/ld-elf/pr18720b.c   |  11 ++++
>  ld/testsuite/ld-elf/pr18720c.c   |  15 +++++
>  6 files changed, 178 insertions(+), 36 deletions(-)
>  create mode 100644 ld/testsuite/ld-elf/pr18720.out
>  create mode 100644 ld/testsuite/ld-elf/pr18720a.c
>  create mode 100644 ld/testsuite/ld-elf/pr18720b.c
>  create mode 100644 ld/testsuite/ld-elf/pr18720c.c
>
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index 846f35e..832b374 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -939,7 +939,8 @@ _bfd_elf_merge_symbol (bfd *abfd,
>                        bfd_boolean *skip,
>                        bfd_boolean *override,
>                        bfd_boolean *type_change_ok,
> -                      bfd_boolean *size_change_ok)
> +                      bfd_boolean *size_change_ok,
> +                      bfd_boolean *matched)
>  {
>    asection *sec, *oldsec;
>    struct elf_link_hash_entry *h;
> @@ -950,6 +951,7 @@ _bfd_elf_merge_symbol (bfd *abfd,
>    bfd_boolean newdyn, olddyn, olddef, newdef, newdyncommon, olddyncommon;
>    bfd_boolean newweak, oldweak, newfunc, oldfunc;
>    const struct elf_backend_data *bed;
> +  char *new_version;
>
>    *skip = FALSE;
>    *override = FALSE;
> @@ -968,6 +970,17 @@ _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 (new_version > name && new_version[-1] != ELF_VER_CHR)
> +       h->hidden = 1;
> +      new_version += 1;
> +      if (new_version[0] == '\0')
> +       new_version = NULL;
> +    }
> +
>    /* For merging, we only care about real symbols.  But we need to make
>       sure that indirect symbol dynamic flags are updated.  */
>    hi = h;
> @@ -975,6 +988,45 @@ _bfd_elf_merge_symbol (bfd *abfd,
>          || h->root.type == bfd_link_hash_warning)
>      h = (struct elf_link_hash_entry *) h->root.u.i.link;
>
> +  if (!*matched)
> +    {
> +      if (hi == h || h->root.type == bfd_link_hash_new)
> +       *matched = TRUE;
> +      else
> +       {
> +         /* OLD_HIDDEN is true if the existing symbol is only visibile
> +            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;
> +         if (!old_hidden && !new_hidden)
> +           /* The new symbol matches the existing symbol if both
> +              aren't hidden.  */
> +           *matched = TRUE;
> +         else
> +           {
> +             /* 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;
> +               }
> +
> +             /* The new symbol matches the existing symbol if they
> +                have the same symbol version.  */
> +             *matched = (old_version == new_version
> +                         || (old_version != NULL
> +                             && new_version != NULL
> +                             && strcmp (old_version, new_version) == 0));
> +           }
> +       }
> +    }
> +
>    /* OLDBFD and OLDSEC are a BFD and an ASECTION associated with the
>       existing symbol.  */
>
> @@ -1047,7 +1099,9 @@ _bfd_elf_merge_symbol (bfd *abfd,
>         }
>        else
>         {
> -         h->dynamic_def = 1;
> +         /* Update the existing symbol only if they match. */
> +         if (*matched)
> +           h->dynamic_def = 1;
>           hi->dynamic_def = 1;
>         }
>      }
> @@ -1618,6 +1672,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
>    char *p;
>    size_t len, shortlen;
>    asection *tmp_sec;
> +  bfd_boolean matched;
>
>    /* If this symbol has a version, and it is the default version, we
>       create an indirect symbol from the default name to the fully
> @@ -1644,10 +1699,11 @@ _bfd_elf_add_default_symbol (bfd *abfd,
>       actually going to define an indirect symbol.  */
>    type_change_ok = FALSE;
>    size_change_ok = FALSE;
> +  matched = TRUE;
>    tmp_sec = sec;
>    if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
>                               &hi, poldbfd, NULL, NULL, &skip, &override,
> -                             &type_change_ok, &size_change_ok))
> +                             &type_change_ok, &size_change_ok, &matched))
>      return FALSE;
>
>    if (skip)
> @@ -1767,7 +1823,7 @@ nondefault:
>    tmp_sec = sec;
>    if (!_bfd_elf_merge_symbol (abfd, info, shortname, sym, &tmp_sec, &value,
>                               &hi, poldbfd, NULL, NULL, &skip, &override,
> -                             &type_change_ok, &size_change_ok))
> +                             &type_change_ok, &size_change_ok, &matched))
>      return FALSE;
>
>    if (skip)
> @@ -1977,26 +2033,14 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
>    if (p != NULL && h->verinfo.vertree == NULL)
>      {
>        struct bfd_elf_version_tree *t;
> -      bfd_boolean hidden;
>
> -      hidden = TRUE;
> -
> -      /* There are two consecutive ELF_VER_CHR characters if this is
> -        not a hidden symbol.  */
>        ++p;
>        if (*p == ELF_VER_CHR)
> -       {
> -         hidden = FALSE;
> -         ++p;
> -       }
> +       ++p;
>
>        /* If there is no version string, we can just return out.  */
>        if (*p == '\0')
> -       {
> -         if (hidden)
> -           h->hidden = 1;
> -         return TRUE;
> -       }
> +       return TRUE;
>
>        /* Look for the version.  If we find it, it is no longer weak.  */
>        for (t = sinfo->info->version_info; t != NULL; t = t->next)
> @@ -2092,9 +2136,6 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
>           sinfo->failed = TRUE;
>           return FALSE;
>         }
> -
> -      if (hidden)
> -       h->hidden = 1;
>      }
>
>    /* If we don't have a version for this symbol, see if we can find
> @@ -3885,6 +3926,7 @@ error_free_dyn:
>        bfd_boolean common;
>        unsigned int old_alignment;
>        bfd *old_bfd;
> +      bfd_boolean matched;
>
>        override = FALSE;
>
> @@ -4019,6 +4061,7 @@ error_free_dyn:
>        size_change_ok = FALSE;
>        type_change_ok = bed->type_change_ok;
>        old_weak = FALSE;
> +      matched = FALSE;
>        old_alignment = 0;
>        old_bfd = NULL;
>        new_sec = sec;
> @@ -4148,13 +4191,16 @@ error_free_dyn:
>           if (!_bfd_elf_merge_symbol (abfd, info, name, isym, &sec, &value,
>                                       sym_hash, &old_bfd, &old_weak,
>                                       &old_alignment, &skip, &override,
> -                                     &type_change_ok, &size_change_ok))
> +                                     &type_change_ok, &size_change_ok,
> +                                     &matched))
>             goto error_free_vers;
>
>           if (skip)
>             continue;
>
> -         if (override)
> +         /* Override a definition only if the new symbol matches the
> +            existing one.  */
> +         if (override && matched)
>             definition = FALSE;
>
>           h = *sym_hash;
> @@ -6810,14 +6856,18 @@ _bfd_elf_link_hash_copy_indirect (struct bfd_link_info *info,
>    struct elf_link_hash_table *htab;
>
>    /* Copy down any references that we may have already seen to the
> -     symbol which just became indirect.  */
> +     symbol which just became indirect if DIR isn't a hidden versioned
> +     symbol.  */
>
> -  dir->ref_dynamic |= ind->ref_dynamic;
> -  dir->ref_regular |= ind->ref_regular;
> -  dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
> -  dir->non_got_ref |= ind->non_got_ref;
> -  dir->needs_plt |= ind->needs_plt;
> -  dir->pointer_equality_needed |= ind->pointer_equality_needed;
> +  if (!dir->hidden)
> +    {
> +      dir->ref_dynamic |= ind->ref_dynamic;
> +      dir->ref_regular |= ind->ref_regular;
> +      dir->ref_regular_nonweak |= ind->ref_regular_nonweak;
> +      dir->non_got_ref |= ind->non_got_ref;
> +      dir->needs_plt |= ind->needs_plt;
> +      dir->pointer_equality_needed |= ind->pointer_equality_needed;
> +    }
>
>    if (ind->root.type != bfd_link_hash_indirect)
>      return;
> @@ -8904,6 +8954,16 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
>    const struct elf_backend_data *bed;
>    long indx;
>    int ret;
> +  /* A symbol is bound locally if it is forced local or it is locally
> +     defined, hidden versioned, not referenced by shared library and
> +     not exported when linking executable.  */
> +  bfd_boolean local_bind = (h->forced_local
> +                           || (flinfo->info->executable
> +                               && !flinfo->info->export_dynamic
> +                               && !h->dynamic
> +                               && !h->ref_dynamic
> +                               && h->def_regular
> +                               && h->hidden));
>
>    if (h->root.type == bfd_link_hash_warning)
>      {
> @@ -8915,12 +8975,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
>    /* Decide whether to output this symbol in this pass.  */
>    if (eoinfo->localsyms)
>      {
> -      if (!h->forced_local)
> +      if (!local_bind)
>         return TRUE;
>      }
>    else
>      {
> -      if (h->forced_local)
> +      if (local_bind)
>         return TRUE;
>      }
>
> @@ -9041,7 +9101,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
>    sym.st_value = 0;
>    sym.st_size = h->size;
>    sym.st_other = h->other;
> -  if (h->forced_local)
> +  if (local_bind)
>      {
>        sym.st_info = ELF_ST_INFO (STB_LOCAL, h->type);
>        /* Turn off visibility on local symbol.  */
> @@ -9223,8 +9283,10 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
>        /* Since there is no version information in the dynamic string,
>          if there is no version info in symbol version section, we will
>          have a run-time problem if not linking executable, referenced
> -        by shared library, or not locally defined.  */
> +        by shared library, not locally defined, or not bound locally.
> +      */
>        if (h->verinfo.verdef == NULL
> +         && !local_bind
>           && (!flinfo->info->executable
>               || h->ref_dynamic
>               || !h->def_regular))
> @@ -9297,7 +9359,9 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
>                 iversym.vs_vers++;
>             }
>
> -         if (h->hidden)
> +         /* Turn on VERSYM_HIDDEN only if the hidden vesioned symbol is
> +            defined locally.  */
> +         if (h->hidden && h->def_regular)
>             iversym.vs_vers |= VERSYM_HIDDEN;
>
>           eversym = (Elf_External_Versym *) flinfo->symver_sec->contents;
> diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
> index 468ef2b..e8ac1ae 100644
> --- a/ld/testsuite/ld-elf/indirect.exp
> +++ b/ld/testsuite/ld-elf/indirect.exp
> @@ -64,7 +64,9 @@ if { ![ld_compile $CC $srcdir/$subdir/indirect1a.c tmpdir/indirect1a.o]
>       || ![ld_compile $CC $srcdir/$subdir/indirect3a.c tmpdir/indirect3a.o]
>       || ![ld_compile $CC $srcdir/$subdir/indirect3b.c tmpdir/indirect3b.o]
>       || ![ld_compile $CC $srcdir/$subdir/indirect4a.c tmpdir/indirect4a.o]
> -     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o] } {
> +     || ![ld_compile $CC $srcdir/$subdir/indirect4b.c tmpdir/indirect4b.o]
> +     || ![ld_compile "$CC -O2 -fPIC -I../bfd" $srcdir/$subdir/pr18720a.c tmpdir/pr18720a.o]
> +     || ![ld_compile $CC $srcdir/$subdir/pr18720b.c tmpdir/pr18720b.o] } {
>      unresolved "Indirect symbol tests"
>      return
>  }
> @@ -79,6 +81,12 @@ set build_tests {
>    {"Build libindirect4c.so"
>     "-shared" "-fPIC"
>     {indirect4c.c} {} "libindirect4c.so"}
> +  {"Build libpr18720c.so"
> +   "-shared" "-fPIC"
> +   {pr18720c.c} {} "libpr18720c.so"}
> +  {"Build pr18720b1.o"
> +   "-r -nostdlib tmpdir/pr18720b.o" ""
> +   {dummy.c} {} "pr18720b1.o"}
>  }
>
>  run_cc_link_tests $build_tests
> @@ -132,6 +140,21 @@ set run_tests {
>      {"Run with libindirect4c.so 4"
>       "tmpdir/libindirect4c.so tmpdir/indirect4b.o tmpdir/indirect4a.o" ""
>       {dummy.c} "indirect4d" "indirect4.out"}
> +    {"Run with libpr18720c.so 1"
> +     "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
> +     {check-ptr-eq.c} "pr18720a" "pr18720.out"}
> +    {"Run with libpr18720c.so 2"
> +     "tmpdir/pr18720a.o tmpdir/libpr18720c.so tmpdir/pr18720b.o" ""
> +     {check-ptr-eq.c} "pr18720b" "pr18720.out"}
> +    {"Run with libpr18720c.so 3"
> +     "tmpdir/pr18720b.o tmpdir/libpr18720c.so tmpdir/pr18720a.o" ""
> +     {check-ptr-eq.c} "pr18720c" "pr18720.out"}
> +    {"Run with libpr18720c.so 4"
> +     "tmpdir/libpr18720c.so tmpdir/pr18720b.o tmpdir/pr18720a.o" ""
> +     {check-ptr-eq.c} "pr18720d" "pr18720.out"}
> +    {"Run with libpr18720c.so 5"
> +     "tmpdir/libpr18720c.so tmpdir/pr18720b1.o tmpdir/pr18720a.o" ""
> +     {check-ptr-eq.c} "pr18720d" "pr18720.out"}
>  }
>
>  run_ld_link_exec_tests [] $run_tests
> diff --git a/ld/testsuite/ld-elf/pr18720.out b/ld/testsuite/ld-elf/pr18720.out
> new file mode 100644
> index 0000000..482e981
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr18720.out
> @@ -0,0 +1,2 @@
> +MAIN
> +DSO
> diff --git a/ld/testsuite/ld-elf/pr18720a.c b/ld/testsuite/ld-elf/pr18720a.c
> new file mode 100644
> index 0000000..752623b
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr18720a.c
> @@ -0,0 +1,27 @@
> +#include <bfd_stdint.h>
> +
> +extern void bar (void);
> +extern void foo (void);
> +extern void foo_alias (void);
> +extern void check_ptr_eq (void *, void *);
> +
> +#if defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4005
> +__attribute__ ((noinline, noclone))
> +#else
> +__attribute__ ((noinline))
> +#endif
> +int
> +foo_p (void)
> +{
> +  return (intptr_t) &foo == 0x12345678 ? 1 : 0;
> +}
> +
> +int
> +main (void)
> +{
> +  foo ();
> +  foo_p ();
> +  bar ();
> +  check_ptr_eq (&foo, &foo_alias);
> +  return 0;
> +}
> diff --git a/ld/testsuite/ld-elf/pr18720b.c b/ld/testsuite/ld-elf/pr18720b.c
> new file mode 100644
> index 0000000..90d376b
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr18720b.c
> @@ -0,0 +1,11 @@
> +#include <stdio.h>
> +
> +void
> +foo (void)
> +{
> +  printf ("MAIN\n");
> +}
> +
> +asm (".symver foo,foo@FOO");
> +asm (".set foo_alias,foo");
> +asm (".global foo_alias");
> diff --git a/ld/testsuite/ld-elf/pr18720c.c b/ld/testsuite/ld-elf/pr18720c.c
> new file mode 100644
> index 0000000..b52cb95
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr18720c.c
> @@ -0,0 +1,15 @@
> +#include <stdio.h>
> +
> +extern void foo (void);
> +
> +void
> +foo (void)
> +{
> +  printf ("DSO\n");
> +}
> +
> +void
> +bar (void)
> +{
> +  foo ();
> +}
> --
> 2.4.3
>



-- 
H.J.

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[Mike Frysinger]

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

On 01 Aug 2015 07:07, H.J. Lu wrote:
> On Sun, Jul 26, 2015 at 03:15:50PM -0700, H.J. Lu wrote:
> > The non-default versioned symbol can only be merged with the versioned
> > symbol with the same symbol version.  _bfd_elf_merge_symbol should
> > check the symbol version before merging the new non-default versioned
> > symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
> > copy any references to the non-default versioned symbol.   We need to
> > bind a symbol locally when linking executable if it is locally defined,
> > non-default versioned, not referenced by shared library and not exported.
> > ---
> > bfd/
> > 
> > 	PR ld/18720
> > 	* elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
> > 	* elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
> > 	if the new symbol matches the existing one.  The new non-default
> > 	versioned symbol symbol matches the existing symbol if they have
> > 	the same symbol version. Update the existing symbol only if they
> > 	match.
> > 	(_bfd_elf_add_default_symbol): Update call to
> > 	_bfd_elf_merge_symbol.
> > 	(elf_link_add_object_symbols): Override a definition only if the
> > 	new symbol matches the existing one.
> > 	(_bfd_elf_link_hash_copy_indirect): Don't copy any references to
> > 	the non-default versioned symbol.
> > 	(elf_link_output_extsym): Bind a symbol locally when linking
> > 	executable if it is locally defined, non-default versioned, not
> > 	referenced by shared library and not exported.
> 
> Here is the upated patch which uses the existing "hidden" field in
> elf_link_hash_entry.  Any objections, comments?

this breaks asan.  simple test case:
$ echo 'main() { fork(); }' > test.c
$ gcc -fsanitize=address -c test.c
$ gcc -fsanitize=address test.o
ld: /usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so: undefined reference to symbol 'fork@GLIBC_2.2.5'
/lib64/libpthread.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

i'm guessing this will also break any library that links against pthread
but the main app itself doesn't.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[H.J. Lu]

On Mon, Feb 1, 2016 at 12:22 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 01 Aug 2015 07:07, H.J. Lu wrote:
>> On Sun, Jul 26, 2015 at 03:15:50PM -0700, H.J. Lu wrote:
>> > The non-default versioned symbol can only be merged with the versioned
>> > symbol with the same symbol version.  _bfd_elf_merge_symbol should
>> > check the symbol version before merging the new non-default versioned
>> > symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
>> > copy any references to the non-default versioned symbol.   We need to
>> > bind a symbol locally when linking executable if it is locally defined,
>> > non-default versioned, not referenced by shared library and not exported.
>> > ---
>> > bfd/
>> >
>> >     PR ld/18720
>> >     * elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
>> >     * elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
>> >     if the new symbol matches the existing one.  The new non-default
>> >     versioned symbol symbol matches the existing symbol if they have
>> >     the same symbol version. Update the existing symbol only if they
>> >     match.
>> >     (_bfd_elf_add_default_symbol): Update call to
>> >     _bfd_elf_merge_symbol.
>> >     (elf_link_add_object_symbols): Override a definition only if the
>> >     new symbol matches the existing one.
>> >     (_bfd_elf_link_hash_copy_indirect): Don't copy any references to
>> >     the non-default versioned symbol.
>> >     (elf_link_output_extsym): Bind a symbol locally when linking
>> >     executable if it is locally defined, non-default versioned, not
>> >     referenced by shared library and not exported.
>>
>> Here is the upated patch which uses the existing "hidden" field in
>> elf_link_hash_entry.  Any objections, comments?
>
> this breaks asan.  simple test case:
> $ echo 'main() { fork(); }' > test.c
> $ gcc -fsanitize=address -c test.c
> $ gcc -fsanitize=address test.o
> ld: /usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so: undefined reference to symbol 'fork@GLIBC_2.2.5'
> /lib64/libpthread.so.0: error adding symbols: DSO missing from command line
> collect2: error: ld returned 1 exit status
>
> i'm guessing this will also break any library that links against pthread
> but the main app itself doesn't.
> -mike

Please open a binutils bug.

Thanks.

-- 
H.J.

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[Mike Frysinger]

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

On 01 Feb 2016 12:47, H.J. Lu wrote:
> On Mon, Feb 1, 2016 at 12:22 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On 01 Aug 2015 07:07, H.J. Lu wrote:
> >> On Sun, Jul 26, 2015 at 03:15:50PM -0700, H.J. Lu wrote:
> >> > The non-default versioned symbol can only be merged with the versioned
> >> > symbol with the same symbol version.  _bfd_elf_merge_symbol should
> >> > check the symbol version before merging the new non-default versioned
> >> > symbol with the existing symbol.  _bfd_elf_link_hash_copy_indirect can't
> >> > copy any references to the non-default versioned symbol.   We need to
> >> > bind a symbol locally when linking executable if it is locally defined,
> >> > non-default versioned, not referenced by shared library and not exported.
> >> > ---
> >> > bfd/
> >> >
> >> >     PR ld/18720
> >> >     * elf-bfd.h (elf_link_hash_entry): Add nondeflt_version.
> >> >     * elflink.c (_bfd_elf_merge_symbol): Add a parameter to indicate
> >> >     if the new symbol matches the existing one.  The new non-default
> >> >     versioned symbol symbol matches the existing symbol if they have
> >> >     the same symbol version. Update the existing symbol only if they
> >> >     match.
> >> >     (_bfd_elf_add_default_symbol): Update call to
> >> >     _bfd_elf_merge_symbol.
> >> >     (elf_link_add_object_symbols): Override a definition only if the
> >> >     new symbol matches the existing one.
> >> >     (_bfd_elf_link_hash_copy_indirect): Don't copy any references to
> >> >     the non-default versioned symbol.
> >> >     (elf_link_output_extsym): Bind a symbol locally when linking
> >> >     executable if it is locally defined, non-default versioned, not
> >> >     referenced by shared library and not exported.
> >>
> >> Here is the upated patch which uses the existing "hidden" field in
> >> elf_link_hash_entry.  Any objections, comments?
> >
> > this breaks asan.  simple test case:
> > $ echo 'main() { fork(); }' > test.c
> > $ gcc -fsanitize=address -c test.c
> > $ gcc -fsanitize=address test.o
> > ld: /usr/lib/gcc/x86_64-pc-linux-gnu/5.3.0/libasan.so: undefined reference to symbol 'fork@GLIBC_2.2.5'
> > /lib64/libpthread.so.0: error adding symbols: DSO missing from command line
> > collect2: error: ld returned 1 exit status
> >
> > i'm guessing this will also break any library that links against pthread
> > but the main app itself doesn't.
> 
> Please open a binutils bug.

done:
https://sourceware.org/bugzilla/show_bug.cgi?id=19553
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[Alan Modra]

On Wed, Aug 05, 2015 at 07:43:21PM -0700, H.J. Lu wrote:
> I will check it in this Friday unless there is an objection.

HJ, git 6e33951edc change to elf_link_output_extsym where you
introduce local_bind, is broken.  You can't change global syms to
STB_LOCAL that late, after _bfd_elf_link_renumber_dynsyms, as otherwise
you run the risk of ordering STB_LOCAL symbols after global symbols.
The ELF gABI is clear that "all symbols with STB_LOCAL binding precede
the weak and global symbols".  (It's also wrong to test flags on a
bfd_link_hash_warning symbol.)

I don't have a testcase for you.  I noticed the bug when looking at
PR20828.  Please revert the elf_link_output_extsym change and
implement by setting forced_local before the last run of
_bfd_elf_link_renumber_dynsyms.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[H.J. Lu]

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

On Sun, Nov 20, 2016 at 7:09 PM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Aug 05, 2015 at 07:43:21PM -0700, H.J. Lu wrote:
>> I will check it in this Friday unless there is an objection.
>
> HJ, git 6e33951edc change to elf_link_output_extsym where you
> introduce local_bind, is broken.  You can't change global syms to
> STB_LOCAL that late, after _bfd_elf_link_renumber_dynsyms, as otherwise
> you run the risk of ordering STB_LOCAL symbols after global symbols.
> The ELF gABI is clear that "all symbols with STB_LOCAL binding precede
> the weak and global symbols".  (It's also wrong to test flags on a
> bfd_link_hash_warning symbol.)
>
> I don't have a testcase for you.  I noticed the bug when looking at
> PR20828.  Please revert the elf_link_output_extsym change and
> implement by setting forced_local before the last run of
> _bfd_elf_link_renumber_dynsyms.
>

How about this patch?


-- 
H.J.

[-- Attachment #2: 0001-Hide-hidden-versioned-symbol-in-executable.patch --]
[-- Type: text/x-patch, Size: 4917 bytes --]

From c8eca1d85c2fd5b3038d381e2be55729a4d36ccc Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 21 Nov 2016 13:05:44 -0800
Subject: [PATCH] Hide hidden versioned symbol in executable

A hidden versioned symbol in executable should be forced local if
it is is locally defined, not referenced by shared library and not
exported.  We must do it before _bfd_elf_link_renumber_dynsyms.
---
 bfd/elflink.c                    | 43 ++++++++++++++++++++--------------------
 ld/testsuite/ld-elf/indirect.exp |  3 +++
 ld/testsuite/ld-elf/pr18720.rd   |  4 ++++
 3 files changed, 28 insertions(+), 22 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr18720.rd

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 76f91e9..04006f9 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -2655,18 +2655,29 @@ _bfd_elf_fix_symbol_flags (struct elf_link_hash_entry *h,
       && (h->root.u.def.section->owner->flags & (DYNAMIC | BFD_PLUGIN)) == 0)
     h->def_regular = 1;
 
+  /* A hidden versioned symbol in executable should be forced local if
+     it is is locally defined, not referenced by shared library and not
+     exported.  */
+  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);
+
   /* If -Bsymbolic was used (which means to bind references to global
      symbols to the definition within the shared object), and this
      symbol was defined in a regular object, then it actually doesn't
      need a PLT entry.  Likewise, if the symbol has non-default
      visibility.  If the symbol has hidden or internal visibility, we
      will force it local.  */
-  if (h->needs_plt
-      && bfd_link_pic (eif->info)
-      && is_elf_hash_table (eif->info->hash)
-      && (SYMBOLIC_BIND (eif->info, h)
-	  || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
-      && h->def_regular)
+  else if (h->needs_plt
+	   && bfd_link_pic (eif->info)
+	   && is_elf_hash_table (eif->info->hash)
+	   && (SYMBOLIC_BIND (eif->info, h)
+	       || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
+	   && h->def_regular)
     {
       bfd_boolean force_local;
 
@@ -9250,16 +9261,6 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   long indx;
   int ret;
   unsigned int type;
-  /* A symbol is bound locally if it is forced local or it is locally
-     defined, hidden versioned, not referenced by shared library and
-     not exported when linking executable.  */
-  bfd_boolean local_bind = (h->forced_local
-			    || (bfd_link_executable (flinfo->info)
-				&& !flinfo->info->export_dynamic
-				&& !h->dynamic
-				&& !h->ref_dynamic
-				&& h->def_regular
-				&& h->versioned == versioned_hidden));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -9271,12 +9272,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* Decide whether to output this symbol in this pass.  */
   if (eoinfo->localsyms)
     {
-      if (!local_bind)
+      if (!h->forced_local)
 	return TRUE;
     }
   else
     {
-      if (local_bind)
+      if (h->forced_local)
 	return TRUE;
     }
 
@@ -9493,7 +9494,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
 	abort ();
       }
 
-  if (local_bind)
+  if (h->forced_local)
     {
       sym.st_info = ELF_ST_INFO (STB_LOCAL, type);
       /* Turn off visibility on local symbol.  */
@@ -9604,10 +9605,8 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
       /* Since there is no version information in the dynamic string,
 	 if there is no version info in symbol version section, we will
 	 have a run-time problem if not linking executable, referenced
-	 by shared library, not locally defined, or not bound locally.
-      */
+	 by shared library, or not bound locally.  */
       if (h->verinfo.verdef == NULL
-	  && !local_bind
 	  && (!bfd_link_executable (flinfo->info)
 	      || h->ref_dynamic
 	      || !h->def_regular))
diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
index b4766fd..3176210 100644
--- a/ld/testsuite/ld-elf/indirect.exp
+++ b/ld/testsuite/ld-elf/indirect.exp
@@ -91,6 +91,9 @@ set build_tests {
   {"Build pr18720b1.o"
    "-r -nostdlib tmpdir/pr18720b.o" ""
    {dummy.c} {} "pr18720b1.o"}
+  {"Build pr18720a"
+   "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
+   {check-ptr-eq.c} {{readelf {--dyn-syms} pr18720.rd}} "pr18720a"}
   {"Build libpr19553b.so"
    "-shared -Wl,--version-script=pr19553.map" "-fPIC"
    {pr19553b.c} {} "libpr19553b.so"}
diff --git a/ld/testsuite/ld-elf/pr18720.rd b/ld/testsuite/ld-elf/pr18720.rd
new file mode 100644
index 0000000..b5e848f
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720.rd
@@ -0,0 +1,4 @@
+#failif
+#...
+.* found at index >= .dynsym's sh_info value .*
+#...
-- 
2.7.4

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[Alan Modra]

On Mon, Nov 21, 2016 at 01:09:20PM -0800, H.J. Lu wrote:
> On Sun, Nov 20, 2016 at 7:09 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Wed, Aug 05, 2015 at 07:43:21PM -0700, H.J. Lu wrote:
> >> I will check it in this Friday unless there is an objection.
> >
> > HJ, git 6e33951edc change to elf_link_output_extsym where you
> > introduce local_bind, is broken.  You can't change global syms to
> > STB_LOCAL that late, after _bfd_elf_link_renumber_dynsyms, as otherwise
> > you run the risk of ordering STB_LOCAL symbols after global symbols.
> > The ELF gABI is clear that "all symbols with STB_LOCAL binding precede
> > the weak and global symbols".  (It's also wrong to test flags on a
> > bfd_link_hash_warning symbol.)
> >
> > I don't have a testcase for you.  I noticed the bug when looking at
> > PR20828.  Please revert the elf_link_output_extsym change and
> > implement by setting forced_local before the last run of
> > _bfd_elf_link_renumber_dynsyms.
> >
> 
> How about this patch?

OK thanks, it looks almost the same as the one I threw together.

You could also move the code hiding weak undefined non-default
visibility symbols.

  /* If a weak undefined symbol has non-default visibility, we also
     hide it from the dynamic linker.  */
  if (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
      && h->root.type == bfd_link_hash_undefweak)
    (*bed->elf_backend_hide_symbol) (eif->info, h, TRUE);

  /* A hidden versioned symbol in executable should be forced local if
     it is is locally defined, not referenced by shared library and not
     exported.  */
  else if (bfd_link_executable (eif->info)
etc.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] PR ld/18720: Properly merge hidden versioned symbol

[H.J. Lu]

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

On Mon, Nov 21, 2016 at 4:14 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Nov 21, 2016 at 01:09:20PM -0800, H.J. Lu wrote:
>> On Sun, Nov 20, 2016 at 7:09 PM, Alan Modra <amodra@gmail.com> wrote:
>> > On Wed, Aug 05, 2015 at 07:43:21PM -0700, H.J. Lu wrote:
>> >> I will check it in this Friday unless there is an objection.
>> >
>> > HJ, git 6e33951edc change to elf_link_output_extsym where you
>> > introduce local_bind, is broken.  You can't change global syms to
>> > STB_LOCAL that late, after _bfd_elf_link_renumber_dynsyms, as otherwise
>> > you run the risk of ordering STB_LOCAL symbols after global symbols.
>> > The ELF gABI is clear that "all symbols with STB_LOCAL binding precede
>> > the weak and global symbols".  (It's also wrong to test flags on a
>> > bfd_link_hash_warning symbol.)
>> >
>> > I don't have a testcase for you.  I noticed the bug when looking at
>> > PR20828.  Please revert the elf_link_output_extsym change and
>> > implement by setting forced_local before the last run of
>> > _bfd_elf_link_renumber_dynsyms.
>> >
>>
>> How about this patch?
>
> OK thanks, it looks almost the same as the one I threw together.
>
> You could also move the code hiding weak undefined non-default
> visibility symbols.
>
>   /* If a weak undefined symbol has non-default visibility, we also
>      hide it from the dynamic linker.  */
>   if (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
>       && h->root.type == bfd_link_hash_undefweak)
>     (*bed->elf_backend_hide_symbol) (eif->info, h, TRUE);
>
>   /* A hidden versioned symbol in executable should be forced local if
>      it is is locally defined, not referenced by shared library and not
>      exported.  */
>   else if (bfd_link_executable (eif->info)
> etc.
>

This is what I checked in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Properly-hide-hidden-versioned-symbol-in-executable.patch --]
[-- Type: text/x-patch, Size: 6101 bytes --]

From 5fcacf7f68b34b275af586e2e72f29aef5522b05 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Mon, 21 Nov 2016 13:05:44 -0800
Subject: [PATCH] Properly hide hidden versioned symbol in executable

A hidden versioned symbol in executable should be forced local if it is
locally defined, not referenced by shared library and not exported.  We
must do it before _bfd_elf_link_renumber_dynsyms.

bfd/

	* elflink.c (_bfd_elf_fix_symbol_flags): Hide hidden versioned
	symbol in executable.
	(elf_link_output_extsym): Don't change bind from global to
	local when linking executable.

ld/

	* testsuite/ld-elf/indirect.exp: Add a test for PR 18720.
	* testsuite/ld-elf/pr18720.rd: New file.
---
 bfd/elflink.c                    | 55 ++++++++++++++++++++--------------------
 ld/testsuite/ld-elf/indirect.exp |  3 +++
 ld/testsuite/ld-elf/pr18720.rd   |  4 +++
 3 files changed, 34 insertions(+), 28 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr18720.rd

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 76f91e9..5abac8d 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -2655,18 +2655,35 @@ _bfd_elf_fix_symbol_flags (struct elf_link_hash_entry *h,
       && (h->root.u.def.section->owner->flags & (DYNAMIC | BFD_PLUGIN)) == 0)
     h->def_regular = 1;
 
+  /* If a weak undefined symbol has non-default visibility, we also
+     hide it from the dynamic linker.  */
+  if (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
+      && h->root.type == bfd_link_hash_undefweak)
+    (*bed->elf_backend_hide_symbol) (eif->info, h, TRUE);
+
+  /* A hidden versioned symbol in executable should be forced local if
+     it is is locally defined, not referenced by shared library and not
+     exported.  */
+  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);
+
   /* If -Bsymbolic was used (which means to bind references to global
      symbols to the definition within the shared object), and this
      symbol was defined in a regular object, then it actually doesn't
      need a PLT entry.  Likewise, if the symbol has non-default
      visibility.  If the symbol has hidden or internal visibility, we
      will force it local.  */
-  if (h->needs_plt
-      && bfd_link_pic (eif->info)
-      && is_elf_hash_table (eif->info->hash)
-      && (SYMBOLIC_BIND (eif->info, h)
-	  || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
-      && h->def_regular)
+  else if (h->needs_plt
+	   && bfd_link_pic (eif->info)
+	   && is_elf_hash_table (eif->info->hash)
+	   && (SYMBOLIC_BIND (eif->info, h)
+	       || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
+	   && h->def_regular)
     {
       bfd_boolean force_local;
 
@@ -2675,12 +2692,6 @@ _bfd_elf_fix_symbol_flags (struct elf_link_hash_entry *h,
       (*bed->elf_backend_hide_symbol) (eif->info, h, force_local);
     }
 
-  /* If a weak undefined symbol has non-default visibility, we also
-     hide it from the dynamic linker.  */
-  if (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT
-      && h->root.type == bfd_link_hash_undefweak)
-    (*bed->elf_backend_hide_symbol) (eif->info, h, TRUE);
-
   /* If this is a weak defined symbol in a dynamic object, and we know
      the real definition in the dynamic object, copy interesting flags
      over to the real definition.  */
@@ -9250,16 +9261,6 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   long indx;
   int ret;
   unsigned int type;
-  /* A symbol is bound locally if it is forced local or it is locally
-     defined, hidden versioned, not referenced by shared library and
-     not exported when linking executable.  */
-  bfd_boolean local_bind = (h->forced_local
-			    || (bfd_link_executable (flinfo->info)
-				&& !flinfo->info->export_dynamic
-				&& !h->dynamic
-				&& !h->ref_dynamic
-				&& h->def_regular
-				&& h->versioned == versioned_hidden));
 
   if (h->root.type == bfd_link_hash_warning)
     {
@@ -9271,12 +9272,12 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
   /* Decide whether to output this symbol in this pass.  */
   if (eoinfo->localsyms)
     {
-      if (!local_bind)
+      if (!h->forced_local)
 	return TRUE;
     }
   else
     {
-      if (local_bind)
+      if (h->forced_local)
 	return TRUE;
     }
 
@@ -9493,7 +9494,7 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
 	abort ();
       }
 
-  if (local_bind)
+  if (h->forced_local)
     {
       sym.st_info = ELF_ST_INFO (STB_LOCAL, type);
       /* Turn off visibility on local symbol.  */
@@ -9604,10 +9605,8 @@ elf_link_output_extsym (struct bfd_hash_entry *bh, void *data)
       /* Since there is no version information in the dynamic string,
 	 if there is no version info in symbol version section, we will
 	 have a run-time problem if not linking executable, referenced
-	 by shared library, not locally defined, or not bound locally.
-      */
+	 by shared library, or not bound locally.  */
       if (h->verinfo.verdef == NULL
-	  && !local_bind
 	  && (!bfd_link_executable (flinfo->info)
 	      || h->ref_dynamic
 	      || !h->def_regular))
diff --git a/ld/testsuite/ld-elf/indirect.exp b/ld/testsuite/ld-elf/indirect.exp
index b4766fd..3176210 100644
--- a/ld/testsuite/ld-elf/indirect.exp
+++ b/ld/testsuite/ld-elf/indirect.exp
@@ -91,6 +91,9 @@ set build_tests {
   {"Build pr18720b1.o"
    "-r -nostdlib tmpdir/pr18720b.o" ""
    {dummy.c} {} "pr18720b1.o"}
+  {"Build pr18720a"
+   "tmpdir/pr18720a.o tmpdir/pr18720b.o tmpdir/libpr18720c.so" ""
+   {check-ptr-eq.c} {{readelf {--dyn-syms} pr18720.rd}} "pr18720a"}
   {"Build libpr19553b.so"
    "-shared -Wl,--version-script=pr19553.map" "-fPIC"
    {pr19553b.c} {} "libpr19553b.so"}
diff --git a/ld/testsuite/ld-elf/pr18720.rd b/ld/testsuite/ld-elf/pr18720.rd
new file mode 100644
index 0000000..b5e848f
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr18720.rd
@@ -0,0 +1,4 @@
+#failif
+#...
+.* found at index >= .dynsym's sh_info value .*
+#...
-- 
2.7.4