This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Cary Coutant <ccoutant at gmail dot com>
- Cc: Binutils <binutils at sourceware dot org>
- Date: Mon, 27 Jul 2015 10:35:37 -0700
- Subject: Re: [PATCH] PR ld/18720: Properly merge non-default versioned symbol
- Authentication-results: sourceware.org; auth=none
- References: <20150726221550 dot GA16472 at gmail dot com> <CAJimCsGatEeKRRYcwYG_v-s5O=wE-2mbFjNkLiHzfi0RdCLu0A at mail dot gmail dot com> <CAMe9rOoGhZG=OxDV5tYwORqT7gwD_FzNArRe59N4uJxyrd6rsw at mail dot gmail dot com> <CAJimCsEh5LG-Q-z7Fq_wqWP8qqWog+wNewa4j=jhhLt_gKjCug at mail dot gmail dot com> <CAMe9rOqNt5K83eRS+3=ArVPWJW+XzyCLSZ2Sb3b4UMx7OsPhwg at mail dot gmail dot com> <CAJimCsH_M3ahgocMQ=sPZQ9Mqp2xE8-S0GyDsya9QGhSy0-2CA at mail dot gmail dot com>
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