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

[Alan Modra <amodra@gmail.com>]

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