[PATCH] ld: Add more tests for --as-needed

[PATCH] ld: Add more tests for --as-needed

[H.J. Lu]

commit fb7331ae2b551247a33e8da5387ebc910c4a7308
Author: Alan Modra <amodra@gmail.com>
Date:   Wed Sep 9 12:16:23 2020 +0930

    lto-18 test

added tests to show the reason to use IR symbols when deciding an
--as-needed library should be loaded.  But the requirement of the
--as-needed library doesn't come from the IR inputs since the IR symbol
reference of the --as-needed library is removed by LTO.  The requirement
comes from another --as-needed library which is referenced by the LTO
output as shown by PR ld/26590.  Not use IR symbols when deciding an
--as-needed library should be loaded exposes the same issue with input
--as-needed libraries as non-LTO inputs.  The current ld behavior is
desirable since it sures that both lazy and non-lazy bindings work the
same way.

Add more tests for --as-needed and add tmpdir/liblto-18b.so DT_NEEDED
to liblto-18c.so.

	PR ld/26590
	* testsuite/ld-elf/pr26590.err: New file.
	* testsuite/ld-elf/pr26590a.c: Likewise.
	* testsuite/ld-elf/pr26590b.c: Likewise.
	* testsuite/ld-elf/pr26590c.c: Likewise.
	* testsuite/ld-elf/pr26590d.c: Likewise.
	* testsuite/ld-elf/shared.exp: Run ld/26590 tests.
	* testsuite/ld-plugin/lto.exp: Add tmpdir/liblto-18b.so DT_NEEDED
	to liblto-18c.so.
---
 ld/testsuite/ld-elf/pr26590.err |  3 ++
 ld/testsuite/ld-elf/pr26590a.c  | 10 ++++++
 ld/testsuite/ld-elf/pr26590b.c  | 10 ++++++
 ld/testsuite/ld-elf/pr26590c.c  | 11 +++++++
 ld/testsuite/ld-elf/pr26590d.c  |  1 +
 ld/testsuite/ld-elf/shared.exp  | 54 +++++++++++++++++++++++++++++++++
 ld/testsuite/ld-plugin/lto.exp  |  2 +-
 7 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 ld/testsuite/ld-elf/pr26590.err
 create mode 100644 ld/testsuite/ld-elf/pr26590a.c
 create mode 100644 ld/testsuite/ld-elf/pr26590b.c
 create mode 100644 ld/testsuite/ld-elf/pr26590c.c
 create mode 100644 ld/testsuite/ld-elf/pr26590d.c

diff --git a/ld/testsuite/ld-elf/pr26590.err b/ld/testsuite/ld-elf/pr26590.err
new file mode 100644
index 0000000000..d8341486bd
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr26590.err
@@ -0,0 +1,3 @@
+#...
+.*: tmpdir/libpr26590b.so: undefined reference to `f1'
+#pass
diff --git a/ld/testsuite/ld-elf/pr26590a.c b/ld/testsuite/ld-elf/pr26590a.c
new file mode 100644
index 0000000000..0ae3a20c14
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr26590a.c
@@ -0,0 +1,10 @@
+int select (void) { return 1; }
+
+extern int f2 (int);
+
+int f1 (int x)
+{
+  if (x > 0)
+    return x * f2 (x - 1);
+  return 1;
+}
diff --git a/ld/testsuite/ld-elf/pr26590b.c b/ld/testsuite/ld-elf/pr26590b.c
new file mode 100644
index 0000000000..da546be79f
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr26590b.c
@@ -0,0 +1,10 @@
+int select (void) { return 2; }
+
+extern int f1 (int);
+
+int f2 (int x)
+{
+  if (x > 0)
+    return x * f1 (x - 1);
+  return 22222;
+}
diff --git a/ld/testsuite/ld-elf/pr26590c.c b/ld/testsuite/ld-elf/pr26590c.c
new file mode 100644
index 0000000000..930cf231ef
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr26590c.c
@@ -0,0 +1,11 @@
+#include <stdio.h>
+
+extern int select ();
+extern int f2 (int);
+
+int main (void)
+{
+  if (select () == 0 && f2 (0) == 22222)
+    printf ("PASS\n");
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr26590d.c b/ld/testsuite/ld-elf/pr26590d.c
new file mode 100644
index 0000000000..cfb27cf38a
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr26590d.c
@@ -0,0 +1 @@
+int select (void) { return 0; }
diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
index 6f53f9c317..bc68bb8435 100644
--- a/ld/testsuite/ld-elf/shared.exp
+++ b/ld/testsuite/ld-elf/shared.exp
@@ -809,6 +809,56 @@ run_cc_link_tests [list \
 	{} \
 	"libpr2404b.a" \
     ] \
+    [list \
+	"Build libpr26590a.so" \
+	"-shared" \
+	"-fPIC" \
+	{pr26590a.c} \
+	{} \
+	"libpr26590a.so" \
+    ] \
+    [list \
+	"Build libpr26590b.so (1)" \
+	"-shared" \
+	"-fPIC" \
+	{pr26590b.c} \
+	{} \
+	"libpr26590b.so" \
+    ] \
+    [list \
+	"Build pr26590c.o and pr26590d.o" \
+	"" \
+	"" \
+	{pr26590c.c pr26590d.c} \
+	{} \
+    ] \
+    [list \
+	"Build pr26590 (1)" \
+	"tmpdir/pr26590c.o tmpdir/pr26590d.o \
+	 -Wl,--as-needed tmpdir/libpr26590a.so tmpdir/libpr26590b.so" \
+	"" \
+	{dummy.c} \
+	{{error_output pr26590.err}} \
+	"pr26590" \
+    ] \
+    [list \
+	"Build libpr26590b.so (2)" \
+	"-shared -Wl,--no-as-needed \
+	 tmpdir/libpr26590a.so" \
+	"-fPIC" \
+	{pr26590b.c} \
+	{} \
+	"libpr26590b.so" \
+    ] \
+    [list \
+	"Build pr26590 (2)" \
+	"tmpdir/pr26590c.o tmpdir/pr26590d.o \
+	 -Wl,--as-needed tmpdir/libpr26590a.so tmpdir/libpr26590b.so" \
+	"" \
+	{dummy.c} \
+	{} \
+	"pr26590" \
+    ] \
 ]
 
 # pr19073.s uses .set, which has a different meaning on alpha.
@@ -1055,6 +1105,10 @@ set run_tests [list \
      "" "" \
      {pr26580-a.c} "pr26580-4" "pr26580-4.out" "-fcommon" "c" "" \
      "-Wl,--no-as-needed tmpdir/libpr26580-2.so" ] \
+    [list "Run pr26590 (2)" \
+     "" "" \
+     {pr26590c.c pr26590d.c} "pr26590" "pass.out" "" "c" "" \
+     "-Wl,--as-needed tmpdir/libpr26590a.so tmpdir/libpr26590b.so" ] \
 ]
 
 # NetBSD ELF systems do not currently support the .*_array sections.
diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp
index 0479e3e403..edfbb33fd8 100644
--- a/ld/testsuite/ld-plugin/lto.exp
+++ b/ld/testsuite/ld-plugin/lto.exp
@@ -408,7 +408,7 @@ set lto_link_elf_tests [list \
    {-shared} {-O2 -fpic} \
    {lto-18b.c} {} {liblto-18b.so}] \
   [list {liblto-18c.so} \
-   {-shared} {-O2 -fpic} \
+   {-shared -Wl,--no-as-needed tmpdir/liblto-18b.so} {-O2 -fpic} \
    {lto-18c.c} {} {liblto-18c.so}] \
   [list {lto-18d.o} \
    {} {-flto -O2} \
-- 
2.26.2

Re: [PATCH] ld: Add more tests for --as-needed

[Alan Modra]

On Wed, Sep 09, 2020 at 09:16:40AM -0700, H.J. Lu via Binutils wrote:
> commit fb7331ae2b551247a33e8da5387ebc910c4a7308
> Author: Alan Modra <amodra@gmail.com>
> Date:   Wed Sep 9 12:16:23 2020 +0930
> 
>     lto-18 test
> 
> added tests to show the reason to use IR symbols when deciding an
> --as-needed library should be loaded.

Exactly.  The testcase compiles and links fine without LTO, and
compiles and links with LTO if references from IR symbols are used to
decide whether a library is needed.  Reverting 1e3b96fd6cf0 results in

FAIL: lto-18 (1)
FAIL: lto-18 (2)

You kept asking for a testcase in the thread where I committed
1e3b96fd6cf0, and now you have one.  I think the testcase is valid,
but like many testcases, is nasty.

That testcase is not the only reason why I think we should be using IR
symbol refs to decide that an as-needed library is needed.

Here are my reasons:

1) --as-needed is supposed to load a shared library if there is a
non-weak undefined symbol remaining from previously loaded objects,
defined by the library.  By previously loaded objects we mean those
loaded on the command line or in scripts prior to the library, similar
to the logic used by the linker to extract objects from archives.
Archive searches must extract objects to satisfy IR references,
because the object in the archive might be an LTO object.  Since
--as-needed was intended to be somewhat modelled on the way ld
extracts objects from archives, it makes sense to do the same for
--as-needed libraries.

2) Trying to only emit DT_NEEDED for --as-needed libraries based on
the final LTO set of symbols is complicated, and has led to a number
of bug reports in the past.  Those bug reports have meant that we keep
some symbol state from an as-needed library that isn't marked as
needed at that point in linking, to feed to the LTO recompilation.
While doing that for references generally won't break anything if
the as-needed library is also found to be not needed after LTO, I
worry that doing the same for symbol definitions will lead to
breakage.  If a symbol being defined might affect LTO output, but the
LTO output not have a reference to the symbol in question, then that
could result in an as-needed library not being marked as needed on the
second pass through libraries after LTO.  The result would be that ld
tells LTO a symbol is defined but it ends up undefined.

3) Further patches complicating the linker like the one you posted at
https://sourceware.org/pipermail/binutils/2020-September/113227.html
are likely to be necessary, if we go back to IR symbols not marking
as-needed libraries as needed.

>  But the requirement of the
> --as-needed library doesn't come from the IR inputs since the IR symbol
> reference of the --as-needed library is removed by LTO.  The requirement
> comes from another --as-needed library which is referenced by the LTO
> output as shown by PR ld/26590.

PR26590 merely shows --as-needed working as designed.

>  Not use IR symbols when deciding an
> --as-needed library should be loaded exposes the same issue with input
> --as-needed libraries as non-LTO inputs.  The current ld behavior is
> desirable since it sures that both lazy and non-lazy bindings work the
> same way.
> 
> Add more tests for --as-needed and add tmpdir/liblto-18b.so DT_NEEDED
> to liblto-18c.so.

No, you may not commit the lto.exp patch.  Shared libraries are not
required to have DT_NEEDED entries.  Imagine for moment that
liblto-18b.so is a library from author B that has some undefined
functions that normally would be provided in the executable.  Author B
doesn't even know about liblto-18c.so, thus no DT_NEEDED for it.
Author C has a similar liblto-18c.so library with some undefined
functions normally provided in the executables.  Author A writing the
app wants to use both liblto-18b.so and liblto-18c.so and realizes
that they satisfy each other's undefined symbols.  So he doesn't need
to write those functions in the executable.  He gets everything
working without LTO, then turns on LTO and it fails mysteriously.

The other tests are fine to commit.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] ld: Add more tests for --as-needed

[H.J. Lu]

On Wed, Sep 9, 2020 at 8:30 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Wed, Sep 09, 2020 at 09:16:40AM -0700, H.J. Lu via Binutils wrote:
> > commit fb7331ae2b551247a33e8da5387ebc910c4a7308
> > Author: Alan Modra <amodra@gmail.com>
> > Date:   Wed Sep 9 12:16:23 2020 +0930
> >
> >     lto-18 test
> >
> > added tests to show the reason to use IR symbols when deciding an
> > --as-needed library should be loaded.
>
> Exactly.  The testcase compiles and links fine without LTO, and
> compiles and links with LTO if references from IR symbols are used to
> decide whether a library is needed.  Reverting 1e3b96fd6cf0 results in
>
> FAIL: lto-18 (1)
> FAIL: lto-18 (2)
>
> You kept asking for a testcase in the thread where I committed
> 1e3b96fd6cf0, and now you have one.  I think the testcase is valid,
> but like many testcases, is nasty.
>
> That testcase is not the only reason why I think we should be using IR
> symbol refs to decide that an as-needed library is needed.
>
> Here are my reasons:
>
> 1) --as-needed is supposed to load a shared library if there is a
> non-weak undefined symbol remaining from previously loaded objects,
> defined by the library.  By previously loaded objects we mean those
> loaded on the command line or in scripts prior to the library, similar
> to the logic used by the linker to extract objects from archives.
> Archive searches must extract objects to satisfy IR references,
> because the object in the archive might be an LTO object.  Since
> --as-needed was intended to be somewhat modelled on the way ld
> extracts objects from archives, it makes sense to do the same for
> --as-needed libraries.
>
> 2) Trying to only emit DT_NEEDED for --as-needed libraries based on
> the final LTO set of symbols is complicated, and has led to a number
> of bug reports in the past.  Those bug reports have meant that we keep
> some symbol state from an as-needed library that isn't marked as
> needed at that point in linking, to feed to the LTO recompilation.
> While doing that for references generally won't break anything if
> the as-needed library is also found to be not needed after LTO, I
> worry that doing the same for symbol definitions will lead to
> breakage.  If a symbol being defined might affect LTO output, but the
> LTO output not have a reference to the symbol in question, then that
> could result in an as-needed library not being marked as needed on the
> second pass through libraries after LTO.  The result would be that ld
> tells LTO a symbol is defined but it ends up undefined.
>
> 3) Further patches complicating the linker like the one you posted at
> https://sourceware.org/pipermail/binutils/2020-September/113227.html
> are likely to be necessary, if we go back to IR symbols not marking
> as-needed libraries as needed.
>
> >  But the requirement of the
> > --as-needed library doesn't come from the IR inputs since the IR symbol
> > reference of the --as-needed library is removed by LTO.  The requirement
> > comes from another --as-needed library which is referenced by the LTO
> > output as shown by PR ld/26590.
>
> PR26590 merely shows --as-needed working as designed.
>
> >  Not use IR symbols when deciding an
> > --as-needed library should be loaded exposes the same issue with input
> > --as-needed libraries as non-LTO inputs.  The current ld behavior is
> > desirable since it sures that both lazy and non-lazy bindings work the
> > same way.
> >
> > Add more tests for --as-needed and add tmpdir/liblto-18b.so DT_NEEDED
> > to liblto-18c.so.
>
> No, you may not commit the lto.exp patch.  Shared libraries are not
> required to have DT_NEEDED entries.  Imagine for moment that
> liblto-18b.so is a library from author B that has some undefined
> functions that normally would be provided in the executable.  Author B
> doesn't even know about liblto-18c.so, thus no DT_NEEDED for it.
> Author C has a similar liblto-18c.so library with some undefined
> functions normally provided in the executables.  Author A writing the
> app wants to use both liblto-18b.so and liblto-18c.so and realizes
> that they satisfy each other's undefined symbols.  So he doesn't need
> to write those functions in the executable.  He gets everything
> working without LTO, then turns on LTO and it fails mysteriously.
>

The problem is with liblto-18c.so and liblto-18b.so.  The same problem
can show up without LTO.  liblto-18c.so and liblto-18b.so should be fixed
if they should work in all cases, with and without LTO.

One advantage of LTO is to remove unused IR symbol references, including
unnecessary DT_NEEDED.  The current ld no longer does that.

-- 
H.J.

Re: [PATCH] ld: Add more tests for --as-needed

[Michael Matz]

Hello,

On Thu, 10 Sep 2020, H.J. Lu via Binutils wrote:

> The problem is with liblto-18c.so and liblto-18b.so.  The same problem 
> can show up without LTO.  liblto-18c.so and liblto-18b.so should be 
> fixed if they should work in all cases, with and without LTO.
> 
> One advantage of LTO is to remove unused IR symbol references, including 
> unnecessary DT_NEEDED.  The current ld no longer does that.

FWIW, I think in this case it's more important to make LTO and non-LTO 
behave the same than it is to make LTO emit fewer DT_NEEDED, even if that 
breaks the other invariant of --as-needed (namely that ldd -u doesn't show 
any libs).  For acceptance of the LTO compilation mode it was (and is) 
very important that any observable changes are kept to a minimum.


Ciao,
Michael.