* [PATCH] elf: Use elf_link_first_hash_entry for first_hash
@ 2024-04-05 23:37 H.J. Lu
2024-04-05 23:53 ` Alan Modra
2024-04-06 1:51 ` Alan Modra
0 siblings, 2 replies; 6+ messages in thread
From: H.J. Lu @ 2024-04-05 23:37 UTC (permalink / raw)
To: binutils; +Cc: amodra
Add elf_link_first_hash_entry and use it for first_hash. Free first_hash
before freeing the main hash table.
PR ld/31482
PR ld/31489
* elf-bfd.h (elf_link_hash_table): Change first_hash to
bfd_hash_table.
* elflink.c (elf_link_first_hash_entry): New.
(elf_link_first_hash_newfunc): Likewise.
(elf_link_add_to_first_hash): Updated.
(elf_link_add_object_symbols): Initialize first_hash with
elf_link_first_hash_newfunc.
(elf_link_add_object_symbols): Updated.
(elf_link_add_archive_symbols): Likewise.
(_bfd_elf_link_hash_table_free): Free first_hash before freeing
the main hash table.
---
bfd/elf-bfd.h | 2 +-
bfd/elflink.c | 87 ++++++++++++++++++++++++++++++++++++---------------
2 files changed, 63 insertions(+), 26 deletions(-)
diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index cf043d6d154..85e66488955 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -739,7 +739,7 @@ struct elf_link_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;
+ struct bfd_hash_table *first_hash;
/* Short-cuts to get to dynamic linker sections. */
asection *sgot;
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 3db517ca1cd..e41b3d6dad7 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -4261,6 +4261,45 @@ _bfd_elf_link_check_relocs (bfd *abfd, struct bfd_link_info *info)
return true;
}
+/* 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;
+};
+
+/* The function to create a new entry in the first definition hash
+ table. */
+
+static struct bfd_hash_entry *
+elf_link_first_hash_newfunc (struct bfd_hash_entry *entry,
+ struct bfd_hash_table *table,
+ const char *string)
+{
+ struct elf_link_first_hash_entry *ret =
+ (struct elf_link_first_hash_entry *) entry;
+
+ /* Allocate the structure if it has not already been allocated by a
+ subclass. */
+ if (ret == NULL)
+ ret = (struct elf_link_first_hash_entry *)
+ bfd_hash_allocate (table,
+ sizeof (struct elf_link_first_hash_entry));
+ if (ret == NULL)
+ return NULL;
+
+ /* Call the allocation method of the superclass. */
+ ret = ((struct elf_link_first_hash_entry *)
+ bfd_hash_newfunc ((struct bfd_hash_entry *) ret, table,
+ string));
+ if (ret != NULL)
+ ret->abfd = NULL;
+
+ return (struct bfd_hash_entry *) ret;
+}
+
/* Add the symbol NAME from ABFD to first hash. */
static void
@@ -4272,19 +4311,16 @@ elf_link_add_to_first_hash (bfd *abfd, struct bfd_link_info *info,
if (htab->first_hash == NULL)
return;
- struct bfd_link_hash_entry *e
- = bfd_link_hash_lookup (htab->first_hash, name, true, false, true);
+ struct elf_link_first_hash_entry *e
+ = ((struct elf_link_first_hash_entry *)
+ bfd_hash_lookup (htab->first_hash, name, true, false));
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_undefined and store ABFD in
- u.undef->abfd. */
- e->type = bfd_link_hash_undefined;
- e->u.undef.abfd = abfd;
- }
+ if (e->abfd == NULL)
+ /* Store ABFD in abfd. */
+ e->abfd = abfd;
}
/* Add symbols from an ELF object file to the linker hash table. */
@@ -4341,11 +4377,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
&& 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)))
+ htab->first_hash = (struct bfd_hash_table *)
+ xmalloc (sizeof (struct bfd_hash_table));
+ if (!bfd_hash_table_init
+ (htab->first_hash, elf_link_first_hash_newfunc,
+ sizeof (struct elf_link_first_hash_entry)))
info->callbacks->einfo
(_("%F%P: first_hash failed to initialize: %E\n"));
}
@@ -5219,10 +5255,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
/* 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)
+ struct elf_link_first_hash_entry *e
+ = ((struct elf_link_first_hash_entry *)
+ bfd_hash_lookup (htab->first_hash, name, false,
+ false));
+ if (e != NULL && e->abfd == abfd)
definition = true;
}
}
@@ -6222,11 +6259,11 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
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)
+ struct elf_link_first_hash_entry *e
+ = ((struct elf_link_first_hash_entry *)
+ bfd_hash_lookup (htab->first_hash, symdef->name,
+ false, false));
+ if (e == NULL || e->abfd != abfd)
continue;
}
@@ -8308,12 +8345,12 @@ _bfd_elf_link_hash_table_free (bfd *obfd)
if (htab->dynstr != NULL)
_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);
+ bfd_hash_table_free (htab->first_hash);
free (htab->first_hash);
}
+ _bfd_generic_link_hash_table_free (obfd);
}
/* This is a hook for the ELF emulation code in the generic linker to
--
2.44.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] elf: Use elf_link_first_hash_entry for first_hash
2024-04-05 23:37 [PATCH] elf: Use elf_link_first_hash_entry for first_hash H.J. Lu
@ 2024-04-05 23:53 ` Alan Modra
2024-04-06 0:10 ` H.J. Lu
2024-04-06 1:51 ` Alan Modra
1 sibling, 1 reply; 6+ messages in thread
From: Alan Modra @ 2024-04-05 23:53 UTC (permalink / raw)
To: H.J. Lu; +Cc: binutils
On Fri, Apr 05, 2024 at 04:37:58PM -0700, H.J. Lu wrote:
> @@ -4341,11 +4377,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> && 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)))
> + htab->first_hash = (struct bfd_hash_table *)
> + xmalloc (sizeof (struct bfd_hash_table));
> + if (!bfd_hash_table_init
> + (htab->first_hash, elf_link_first_hash_newfunc,
> + sizeof (struct elf_link_first_hash_entry)))
> info->callbacks->einfo
> (_("%F%P: first_hash failed to initialize: %E\n"));
> }
I didn't notice this before, but you shouldn't really be using xmalloc
in libbfd. Yes, as a practical matter it isn't so relevant for
linker-only bfd functions, but it's probably worth fixing here.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] elf: Use elf_link_first_hash_entry for first_hash
2024-04-05 23:53 ` Alan Modra
@ 2024-04-06 0:10 ` H.J. Lu
0 siblings, 0 replies; 6+ messages in thread
From: H.J. Lu @ 2024-04-06 0:10 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
On Fri, Apr 5, 2024 at 4:53 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Apr 05, 2024 at 04:37:58PM -0700, H.J. Lu wrote:
> > @@ -4341,11 +4377,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> > && 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)))
> > + htab->first_hash = (struct bfd_hash_table *)
> > + xmalloc (sizeof (struct bfd_hash_table));
> > + if (!bfd_hash_table_init
> > + (htab->first_hash, elf_link_first_hash_newfunc,
> > + sizeof (struct elf_link_first_hash_entry)))
> > info->callbacks->einfo
> > (_("%F%P: first_hash failed to initialize: %E\n"));
> > }
>
> I didn't notice this before, but you shouldn't really be using xmalloc
> in libbfd. Yes, as a practical matter it isn't so relevant for
> linker-only bfd functions, but it's probably worth fixing here.
>
>
Done:
https://sourceware.org/pipermail/binutils/2024-April/133419.html
--
H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] elf: Use elf_link_first_hash_entry for first_hash
2024-04-05 23:37 [PATCH] elf: Use elf_link_first_hash_entry for first_hash H.J. Lu
2024-04-05 23:53 ` Alan Modra
@ 2024-04-06 1:51 ` Alan Modra
2024-04-06 2:14 ` Alan Modra
1 sibling, 1 reply; 6+ messages in thread
From: Alan Modra @ 2024-04-06 1:51 UTC (permalink / raw)
To: H.J. Lu; +Cc: binutils
Looks like we still have a problem with accesses to freed memory.
This following is from link stage of LTO 1 test under valgrind.
==1443263== Invalid read of size 1
==1443263== at 0x484CFE4: strcmp (vg_replace_strmem.c:939)
==1443263== by 0x56E16C: bfd_hash_lookup (hash.c:564)
==1443263== by 0x5A3C8F: elf_link_add_to_first_hash (elflink.c:4316)
==1443263== by 0x5AE60F: elf_link_add_object_symbols (elflink.c:5663)
==1443263== by 0x5B0672: bfd_elf_link_add_symbols (elflink.c:6333)
==1443263== by 0x41448F: load_symbols (ldlang.c:3129)
==1443263== by 0x4149D8: open_input_bfds (ldlang.c:3621)
==1443263== by 0x414968: open_input_bfds (ldlang.c:3569)
==1443263== by 0x4166A2: lang_process (ldlang.c:8162)
==1443263== by 0x4194D5: main (ldmain.c:504)
==1443263== Address 0x525e230 is 192 bytes inside a block of size 4,064 free'd
==1443263== at 0x484810F: free (vg_replace_malloc.c:974)
==1443263== by 0x8D4D87: objalloc_free_block (objalloc.c:248)
==1443263== by 0x5AEACC: elf_link_add_object_symbols (elflink.c:5790)
==1443263== by 0x5B0672: bfd_elf_link_add_symbols (elflink.c:6333)
==1443263== by 0x41448F: load_symbols (ldlang.c:3129)
==1443263== by 0x4149D8: open_input_bfds (ldlang.c:3621)
==1443263== by 0x414968: open_input_bfds (ldlang.c:3569)
==1443263== by 0x4166A2: lang_process (ldlang.c:8162)
==1443263== by 0x4194D5: main (ldmain.c:504)
==1443263== Block was alloc'd at
==1443263== at 0x4845828: malloc (vg_replace_malloc.c:431)
==1443263== by 0x8D4C10: _objalloc_alloc (objalloc.c:159)
==1443263== by 0x56E2EF: bfd_hash_allocate (hash.c:756)
==1443263== by 0x5833B8: _bfd_x86_elf_link_hash_newfunc (elfxx-x86.c:627)
==1443263== by 0x56DF72: bfd_hash_insert (hash.c:611)
==1443263== by 0x56E19E: bfd_hash_lookup (hash.c:586)
==1443263== by 0x56FE22: bfd_link_hash_lookup (linker.c:515)
==1443263== by 0x5A2D1B: elf_link_hash_lookup (elf-bfd.h:779)
==1443263== by 0x5AA12B: _bfd_elf_merge_symbol (elflink.c:1129)
==1443263== by 0x5AB3DB: _bfd_elf_add_default_symbol (elflink.c:2133)
==1443263== by 0x5AF282: elf_link_add_object_symbols (elflink.c:5413)
==1443263== by 0x5B0672: bfd_elf_link_add_symbols (elflink.c:6333)
The objalloc_free_block is the one when restoring the linker hash
table to as it was prior to adding symbols for an as-needed library
that was found to not be needed. I'd rather not fix this by just
copying the name when adding symbols to first_hash, if at all
possible.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] elf: Use elf_link_first_hash_entry for first_hash
2024-04-06 1:51 ` Alan Modra
@ 2024-04-06 2:14 ` Alan Modra
2024-04-06 7:28 ` Alan Modra
0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2024-04-06 2:14 UTC (permalink / raw)
To: H.J. Lu; +Cc: binutils
On Sat, Apr 06, 2024 at 12:21:05PM +1030, Alan Modra wrote:
> Looks like we still have a problem with accesses to freed memory.
> This following is from link stage of LTO 1 test under valgrind.
>
> ==1443263== Invalid read of size 1
> ==1443263== at 0x484CFE4: strcmp (vg_replace_strmem.c:939)
> ==1443263== by 0x56E16C: bfd_hash_lookup (hash.c:564)
> ==1443263== by 0x5A3C8F: elf_link_add_to_first_hash (elflink.c:4316)
> ==1443263== by 0x5AE60F: elf_link_add_object_symbols (elflink.c:5663)
> ==1443263== by 0x5B0672: bfd_elf_link_add_symbols (elflink.c:6333)
> ==1443263== by 0x41448F: load_symbols (ldlang.c:3129)
> ==1443263== by 0x4149D8: open_input_bfds (ldlang.c:3621)
> ==1443263== by 0x414968: open_input_bfds (ldlang.c:3569)
> ==1443263== by 0x4166A2: lang_process (ldlang.c:8162)
> ==1443263== by 0x4194D5: main (ldmain.c:504)
> ==1443263== Address 0x525e230 is 192 bytes inside a block of size 4,064 free'd
> ==1443263== at 0x484810F: free (vg_replace_malloc.c:974)
> ==1443263== by 0x8D4D87: objalloc_free_block (objalloc.c:248)
> ==1443263== by 0x5AEACC: elf_link_add_object_symbols (elflink.c:5790)
> ==1443263== by 0x5B0672: bfd_elf_link_add_symbols (elflink.c:6333)
> ==1443263== by 0x41448F: load_symbols (ldlang.c:3129)
> ==1443263== by 0x4149D8: open_input_bfds (ldlang.c:3621)
> ==1443263== by 0x414968: open_input_bfds (ldlang.c:3569)
> ==1443263== by 0x4166A2: lang_process (ldlang.c:8162)
> ==1443263== by 0x4194D5: main (ldmain.c:504)
> ==1443263== Block was alloc'd at
> ==1443263== at 0x4845828: malloc (vg_replace_malloc.c:431)
> ==1443263== by 0x8D4C10: _objalloc_alloc (objalloc.c:159)
> ==1443263== by 0x56E2EF: bfd_hash_allocate (hash.c:756)
> ==1443263== by 0x5833B8: _bfd_x86_elf_link_hash_newfunc (elfxx-x86.c:627)
> ==1443263== by 0x56DF72: bfd_hash_insert (hash.c:611)
> ==1443263== by 0x56E19E: bfd_hash_lookup (hash.c:586)
> ==1443263== by 0x56FE22: bfd_link_hash_lookup (linker.c:515)
> ==1443263== by 0x5A2D1B: elf_link_hash_lookup (elf-bfd.h:779)
> ==1443263== by 0x5AA12B: _bfd_elf_merge_symbol (elflink.c:1129)
> ==1443263== by 0x5AB3DB: _bfd_elf_add_default_symbol (elflink.c:2133)
> ==1443263== by 0x5AF282: elf_link_add_object_symbols (elflink.c:5413)
> ==1443263== by 0x5B0672: bfd_elf_link_add_symbols (elflink.c:6333)
>
> The objalloc_free_block is the one when restoring the linker hash
> table to as it was prior to adding symbols for an as-needed library
> that was found to not be needed. I'd rather not fix this by just
> copying the name when adding symbols to first_hash, if at all
> possible.
As expected, this fixes the problem
diff --git a/bfd/elflink.c b/bfd/elflink.c
index e41b3d6dad7..496c4b2280f 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -4313,7 +4313,7 @@ elf_link_add_to_first_hash (bfd *abfd, struct bfd_link_info *info,
struct elf_link_first_hash_entry *e
= ((struct elf_link_first_hash_entry *)
- bfd_hash_lookup (htab->first_hash, name, true, false));
+ bfd_hash_lookup (htab->first_hash, name, true, true));
if (e == NULL)
info->callbacks->einfo
(_("%F%P: %pB: failed to add %s to first hash\n"), abfd, name);
This also, which might be a better fix.
diff --git a/bfd/elflink.c b/bfd/elflink.c
index e41b3d6dad7..a30c3af18a8 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1957,7 +1957,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
dynamic = (abfd->flags & DYNAMIC) != 0;
shortlen = p - name;
- shortname = (char *) bfd_hash_allocate (&info->hash->table, shortlen + 1);
+ shortname = bfd_alloc (abfd, shortlen + 1);
if (shortname == NULL)
return false;
memcpy (shortname, name, shortlen);
@@ -2120,7 +2120,7 @@ _bfd_elf_add_default_symbol (bfd *abfd,
nondefault:
len = strlen (name);
- shortname = (char *) bfd_hash_allocate (&info->hash->table, len);
+ shortname = bfd_alloc (abfd, len);
if (shortname == NULL)
return false;
memcpy (shortname, name, shortlen);
@@ -5202,7 +5202,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
&& isym->st_shndx != SHN_UNDEF)
++newlen;
- newname = (char *) bfd_hash_allocate (&htab->root.table, newlen);
+ newname = bfd_alloc (abfd, newlen);
if (newname == NULL)
goto error_free_vers;
memcpy (newname, name, namelen);
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] elf: Use elf_link_first_hash_entry for first_hash
2024-04-06 2:14 ` Alan Modra
@ 2024-04-06 7:28 ` Alan Modra
0 siblings, 0 replies; 6+ messages in thread
From: Alan Modra @ 2024-04-06 7:28 UTC (permalink / raw)
To: H.J. Lu; +Cc: binutils
I settled on this one.
PR ld/31482
PR ld/31489
* elflink.c (elf_link_add_to_first_hash): Add "copy" param.
(elf_link_add_object_symbols): Flag that name must be copied
when appending version string to symbol name.
diff --git a/bfd/elflink.c b/bfd/elflink.c
index e41b3d6dad7..dadac2522d5 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -4304,7 +4304,7 @@ elf_link_first_hash_newfunc (struct bfd_hash_entry *entry,
static void
elf_link_add_to_first_hash (bfd *abfd, struct bfd_link_info *info,
- const char *name)
+ const char *name, bool copy)
{
struct elf_link_hash_table *htab = elf_hash_table (info);
/* Skip if there is no first hash. */
@@ -4313,7 +4313,7 @@ elf_link_add_to_first_hash (bfd *abfd, struct bfd_link_info *info,
struct elf_link_first_hash_entry *e
= ((struct elf_link_first_hash_entry *)
- bfd_hash_lookup (htab->first_hash, name, true, false));
+ bfd_hash_lookup (htab->first_hash, name, true, copy));
if (e == NULL)
info->callbacks->einfo
(_("%F%P: %pB: failed to add %s to first hash\n"), abfd, name);
@@ -4920,6 +4920,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
asection *sec, *new_sec;
flagword flags;
const char *name;
+ bool must_copy_name = false;
struct elf_link_hash_entry *h;
struct elf_link_hash_entry *hi;
bool definition;
@@ -5217,6 +5218,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
memcpy (p, verstr, verlen + 1);
name = newname;
+ /* Since bfd_hash_alloc is used for "name", the string
+ must be copied if added to first_hash. The string
+ memory can be freed when an --as-needed library is
+ not needed. */
+ must_copy_name = true;
}
/* If this symbol has default visibility and the user has
@@ -5660,7 +5666,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
&& h->root.u.def.section->owner == abfd)
/* Add this symbol to first hash if this shared
object has the first definition. */
- elf_link_add_to_first_hash (abfd, info, name);
+ elf_link_add_to_first_hash (abfd, info, name, must_copy_name);
}
}
}
@@ -6108,7 +6114,7 @@ _bfd_elf_archive_symbol_lookup (bfd *abfd,
{
/* Add this symbol to first hash if this archive has the first
definition. */
- elf_link_add_to_first_hash (abfd, info, name);
+ elf_link_add_to_first_hash (abfd, info, name, false);
return h;
}
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-06 7:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-05 23:37 [PATCH] elf: Use elf_link_first_hash_entry for first_hash H.J. Lu
2024-04-05 23:53 ` Alan Modra
2024-04-06 0:10 ` H.J. Lu
2024-04-06 1:51 ` Alan Modra
2024-04-06 2:14 ` Alan Modra
2024-04-06 7:28 ` Alan Modra
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).