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

[Alan Modra <amodra@gmail.com>]

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