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

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

[H.J. Lu]

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.

ld/

	PR ld/26530
	* testsuite/ld-plugin/lto.exp: Run PR ld/26530 tests.
	* testsuite/ld-plugin/pr26530a.c: New file.
	* testsuite/ld-plugin/pr26530b.c: Likewise.
	* testsuite/ld-plugin/pr26530c.c: Likewise.
---
 bfd/elflink.c                     | 20 ++++++++++++++------
 ld/testsuite/ld-plugin/lto.exp    | 20 ++++++++++++++++++++
 ld/testsuite/ld-plugin/pr26530a.c |  5 +++++
 ld/testsuite/ld-plugin/pr26530b.c |  7 +++++++
 ld/testsuite/ld-plugin/pr26530c.c | 12 ++++++++++++
 5 files changed, 58 insertions(+), 6 deletions(-)
 create mode 100644 ld/testsuite/ld-plugin/pr26530a.c
 create mode 100644 ld/testsuite/ld-plugin/pr26530b.c
 create mode 100644 ld/testsuite/ld-plugin/pr26530c.c

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 17a423270e..97e6ece97e 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -5340,7 +5340,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
 	  struct elf_link_hash_entry *h;
 	  bfd_size_type size;
 	  unsigned int alignment_power;
-	  unsigned int non_ir_ref_dynamic;
+	  struct elf_link_hash_entry preserved_h;
 
 	  for (p = htab->root.table.table[i]; p != NULL; p = p->next)
 	    {
@@ -5362,10 +5362,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
 		  size = 0;
 		  alignment_power = 0;
 		}
-	      /* Preserve non_ir_ref_dynamic so that this symbol
-		 will be exported when the dynamic lib becomes needed
-		 in the second pass.  */
-	      non_ir_ref_dynamic = h->root.non_ir_ref_dynamic;
+	      /* Preserve some bits so that this symbol will be exported
+		 when the dynamic lib becomes needed in the second pass.
+		 and to avoid linking in an archive after the dynamic
+		 lib.  */
+	      preserved_h = *h;
 	      memcpy (p, old_ent, htab->root.table.entsize);
 	      old_ent = (char *) old_ent + htab->root.table.entsize;
 	      h = (struct elf_link_hash_entry *) p;
@@ -5382,7 +5383,9 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
 		  if (alignment_power > h->root.u.c.p->alignment_power)
 		    h->root.u.c.p->alignment_power = alignment_power;
 		}
-	      h->root.non_ir_ref_dynamic = non_ir_ref_dynamic;
+	      h->root.non_ir_ref_dynamic
+		= preserved_h.root.non_ir_ref_dynamic;
+	      h->dynamic_def = preserved_h.dynamic_def;
 	    }
 	}
 
@@ -5819,6 +5822,11 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info)
 	  if (h == NULL)
 	    continue;
 
+	  /* Don't load the archive element after seeing a definition
+	     in a shared object.  */
+	  if (h->dynamic_def)
+	    continue;
+
 	  if (h->root.type == bfd_link_hash_undefined)
 	    {
 	      /* If the archive element has already been loaded then one
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index 6a37025db5..8331500b33 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -419,6 +419,18 @@ set lto_link_elf_tests [list \
    "-shared -Wl,--exclude-libs,ALL tmpdir/pr25618a.o tmpdir/pr25618.a" \
    "-fpic" \
    {dummy.c} {{readelf {--dyn-syms --wide} pr25618.d}} "pr25618.so" "c++"] \
+  [list "Build pr26530a.so" \
+   "-shared" "-O2 -fpic $no_lto" \
+   {pr26530a.c} {} "pr26530a.so" "c"] \
+  [list "Build pr26530b.so" \
+   "-shared" "-O2 -fpic $no_lto" \
+   {pr26530b.c} {} "pr26530b.so" "c"] \
+  [list "Build pr26530a.a" \
+   "" "-O2 $no_lto" \
+   {pr26530a.c} {} "pr26530a.a" "c"] \
+  [list "Build pr26530b.a" \
+   "" "-O2 $no_lto" \
+   {pr26530b.c} {} "pr26530b.a" "c"] \
 ]
 
 # PR 14918 checks that libgcc is not spuriously included in a shared link of
@@ -582,6 +594,14 @@ set lto_run_elf_shared_tests [list \
   [list {pr22220b} \
    {-flto -fuse-linker-plugin -Wl,--no-as-needed tmpdir/pr22220lib.so tmpdir/pr22220main.o} {} \
    {dummy.c} {pr22220b.exe} {pass.out} {} {c++}] \
+  [list "pr26530a" \
+   "-O2 -flto -fuse-linker-plugin" "" \
+   {pr26530c.c} "pr26530a.exe" "pass.out" "-O2 -flto" "c" {} \
+   "-Wl,--as-needed tmpdir/pr26530a.so tmpdir/pr26530b.a"] \
+  [list "pr26530b" \
+   "-O2 -flto -fuse-linker-plugin" "" \
+   {pr26530c.c} "pr26530b.exe" "pass.out" "-O2 -flto" "c" {} \
+   "-Wl,--as-needed tmpdir/pr26530a.a tmpdir/pr26530b.so"] \
 ]
 
 # LTO run-time tests for ELF
diff --git a/ld/testsuite/ld-plugin/pr26530a.c b/ld/testsuite/ld-plugin/pr26530a.c
new file mode 100644
index 0000000000..08e746911e
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr26530a.c
@@ -0,0 +1,5 @@
+const char *
+lrealpath (const char *a)
+{
+  return a;
+}
diff --git a/ld/testsuite/ld-plugin/pr26530b.c b/ld/testsuite/ld-plugin/pr26530b.c
new file mode 100644
index 0000000000..0eb003f470
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr26530b.c
@@ -0,0 +1,7 @@
+#include <stdlib.h>
+
+void
+lrealpath (void)
+{
+  abort ();
+}
diff --git a/ld/testsuite/ld-plugin/pr26530c.c b/ld/testsuite/ld-plugin/pr26530c.c
new file mode 100644
index 0000000000..c8ef23675e
--- /dev/null
+++ b/ld/testsuite/ld-plugin/pr26530c.c
@@ -0,0 +1,12 @@
+#include <stdio.h>
+
+const char *lrealpath(const char *a);
+
+int
+main(int argc, char **argv)
+{
+  const char *path = lrealpath (argv[0]);
+  if (path[0] != '\0')
+    printf ("PASS\n");
+  return 0;
+}
-- 
2.26.2

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

[Alan Modra]

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?

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

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?


-- 
H.J.

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

[Alan Modra]

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..

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

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;

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

[Alan Modra]

On Fri, Aug 28, 2020 at 05:53:10AM -0700, H.J. Lu wrote:
> 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.

Please explain "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;

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Fri, Aug 28, 2020 at 7:49 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Aug 28, 2020 at 05:53:10AM -0700, H.J. Lu wrote:
> > 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.
>
> Please explain "doesn't work".

PR ld/26530 test still failed.

> > 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;
>
> --
> Alan Modra
> Australia Development Lab, IBM



-- 
H.J.

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

[Martin Liška]

On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote:
> PR ld/26530 test still failed.

Hello.

Is there any progress on this please?

Thanks,
Martin

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

[Alan Modra]

On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote:
> On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote:
> > PR ld/26530 test still failed.
> 
> Hello.
> 
> Is there any progress on this please?

I think what we want is the following, and some tweaking of the
testsuite to remove FAIL: PR ld/15146 (1).  That test relied on not
loading a shared lib due to an IR symbol reference, but the logic
added by git commit 3d5bef4c0871 has already been reverted.

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5c085b14b7..346f534911 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -4977,11 +4977,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
 	     object and a shared object.  */
 	  bfd_boolean dynsym = FALSE;
 
-	  /* Plugin symbols aren't normal.  Don't set def_regular or
-	     ref_regular for them, or make them dynamic.  */
-	  if ((abfd->flags & BFD_PLUGIN) != 0)
-	    ;
-	  else if (! dynamic)
+	  if (! dynamic)
 	    {
 	      if (! definition)
 		{
@@ -5162,10 +5158,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 +5184,7 @@ 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;

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Wed, Sep 2, 2020 at 1:12 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote:
> > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote:
> > > PR ld/26530 test still failed.
> >
> > Hello.
> >
> > Is there any progress on this please?
>
> I think what we want is the following, and some tweaking of the
> testsuite to remove FAIL: PR ld/15146 (1).  That test relied on not
> loading a shared lib due to an IR symbol reference, but the logic
> added by git commit 3d5bef4c0871 has already been reverted.
>
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index 5c085b14b7..346f534911 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -4977,11 +4977,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
>              object and a shared object.  */
>           bfd_boolean dynsym = FALSE;
>
> -         /* Plugin symbols aren't normal.  Don't set def_regular or
> -            ref_regular for them, or make them dynamic.  */
> -         if ((abfd->flags & BFD_PLUGIN) != 0)
> -           ;
> -         else if (! dynamic)
> +         if (! dynamic)
>             {
>               if (! definition)
>                 {
> @@ -5162,10 +5158,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 +5184,7 @@ 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;

Will it add a shared library to DT_NEEDED even if the IR symbol reference is
removed by LTO?

-- 
H.J.

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

[Alan Modra]

On Wed, Sep 02, 2020 at 04:56:48AM -0700, H.J. Lu wrote:
> On Wed, Sep 2, 2020 at 1:12 AM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote:
> > > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote:
> > > > PR ld/26530 test still failed.
> > >
> > > Hello.
> > >
> > > Is there any progress on this please?
> >
> > I think what we want is the following, and some tweaking of the
> > testsuite to remove FAIL: PR ld/15146 (1).  That test relied on not
> > loading a shared lib due to an IR symbol reference, but the logic
> > added by git commit 3d5bef4c0871 has already been reverted.
> >
> > diff --git a/bfd/elflink.c b/bfd/elflink.c
> > index 5c085b14b7..346f534911 100644
> > --- a/bfd/elflink.c
> > +++ b/bfd/elflink.c
> > @@ -4977,11 +4977,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> >              object and a shared object.  */
> >           bfd_boolean dynsym = FALSE;
> >
> > -         /* Plugin symbols aren't normal.  Don't set def_regular or
> > -            ref_regular for them, or make them dynamic.  */
> > -         if ((abfd->flags & BFD_PLUGIN) != 0)
> > -           ;
> > -         else if (! dynamic)
> > +         if (! dynamic)
> >             {
> >               if (! definition)
> >                 {
> > @@ -5162,10 +5158,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 +5184,7 @@ 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;
> 
> Will it add a shared library to DT_NEEDED even if the IR symbol reference is
> removed by LTO?

Yes.  That can't be helped.  I know we worried about unnecessary
--as-needed shared libraries in the past when IR symbols disappear
after LTO, but I can't see a simple and reliable way for the linker to
be correct.

It's reasonably obvious that we need to load archive elements when
they define IR referenced symbols, because the archive element might
be an LTO object.  What's not so obvious is whether loading of shared
libraries should follow the same rule.  I think they should, in order
for LTO to get symbol resolution correct in corner cases.  Basically
LTO needs to know what shared libraries are loaded before
recompilation.  See commit a896df97b952 log comments.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Wed, Sep 2, 2020 at 6:05 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Sep 02, 2020 at 04:56:48AM -0700, H.J. Lu wrote:
> > On Wed, Sep 2, 2020 at 1:12 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote:
> > > > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote:
> > > > > PR ld/26530 test still failed.
> > > >
> > > > Hello.
> > > >
> > > > Is there any progress on this please?
> > >
> > > I think what we want is the following, and some tweaking of the
> > > testsuite to remove FAIL: PR ld/15146 (1).  That test relied on not
> > > loading a shared lib due to an IR symbol reference, but the logic
> > > added by git commit 3d5bef4c0871 has already been reverted.
> > >
> > > diff --git a/bfd/elflink.c b/bfd/elflink.c
> > > index 5c085b14b7..346f534911 100644
> > > --- a/bfd/elflink.c
> > > +++ b/bfd/elflink.c
> > > @@ -4977,11 +4977,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
> > >              object and a shared object.  */
> > >           bfd_boolean dynsym = FALSE;
> > >
> > > -         /* Plugin symbols aren't normal.  Don't set def_regular or
> > > -            ref_regular for them, or make them dynamic.  */
> > > -         if ((abfd->flags & BFD_PLUGIN) != 0)
> > > -           ;
> > > -         else if (! dynamic)
> > > +         if (! dynamic)
> > >             {
> > >               if (! definition)
> > >                 {
> > > @@ -5162,10 +5158,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 +5184,7 @@ 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;
> >
> > Will it add a shared library to DT_NEEDED even if the IR symbol reference is
> > removed by LTO?
>
> Yes.  That can't be helped.  I know we worried about unnecessary
> --as-needed shared libraries in the past when IR symbols disappear
> after LTO, but I can't see a simple and reliable way for the linker to
> be correct.

My patch isn't very complicated.

> It's reasonably obvious that we need to load archive elements when
> they define IR referenced symbols, because the archive element might
> be an LTO object.  What's not so obvious is whether loading of shared
> libraries should follow the same rule.  I think they should, in order
> for LTO to get symbol resolution correct in corner cases.  Basically
> LTO needs to know what shared libraries are loaded before
> recompilation.  See commit a896df97b952 log comments.

There is dynamic_def for this purpose.

-- 
H.J.

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

[Alan Modra]

On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > It's reasonably obvious that we need to load archive elements when
> > they define IR referenced symbols, because the archive element might
> > be an LTO object.  What's not so obvious is whether loading of shared
> > libraries should follow the same rule.  I think they should, in order
> > for LTO to get symbol resolution correct in corner cases.  Basically
> > LTO needs to know what shared libraries are loaded before
> > recompilation.  See commit a896df97b952 log comments.
> 
> There is dynamic_def for this purpose.

Your patch doesn't make changes to ld/plugin.c to inform LTO of the
availability of these symbols.  And if you did, then how does the
linker work out whether or not the LTO recompilation depended on those
symbols?  If it did change LTO recompilation then you had better
ensure the library really is loaded.  By the time you work all of that
out, if it is even possible, your patch will likely be very
complicated indeed.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > It's reasonably obvious that we need to load archive elements when
> > > they define IR referenced symbols, because the archive element might
> > > be an LTO object.  What's not so obvious is whether loading of shared
> > > libraries should follow the same rule.  I think they should, in order
> > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > LTO needs to know what shared libraries are loaded before
> > > recompilation.  See commit a896df97b952 log comments.
> >
> > There is dynamic_def for this purpose.
>
> Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> availability of these symbols.  And if you did, then how does the
> linker work out whether or not the LTO recompilation depended on those
> symbols?  If it did change LTO recompilation then you had better
> ensure the library really is loaded.  By the time you work all of that
> out, if it is even possible, your patch will likely be very
> complicated indeed.

A testcase?


-- 
H.J.

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

[Alan Modra]

On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > It's reasonably obvious that we need to load archive elements when
> > > > they define IR referenced symbols, because the archive element might
> > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > libraries should follow the same rule.  I think they should, in order
> > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > LTO needs to know what shared libraries are loaded before
> > > > recompilation.  See commit a896df97b952 log comments.
> > >
> > > There is dynamic_def for this purpose.
> >
> > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > availability of these symbols.  And if you did, then how does the
> > linker work out whether or not the LTO recompilation depended on those
> > symbols?  If it did change LTO recompilation then you had better
> > ensure the library really is loaded.  By the time you work all of that
> > out, if it is even possible, your patch will likely be very
> > complicated indeed.
> 
> A testcase?

What don't you understand from my emails in this thread?  I suggest
you look at what happened with the testcase in PR26314, in regard to
my comment
    The lto recompilation didn't see symbol references from libbfd.so and
    variables like _xexit_cleanup are made local in the recompiled
    objects.  Oops, two copies of them.

That's critical in understanding why LTO needs to know about symbols
from shared libraries before LTO recompilation.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > It's reasonably obvious that we need to load archive elements when
> > > > > they define IR referenced symbols, because the archive element might
> > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > libraries should follow the same rule.  I think they should, in order
> > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > LTO needs to know what shared libraries are loaded before
> > > > > recompilation.  See commit a896df97b952 log comments.
> > > >
> > > > There is dynamic_def for this purpose.
> > >
> > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > availability of these symbols.  And if you did, then how does the
> > > linker work out whether or not the LTO recompilation depended on those
> > > symbols?  If it did change LTO recompilation then you had better
> > > ensure the library really is loaded.  By the time you work all of that
> > > out, if it is even possible, your patch will likely be very
> > > complicated indeed.
> >
> > A testcase?
>
> What don't you understand from my emails in this thread?  I suggest
> you look at what happened with the testcase in PR26314, in regard to
> my comment
>     The lto recompilation didn't see symbol references from libbfd.so and
>     variables like _xexit_cleanup are made local in the recompiled
>     objects.  Oops, two copies of them.

A testcase?

> That's critical in understanding why LTO needs to know about symbols
> from shared libraries before LTO recompilation.
>

Linker is complicated and LTO makes it even more complicated.
Without a testcase, we can't be sure that the bug is really fixed
nor we can be sure it won't regress.


-- 
H.J.

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

[Alan Modra]

On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote:
> On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > > It's reasonably obvious that we need to load archive elements when
> > > > > > they define IR referenced symbols, because the archive element might
> > > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > > libraries should follow the same rule.  I think they should, in order
> > > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > > LTO needs to know what shared libraries are loaded before
> > > > > > recompilation.  See commit a896df97b952 log comments.
> > > > >
> > > > > There is dynamic_def for this purpose.
> > > >
> > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > > availability of these symbols.  And if you did, then how does the
> > > > linker work out whether or not the LTO recompilation depended on those
> > > > symbols?  If it did change LTO recompilation then you had better
> > > > ensure the library really is loaded.  By the time you work all of that
> > > > out, if it is even possible, your patch will likely be very
> > > > complicated indeed.
> > >
> > > A testcase?
> >
> > What don't you understand from my emails in this thread?  I suggest
> > you look at what happened with the testcase in PR26314, in regard to
> > my comment
> >     The lto recompilation didn't see symbol references from libbfd.so and
> >     variables like _xexit_cleanup are made local in the recompiled
> >     objects.  Oops, two copies of them.
> 
> A testcase?

You kindly provided it yourself.
https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4

It takes only a small amount of digging to see the _xexit_cleanup
problem.

> 
> > That's critical in understanding why LTO needs to know about symbols
> > from shared libraries before LTO recompilation.
> >
> 
> Linker is complicated and LTO makes it even more complicated.
> Without a testcase, we can't be sure that the bug is really fixed
> nor we can be sure it won't regress.
> 
> 
> -- 
> H.J.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Wed, Sep 2, 2020 at 11:07 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote:
> > On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > > > >
> > > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > > > It's reasonably obvious that we need to load archive elements when
> > > > > > > they define IR referenced symbols, because the archive element might
> > > > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > > > libraries should follow the same rule.  I think they should, in order
> > > > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > > > LTO needs to know what shared libraries are loaded before
> > > > > > > recompilation.  See commit a896df97b952 log comments.
> > > > > >
> > > > > > There is dynamic_def for this purpose.
> > > > >
> > > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > > > availability of these symbols.  And if you did, then how does the
> > > > > linker work out whether or not the LTO recompilation depended on those
> > > > > symbols?  If it did change LTO recompilation then you had better
> > > > > ensure the library really is loaded.  By the time you work all of that
> > > > > out, if it is even possible, your patch will likely be very
> > > > > complicated indeed.
> > > >
> > > > A testcase?
> > >
> > > What don't you understand from my emails in this thread?  I suggest
> > > you look at what happened with the testcase in PR26314, in regard to
> > > my comment
> > >     The lto recompilation didn't see symbol references from libbfd.so and
> > >     variables like _xexit_cleanup are made local in the recompiled
> > >     objects.  Oops, two copies of them.
> >
> > A testcase?
>
> You kindly provided it yourself.
> https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4
>
> It takes only a small amount of digging to see the _xexit_cleanup
> problem.

This particular problem came from:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96385

where GCC generated incorrect output and I do have a mitigation
patch.

-- 
H.J.

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

[Alan Modra]

On Thu, Sep 03, 2020 at 04:34:23AM -0700, H.J. Lu wrote:
> On Wed, Sep 2, 2020 at 11:07 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote:
> > > On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > > > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > > > > It's reasonably obvious that we need to load archive elements when
> > > > > > > > they define IR referenced symbols, because the archive element might
> > > > > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > > > > libraries should follow the same rule.  I think they should, in order
> > > > > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > > > > LTO needs to know what shared libraries are loaded before
> > > > > > > > recompilation.  See commit a896df97b952 log comments.
> > > > > > >
> > > > > > > There is dynamic_def for this purpose.
> > > > > >
> > > > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > > > > availability of these symbols.  And if you did, then how does the
> > > > > > linker work out whether or not the LTO recompilation depended on those
> > > > > > symbols?  If it did change LTO recompilation then you had better
> > > > > > ensure the library really is loaded.  By the time you work all of that
> > > > > > out, if it is even possible, your patch will likely be very
> > > > > > complicated indeed.
> > > > >
> > > > > A testcase?
> > > >
> > > > What don't you understand from my emails in this thread?  I suggest
> > > > you look at what happened with the testcase in PR26314, in regard to
> > > > my comment
> > > >     The lto recompilation didn't see symbol references from libbfd.so and
> > > >     variables like _xexit_cleanup are made local in the recompiled
> > > >     objects.  Oops, two copies of them.
> > >
> > > A testcase?
> >
> > You kindly provided it yourself.
> > https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4
> >
> > It takes only a small amount of digging to see the _xexit_cleanup
> > problem.
> 
> This particular problem came from:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96385
> 
> where GCC generated incorrect output and I do have a mitigation
> patch.

Yes, you gave that as a testcase for unwanted dynamic symbols.  I
suspected there was more than just that going on, so didn't approve
the entirety of your patch.  While my initial guess at a ranlib issue
was wrong, on spending time properly analysing the testcase I found
that we had a problem with symbol info we hand off to LTO.
Specifically, we need to tell LTO about symbols in all shared
libraries loaded.  That means we can't load extra shared libraries
after LTO recompilation, at least, not those that affect the set of
symbols that LTO cares about, the IR symbols.

Committed.

bfd/
	PR 15146
	PR 26314
	PR 26530
	* elflink.c (elf_link_add_object_symbols): Do set def_regular
	and ref_regular for IR symbols.  Don't clear dynsym, allowing
	IR symbols to load --as-needed shared libraries, but prevent
	IR symbols from becoming dynamic.
ld/
	* testsuite/ld-plugin/lto.exp: Don't run pr15146 tests.
	* testsuite/ld-plugin/pr15146.d: Delete.
	* testsuite/ld-plugin/pr15146a.c: Delete.
	* testsuite/ld-plugin/pr15146b.c: Delete.
	* testsuite/ld-plugin/pr15146c.c: Delete.
	* testsuite/ld-plugin/pr15146d.c: Delete.

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 5c085b14b7..1384c1a46b 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -4977,11 +4977,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
 	     object and a shared object.  */
 	  bfd_boolean dynsym = FALSE;
 
-	  /* Plugin symbols aren't normal.  Don't set def_regular or
-	     ref_regular for them, or make them dynamic.  */
-	  if ((abfd->flags & BFD_PLUGIN) != 0)
-	    ;
-	  else if (! dynamic)
+	  if (! dynamic)
 	    {
 	      if (! definition)
 		{
@@ -5162,10 +5158,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 +5184,7 @@ 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;
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index adad1e4895..684d1db314 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -320,21 +320,6 @@ set lto_link_elf_tests [list \
   [list "PR ld/13244" \
    "-shared -O2 -fPIC -flto -fuse-linker-plugin -nostdlib" "-O2 -fno-early-inlining -flto" \
    {pr13244.c} {{"readelf" {-s --wide} "pr13244.d"}} "pr13244.so" "c"] \
-  [list "Build libpr15146a.a" \
-   "$plug_opt" "-flto -O2" \
-   {pr15146a.c} {} "lib15146a.a"] \
-  [list "Build pr15146b.so" \
-   "-shared" "-O2 -fpic" \
-   {pr15146b.c} {} "pr15146b.so" "c"] \
-  [list "Build pr15146c.so" \
-   "-shared -Wl,--no-as-needed tmpdir/pr15146b.so" "-O2 -fpic $no_lto" \
-   {pr15146c.c} {} "pr15146c.so" "c"] \
-  [list "PR ld/15146 (1)" \
-   "-O2 -flto -fuse-linker-plugin -Wl,-rpath-link,. -Wl,--no-copy-dt-needed-entries -Wl,--no-as-needed tmpdir/pr15146a.o tmpdir/pr15146c.so" "" \
-   {dummy.c} {{"readelf" {-d} "pr15146.d"}} "pr15146a.exe"] \
-  [list "Build libpr15146d.a" \
-   "$plug_opt" "-flto -O2" \
-   {pr15146d.c} {} "lib15146d.a"] \
   [list "Build libpr16746a.a" \
    "" "" \
    {pr16746a.c pr16746b.c} {} "lib15146a.a"] \
@@ -605,13 +590,6 @@ run_cc_link_tests $lto_compile_elf_tests
 # Restrict these to ELF targets that support shared libs and PIC.
 if { [is_elf_format] && [check_lto_shared_available] } {
     run_cc_link_tests $lto_link_elf_tests
-    set testname "PR ld/15146 (2)"
-    set exec_output [run_host_cmd "$CC" "-O2 -flto -fuse-linker-plugin -Wl,-rpath-link,. -Wl,--no-copy-dt-needed-entries -Wl,--no-as-needed tmpdir/pr15146d.o tmpdir/pr15146c.so"]
-    if { [ regexp "undefined reference to symbol '\\.?xxx'" $exec_output ] } {
-	pass $testname
-    } {
-	fail $testname
-    }
     set testname "PR ld/16746 (3)"
     set exec_output [run_host_cmd "$CC" "-O2 -flto -fuse-linker-plugin tmpdir/pr16746b.o tmpdir/pr16746d.o"]
     if { [ regexp "warning: \\.?foobar" $exec_output ] && ![ regexp "symbol from plugin" $exec_output ] } {
diff --git a/ld/testsuite/ld-plugin/pr15146.d b/ld/testsuite/ld-plugin/pr15146.d
deleted file mode 100644
index 48d4b85446..0000000000
--- a/ld/testsuite/ld-plugin/pr15146.d
+++ /dev/null
@@ -1,4 +0,0 @@
-#failif
-#...
- +0x[0-9a-f]+ +\(NEEDED\) +Shared library: +\[.*pr15146b.so\]
-#...
diff --git a/ld/testsuite/ld-plugin/pr15146a.c b/ld/testsuite/ld-plugin/pr15146a.c
deleted file mode 100644
index a22860af5b..0000000000
--- a/ld/testsuite/ld-plugin/pr15146a.c
+++ /dev/null
@@ -1,13 +0,0 @@
-extern int xxx;
-
-int
-bar (void)
-{
-  return xxx;
-}
-
-int
-main ()
-{ 
-  return 0;
-}
diff --git a/ld/testsuite/ld-plugin/pr15146b.c b/ld/testsuite/ld-plugin/pr15146b.c
deleted file mode 100644
index 90eb21ea55..0000000000
--- a/ld/testsuite/ld-plugin/pr15146b.c
+++ /dev/null
@@ -1 +0,0 @@
-int xxx = 3;
diff --git a/ld/testsuite/ld-plugin/pr15146c.c b/ld/testsuite/ld-plugin/pr15146c.c
deleted file mode 100644
index e69de29bb2..0000000000
diff --git a/ld/testsuite/ld-plugin/pr15146d.c b/ld/testsuite/ld-plugin/pr15146d.c
deleted file mode 100644
index ba1e0abfa6..0000000000
--- a/ld/testsuite/ld-plugin/pr15146d.c
+++ /dev/null
@@ -1,7 +0,0 @@
-extern int xxx;
-
-int
-main ()
-{ 
-  return xxx;
-}

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Fri, Sep 4, 2020 at 12:25 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Thu, Sep 03, 2020 at 04:34:23AM -0700, H.J. Lu wrote:
> > On Wed, Sep 2, 2020 at 11:07 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote:
> > > > On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
> > > > >
> > > > > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > > > > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > > > > > It's reasonably obvious that we need to load archive elements when
> > > > > > > > > they define IR referenced symbols, because the archive element might
> > > > > > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > > > > > libraries should follow the same rule.  I think they should, in order
> > > > > > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > > > > > LTO needs to know what shared libraries are loaded before
> > > > > > > > > recompilation.  See commit a896df97b952 log comments.
> > > > > > > >
> > > > > > > > There is dynamic_def for this purpose.
> > > > > > >
> > > > > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > > > > > availability of these symbols.  And if you did, then how does the
> > > > > > > linker work out whether or not the LTO recompilation depended on those
> > > > > > > symbols?  If it did change LTO recompilation then you had better
> > > > > > > ensure the library really is loaded.  By the time you work all of that
> > > > > > > out, if it is even possible, your patch will likely be very
> > > > > > > complicated indeed.
> > > > > >
> > > > > > A testcase?
> > > > >
> > > > > What don't you understand from my emails in this thread?  I suggest
> > > > > you look at what happened with the testcase in PR26314, in regard to
> > > > > my comment
> > > > >     The lto recompilation didn't see symbol references from libbfd.so and
> > > > >     variables like _xexit_cleanup are made local in the recompiled
> > > > >     objects.  Oops, two copies of them.
> > > >
> > > > A testcase?
> > >
> > > You kindly provided it yourself.
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4
> > >
> > > It takes only a small amount of digging to see the _xexit_cleanup
> > > problem.
> >
> > This particular problem came from:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96385
> >
> > where GCC generated incorrect output and I do have a mitigation
> > patch.
>
> Yes, you gave that as a testcase for unwanted dynamic symbols.  I
> suspected there was more than just that going on, so didn't approve
> the entirety of your patch.  While my initial guess at a ranlib issue
> was wrong, on spending time properly analysing the testcase I found
> that we had a problem with symbol info we hand off to LTO.
> Specifically, we need to tell LTO about symbols in all shared
> libraries loaded.  That means we can't load extra shared libraries
> after LTO recompilation, at least, not those that affect the set of
> symbols that LTO cares about, the IR symbols.
>
> Committed.
>
> bfd/
>         PR 15146
>         PR 26314
>         PR 26530
>         * elflink.c (elf_link_add_object_symbols): Do set def_regular
>         and ref_regular for IR symbols.  Don't clear dynsym, allowing
>         IR symbols to load --as-needed shared libraries, but prevent
>         IR symbols from becoming dynamic.
> ld/
>         * testsuite/ld-plugin/lto.exp: Don't run pr15146 tests.
>         * testsuite/ld-plugin/pr15146.d: Delete.
>         * testsuite/ld-plugin/pr15146a.c: Delete.
>         * testsuite/ld-plugin/pr15146b.c: Delete.
>         * testsuite/ld-plugin/pr15146c.c: Delete.
>         * testsuite/ld-plugin/pr15146d.c: Delete.

Are we loading any non-IR objects after LTO symbol resolution?
If yes, why is shared object disallowed?

You make changes for PR without any testcases added.  How do
you know PR is really fixed? How do you make sure that it stays fixed?


-- 
H.J.

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

[Alan Modra]

On Fri, Sep 04, 2020 at 04:06:36AM -0700, H.J. Lu wrote:
> On Fri, Sep 4, 2020 at 12:25 AM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Thu, Sep 03, 2020 at 04:34:23AM -0700, H.J. Lu wrote:
> > > On Wed, Sep 2, 2020 at 11:07 PM Alan Modra <amodra@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote:
> > > > > On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > > > > > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > > > > > > It's reasonably obvious that we need to load archive elements when
> > > > > > > > > > they define IR referenced symbols, because the archive element might
> > > > > > > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > > > > > > libraries should follow the same rule.  I think they should, in order
> > > > > > > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > > > > > > LTO needs to know what shared libraries are loaded before
> > > > > > > > > > recompilation.  See commit a896df97b952 log comments.
> > > > > > > > >
> > > > > > > > > There is dynamic_def for this purpose.
> > > > > > > >
> > > > > > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > > > > > > availability of these symbols.  And if you did, then how does the
> > > > > > > > linker work out whether or not the LTO recompilation depended on those
> > > > > > > > symbols?  If it did change LTO recompilation then you had better
> > > > > > > > ensure the library really is loaded.  By the time you work all of that
> > > > > > > > out, if it is even possible, your patch will likely be very
> > > > > > > > complicated indeed.
> > > > > > >
> > > > > > > A testcase?
> > > > > >
> > > > > > What don't you understand from my emails in this thread?  I suggest
> > > > > > you look at what happened with the testcase in PR26314, in regard to
> > > > > > my comment
> > > > > >     The lto recompilation didn't see symbol references from libbfd.so and
> > > > > >     variables like _xexit_cleanup are made local in the recompiled
> > > > > >     objects.  Oops, two copies of them.
> > > > >
> > > > > A testcase?
> > > >
> > > > You kindly provided it yourself.
> > > > https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4
> > > >
> > > > It takes only a small amount of digging to see the _xexit_cleanup
> > > > problem.
> > >
> > > This particular problem came from:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96385
> > >
> > > where GCC generated incorrect output and I do have a mitigation
> > > patch.
> >
> > Yes, you gave that as a testcase for unwanted dynamic symbols.  I
> > suspected there was more than just that going on, so didn't approve
> > the entirety of your patch.  While my initial guess at a ranlib issue
> > was wrong, on spending time properly analysing the testcase I found
> > that we had a problem with symbol info we hand off to LTO.
> > Specifically, we need to tell LTO about symbols in all shared
> > libraries loaded.  That means we can't load extra shared libraries
> > after LTO recompilation, at least, not those that affect the set of
> > symbols that LTO cares about, the IR symbols.
> >
> > Committed.
> >
> > bfd/
> >         PR 15146
> >         PR 26314
> >         PR 26530
> >         * elflink.c (elf_link_add_object_symbols): Do set def_regular
> >         and ref_regular for IR symbols.  Don't clear dynsym, allowing
> >         IR symbols to load --as-needed shared libraries, but prevent
> >         IR symbols from becoming dynamic.
> > ld/
> >         * testsuite/ld-plugin/lto.exp: Don't run pr15146 tests.
> >         * testsuite/ld-plugin/pr15146.d: Delete.
> >         * testsuite/ld-plugin/pr15146a.c: Delete.
> >         * testsuite/ld-plugin/pr15146b.c: Delete.
> >         * testsuite/ld-plugin/pr15146c.c: Delete.
> >         * testsuite/ld-plugin/pr15146d.c: Delete.
> 
> Are we loading any non-IR objects after LTO symbol resolution?
> If yes, why is shared object disallowed?
> 
> You make changes for PR without any testcases added.  How do
> you know PR is really fixed? How do you make sure that it stays fixed?

It's easy to make a testcase showing that loading an archive after an
--as-needed shared library is wrong, like the one you posted at
https://sourceware.org/pipermail/binutils/2020-August/112996.html
where functions in the archive differ from those in the shared
library.  That's OK to commit, BTW.  I would have liked something
closer to the situation in pr26314, where the archive functions are
identical to those in the shared library.  So far I've only managed
that by cheating using protected visibility.

I also spent quite a lot of time looking for a testcase that showed a
real problem with duplicated static variables like that of
_xexit_cleanup I saw, and failed.  Thing is, a duplicated static isn't
a problem if it is never used.  I suspect that is the case with
libbfd.so and libiberty.a compiled with -flto.  So I don't have a
small testcase to justify removing the special treatment of IR symbol
references with respect to as-needed libraries, but I still think that
is the right thing to do.

-- 
Alan Modra
Australia Development Lab, IBM

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

[H.J. Lu]

On Mon, Sep 7, 2020 at 10:42 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Fri, Sep 04, 2020 at 04:06:36AM -0700, H.J. Lu wrote:
> > On Fri, Sep 4, 2020 at 12:25 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Thu, Sep 03, 2020 at 04:34:23AM -0700, H.J. Lu wrote:
> > > > On Wed, Sep 2, 2020 at 11:07 PM Alan Modra <amodra@gmail.com> wrote:
> > > > >
> > > > > On Wed, Sep 02, 2020 at 07:16:14PM -0700, H.J. Lu wrote:
> > > > > > On Wed, Sep 2, 2020 at 6:31 PM Alan Modra <amodra@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, Sep 02, 2020 at 07:35:58AM -0700, H.J. Lu wrote:
> > > > > > > > On Wed, Sep 2, 2020 at 7:29 AM Alan Modra <amodra@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Sep 02, 2020 at 06:22:08AM -0700, H.J. Lu wrote:
> > > > > > > > > > > It's reasonably obvious that we need to load archive elements when
> > > > > > > > > > > they define IR referenced symbols, because the archive element might
> > > > > > > > > > > be an LTO object.  What's not so obvious is whether loading of shared
> > > > > > > > > > > libraries should follow the same rule.  I think they should, in order
> > > > > > > > > > > for LTO to get symbol resolution correct in corner cases.  Basically
> > > > > > > > > > > LTO needs to know what shared libraries are loaded before
> > > > > > > > > > > recompilation.  See commit a896df97b952 log comments.
> > > > > > > > > >
> > > > > > > > > > There is dynamic_def for this purpose.
> > > > > > > > >
> > > > > > > > > Your patch doesn't make changes to ld/plugin.c to inform LTO of the
> > > > > > > > > availability of these symbols.  And if you did, then how does the
> > > > > > > > > linker work out whether or not the LTO recompilation depended on those
> > > > > > > > > symbols?  If it did change LTO recompilation then you had better
> > > > > > > > > ensure the library really is loaded.  By the time you work all of that
> > > > > > > > > out, if it is even possible, your patch will likely be very
> > > > > > > > > complicated indeed.
> > > > > > > >
> > > > > > > > A testcase?
> > > > > > >
> > > > > > > What don't you understand from my emails in this thread?  I suggest
> > > > > > > you look at what happened with the testcase in PR26314, in regard to
> > > > > > > my comment
> > > > > > >     The lto recompilation didn't see symbol references from libbfd.so and
> > > > > > >     variables like _xexit_cleanup are made local in the recompiled
> > > > > > >     objects.  Oops, two copies of them.
> > > > > >
> > > > > > A testcase?
> > > > >
> > > > > You kindly provided it yourself.
> > > > > https://sourceware.org/bugzilla/show_bug.cgi?id=26314#c4
> > > > >
> > > > > It takes only a small amount of digging to see the _xexit_cleanup
> > > > > problem.
> > > >
> > > > This particular problem came from:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96385
> > > >
> > > > where GCC generated incorrect output and I do have a mitigation
> > > > patch.
> > >
> > > Yes, you gave that as a testcase for unwanted dynamic symbols.  I
> > > suspected there was more than just that going on, so didn't approve
> > > the entirety of your patch.  While my initial guess at a ranlib issue
> > > was wrong, on spending time properly analysing the testcase I found
> > > that we had a problem with symbol info we hand off to LTO.
> > > Specifically, we need to tell LTO about symbols in all shared
> > > libraries loaded.  That means we can't load extra shared libraries
> > > after LTO recompilation, at least, not those that affect the set of
> > > symbols that LTO cares about, the IR symbols.
> > >
> > > Committed.
> > >
> > > bfd/
> > >         PR 15146
> > >         PR 26314
> > >         PR 26530
> > >         * elflink.c (elf_link_add_object_symbols): Do set def_regular
> > >         and ref_regular for IR symbols.  Don't clear dynsym, allowing
> > >         IR symbols to load --as-needed shared libraries, but prevent
> > >         IR symbols from becoming dynamic.
> > > ld/
> > >         * testsuite/ld-plugin/lto.exp: Don't run pr15146 tests.
> > >         * testsuite/ld-plugin/pr15146.d: Delete.
> > >         * testsuite/ld-plugin/pr15146a.c: Delete.
> > >         * testsuite/ld-plugin/pr15146b.c: Delete.
> > >         * testsuite/ld-plugin/pr15146c.c: Delete.
> > >         * testsuite/ld-plugin/pr15146d.c: Delete.
> >
> > Are we loading any non-IR objects after LTO symbol resolution?
> > If yes, why is shared object disallowed?
> >
> > You make changes for PR without any testcases added.  How do
> > you know PR is really fixed? How do you make sure that it stays fixed?
>
> It's easy to make a testcase showing that loading an archive after an
> --as-needed shared library is wrong, like the one you posted at
> https://sourceware.org/pipermail/binutils/2020-August/112996.html
> where functions in the archive differ from those in the shared
> library.  That's OK to commit, BTW.  I would have liked something
> closer to the situation in pr26314, where the archive functions are
> identical to those in the shared library.  So far I've only managed
> that by cheating using protected visibility.
>
> I also spent quite a lot of time looking for a testcase that showed a
> real problem with duplicated static variables like that of
> _xexit_cleanup I saw, and failed.  Thing is, a duplicated static isn't
> a problem if it is never used.  I suspect that is the case with
> libbfd.so and libiberty.a compiled with -flto.  So I don't have a
> small testcase to justify removing the special treatment of IR symbol
> references with respect to as-needed libraries, but I still think that
> is the right thing to do.
>

I have an updated patch:

https://sourceware.org/pipermail/binutils/2020-September/113227.html

which changes the IR resolution from LDPR_UNDEF to LDPR_RESOLVED_DYN
if the symbol is defined in a DT_NEEDED shared object in the first pass.

--
H.J.