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