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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.