Re: [PATCH] ld: Add _bfd_elf_link_hide_sym_by_version

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

On Mon, May 21, 2018 at 8:06 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> bfd_hide_sym_by_version can't be used to check if a versioned symbol is
> hidden.  This patch adds _bfd_elf_link_hide_sym_by_version to support
> both versioned and unversioned symbols by extracting versioned symbol
> check from _bfd_elf_link_assign_sym_version.
>
> OK for master?
>
> H.J.
> ---
> bfd/
>
>         PR ld/23194
>         * elf-bfd.h (_bfd_elf_link_hide_sym_by_version): New.
>         * elflink.c (_bfd_elf_link_hide_versioned_symbol): New function.
>         Extracted from _bfd_elf_link_assign_sym_version.
>         (_bfd_elf_link_hide_sym_by_version): New function.
>         (_bfd_elf_link_assign_sym_version): Use
>         _bfd_elf_link_hide_versioned_symbol.
>         * elfxx-x86.c (_bfd_x86_elf_link_symbol_references_local): Call
>         _bfd_elf_link_hide_sym_by_version instead of
>         bfd_hide_sym_by_version.  Don't check unversioned symbol.
>
> ld/
>
>         PR ld/23194
>         * testsuite/ld-i386/pr23194.d: Expect only R_386_GLOB_DAT
>         against foobar.
>         * testsuite/ld-i386/pr23194.map: Add foobar.
>         * testsuite/ld-x86-64/pr23194.map: Likewise.
>         * testsuite/ld-i386/pr23194.s: Add a common foobar symbol.
>         * testsuite/ld-x86-64/pr23194.s: Likewise.
>         * testsuite/ld-x86-64/pr23194.d: Expect only R_X86_64_GLOB_DAT
>         against foobar.
> ---
>  bfd/elf-bfd.h                      |   3 +
>  bfd/elflink.c                      | 164 +++++++++++++++++++++--------
>  bfd/elfxx-x86.c                    |  11 +-
>  ld/testsuite/ld-i386/pr23194.d     |   2 +-
>  ld/testsuite/ld-i386/pr23194.map   |   1 +
>  ld/testsuite/ld-i386/pr23194.s     |   3 +-
>  ld/testsuite/ld-x86-64/pr23194.d   |   2 +-
>  ld/testsuite/ld-x86-64/pr23194.map |   1 +
>  ld/testsuite/ld-x86-64/pr23194.s   |   3 +-
>  9 files changed, 134 insertions(+), 56 deletions(-)
>
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index 7b746900ba..ad958f3c86 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -2184,6 +2184,9 @@ extern const struct bfd_elf_special_section *_bfd_elf_get_special_section
>  extern const struct bfd_elf_special_section *_bfd_elf_get_sec_type_attr
>    (bfd *, asection *);
>
> +extern bfd_boolean _bfd_elf_link_hide_sym_by_version
> +  (struct bfd_link_info *, struct elf_link_hash_entry *);
> +
>  /* If the target doesn't have reloc handling written yet:  */
>  extern bfd_boolean _bfd_elf_no_info_to_howto
>    (bfd *, arelent *, Elf_Internal_Rela *);
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index ce3765e45b..734f8f88ad 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -2221,6 +2221,115 @@ _bfd_elf_link_find_version_dependencies (struct elf_link_hash_entry *h,
>    return TRUE;
>  }
>
> +/* Return TRUE and set *HIDE to TRUE if the versioned symbol is
> +   hidden.  Set *T_P to NULL if there is no match.  */
> +
> +static bfd_boolean
> +_bfd_elf_link_hide_versioned_symbol (struct bfd_link_info *info,
> +                                    struct elf_link_hash_entry *h,
> +                                    const char *version_p,
> +                                    struct bfd_elf_version_tree **t_p,
> +                                    bfd_boolean *hide)
> +{
> +  struct bfd_elf_version_tree *t;
> +
> +  /* Look for the version.  If we find it, it is no longer weak.  */
> +  for (t = info->version_info; t != NULL; t = t->next)
> +    {
> +      if (strcmp (t->name, version_p) == 0)
> +       {
> +         size_t len;
> +         char *alc;
> +         struct bfd_elf_version_expr *d;
> +
> +         len = version_p - h->root.root.string;
> +         alc = (char *) bfd_malloc (len);
> +         if (alc == NULL)
> +           return FALSE;
> +         memcpy (alc, h->root.root.string, len - 1);
> +         alc[len - 1] = '\0';
> +         if (alc[len - 2] == ELF_VER_CHR)
> +           alc[len - 2] = '\0';
> +
> +         h->verinfo.vertree = t;
> +         t->used = TRUE;
> +         d = NULL;
> +
> +         if (t->globals.list != NULL)
> +           d = (*t->match) (&t->globals, NULL, alc);
> +
> +         /* See if there is anything to force this symbol to
> +            local scope.  */
> +         if (d == NULL && t->locals.list != NULL)
> +           {
> +             d = (*t->match) (&t->locals, NULL, alc);
> +             if (d != NULL
> +                 && h->dynindx != -1
> +                 && ! info->export_dynamic)
> +               *hide = TRUE;
> +           }
> +
> +         free (alc);
> +         break;
> +       }
> +    }
> +
> +  *t_p = t;
> +
> +  return TRUE;
> +}
> +
> +/* Return TRUE if the symbol H is hidden by version script.  */
> +
> +bfd_boolean
> +_bfd_elf_link_hide_sym_by_version (struct bfd_link_info *info,
> +                                  struct elf_link_hash_entry *h)
> +{
> +  const char *p;
> +  bfd_boolean hide = FALSE;
> +  const struct elf_backend_data *bed
> +    = get_elf_backend_data (info->output_bfd);
> +
> +  /* Version script only hides symbols defined in regular objects.  */
> +  if (!h->def_regular && !ELF_COMMON_DEF_P (h))
> +    return TRUE;
> +
> +  p = strchr (h->root.root.string, ELF_VER_CHR);
> +  if (p != NULL && h->verinfo.vertree == NULL)
> +    {
> +      struct bfd_elf_version_tree *t;
> +
> +      ++p;
> +      if (*p == ELF_VER_CHR)
> +       ++p;
> +
> +      if (*p != '\0'
> +         && _bfd_elf_link_hide_versioned_symbol (info, h, p, &t, &hide)
> +         && hide)
> +       {
> +         if (hide)
> +           (*bed->elf_backend_hide_symbol) (info, h, TRUE);
> +         return TRUE;
> +       }
> +    }
> +
> +  /* If we don't have a version for this symbol, see if we can find
> +     something.  */
> +  if (h->verinfo.vertree == NULL && info->version_info != NULL)
> +    {
> +      h->verinfo.vertree
> +       = bfd_find_version_for_sym (info->version_info,
> +                                   h->root.root.string, &hide);
> +      if (h->verinfo.vertree != NULL && hide)
> +       {
> +         (*bed->elf_backend_hide_symbol) (info, h, TRUE);
> +         return TRUE;
> +       }
> +    }
> +
> +  return FALSE;
> +}
> +
>  /* Figure out appropriate versions for all the symbols.  We may not
>     have the version number script until we have read all of the input
>     files, so until that point we don't know which symbols should be
> @@ -2234,6 +2343,7 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
>    const struct elf_backend_data *bed;
>    struct elf_info_failed eif;
>    char *p;
> +  bfd_boolean hide;
>
>    sinfo = (struct elf_info_failed *) data;
>    info = sinfo->info;
> @@ -2253,6 +2363,7 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
>    if (!h->def_regular)
>      return TRUE;
>
> +  hide = FALSE;
>    bed = get_elf_backend_data (info->output_bfd);
>    p = strchr (h->root.root.string, ELF_VER_CHR);
>    if (p != NULL && h->verinfo.vertree == NULL)
> @@ -2267,50 +2378,15 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
>        if (*p == '\0')
>         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)
> +      if (!_bfd_elf_link_hide_versioned_symbol (info, h, p, &t, &hide))
>         {
> -         if (strcmp (t->name, p) == 0)
> -           {
> -             size_t len;
> -             char *alc;
> -             struct bfd_elf_version_expr *d;
> -
> -             len = p - h->root.root.string;
> -             alc = (char *) bfd_malloc (len);
> -             if (alc == NULL)
> -               {
> -                 sinfo->failed = TRUE;
> -                 return FALSE;
> -               }
> -             memcpy (alc, h->root.root.string, len - 1);
> -             alc[len - 1] = '\0';
> -             if (alc[len - 2] == ELF_VER_CHR)
> -               alc[len - 2] = '\0';
> -
> -             h->verinfo.vertree = t;
> -             t->used = TRUE;
> -             d = NULL;
> -
> -             if (t->globals.list != NULL)
> -               d = (*t->match) (&t->globals, NULL, alc);
> -
> -             /* See if there is anything to force this symbol to
> -                local scope.  */
> -             if (d == NULL && t->locals.list != NULL)
> -               {
> -                 d = (*t->match) (&t->locals, NULL, alc);
> -                 if (d != NULL
> -                     && h->dynindx != -1
> -                     && ! info->export_dynamic)
> -                   (*bed->elf_backend_hide_symbol) (info, h, TRUE);
> -               }
> -
> -             free (alc);
> -             break;
> -           }
> +         sinfo->failed = TRUE;
> +         return FALSE;
>         }
>
> +      if (hide)
> +       (*bed->elf_backend_hide_symbol) (info, h, TRUE);
> +
>        /* If we are building an application, we need to create a
>          version node for this version.  */
>        if (t == NULL && bfd_link_executable (info))
> @@ -2366,10 +2442,10 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
>
>    /* If we don't have a version for this symbol, see if we can find
>       something.  */
> -  if (h->verinfo.vertree == NULL && sinfo->info->version_info != NULL)
> +  if (!hide
> +      && h->verinfo.vertree == NULL
> +      && sinfo->info->version_info != NULL)
>      {
> -      bfd_boolean hide;
> -
>        h->verinfo.vertree
>         = bfd_find_version_for_sym (sinfo->info->version_info,
>                                     h->root.root.string, &hide);
> diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
> index 29d92d288a..cb2487eacd 100644
> --- a/bfd/elfxx-x86.c
> +++ b/bfd/elfxx-x86.c
> @@ -2038,21 +2038,16 @@ _bfd_x86_elf_link_symbol_references_local (struct bfd_link_info *info,
>                   && htab->interp == NULL)
>               || info->dynamic_undefined_weak == 0))
>        || ((h->def_regular || ELF_COMMON_DEF_P (h))
> -         && h->versioned == unversioned
>           && info->version_info != NULL
> -         && bfd_hide_sym_by_version (info->version_info,
> -                                     h->root.root.string)))
> +         && _bfd_elf_link_hide_sym_by_version (info, h)))
>      {
>        eh->local_ref = 2;
>        return TRUE;
>      }
>
>    /* Symbols created by HIDDEN and PROVIDE_HIDDEN assignments in linker
> -     script aren't forced local here yet.  bfd_hide_sym_by_version
> -     can't be used to check if a versioned symbol is hidden.  It has to
> -     be syncd with _bfd_elf_link_assign_sym_version to get the correct
> -     answer.  */
> -  if (!h->root.ldscript_def && h->versioned == unversioned)
> +     script aren't forced local here yet.  */
> +  if (!h->root.ldscript_def)
>      eh->local_ref = 1;
>
>    return FALSE;

Any comments? I need _bfd_elf_link_hide_sym_by_version to check if
both versioned
and unversioned symbols are hidden by version script.  If we don't
want it in elflink.c,
I can put it in elfxx-x86.c.

Thanks.

-- 
H.J.