PR ld/18720: Properly merge hidden versioned symbol

Message ID CAMe9rOqQjRNuaS-FvZ-3w6fNUNfwJq3CifmKkscSUCE_tcQfEw@mail.gmail.com
State Superseded
Headers show

Commit Message

H.J. Lu Nov. 21, 2016, 9:09 p.m.
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.

Comments

Alan Modra Nov. 22, 2016, 12:14 a.m. | #1
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

Patch

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