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