* [PATCH v3] elf: Always honor the first definition in shared object and archive
@ 2024-03-15 19:43 H.J. Lu
2024-03-28 13:30 ` PING: " H.J. Lu
2024-04-04 23:54 ` Alan Modra
0 siblings, 2 replies; 11+ messages in thread
From: H.J. Lu @ 2024-03-15 19:43 UTC (permalink / raw)
To: binutils
GCC doesn't put builtin function symbol references, which are defined in
the shared C library, in the IR symbol table. When linker rescans shared
objects and archives for newly added symbol references generated from the
IR inputs, it skips definitions of the builtin functions in shared
objects and archives.
Add first_hash to elf_link_hash_table to track unreferenced definitions
defined first in shared objects and archives. Always use them to resolve
any references.
bfd/
PR ld/31482
PR ld/31489
* elf-bfd.h (elf_link_hash_table): Add first_hash.
* elflink.c (elf_link_add_object_symbols): Initialize first_hash
for an IR input. Always use the first definition in shared
object. Add the first unreferenced dynamic definition to
first_hash.
(_bfd_elf_archive_symbol_lookup): Add the first unreferenced
definition to first_hash..
(elf_link_add_archive_symbols): Use the symbol definition in
archive if symbol is defined first in this archive.
(_bfd_elf_link_hash_table_free): Also free first_hash.
ld/
PR ld/31482
PR ld/31489
* testsuite/ld-plugin/lto.exp: Add PR ld/31482 and PR ld/31489
tests.
* testsuite/ld-elf/pr31482a-no-lto.c: New file.
* testsuite/ld-elf/pr31482b-no-lto.c: Likewise.
* testsuite/ld-elf/pr31482c-no-lto.c: Likewise.
* testsuite/ld-elf/pr31482d-no-lto.c: Likewise.
* testsuite/ld-plugin/pass1.out: Likewise.
* testsuite/ld-plugin/pr31482a.c: Likewise.
* testsuite/ld-plugin/pr31482b.c: Likewise.
* testsuite/ld-plugin/pr31482c.c: Likewise.
---
bfd/elf-bfd.h | 4 +
bfd/elflink.c | 171 ++++++++++++++++++++------
ld/testsuite/ld-elf/pr31482a-no-lto.c | 8 ++
ld/testsuite/ld-elf/pr31482b-no-lto.c | 10 ++
ld/testsuite/ld-elf/pr31482c-no-lto.c | 15 +++
ld/testsuite/ld-elf/pr31482d-no-lto.c | 9 ++
ld/testsuite/ld-elf/shared.exp | 36 ++++++
ld/testsuite/ld-plugin/lto.exp | 32 +++++
ld/testsuite/ld-plugin/pass1.out | 1 +
ld/testsuite/ld-plugin/pr31482a.c | 8 ++
ld/testsuite/ld-plugin/pr31482b.c | 9 ++
ld/testsuite/ld-plugin/pr31482c.c | 9 ++
12 files changed, 275 insertions(+), 37 deletions(-)
create mode 100644 ld/testsuite/ld-elf/pr31482a-no-lto.c
create mode 100644 ld/testsuite/ld-elf/pr31482b-no-lto.c
create mode 100644 ld/testsuite/ld-elf/pr31482c-no-lto.c
create mode 100644 ld/testsuite/ld-elf/pr31482d-no-lto.c
create mode 100644 ld/testsuite/ld-plugin/pass1.out
create mode 100644 ld/testsuite/ld-plugin/pr31482a.c
create mode 100644 ld/testsuite/ld-plugin/pr31482b.c
create mode 100644 ld/testsuite/ld-plugin/pr31482c.c
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index c5d325435b6..034afa65593 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -737,6 +737,10 @@ struct elf_link_hash_table
/* Small local sym cache. */
struct sym_cache sym_cache;
+ /* Hash table of symbols which are first defined in archives or shared
+ objects when there are any IR inputs. */
+ struct bfd_link_hash_table *first_hash;
+
/* Short-cuts to get to dynamic linker sections. */
asection *sgot;
asection *sgotplt;
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5a6cb07b2ce..797ba8231f0 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -4265,7 +4265,22 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
}
if ((abfd->flags & DYNAMIC) == 0)
- dynamic = false;
+ {
+ dynamic = false;
+ if ((abfd->flags & BFD_PLUGIN) != 0
+ && is_elf_hash_table (&htab->root)
+ && htab->first_hash == NULL)
+ {
+ /* Initialize first_hash for an IR input. */
+ htab->first_hash = (struct bfd_link_hash_table *)
+ xmalloc (sizeof (struct bfd_link_hash_table));
+ if (!bfd_hash_table_init (&htab->first_hash->table,
+ _bfd_link_hash_newfunc,
+ sizeof (struct bfd_link_hash_entry)))
+ info->callbacks->einfo
+ (_("%F%P: first_hash failed to initialize: %E\n"));
+ }
+ }
else
{
dynamic = true;
@@ -5118,16 +5133,31 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
if (skip)
continue;
- /* Override a definition only if the new symbol matches the
- existing one. */
- if (override && matched)
- definition = false;
-
h = *sym_hash;
while (h->root.type == bfd_link_hash_indirect
|| h->root.type == bfd_link_hash_warning)
h = (struct elf_link_hash_entry *) h->root.u.i.link;
+ /* Override a definition only if the new symbol matches the
+ existing one. */
+ if (override && matched)
+ {
+ definition = false;
+ if (htab->first_hash != NULL
+ && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
+ && h->root.non_ir_ref_regular)
+ {
+ /* When reloading --as-needed shared objects for new
+ symbols added from IR inputs, if this shared object
+ has the first definition, use it. */
+ struct bfd_link_hash_entry *e
+ = bfd_link_hash_lookup (htab->first_hash, name,
+ false, false, true);
+ if (e != NULL && e->u.undef.abfd == abfd)
+ definition = true;
+ }
+ }
+
if (h->versioned != unversioned
&& elf_tdata (abfd)->verdef != NULL
&& vernum > 1
@@ -5477,8 +5507,9 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
if (!add_needed
&& matched
&& definition
- && h->root.type != bfd_link_hash_indirect
- && ((dynsym
+ && h->root.type != bfd_link_hash_indirect)
+ {
+ if ((dynsym
&& h->ref_regular_nonweak)
|| (old_bfd != NULL
&& (old_bfd->flags & BFD_PLUGIN) != 0
@@ -5487,37 +5518,60 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
|| (h->ref_dynamic_nonweak
&& (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
&& !on_needed_list (elf_dt_name (abfd),
- htab->needed, NULL))))
- {
- const char *soname = elf_dt_name (abfd);
+ htab->needed, NULL)))
+ {
+ const char *soname = elf_dt_name (abfd);
+
+ info->callbacks->minfo ("%!", soname, old_bfd,
+ h->root.root.string);
+
+ /* A symbol from a library loaded via DT_NEEDED of some
+ other library is referenced by a regular object.
+ Add a DT_NEEDED entry for it. Issue an error if
+ --no-add-needed is used and the reference was not
+ a weak one. */
+ if (old_bfd != NULL
+ && (elf_dyn_lib_class (abfd) & DYN_NO_NEEDED) != 0)
+ {
+ _bfd_error_handler
+ /* xgettext:c-format */
+ (_("%pB: undefined reference to symbol '%s'"),
+ old_bfd, name);
+ bfd_set_error (bfd_error_missing_dso);
+ goto error_free_vers;
+ }
- info->callbacks->minfo ("%!", soname, old_bfd,
- h->root.root.string);
+ elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
+ (elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
- /* A symbol from a library loaded via DT_NEEDED of some
- other library is referenced by a regular object.
- Add a DT_NEEDED entry for it. Issue an error if
- --no-add-needed is used and the reference was not
- a weak one. */
- if (old_bfd != NULL
- && (elf_dyn_lib_class (abfd) & DYN_NO_NEEDED) != 0)
- {
- _bfd_error_handler
- /* xgettext:c-format */
- (_("%pB: undefined reference to symbol '%s'"),
- old_bfd, name);
- bfd_set_error (bfd_error_missing_dso);
- goto error_free_vers;
+ /* Create dynamic sections for backends that require
+ that be done before setup_gnu_properties. */
+ if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
+ return false;
+ add_needed = true;
}
+ else if (dynamic
+ && htab->first_hash != NULL
+ && h->root.u.def.section->owner == abfd)
+ {
+ /* Add this symbol to first hash if this shared
+ object has the first definition. */
+ struct bfd_link_hash_entry *e
+ = bfd_link_hash_lookup (htab->first_hash, name, true,
+ false, true);
+ if (e == NULL)
+ info->callbacks->einfo
+ (_("%F%P: %pB: failed to add %s to first hash\n"),
+ abfd, name);
- elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
- (elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
-
- /* Create dynamic sections for backends that require
- that be done before setup_gnu_properties. */
- if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
- return false;
- add_needed = true;
+ if (e->type == bfd_link_hash_new)
+ {
+ /* Change the type to bfd_link_hash_defined and
+ store ABFD in u.undef->abfd. */
+ e->type = bfd_link_hash_defined;
+ e->u.undef.abfd = abfd;
+ }
+ }
}
}
}
@@ -5963,7 +6017,30 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
p = strchr (name, ELF_VER_CHR);
if (p == NULL || p[1] != ELF_VER_CHR)
- return h;
+ {
+ struct elf_link_hash_table *htab = elf_hash_table (info);
+ if (htab->first_hash != NULL)
+ {
+ /* Add this symbol to first hash if this archive has the
+ first definition. */
+ struct bfd_link_hash_entry *e
+ = bfd_link_hash_lookup (htab->first_hash, name, true,
+ false, true);
+ if (e == NULL)
+ info->callbacks->einfo
+ (_("%F%P: %pB: failed to add %s to first hash\n"),
+ abfd, name);
+
+ if (e->type == bfd_link_hash_new)
+ {
+ /* Change the type to bfd_link_hash_defined and store
+ ABFD in u.undef->abfd. */
+ e->type = bfd_link_hash_defined;
+ e->u.undef.abfd = abfd;
+ }
+ }
+ return h;
+ }
/* First check with only one `@'. */
len = strlen (name);
@@ -6102,7 +6179,22 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
if (h->type != bfd_link_hash_undefweak)
/* Symbol must be defined. Don't check it again. */
included[i] = true;
- continue;
+
+ /* Ignore the archive if the symbol isn't defined in a
+ shared object. */
+ if (!((struct elf_link_hash_entry *) h)->def_dynamic)
+ continue;
+ /* Ignore the dynamic definition if symbol is first
+ defined in this archive. */
+ struct elf_link_hash_table *htab = elf_hash_table (info);
+ if (htab->first_hash == NULL)
+ continue;
+ struct bfd_link_hash_entry *e
+ = bfd_link_hash_lookup (htab->first_hash,
+ symdef->name, false, false,
+ true);
+ if (e == NULL || e->u.undef.abfd != abfd)
+ continue;
}
/* We need to include this archive member. */
@@ -8185,6 +8277,11 @@ _bfd_elf_link_hash_table_free (bfd *obfd)
_bfd_elf_strtab_free (htab->dynstr);
_bfd_merge_sections_free (htab->merge_info);
_bfd_generic_link_hash_table_free (obfd);
+ if (htab->first_hash != NULL)
+ {
+ bfd_hash_table_free (&htab->first_hash->table);
+ free (htab->first_hash);
+ }
}
/* This is a hook for the ELF emulation code in the generic linker to
diff --git a/ld/testsuite/ld-elf/pr31482a-no-lto.c b/ld/testsuite/ld-elf/pr31482a-no-lto.c
new file mode 100644
index 00000000000..abc23be0714
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr31482a-no-lto.c
@@ -0,0 +1,8 @@
+extern void foo (void);
+
+int
+main()
+{
+ foo ();
+ return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr31482b-no-lto.c b/ld/testsuite/ld-elf/pr31482b-no-lto.c
new file mode 100644
index 00000000000..f88254d4ee1
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr31482b-no-lto.c
@@ -0,0 +1,10 @@
+void
+func (void)
+{
+}
+
+void
+bar (void)
+{
+ func ();
+}
diff --git a/ld/testsuite/ld-elf/pr31482c-no-lto.c b/ld/testsuite/ld-elf/pr31482c-no-lto.c
new file mode 100644
index 00000000000..bf326dd399d
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr31482c-no-lto.c
@@ -0,0 +1,15 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+void
+abort (void)
+{
+ printf ("PASS\n");
+ exit (0);
+}
+
+void
+foo (void)
+{
+ abort ();
+}
diff --git a/ld/testsuite/ld-elf/pr31482d-no-lto.c b/ld/testsuite/ld-elf/pr31482d-no-lto.c
new file mode 100644
index 00000000000..7cdaff09c72
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr31482d-no-lto.c
@@ -0,0 +1,9 @@
+void
+func (void)
+{
+}
+
+void
+foo (void)
+{
+}
diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
index 277dc7bf2de..9e89077af89 100644
--- a/ld/testsuite/ld-elf/shared.exp
+++ b/ld/testsuite/ld-elf/shared.exp
@@ -912,6 +912,30 @@ run_cc_link_tests [list \
{{readelf {--dyn-syms --wide} pr28348.rd}} \
"pr28348" \
] \
+ [list \
+ "Build pr31482b-no-lto.so" \
+ "-shared" \
+ "-fPIC" \
+ {pr31482b-no-lto.c} \
+ {} \
+ "pr31482b-no-lto.so" \
+ ] \
+ [list \
+ "Build pr31482c-no-lto.a" \
+ "" \
+ "" \
+ {pr31482c-no-lto.c} \
+ {} \
+ "pr31482c-no-lto.a" \
+ ] \
+ [list \
+ "Build pr31482d-no-lto.a" \
+ "" \
+ "" \
+ {pr31482d-no-lto.c} \
+ {} \
+ "pr31482d-no-lto.a" \
+ ] \
]
# pr19073.s uses .set, which has a different meaning on alpha.
@@ -1165,6 +1189,18 @@ set run_tests [list \
"" "" \
{pr26590c.c pr26590d.c} "pr26590" "pass.out" "" "c" "" \
"-Wl,--as-needed tmpdir/libpr26590a.so tmpdir/libpr26590b.so" ] \
+ [list "Run pr31482 (no-lto)" \
+ "-Wl,--no-as-needed" \
+ "" \
+ {pr31482a-no-lto.c} \
+ "pr31482-no-lto" \
+ "pass.out" \
+ "" \
+ "c" \
+ "" \
+ "tmpdir/pr31482b-no-lto.so tmpdir/pr31482c-no-lto.a \
+ tmpdir/pr31482d-no-lto.a" \
+ ] \
]
# NetBSD ELF systems do not currently support the .*_array sections.
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index cf1691fec6d..9e53e2d3cec 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -539,6 +539,22 @@ set lto_link_elf_tests [list \
"" \
"pr30281.so" \
] \
+ [list \
+ "Build pr31482b.a" \
+ "" \
+ "" \
+ {pr31482b.c} \
+ "" \
+ "pr31482b.a" \
+ ] \
+ [list \
+ "Build pr31482c.so" \
+ "-shared" \
+ "-fPIC" \
+ {pr31482c.c} \
+ "" \
+ "pr31482c.so" \
+ ] \
]
# PR 14918 checks that libgcc is not spuriously included in a shared link of
@@ -722,6 +738,22 @@ set lto_run_elf_shared_tests [list \
{-Wl,--as-needed,-R,tmpdir} {} \
{lto-19c.c} {lto-19.exe} {pass.out} {-flto -O2} {c} {} \
{tmpdir/liblto-19.so tmpdir/liblto-19.a}] \
+ [list {pr31482a} \
+ {-Wl,--no-as-needed,-R,tmpdir} {} \
+ {pr31482a.c} {pr31482a.exe} {pass.out} {-flto} {c} {} \
+ {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
+ [list {pr31482b} \
+ {-Wl,--no-as-needed,-R,tmpdir} {} \
+ {pr31482a.c} {pr31482b.exe} {pass1.out} {-flto} {c} {} \
+ {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
+ [list {pr31489a} \
+ {-Wl,--as-needed,-R,tmpdir} {} \
+ {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
+ {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
+ [list {pr31489b} \
+ {-Wl,--as-needed,-R,tmpdir} {} \
+ {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
+ {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
]
# LTO run-time tests for ELF
diff --git a/ld/testsuite/ld-plugin/pass1.out b/ld/testsuite/ld-plugin/pass1.out
new file mode 100644
index 00000000000..8e5c818f1a6
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pass1.out
@@ -0,0 +1 @@
+PASS1
diff --git a/ld/testsuite/ld-plugin/pr31482a.c b/ld/testsuite/ld-plugin/pr31482a.c
new file mode 100644
index 00000000000..0693e471123
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr31482a.c
@@ -0,0 +1,8 @@
+#include <stdlib.h>
+
+int
+main()
+{
+ abort ();
+ return 0;
+}
diff --git a/ld/testsuite/ld-plugin/pr31482b.c b/ld/testsuite/ld-plugin/pr31482b.c
new file mode 100644
index 00000000000..3c241735c16
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr31482b.c
@@ -0,0 +1,9 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+void
+abort (void)
+{
+ printf ("PASS\n");
+ exit (0);
+}
diff --git a/ld/testsuite/ld-plugin/pr31482c.c b/ld/testsuite/ld-plugin/pr31482c.c
new file mode 100644
index 00000000000..5c3b5099b52
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr31482c.c
@@ -0,0 +1,9 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+void
+abort (void)
+{
+ printf ("PASS1\n");
+ exit (0);
+}
--
2.44.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* PING: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-03-15 19:43 [PATCH v3] elf: Always honor the first definition in shared object and archive H.J. Lu
@ 2024-03-28 13:30 ` H.J. Lu
2024-04-04 13:29 ` PING^2: " H.J. Lu
2024-04-04 23:54 ` Alan Modra
1 sibling, 1 reply; 11+ messages in thread
From: H.J. Lu @ 2024-03-28 13:30 UTC (permalink / raw)
To: Binutils, Nick Clifton, Alan Modra
On Fri, Mar 15, 2024 at 12:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> GCC doesn't put builtin function symbol references, which are defined in
> the shared C library, in the IR symbol table. When linker rescans shared
> objects and archives for newly added symbol references generated from the
> IR inputs, it skips definitions of the builtin functions in shared
> objects and archives.
>
> Add first_hash to elf_link_hash_table to track unreferenced definitions
> defined first in shared objects and archives. Always use them to resolve
> any references.
>
> bfd/
>
> PR ld/31482
> PR ld/31489
> * elf-bfd.h (elf_link_hash_table): Add first_hash.
> * elflink.c (elf_link_add_object_symbols): Initialize first_hash
> for an IR input. Always use the first definition in shared
> object. Add the first unreferenced dynamic definition to
> first_hash.
> (_bfd_elf_archive_symbol_lookup): Add the first unreferenced
> definition to first_hash..
> (elf_link_add_archive_symbols): Use the symbol definition in
> archive if symbol is defined first in this archive.
> (_bfd_elf_link_hash_table_free): Also free first_hash.
>
> ld/
>
> PR ld/31482
> PR ld/31489
> * testsuite/ld-plugin/lto.exp: Add PR ld/31482 and PR ld/31489
> tests.
> * testsuite/ld-elf/pr31482a-no-lto.c: New file.
> * testsuite/ld-elf/pr31482b-no-lto.c: Likewise.
> * testsuite/ld-elf/pr31482c-no-lto.c: Likewise.
> * testsuite/ld-elf/pr31482d-no-lto.c: Likewise.
> * testsuite/ld-plugin/pass1.out: Likewise.
> * testsuite/ld-plugin/pr31482a.c: Likewise.
> * testsuite/ld-plugin/pr31482b.c: Likewise.
> * testsuite/ld-plugin/pr31482c.c: Likewise.
> ---
> bfd/elf-bfd.h | 4 +
> bfd/elflink.c | 171 ++++++++++++++++++++------
> ld/testsuite/ld-elf/pr31482a-no-lto.c | 8 ++
> ld/testsuite/ld-elf/pr31482b-no-lto.c | 10 ++
> ld/testsuite/ld-elf/pr31482c-no-lto.c | 15 +++
> ld/testsuite/ld-elf/pr31482d-no-lto.c | 9 ++
> ld/testsuite/ld-elf/shared.exp | 36 ++++++
> ld/testsuite/ld-plugin/lto.exp | 32 +++++
> ld/testsuite/ld-plugin/pass1.out | 1 +
> ld/testsuite/ld-plugin/pr31482a.c | 8 ++
> ld/testsuite/ld-plugin/pr31482b.c | 9 ++
> ld/testsuite/ld-plugin/pr31482c.c | 9 ++
> 12 files changed, 275 insertions(+), 37 deletions(-)
> create mode 100644 ld/testsuite/ld-elf/pr31482a-no-lto.c
> create mode 100644 ld/testsuite/ld-elf/pr31482b-no-lto.c
> create mode 100644 ld/testsuite/ld-elf/pr31482c-no-lto.c
> create mode 100644 ld/testsuite/ld-elf/pr31482d-no-lto.c
> create mode 100644 ld/testsuite/ld-plugin/pass1.out
> create mode 100644 ld/testsuite/ld-plugin/pr31482a.c
> create mode 100644 ld/testsuite/ld-plugin/pr31482b.c
> create mode 100644 ld/testsuite/ld-plugin/pr31482c.c
>
> diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> index c5d325435b6..034afa65593 100644
> --- a/bfd/elf-bfd.h
> +++ b/bfd/elf-bfd.h
> @@ -737,6 +737,10 @@ struct elf_link_hash_table
> /* Small local sym cache. */
> struct sym_cache sym_cache;
>
> + /* Hash table of symbols which are first defined in archives or shared
> + objects when there are any IR inputs. */
> + struct bfd_link_hash_table *first_hash;
> +
> /* Short-cuts to get to dynamic linker sections. */
> asection *sgot;
> asection *sgotplt;
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index 5a6cb07b2ce..797ba8231f0 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -4265,7 +4265,22 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> }
>
> if ((abfd->flags & DYNAMIC) == 0)
> - dynamic = false;
> + {
> + dynamic = false;
> + if ((abfd->flags & BFD_PLUGIN) != 0
> + && is_elf_hash_table (&htab->root)
> + && htab->first_hash == NULL)
> + {
> + /* Initialize first_hash for an IR input. */
> + htab->first_hash = (struct bfd_link_hash_table *)
> + xmalloc (sizeof (struct bfd_link_hash_table));
> + if (!bfd_hash_table_init (&htab->first_hash->table,
> + _bfd_link_hash_newfunc,
> + sizeof (struct bfd_link_hash_entry)))
> + info->callbacks->einfo
> + (_("%F%P: first_hash failed to initialize: %E\n"));
> + }
> + }
> else
> {
> dynamic = true;
> @@ -5118,16 +5133,31 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> if (skip)
> continue;
>
> - /* Override a definition only if the new symbol matches the
> - existing one. */
> - if (override && matched)
> - definition = false;
> -
> h = *sym_hash;
> while (h->root.type == bfd_link_hash_indirect
> || h->root.type == bfd_link_hash_warning)
> h = (struct elf_link_hash_entry *) h->root.u.i.link;
>
> + /* Override a definition only if the new symbol matches the
> + existing one. */
> + if (override && matched)
> + {
> + definition = false;
> + if (htab->first_hash != NULL
> + && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
> + && h->root.non_ir_ref_regular)
> + {
> + /* When reloading --as-needed shared objects for new
> + symbols added from IR inputs, if this shared object
> + has the first definition, use it. */
> + struct bfd_link_hash_entry *e
> + = bfd_link_hash_lookup (htab->first_hash, name,
> + false, false, true);
> + if (e != NULL && e->u.undef.abfd == abfd)
> + definition = true;
> + }
> + }
> +
> if (h->versioned != unversioned
> && elf_tdata (abfd)->verdef != NULL
> && vernum > 1
> @@ -5477,8 +5507,9 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> if (!add_needed
> && matched
> && definition
> - && h->root.type != bfd_link_hash_indirect
> - && ((dynsym
> + && h->root.type != bfd_link_hash_indirect)
> + {
> + if ((dynsym
> && h->ref_regular_nonweak)
> || (old_bfd != NULL
> && (old_bfd->flags & BFD_PLUGIN) != 0
> @@ -5487,37 +5518,60 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> || (h->ref_dynamic_nonweak
> && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
> && !on_needed_list (elf_dt_name (abfd),
> - htab->needed, NULL))))
> - {
> - const char *soname = elf_dt_name (abfd);
> + htab->needed, NULL)))
> + {
> + const char *soname = elf_dt_name (abfd);
> +
> + info->callbacks->minfo ("%!", soname, old_bfd,
> + h->root.root.string);
> +
> + /* A symbol from a library loaded via DT_NEEDED of some
> + other library is referenced by a regular object.
> + Add a DT_NEEDED entry for it. Issue an error if
> + --no-add-needed is used and the reference was not
> + a weak one. */
> + if (old_bfd != NULL
> + && (elf_dyn_lib_class (abfd) & DYN_NO_NEEDED) != 0)
> + {
> + _bfd_error_handler
> + /* xgettext:c-format */
> + (_("%pB: undefined reference to symbol '%s'"),
> + old_bfd, name);
> + bfd_set_error (bfd_error_missing_dso);
> + goto error_free_vers;
> + }
>
> - info->callbacks->minfo ("%!", soname, old_bfd,
> - h->root.root.string);
> + elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
> + (elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
>
> - /* A symbol from a library loaded via DT_NEEDED of some
> - other library is referenced by a regular object.
> - Add a DT_NEEDED entry for it. Issue an error if
> - --no-add-needed is used and the reference was not
> - a weak one. */
> - if (old_bfd != NULL
> - && (elf_dyn_lib_class (abfd) & DYN_NO_NEEDED) != 0)
> - {
> - _bfd_error_handler
> - /* xgettext:c-format */
> - (_("%pB: undefined reference to symbol '%s'"),
> - old_bfd, name);
> - bfd_set_error (bfd_error_missing_dso);
> - goto error_free_vers;
> + /* Create dynamic sections for backends that require
> + that be done before setup_gnu_properties. */
> + if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
> + return false;
> + add_needed = true;
> }
> + else if (dynamic
> + && htab->first_hash != NULL
> + && h->root.u.def.section->owner == abfd)
> + {
> + /* Add this symbol to first hash if this shared
> + object has the first definition. */
> + struct bfd_link_hash_entry *e
> + = bfd_link_hash_lookup (htab->first_hash, name, true,
> + false, true);
> + if (e == NULL)
> + info->callbacks->einfo
> + (_("%F%P: %pB: failed to add %s to first hash\n"),
> + abfd, name);
>
> - elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
> - (elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
> -
> - /* Create dynamic sections for backends that require
> - that be done before setup_gnu_properties. */
> - if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
> - return false;
> - add_needed = true;
> + if (e->type == bfd_link_hash_new)
> + {
> + /* Change the type to bfd_link_hash_defined and
> + store ABFD in u.undef->abfd. */
> + e->type = bfd_link_hash_defined;
> + e->u.undef.abfd = abfd;
> + }
> + }
> }
> }
> }
> @@ -5963,7 +6017,30 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
>
> p = strchr (name, ELF_VER_CHR);
> if (p == NULL || p[1] != ELF_VER_CHR)
> - return h;
> + {
> + struct elf_link_hash_table *htab = elf_hash_table (info);
> + if (htab->first_hash != NULL)
> + {
> + /* Add this symbol to first hash if this archive has the
> + first definition. */
> + struct bfd_link_hash_entry *e
> + = bfd_link_hash_lookup (htab->first_hash, name, true,
> + false, true);
> + if (e == NULL)
> + info->callbacks->einfo
> + (_("%F%P: %pB: failed to add %s to first hash\n"),
> + abfd, name);
> +
> + if (e->type == bfd_link_hash_new)
> + {
> + /* Change the type to bfd_link_hash_defined and store
> + ABFD in u.undef->abfd. */
> + e->type = bfd_link_hash_defined;
> + e->u.undef.abfd = abfd;
> + }
> + }
> + return h;
> + }
>
> /* First check with only one `@'. */
> len = strlen (name);
> @@ -6102,7 +6179,22 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
> if (h->type != bfd_link_hash_undefweak)
> /* Symbol must be defined. Don't check it again. */
> included[i] = true;
> - continue;
> +
> + /* Ignore the archive if the symbol isn't defined in a
> + shared object. */
> + if (!((struct elf_link_hash_entry *) h)->def_dynamic)
> + continue;
> + /* Ignore the dynamic definition if symbol is first
> + defined in this archive. */
> + struct elf_link_hash_table *htab = elf_hash_table (info);
> + if (htab->first_hash == NULL)
> + continue;
> + struct bfd_link_hash_entry *e
> + = bfd_link_hash_lookup (htab->first_hash,
> + symdef->name, false, false,
> + true);
> + if (e == NULL || e->u.undef.abfd != abfd)
> + continue;
> }
>
> /* We need to include this archive member. */
> @@ -8185,6 +8277,11 @@ _bfd_elf_link_hash_table_free (bfd *obfd)
> _bfd_elf_strtab_free (htab->dynstr);
> _bfd_merge_sections_free (htab->merge_info);
> _bfd_generic_link_hash_table_free (obfd);
> + if (htab->first_hash != NULL)
> + {
> + bfd_hash_table_free (&htab->first_hash->table);
> + free (htab->first_hash);
> + }
> }
>
> /* This is a hook for the ELF emulation code in the generic linker to
> diff --git a/ld/testsuite/ld-elf/pr31482a-no-lto.c b/ld/testsuite/ld-elf/pr31482a-no-lto.c
> new file mode 100644
> index 00000000000..abc23be0714
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr31482a-no-lto.c
> @@ -0,0 +1,8 @@
> +extern void foo (void);
> +
> +int
> +main()
> +{
> + foo ();
> + return 0;
> +}
> diff --git a/ld/testsuite/ld-elf/pr31482b-no-lto.c b/ld/testsuite/ld-elf/pr31482b-no-lto.c
> new file mode 100644
> index 00000000000..f88254d4ee1
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr31482b-no-lto.c
> @@ -0,0 +1,10 @@
> +void
> +func (void)
> +{
> +}
> +
> +void
> +bar (void)
> +{
> + func ();
> +}
> diff --git a/ld/testsuite/ld-elf/pr31482c-no-lto.c b/ld/testsuite/ld-elf/pr31482c-no-lto.c
> new file mode 100644
> index 00000000000..bf326dd399d
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr31482c-no-lto.c
> @@ -0,0 +1,15 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +void
> +abort (void)
> +{
> + printf ("PASS\n");
> + exit (0);
> +}
> +
> +void
> +foo (void)
> +{
> + abort ();
> +}
> diff --git a/ld/testsuite/ld-elf/pr31482d-no-lto.c b/ld/testsuite/ld-elf/pr31482d-no-lto.c
> new file mode 100644
> index 00000000000..7cdaff09c72
> --- /dev/null
> +++ b/ld/testsuite/ld-elf/pr31482d-no-lto.c
> @@ -0,0 +1,9 @@
> +void
> +func (void)
> +{
> +}
> +
> +void
> +foo (void)
> +{
> +}
> diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
> index 277dc7bf2de..9e89077af89 100644
> --- a/ld/testsuite/ld-elf/shared.exp
> +++ b/ld/testsuite/ld-elf/shared.exp
> @@ -912,6 +912,30 @@ run_cc_link_tests [list \
> {{readelf {--dyn-syms --wide} pr28348.rd}} \
> "pr28348" \
> ] \
> + [list \
> + "Build pr31482b-no-lto.so" \
> + "-shared" \
> + "-fPIC" \
> + {pr31482b-no-lto.c} \
> + {} \
> + "pr31482b-no-lto.so" \
> + ] \
> + [list \
> + "Build pr31482c-no-lto.a" \
> + "" \
> + "" \
> + {pr31482c-no-lto.c} \
> + {} \
> + "pr31482c-no-lto.a" \
> + ] \
> + [list \
> + "Build pr31482d-no-lto.a" \
> + "" \
> + "" \
> + {pr31482d-no-lto.c} \
> + {} \
> + "pr31482d-no-lto.a" \
> + ] \
> ]
>
> # pr19073.s uses .set, which has a different meaning on alpha.
> @@ -1165,6 +1189,18 @@ set run_tests [list \
> "" "" \
> {pr26590c.c pr26590d.c} "pr26590" "pass.out" "" "c" "" \
> "-Wl,--as-needed tmpdir/libpr26590a.so tmpdir/libpr26590b.so" ] \
> + [list "Run pr31482 (no-lto)" \
> + "-Wl,--no-as-needed" \
> + "" \
> + {pr31482a-no-lto.c} \
> + "pr31482-no-lto" \
> + "pass.out" \
> + "" \
> + "c" \
> + "" \
> + "tmpdir/pr31482b-no-lto.so tmpdir/pr31482c-no-lto.a \
> + tmpdir/pr31482d-no-lto.a" \
> + ] \
> ]
>
> # NetBSD ELF systems do not currently support the .*_array sections.
> diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
> index cf1691fec6d..9e53e2d3cec 100644
> --- a/ld/testsuite/ld-plugin/lto.exp
> +++ b/ld/testsuite/ld-plugin/lto.exp
> @@ -539,6 +539,22 @@ set lto_link_elf_tests [list \
> "" \
> "pr30281.so" \
> ] \
> + [list \
> + "Build pr31482b.a" \
> + "" \
> + "" \
> + {pr31482b.c} \
> + "" \
> + "pr31482b.a" \
> + ] \
> + [list \
> + "Build pr31482c.so" \
> + "-shared" \
> + "-fPIC" \
> + {pr31482c.c} \
> + "" \
> + "pr31482c.so" \
> + ] \
> ]
>
> # PR 14918 checks that libgcc is not spuriously included in a shared link of
> @@ -722,6 +738,22 @@ set lto_run_elf_shared_tests [list \
> {-Wl,--as-needed,-R,tmpdir} {} \
> {lto-19c.c} {lto-19.exe} {pass.out} {-flto -O2} {c} {} \
> {tmpdir/liblto-19.so tmpdir/liblto-19.a}] \
> + [list {pr31482a} \
> + {-Wl,--no-as-needed,-R,tmpdir} {} \
> + {pr31482a.c} {pr31482a.exe} {pass.out} {-flto} {c} {} \
> + {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> + [list {pr31482b} \
> + {-Wl,--no-as-needed,-R,tmpdir} {} \
> + {pr31482a.c} {pr31482b.exe} {pass1.out} {-flto} {c} {} \
> + {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> + [list {pr31489a} \
> + {-Wl,--as-needed,-R,tmpdir} {} \
> + {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
> + {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> + [list {pr31489b} \
> + {-Wl,--as-needed,-R,tmpdir} {} \
> + {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
> + {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> ]
>
> # LTO run-time tests for ELF
> diff --git a/ld/testsuite/ld-plugin/pass1.out b/ld/testsuite/ld-plugin/pass1.out
> new file mode 100644
> index 00000000000..8e5c818f1a6
> --- /dev/null
> +++ b/ld/testsuite/ld-plugin/pass1.out
> @@ -0,0 +1 @@
> +PASS1
> diff --git a/ld/testsuite/ld-plugin/pr31482a.c b/ld/testsuite/ld-plugin/pr31482a.c
> new file mode 100644
> index 00000000000..0693e471123
> --- /dev/null
> +++ b/ld/testsuite/ld-plugin/pr31482a.c
> @@ -0,0 +1,8 @@
> +#include <stdlib.h>
> +
> +int
> +main()
> +{
> + abort ();
> + return 0;
> +}
> diff --git a/ld/testsuite/ld-plugin/pr31482b.c b/ld/testsuite/ld-plugin/pr31482b.c
> new file mode 100644
> index 00000000000..3c241735c16
> --- /dev/null
> +++ b/ld/testsuite/ld-plugin/pr31482b.c
> @@ -0,0 +1,9 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +void
> +abort (void)
> +{
> + printf ("PASS\n");
> + exit (0);
> +}
> diff --git a/ld/testsuite/ld-plugin/pr31482c.c b/ld/testsuite/ld-plugin/pr31482c.c
> new file mode 100644
> index 00000000000..5c3b5099b52
> --- /dev/null
> +++ b/ld/testsuite/ld-plugin/pr31482c.c
> @@ -0,0 +1,9 @@
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +void
> +abort (void)
> +{
> + printf ("PASS1\n");
> + exit (0);
> +}
> --
> 2.44.0
>
PING.
--
H.J.
^ permalink raw reply [flat|nested] 11+ messages in thread
* PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-03-28 13:30 ` PING: " H.J. Lu
@ 2024-04-04 13:29 ` H.J. Lu
2024-04-05 10:15 ` Nick Clifton
0 siblings, 1 reply; 11+ messages in thread
From: H.J. Lu @ 2024-04-04 13:29 UTC (permalink / raw)
To: Binutils, Nick Clifton, Alan Modra
On Thu, Mar 28, 2024 at 6:30 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Mar 15, 2024 at 12:43 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > GCC doesn't put builtin function symbol references, which are defined in
> > the shared C library, in the IR symbol table. When linker rescans shared
> > objects and archives for newly added symbol references generated from the
> > IR inputs, it skips definitions of the builtin functions in shared
> > objects and archives.
> >
> > Add first_hash to elf_link_hash_table to track unreferenced definitions
> > defined first in shared objects and archives. Always use them to resolve
> > any references.
> >
> > bfd/
> >
> > PR ld/31482
> > PR ld/31489
> > * elf-bfd.h (elf_link_hash_table): Add first_hash.
> > * elflink.c (elf_link_add_object_symbols): Initialize first_hash
> > for an IR input. Always use the first definition in shared
> > object. Add the first unreferenced dynamic definition to
> > first_hash.
> > (_bfd_elf_archive_symbol_lookup): Add the first unreferenced
> > definition to first_hash..
> > (elf_link_add_archive_symbols): Use the symbol definition in
> > archive if symbol is defined first in this archive.
> > (_bfd_elf_link_hash_table_free): Also free first_hash.
> >
> > ld/
> >
> > PR ld/31482
> > PR ld/31489
> > * testsuite/ld-plugin/lto.exp: Add PR ld/31482 and PR ld/31489
> > tests.
> > * testsuite/ld-elf/pr31482a-no-lto.c: New file.
> > * testsuite/ld-elf/pr31482b-no-lto.c: Likewise.
> > * testsuite/ld-elf/pr31482c-no-lto.c: Likewise.
> > * testsuite/ld-elf/pr31482d-no-lto.c: Likewise.
> > * testsuite/ld-plugin/pass1.out: Likewise.
> > * testsuite/ld-plugin/pr31482a.c: Likewise.
> > * testsuite/ld-plugin/pr31482b.c: Likewise.
> > * testsuite/ld-plugin/pr31482c.c: Likewise.
> > ---
> > bfd/elf-bfd.h | 4 +
> > bfd/elflink.c | 171 ++++++++++++++++++++------
> > ld/testsuite/ld-elf/pr31482a-no-lto.c | 8 ++
> > ld/testsuite/ld-elf/pr31482b-no-lto.c | 10 ++
> > ld/testsuite/ld-elf/pr31482c-no-lto.c | 15 +++
> > ld/testsuite/ld-elf/pr31482d-no-lto.c | 9 ++
> > ld/testsuite/ld-elf/shared.exp | 36 ++++++
> > ld/testsuite/ld-plugin/lto.exp | 32 +++++
> > ld/testsuite/ld-plugin/pass1.out | 1 +
> > ld/testsuite/ld-plugin/pr31482a.c | 8 ++
> > ld/testsuite/ld-plugin/pr31482b.c | 9 ++
> > ld/testsuite/ld-plugin/pr31482c.c | 9 ++
> > 12 files changed, 275 insertions(+), 37 deletions(-)
> > create mode 100644 ld/testsuite/ld-elf/pr31482a-no-lto.c
> > create mode 100644 ld/testsuite/ld-elf/pr31482b-no-lto.c
> > create mode 100644 ld/testsuite/ld-elf/pr31482c-no-lto.c
> > create mode 100644 ld/testsuite/ld-elf/pr31482d-no-lto.c
> > create mode 100644 ld/testsuite/ld-plugin/pass1.out
> > create mode 100644 ld/testsuite/ld-plugin/pr31482a.c
> > create mode 100644 ld/testsuite/ld-plugin/pr31482b.c
> > create mode 100644 ld/testsuite/ld-plugin/pr31482c.c
> >
> > diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
> > index c5d325435b6..034afa65593 100644
> > --- a/bfd/elf-bfd.h
> > +++ b/bfd/elf-bfd.h
> > @@ -737,6 +737,10 @@ struct elf_link_hash_table
> > /* Small local sym cache. */
> > struct sym_cache sym_cache;
> >
> > + /* Hash table of symbols which are first defined in archives or shared
> > + objects when there are any IR inputs. */
> > + struct bfd_link_hash_table *first_hash;
> > +
> > /* Short-cuts to get to dynamic linker sections. */
> > asection *sgot;
> > asection *sgotplt;
> > diff --git a/bfd/elflink.c b/bfd/elflink.c
> > index 5a6cb07b2ce..797ba8231f0 100644
> > --- a/bfd/elflink.c
> > +++ b/bfd/elflink.c
> > @@ -4265,7 +4265,22 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> > }
> >
> > if ((abfd->flags & DYNAMIC) == 0)
> > - dynamic = false;
> > + {
> > + dynamic = false;
> > + if ((abfd->flags & BFD_PLUGIN) != 0
> > + && is_elf_hash_table (&htab->root)
> > + && htab->first_hash == NULL)
> > + {
> > + /* Initialize first_hash for an IR input. */
> > + htab->first_hash = (struct bfd_link_hash_table *)
> > + xmalloc (sizeof (struct bfd_link_hash_table));
> > + if (!bfd_hash_table_init (&htab->first_hash->table,
> > + _bfd_link_hash_newfunc,
> > + sizeof (struct bfd_link_hash_entry)))
> > + info->callbacks->einfo
> > + (_("%F%P: first_hash failed to initialize: %E\n"));
> > + }
> > + }
> > else
> > {
> > dynamic = true;
> > @@ -5118,16 +5133,31 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> > if (skip)
> > continue;
> >
> > - /* Override a definition only if the new symbol matches the
> > - existing one. */
> > - if (override && matched)
> > - definition = false;
> > -
> > h = *sym_hash;
> > while (h->root.type == bfd_link_hash_indirect
> > || h->root.type == bfd_link_hash_warning)
> > h = (struct elf_link_hash_entry *) h->root.u.i.link;
> >
> > + /* Override a definition only if the new symbol matches the
> > + existing one. */
> > + if (override && matched)
> > + {
> > + definition = false;
> > + if (htab->first_hash != NULL
> > + && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
> > + && h->root.non_ir_ref_regular)
> > + {
> > + /* When reloading --as-needed shared objects for new
> > + symbols added from IR inputs, if this shared object
> > + has the first definition, use it. */
> > + struct bfd_link_hash_entry *e
> > + = bfd_link_hash_lookup (htab->first_hash, name,
> > + false, false, true);
> > + if (e != NULL && e->u.undef.abfd == abfd)
> > + definition = true;
> > + }
> > + }
> > +
> > if (h->versioned != unversioned
> > && elf_tdata (abfd)->verdef != NULL
> > && vernum > 1
> > @@ -5477,8 +5507,9 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> > if (!add_needed
> > && matched
> > && definition
> > - && h->root.type != bfd_link_hash_indirect
> > - && ((dynsym
> > + && h->root.type != bfd_link_hash_indirect)
> > + {
> > + if ((dynsym
> > && h->ref_regular_nonweak)
> > || (old_bfd != NULL
> > && (old_bfd->flags & BFD_PLUGIN) != 0
> > @@ -5487,37 +5518,60 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> > || (h->ref_dynamic_nonweak
> > && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
> > && !on_needed_list (elf_dt_name (abfd),
> > - htab->needed, NULL))))
> > - {
> > - const char *soname = elf_dt_name (abfd);
> > + htab->needed, NULL)))
> > + {
> > + const char *soname = elf_dt_name (abfd);
> > +
> > + info->callbacks->minfo ("%!", soname, old_bfd,
> > + h->root.root.string);
> > +
> > + /* A symbol from a library loaded via DT_NEEDED of some
> > + other library is referenced by a regular object.
> > + Add a DT_NEEDED entry for it. Issue an error if
> > + --no-add-needed is used and the reference was not
> > + a weak one. */
> > + if (old_bfd != NULL
> > + && (elf_dyn_lib_class (abfd) & DYN_NO_NEEDED) != 0)
> > + {
> > + _bfd_error_handler
> > + /* xgettext:c-format */
> > + (_("%pB: undefined reference to symbol '%s'"),
> > + old_bfd, name);
> > + bfd_set_error (bfd_error_missing_dso);
> > + goto error_free_vers;
> > + }
> >
> > - info->callbacks->minfo ("%!", soname, old_bfd,
> > - h->root.root.string);
> > + elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
> > + (elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
> >
> > - /* A symbol from a library loaded via DT_NEEDED of some
> > - other library is referenced by a regular object.
> > - Add a DT_NEEDED entry for it. Issue an error if
> > - --no-add-needed is used and the reference was not
> > - a weak one. */
> > - if (old_bfd != NULL
> > - && (elf_dyn_lib_class (abfd) & DYN_NO_NEEDED) != 0)
> > - {
> > - _bfd_error_handler
> > - /* xgettext:c-format */
> > - (_("%pB: undefined reference to symbol '%s'"),
> > - old_bfd, name);
> > - bfd_set_error (bfd_error_missing_dso);
> > - goto error_free_vers;
> > + /* Create dynamic sections for backends that require
> > + that be done before setup_gnu_properties. */
> > + if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
> > + return false;
> > + add_needed = true;
> > }
> > + else if (dynamic
> > + && htab->first_hash != NULL
> > + && h->root.u.def.section->owner == abfd)
> > + {
> > + /* Add this symbol to first hash if this shared
> > + object has the first definition. */
> > + struct bfd_link_hash_entry *e
> > + = bfd_link_hash_lookup (htab->first_hash, name, true,
> > + false, true);
> > + if (e == NULL)
> > + info->callbacks->einfo
> > + (_("%F%P: %pB: failed to add %s to first hash\n"),
> > + abfd, name);
> >
> > - elf_dyn_lib_class (abfd) = (enum dynamic_lib_link_class)
> > - (elf_dyn_lib_class (abfd) & ~DYN_AS_NEEDED);
> > -
> > - /* Create dynamic sections for backends that require
> > - that be done before setup_gnu_properties. */
> > - if (!_bfd_elf_link_create_dynamic_sections (abfd, info))
> > - return false;
> > - add_needed = true;
> > + if (e->type == bfd_link_hash_new)
> > + {
> > + /* Change the type to bfd_link_hash_defined and
> > + store ABFD in u.undef->abfd. */
> > + e->type = bfd_link_hash_defined;
> > + e->u.undef.abfd = abfd;
> > + }
> > + }
> > }
> > }
> > }
> > @@ -5963,7 +6017,30 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
> >
> > p = strchr (name, ELF_VER_CHR);
> > if (p == NULL || p[1] != ELF_VER_CHR)
> > - return h;
> > + {
> > + struct elf_link_hash_table *htab = elf_hash_table (info);
> > + if (htab->first_hash != NULL)
> > + {
> > + /* Add this symbol to first hash if this archive has the
> > + first definition. */
> > + struct bfd_link_hash_entry *e
> > + = bfd_link_hash_lookup (htab->first_hash, name, true,
> > + false, true);
> > + if (e == NULL)
> > + info->callbacks->einfo
> > + (_("%F%P: %pB: failed to add %s to first hash\n"),
> > + abfd, name);
> > +
> > + if (e->type == bfd_link_hash_new)
> > + {
> > + /* Change the type to bfd_link_hash_defined and store
> > + ABFD in u.undef->abfd. */
> > + e->type = bfd_link_hash_defined;
> > + e->u.undef.abfd = abfd;
> > + }
> > + }
> > + return h;
> > + }
> >
> > /* First check with only one `@'. */
> > len = strlen (name);
> > @@ -6102,7 +6179,22 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
> > if (h->type != bfd_link_hash_undefweak)
> > /* Symbol must be defined. Don't check it again. */
> > included[i] = true;
> > - continue;
> > +
> > + /* Ignore the archive if the symbol isn't defined in a
> > + shared object. */
> > + if (!((struct elf_link_hash_entry *) h)->def_dynamic)
> > + continue;
> > + /* Ignore the dynamic definition if symbol is first
> > + defined in this archive. */
> > + struct elf_link_hash_table *htab = elf_hash_table (info);
> > + if (htab->first_hash == NULL)
> > + continue;
> > + struct bfd_link_hash_entry *e
> > + = bfd_link_hash_lookup (htab->first_hash,
> > + symdef->name, false, false,
> > + true);
> > + if (e == NULL || e->u.undef.abfd != abfd)
> > + continue;
> > }
> >
> > /* We need to include this archive member. */
> > @@ -8185,6 +8277,11 @@ _bfd_elf_link_hash_table_free (bfd *obfd)
> > _bfd_elf_strtab_free (htab->dynstr);
> > _bfd_merge_sections_free (htab->merge_info);
> > _bfd_generic_link_hash_table_free (obfd);
> > + if (htab->first_hash != NULL)
> > + {
> > + bfd_hash_table_free (&htab->first_hash->table);
> > + free (htab->first_hash);
> > + }
> > }
> >
> > /* This is a hook for the ELF emulation code in the generic linker to
> > diff --git a/ld/testsuite/ld-elf/pr31482a-no-lto.c b/ld/testsuite/ld-elf/pr31482a-no-lto.c
> > new file mode 100644
> > index 00000000000..abc23be0714
> > --- /dev/null
> > +++ b/ld/testsuite/ld-elf/pr31482a-no-lto.c
> > @@ -0,0 +1,8 @@
> > +extern void foo (void);
> > +
> > +int
> > +main()
> > +{
> > + foo ();
> > + return 0;
> > +}
> > diff --git a/ld/testsuite/ld-elf/pr31482b-no-lto.c b/ld/testsuite/ld-elf/pr31482b-no-lto.c
> > new file mode 100644
> > index 00000000000..f88254d4ee1
> > --- /dev/null
> > +++ b/ld/testsuite/ld-elf/pr31482b-no-lto.c
> > @@ -0,0 +1,10 @@
> > +void
> > +func (void)
> > +{
> > +}
> > +
> > +void
> > +bar (void)
> > +{
> > + func ();
> > +}
> > diff --git a/ld/testsuite/ld-elf/pr31482c-no-lto.c b/ld/testsuite/ld-elf/pr31482c-no-lto.c
> > new file mode 100644
> > index 00000000000..bf326dd399d
> > --- /dev/null
> > +++ b/ld/testsuite/ld-elf/pr31482c-no-lto.c
> > @@ -0,0 +1,15 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +void
> > +abort (void)
> > +{
> > + printf ("PASS\n");
> > + exit (0);
> > +}
> > +
> > +void
> > +foo (void)
> > +{
> > + abort ();
> > +}
> > diff --git a/ld/testsuite/ld-elf/pr31482d-no-lto.c b/ld/testsuite/ld-elf/pr31482d-no-lto.c
> > new file mode 100644
> > index 00000000000..7cdaff09c72
> > --- /dev/null
> > +++ b/ld/testsuite/ld-elf/pr31482d-no-lto.c
> > @@ -0,0 +1,9 @@
> > +void
> > +func (void)
> > +{
> > +}
> > +
> > +void
> > +foo (void)
> > +{
> > +}
> > diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
> > index 277dc7bf2de..9e89077af89 100644
> > --- a/ld/testsuite/ld-elf/shared.exp
> > +++ b/ld/testsuite/ld-elf/shared.exp
> > @@ -912,6 +912,30 @@ run_cc_link_tests [list \
> > {{readelf {--dyn-syms --wide} pr28348.rd}} \
> > "pr28348" \
> > ] \
> > + [list \
> > + "Build pr31482b-no-lto.so" \
> > + "-shared" \
> > + "-fPIC" \
> > + {pr31482b-no-lto.c} \
> > + {} \
> > + "pr31482b-no-lto.so" \
> > + ] \
> > + [list \
> > + "Build pr31482c-no-lto.a" \
> > + "" \
> > + "" \
> > + {pr31482c-no-lto.c} \
> > + {} \
> > + "pr31482c-no-lto.a" \
> > + ] \
> > + [list \
> > + "Build pr31482d-no-lto.a" \
> > + "" \
> > + "" \
> > + {pr31482d-no-lto.c} \
> > + {} \
> > + "pr31482d-no-lto.a" \
> > + ] \
> > ]
> >
> > # pr19073.s uses .set, which has a different meaning on alpha.
> > @@ -1165,6 +1189,18 @@ set run_tests [list \
> > "" "" \
> > {pr26590c.c pr26590d.c} "pr26590" "pass.out" "" "c" "" \
> > "-Wl,--as-needed tmpdir/libpr26590a.so tmpdir/libpr26590b.so" ] \
> > + [list "Run pr31482 (no-lto)" \
> > + "-Wl,--no-as-needed" \
> > + "" \
> > + {pr31482a-no-lto.c} \
> > + "pr31482-no-lto" \
> > + "pass.out" \
> > + "" \
> > + "c" \
> > + "" \
> > + "tmpdir/pr31482b-no-lto.so tmpdir/pr31482c-no-lto.a \
> > + tmpdir/pr31482d-no-lto.a" \
> > + ] \
> > ]
> >
> > # NetBSD ELF systems do not currently support the .*_array sections.
> > diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
> > index cf1691fec6d..9e53e2d3cec 100644
> > --- a/ld/testsuite/ld-plugin/lto.exp
> > +++ b/ld/testsuite/ld-plugin/lto.exp
> > @@ -539,6 +539,22 @@ set lto_link_elf_tests [list \
> > "" \
> > "pr30281.so" \
> > ] \
> > + [list \
> > + "Build pr31482b.a" \
> > + "" \
> > + "" \
> > + {pr31482b.c} \
> > + "" \
> > + "pr31482b.a" \
> > + ] \
> > + [list \
> > + "Build pr31482c.so" \
> > + "-shared" \
> > + "-fPIC" \
> > + {pr31482c.c} \
> > + "" \
> > + "pr31482c.so" \
> > + ] \
> > ]
> >
> > # PR 14918 checks that libgcc is not spuriously included in a shared link of
> > @@ -722,6 +738,22 @@ set lto_run_elf_shared_tests [list \
> > {-Wl,--as-needed,-R,tmpdir} {} \
> > {lto-19c.c} {lto-19.exe} {pass.out} {-flto -O2} {c} {} \
> > {tmpdir/liblto-19.so tmpdir/liblto-19.a}] \
> > + [list {pr31482a} \
> > + {-Wl,--no-as-needed,-R,tmpdir} {} \
> > + {pr31482a.c} {pr31482a.exe} {pass.out} {-flto} {c} {} \
> > + {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> > + [list {pr31482b} \
> > + {-Wl,--no-as-needed,-R,tmpdir} {} \
> > + {pr31482a.c} {pr31482b.exe} {pass1.out} {-flto} {c} {} \
> > + {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> > + [list {pr31489a} \
> > + {-Wl,--as-needed,-R,tmpdir} {} \
> > + {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
> > + {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> > + [list {pr31489b} \
> > + {-Wl,--as-needed,-R,tmpdir} {} \
> > + {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
> > + {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> > ]
> >
> > # LTO run-time tests for ELF
> > diff --git a/ld/testsuite/ld-plugin/pass1.out b/ld/testsuite/ld-plugin/pass1.out
> > new file mode 100644
> > index 00000000000..8e5c818f1a6
> > --- /dev/null
> > +++ b/ld/testsuite/ld-plugin/pass1.out
> > @@ -0,0 +1 @@
> > +PASS1
> > diff --git a/ld/testsuite/ld-plugin/pr31482a.c b/ld/testsuite/ld-plugin/pr31482a.c
> > new file mode 100644
> > index 00000000000..0693e471123
> > --- /dev/null
> > +++ b/ld/testsuite/ld-plugin/pr31482a.c
> > @@ -0,0 +1,8 @@
> > +#include <stdlib.h>
> > +
> > +int
> > +main()
> > +{
> > + abort ();
> > + return 0;
> > +}
> > diff --git a/ld/testsuite/ld-plugin/pr31482b.c b/ld/testsuite/ld-plugin/pr31482b.c
> > new file mode 100644
> > index 00000000000..3c241735c16
> > --- /dev/null
> > +++ b/ld/testsuite/ld-plugin/pr31482b.c
> > @@ -0,0 +1,9 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +void
> > +abort (void)
> > +{
> > + printf ("PASS\n");
> > + exit (0);
> > +}
> > diff --git a/ld/testsuite/ld-plugin/pr31482c.c b/ld/testsuite/ld-plugin/pr31482c.c
> > new file mode 100644
> > index 00000000000..5c3b5099b52
> > --- /dev/null
> > +++ b/ld/testsuite/ld-plugin/pr31482c.c
> > @@ -0,0 +1,9 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +void
> > +abort (void)
> > +{
> > + printf ("PASS1\n");
> > + exit (0);
> > +}
> > --
> > 2.44.0
> >
>
> PING.
>
PING.
--
H.J.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-03-15 19:43 [PATCH v3] elf: Always honor the first definition in shared object and archive H.J. Lu
2024-03-28 13:30 ` PING: " H.J. Lu
@ 2024-04-04 23:54 ` Alan Modra
2024-04-05 1:50 ` H.J. Lu
1 sibling, 1 reply; 11+ messages in thread
From: Alan Modra @ 2024-04-04 23:54 UTC (permalink / raw)
To: H.J. Lu; +Cc: binutils
On Fri, Mar 15, 2024 at 12:43:50PM -0700, H.J. Lu wrote:
> GCC doesn't put builtin function symbol references, which are defined in
> the shared C library, in the IR symbol table.
Should we be covering for what looks like a gcc bug? At the expense
of yet another hash table.
> + /* Hash table of symbols which are first defined in archives or shared
> + objects when there are any IR inputs. */
> + struct bfd_link_hash_table *first_hash;
> +
It looks to me that you just need a hash table that maps a symbol name
to a bfd. Maybe use libiberty/hashtab.c hash instead, to cut down
memory for entries?
> + if (e->type == bfd_link_hash_new)
> + {
> + /* Change the type to bfd_link_hash_defined and
> + store ABFD in u.undef->abfd. */
> + e->type = bfd_link_hash_defined;
> + e->u.undef.abfd = abfd;
> + }
That's a bit rude, using bfd_link_hash_defined and a union field that
belongs to bfd_link_hash_undefined.
> @@ -5963,7 +6017,30 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
>
> p = strchr (name, ELF_VER_CHR);
> if (p == NULL || p[1] != ELF_VER_CHR)
> - return h;
> + {
> + struct elf_link_hash_table *htab = elf_hash_table (info);
> + if (htab->first_hash != NULL)
> + {
> + /* Add this symbol to first hash if this archive has the
> + first definition. */
> + struct bfd_link_hash_entry *e
> + = bfd_link_hash_lookup (htab->first_hash, name, true,
> + false, true);
> + if (e == NULL)
> + info->callbacks->einfo
> + (_("%F%P: %pB: failed to add %s to first hash\n"),
> + abfd, name);
> +
> + if (e->type == bfd_link_hash_new)
> + {
> + /* Change the type to bfd_link_hash_defined and store
> + ABFD in u.undef->abfd. */
> + e->type = bfd_link_hash_defined;
> + e->u.undef.abfd = abfd;
> + }
> + }
> + return h;
> + }
At first glance this looks like a duplicate of the code in
elf_link_add_object_symbols adding an entry to first_hash, and
therefore could be extracted to a common function.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-04 23:54 ` Alan Modra
@ 2024-04-05 1:50 ` H.J. Lu
0 siblings, 0 replies; 11+ messages in thread
From: H.J. Lu @ 2024-04-05 1:50 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
On Thu, Apr 4, 2024 at 4:54 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Mar 15, 2024 at 12:43:50PM -0700, H.J. Lu wrote:
> > GCC doesn't put builtin function symbol references, which are defined in
> > the shared C library, in the IR symbol table.
>
> Should we be covering for what looks like a gcc bug? At the expense
> of yet another hash table.
I think we should. The built function references may be inlined by GCC.
It is reasonable for them not to show up in IR symbol tables.
> > + /* Hash table of symbols which are first defined in archives or shared
> > + objects when there are any IR inputs. */
> > + struct bfd_link_hash_table *first_hash;
> > +
>
> It looks to me that you just need a hash table that maps a symbol name
> to a bfd. Maybe use libiberty/hashtab.c hash instead, to cut down
> memory for entries?
I prefer not to deal with libiberty/hashtab.c.
> > + if (e->type == bfd_link_hash_new)
> > + {
> > + /* Change the type to bfd_link_hash_defined and
> > + store ABFD in u.undef->abfd. */
> > + e->type = bfd_link_hash_defined;
> > + e->u.undef.abfd = abfd;
> > + }
>
> That's a bit rude, using bfd_link_hash_defined and a union field that
> belongs to bfd_link_hash_undefined.
I changed it to bfd_link_hash_undefined.
> > @@ -5963,7 +6017,30 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
> >
> > p = strchr (name, ELF_VER_CHR);
> > if (p == NULL || p[1] != ELF_VER_CHR)
> > - return h;
> > + {
> > + struct elf_link_hash_table *htab = elf_hash_table (info);
> > + if (htab->first_hash != NULL)
> > + {
> > + /* Add this symbol to first hash if this archive has the
> > + first definition. */
> > + struct bfd_link_hash_entry *e
> > + = bfd_link_hash_lookup (htab->first_hash, name, true,
> > + false, true);
> > + if (e == NULL)
> > + info->callbacks->einfo
> > + (_("%F%P: %pB: failed to add %s to first hash\n"),
> > + abfd, name);
> > +
> > + if (e->type == bfd_link_hash_new)
> > + {
> > + /* Change the type to bfd_link_hash_defined and store
> > + ABFD in u.undef->abfd. */
> > + e->type = bfd_link_hash_defined;
> > + e->u.undef.abfd = abfd;
> > + }
> > + }
> > + return h;
> > + }
>
> At first glance this looks like a duplicate of the code in
> elf_link_add_object_symbols adding an entry to first_hash, and
> therefore could be extracted to a common function.
>
I added elf_link_add_to_first_hash in the v2 patch:
https://sourceware.org/pipermail/binutils/2024-April/133395.html
Thanks.
--
H.J.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-04 13:29 ` PING^2: " H.J. Lu
@ 2024-04-05 10:15 ` Nick Clifton
2024-04-05 12:00 ` H.J. Lu
0 siblings, 1 reply; 11+ messages in thread
From: Nick Clifton @ 2024-04-05 10:15 UTC (permalink / raw)
To: H.J. Lu, Binutils, Alan Modra
Hi H.J.
>>> GCC doesn't put builtin function symbol references, which are defined in
>>> the shared C library, in the IR symbol table. When linker rescans shared
>>> objects and archives for newly added symbol references generated from the
>>> IR inputs, it skips definitions of the builtin functions in shared
>>> objects and archives.
>>>
>>> Add first_hash to elf_link_hash_table to track unreferenced definitions
>>> defined first in shared objects and archives. Always use them to resolve
>>> any references.
Approved - please apply.
Cheers
Nick
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-05 10:15 ` Nick Clifton
@ 2024-04-05 12:00 ` H.J. Lu
2024-04-05 22:16 ` Alan Modra
0 siblings, 1 reply; 11+ messages in thread
From: H.J. Lu @ 2024-04-05 12:00 UTC (permalink / raw)
To: Nick Clifton; +Cc: Binutils, Alan Modra
On Fri, Apr 5, 2024 at 3:15 AM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi H.J.
>
> >>> GCC doesn't put builtin function symbol references, which are defined in
> >>> the shared C library, in the IR symbol table. When linker rescans shared
> >>> objects and archives for newly added symbol references generated from the
> >>> IR inputs, it skips definitions of the builtin functions in shared
> >>> objects and archives.
> >>>
> >>> Add first_hash to elf_link_hash_table to track unreferenced definitions
> >>> defined first in shared objects and archives. Always use them to resolve
> >>> any references.
>
> Approved - please apply.
>
> Cheers
> Nick
>
I am checking in the v2 patch:
https://sourceware.org/pipermail/binutils/2024-April/133395.html
Thanks.
--
H.J.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-05 12:00 ` H.J. Lu
@ 2024-04-05 22:16 ` Alan Modra
2024-04-05 23:37 ` H.J. Lu
0 siblings, 1 reply; 11+ messages in thread
From: Alan Modra @ 2024-04-05 22:16 UTC (permalink / raw)
To: H.J. Lu; +Cc: Nick Clifton, Binutils
On Fri, Apr 05, 2024 at 05:00:28AM -0700, H.J. Lu wrote:
> On Fri, Apr 5, 2024 at 3:15 AM Nick Clifton <nickc@redhat.com> wrote:
> >
> > Hi H.J.
> >
> > >>> GCC doesn't put builtin function symbol references, which are defined in
> > >>> the shared C library, in the IR symbol table. When linker rescans shared
> > >>> objects and archives for newly added symbol references generated from the
> > >>> IR inputs, it skips definitions of the builtin functions in shared
> > >>> objects and archives.
> > >>>
> > >>> Add first_hash to elf_link_hash_table to track unreferenced definitions
> > >>> defined first in shared objects and archives. Always use them to resolve
> > >>> any references.
> >
> > Approved - please apply.
> >
> > Cheers
> > Nick
> >
>
> I am checking in the v2 patch:
>
> https://sourceware.org/pipermail/binutils/2024-April/133395.html
I had some further requests to make.
1) If you are going to use a bfd_hash_table based first_hash, please
don't use bfd_link_hash_table. Instead extend bfd_hash_entry with a
single abfd field. You'll need to write a newfunc, but that's about
all that is required. There are multiple examples of this in the
source, eg. bfd/stabs.c stab_link_includes_entry.
2) You are accessing freed memory in _bfd_elf_link_hash_table_free.
Free first_hash before freeing the main hash table. That needs fixing
immediately.
3) Please ensure tests have different names. This
[list {pr31482a} \
{-Wl,--no-as-needed,-R,tmpdir} {} \
{pr31482a.c} {pr31482a.exe} {pass.out} {-flto} {c} {} \
{tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
[list {pr31482b} \
{-Wl,--no-as-needed,-R,tmpdir} {} \
{pr31482a.c} {pr31482b.exe} {pass1.out} {-flto} {c} {} \
{tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
[list {pr31489a} \
{-Wl,--as-needed,-R,tmpdir} {} \
{pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
{tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
[list {pr31489b} \
{-Wl,--as-needed,-R,tmpdir} {} \
{pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
{tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
has two tests called pr31489a and two called pr31489b.
"FAIL: pr31489a" as the output of make check isn't as helpful as it
could be. Better would be to add --no-as-needed and --as-needed to
the test names.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-05 22:16 ` Alan Modra
@ 2024-04-05 23:37 ` H.J. Lu
2024-04-05 23:45 ` Alan Modra
0 siblings, 1 reply; 11+ messages in thread
From: H.J. Lu @ 2024-04-05 23:37 UTC (permalink / raw)
To: Alan Modra; +Cc: Nick Clifton, Binutils
On Fri, Apr 5, 2024 at 3:16 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Apr 05, 2024 at 05:00:28AM -0700, H.J. Lu wrote:
> > On Fri, Apr 5, 2024 at 3:15 AM Nick Clifton <nickc@redhat.com> wrote:
> > >
> > > Hi H.J.
> > >
> > > >>> GCC doesn't put builtin function symbol references, which are defined in
> > > >>> the shared C library, in the IR symbol table. When linker rescans shared
> > > >>> objects and archives for newly added symbol references generated from the
> > > >>> IR inputs, it skips definitions of the builtin functions in shared
> > > >>> objects and archives.
> > > >>>
> > > >>> Add first_hash to elf_link_hash_table to track unreferenced definitions
> > > >>> defined first in shared objects and archives. Always use them to resolve
> > > >>> any references.
> > >
> > > Approved - please apply.
> > >
> > > Cheers
> > > Nick
> > >
> >
> > I am checking in the v2 patch:
> >
> > https://sourceware.org/pipermail/binutils/2024-April/133395.html
>
> I had some further requests to make.
> 1) If you are going to use a bfd_hash_table based first_hash, please
> don't use bfd_link_hash_table. Instead extend bfd_hash_entry with a
> single abfd field. You'll need to write a newfunc, but that's about
> all that is required. There are multiple examples of this in the
> source, eg. bfd/stabs.c stab_link_includes_entry.
I will add
/* An entry in the first definition hash table. */
struct elf_link_first_hash_entry
{
struct bfd_hash_entry root;
/* The object of the first definition. */
bfd *abfd;
};
> 2) You are accessing freed memory in _bfd_elf_link_hash_table_free.
> Free first_hash before freeing the main hash table. That needs fixing
> immediately.
I will change it to
if (htab->first_hash != NULL)
{
bfd_hash_table_free (htab->first_hash);
free (htab->first_hash);
}
_bfd_generic_link_hash_table_free (obfd);
> 3) Please ensure tests have different names. This
> [list {pr31482a} \
> {-Wl,--no-as-needed,-R,tmpdir} {} \
> {pr31482a.c} {pr31482a.exe} {pass.out} {-flto} {c} {} \
> {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> [list {pr31482b} \
> {-Wl,--no-as-needed,-R,tmpdir} {} \
> {pr31482a.c} {pr31482b.exe} {pass1.out} {-flto} {c} {} \
> {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> [list {pr31489a} \
> {-Wl,--as-needed,-R,tmpdir} {} \
> {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
> {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> [list {pr31489b} \
> {-Wl,--as-needed,-R,tmpdir} {} \
> {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
> {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> has two tests called pr31489a and two called pr31489b.
> "FAIL: pr31489a" as the output of make check isn't as helpful as it
> could be. Better would be to add --no-as-needed and --as-needed to
> the test names.
>
I got
ld-plugin/lto.exp: [list {pr31489a} \
ld-plugin/lto.exp: {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
and
ld-plugin/lto.exp: [list {pr31489b} \
ld-plugin/lto.exp: {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
I only saw one pr31489a and one pr31489b. Where are
the second ones?
--
H.J.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-05 23:37 ` H.J. Lu
@ 2024-04-05 23:45 ` Alan Modra
2024-04-05 23:48 ` H.J. Lu
0 siblings, 1 reply; 11+ messages in thread
From: Alan Modra @ 2024-04-05 23:45 UTC (permalink / raw)
To: H.J. Lu; +Cc: Nick Clifton, Binutils
On Fri, Apr 05, 2024 at 04:37:11PM -0700, H.J. Lu wrote:
> On Fri, Apr 5, 2024 at 3:16 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Fri, Apr 05, 2024 at 05:00:28AM -0700, H.J. Lu wrote:
> > > On Fri, Apr 5, 2024 at 3:15 AM Nick Clifton <nickc@redhat.com> wrote:
> > > >
> > > > Hi H.J.
> > > >
> > > > >>> GCC doesn't put builtin function symbol references, which are defined in
> > > > >>> the shared C library, in the IR symbol table. When linker rescans shared
> > > > >>> objects and archives for newly added symbol references generated from the
> > > > >>> IR inputs, it skips definitions of the builtin functions in shared
> > > > >>> objects and archives.
> > > > >>>
> > > > >>> Add first_hash to elf_link_hash_table to track unreferenced definitions
> > > > >>> defined first in shared objects and archives. Always use them to resolve
> > > > >>> any references.
> > > >
> > > > Approved - please apply.
> > > >
> > > > Cheers
> > > > Nick
> > > >
> > >
> > > I am checking in the v2 patch:
> > >
> > > https://sourceware.org/pipermail/binutils/2024-April/133395.html
> >
> > I had some further requests to make.
> > 1) If you are going to use a bfd_hash_table based first_hash, please
> > don't use bfd_link_hash_table. Instead extend bfd_hash_entry with a
> > single abfd field. You'll need to write a newfunc, but that's about
> > all that is required. There are multiple examples of this in the
> > source, eg. bfd/stabs.c stab_link_includes_entry.
>
> I will add
>
> /* An entry in the first definition hash table. */
>
> struct elf_link_first_hash_entry
> {
> struct bfd_hash_entry root;
> /* The object of the first definition. */
> bfd *abfd;
> };
>
> > 2) You are accessing freed memory in _bfd_elf_link_hash_table_free.
> > Free first_hash before freeing the main hash table. That needs fixing
> > immediately.
>
> I will change it to
>
> if (htab->first_hash != NULL)
> {
> bfd_hash_table_free (htab->first_hash);
> free (htab->first_hash);
> }
> _bfd_generic_link_hash_table_free (obfd);
Thanks, that along with the elf_link_first_hast_entry change is
preapproved.
> > 3) Please ensure tests have different names. This
> > [list {pr31482a} \
> > {-Wl,--no-as-needed,-R,tmpdir} {} \
> > {pr31482a.c} {pr31482a.exe} {pass.out} {-flto} {c} {} \
> > {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> > [list {pr31482b} \
> > {-Wl,--no-as-needed,-R,tmpdir} {} \
> > {pr31482a.c} {pr31482b.exe} {pass1.out} {-flto} {c} {} \
> > {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> > [list {pr31489a} \
> > {-Wl,--as-needed,-R,tmpdir} {} \
> > {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
> > {tmpdir/pr31482b.a tmpdir/pr31482c.so}] \
> > [list {pr31489b} \
> > {-Wl,--as-needed,-R,tmpdir} {} \
> > {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
> > {tmpdir/pr31482c.so tmpdir/pr31482b.a}] \
> > has two tests called pr31489a and two called pr31489b.
> > "FAIL: pr31489a" as the output of make check isn't as helpful as it
> > could be. Better would be to add --no-as-needed and --as-needed to
> > the test names.
> >
>
> I got
>
> ld-plugin/lto.exp: [list {pr31489a} \
> ld-plugin/lto.exp: {pr31482a.c} {pr31489a.exe} {pass.out} {-flto} {c} {} \
>
> and
>
> ld-plugin/lto.exp: [list {pr31489b} \
> ld-plugin/lto.exp: {pr31482a.c} {pr31489b.exe} {pass1.out} {-flto} {c} {} \
>
> I only saw one pr31489a and one pr31489b. Where are
> the second ones?
Oops. You chose different pr numbers. I can't read, sorry. :-(
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: PING^2: [PATCH v3] elf: Always honor the first definition in shared object and archive
2024-04-05 23:45 ` Alan Modra
@ 2024-04-05 23:48 ` H.J. Lu
0 siblings, 0 replies; 11+ messages in thread
From: H.J. Lu @ 2024-04-05 23:48 UTC (permalink / raw)
To: Alan Modra; +Cc: Nick Clifton, Binutils
On Fri, Apr 5, 2024 at 4:45 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Apr 05, 2024 at 04:37:11PM -0700, H.J. Lu wrote:
> > On Fri, Apr 5, 2024 at 3:16 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Fri, Apr 05, 2024 at 05:00:28AM -0700, H.J. Lu wrote:
> > > > On Fri, Apr 5, 2024 at 3:15 AM Nick Clifton <nickc@redhat.com> wrote:
> > > > >
> > > > > Hi H.J.
> > > > >
> > > > > >>> GCC doesn't put builtin function symbol references, which are defined in
> > > > > >>> the shared C library, in the IR symbol table. When linker rescans shared
> > > > > >>> objects and archives for newly added symbol references generated from the
> > > > > >>> IR inputs, it skips definitions of the builtin functions in shared
> > > > > >>> objects and archives.
> > > > > >>>
> > > > > >>> Add first_hash to elf_link_hash_table to track unreferenced definitions
> > > > > >>> defined first in shared objects and archives. Always use them to resolve
> > > > > >>> any references.
> > > > >
> > > > > Approved - please apply.
> > > > >
> > > > > Cheers
> > > > > Nick
> > > > >
> > > >
> > > > I am checking in the v2 patch:
> > > >
> > > > https://sourceware.org/pipermail/binutils/2024-April/133395.html
> > >
> > > I had some further requests to make.
> > > 1) If you are going to use a bfd_hash_table based first_hash, please
> > > don't use bfd_link_hash_table. Instead extend bfd_hash_entry with a
> > > single abfd field. You'll need to write a newfunc, but that's about
> > > all that is required. There are multiple examples of this in the
> > > source, eg. bfd/stabs.c stab_link_includes_entry.
> >
> > I will add
> >
> > /* An entry in the first definition hash table. */
> >
> > struct elf_link_first_hash_entry
> > {
> > struct bfd_hash_entry root;
> > /* The object of the first definition. */
> > bfd *abfd;
> > };
> >
> > > 2) You are accessing freed memory in _bfd_elf_link_hash_table_free.
> > > Free first_hash before freeing the main hash table. That needs fixing
> > > immediately.
> >
> > I will change it to
> >
> > if (htab->first_hash != NULL)
> > {
> > bfd_hash_table_free (htab->first_hash);
> > free (htab->first_hash);
> > }
> > _bfd_generic_link_hash_table_free (obfd);
>
> Thanks, that along with the elf_link_first_hast_entry change is
> preapproved.
I am checking in this:
https://sourceware.org/pipermail/binutils/2024-April/133415.html
Thanks.
--
H.J.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-04-05 23:49 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-15 19:43 [PATCH v3] elf: Always honor the first definition in shared object and archive H.J. Lu
2024-03-28 13:30 ` PING: " H.J. Lu
2024-04-04 13:29 ` PING^2: " H.J. Lu
2024-04-05 10:15 ` Nick Clifton
2024-04-05 12:00 ` H.J. Lu
2024-04-05 22:16 ` Alan Modra
2024-04-05 23:37 ` H.J. Lu
2024-04-05 23:45 ` Alan Modra
2024-04-05 23:48 ` H.J. Lu
2024-04-04 23:54 ` Alan Modra
2024-04-05 1:50 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).