Re: [PATCH v3] elf: Always honor the first definition in shared object and archive

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

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.