This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]