Re: [PATCH] elf: Don't load archive element after dynamic definition

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

On Thu, Aug 27, 2020 at 6:59 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Aug 27, 2020 at 07:05:15AM -0700, H.J. Lu wrote:
> > On Thu, Aug 27, 2020 at 6:53 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote:
> > > > Don't load the archive element after seeing a definition in a shared
> > > > object.
> > > >
> > > > bfd/
> > > >
> > > >       PR ld/26530
> > > >       * elflink.c (elf_link_add_object_symbols): Also preserve the
> > > >       dynamic_def bit for --as-needed.
> > > >       (elf_link_add_archive_symbols): Don't load the archive element
> > > >       after seeing a definition in a shared object.
> > >
> > > This seems over complicated to me.  Why don't we just load the
> > > as-needed shared library on the first pass?
> > >
> >
> > In the first pass, when we are loading a shared object,  do we have
> > the IR symbol resolution?
>
> No, but why should that matter?  Adding DT_NEEDED for an as-needed
> library that turns out to not be needed after LTO is not a problem
> IMO.  Also I think it may be necessary to load shared libraries on the
> first pass for LTO to work properly in corner cases.  See the comment
> in git commit a896df97b952.
>
> The condition we use to add --as-needed libraries is:
>           if (!add_needed
>               && matched
>               && definition
>               && ((dynsym
>                    && h->ref_regular_nonweak)
>                   || (h->ref_dynamic_nonweak
>                       && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0
>                       && !on_needed_list (elf_dt_name (abfd),
>                                           htab->needed, NULL))))
> Note the test of dynsym, but earlier
>           /* Nor should we make plugin symbols dynamic.  */
>           if ((abfd->flags & BFD_PLUGIN) != 0)
>             dynsym = FALSE;
>
> So I'm thinking perhaps we should delete the above lines and instead
> add the BFD_PLUGIN condition where we make symbols dynamic.
>
>           if (dynsym && (abfd->flags & BFD_PLUGIN) == 0 && h->dynindx == -1)
>             {
>               if (! bfd_elf_link_record_dynamic_symbol (info, h))
>
> I haven't tested the idea..
>

I tested the patch enclosed here and it doesn't work.

In the first pass, when we are loading a shared object, we can't assume
that the shared object is needed simply because it satisfies a reference
from an IR object without the IR symbol resolution.

-- 
H.J.
----
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5c085b14b7..3c559bafc5 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -5162,10 +5162,6 @@ elf_link_add_object_symbols (bfd *abfd, struct
bfd_link_info *info)
         && !bfd_link_relocatable (info))
       dynsym = FALSE;

-    /* Nor should we make plugin symbols dynamic.  */
-    if ((abfd->flags & BFD_PLUGIN) != 0)
-      dynsym = FALSE;
-
     if (definition)
       {
         h->target_internal = isym->st_target_internal;
@@ -5192,7 +5188,9 @@ elf_link_add_object_symbols (bfd *abfd, struct
bfd_link_info *info)
      }
       }

-    if (dynsym && h->dynindx == -1)
+    if (dynsym
+        && (abfd->flags & BFD_PLUGIN) == 0
+        && h->dynindx == -1)
       {
         if (! bfd_elf_link_record_dynamic_symbol (info, h))
      goto error_free_vers;