[PATCH] PR ld/18720: Properly merge hidden versioned symbol
H.J. Lu
hjl.tools@gmail.com
Thu Aug 6 02:43:00 GMT 2015
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.
More information about the Binutils
mailing list