* [PATCH] elf: Don't load archive element after dynamic definition @ 2020-08-25 17:28 H.J. Lu 2020-08-27 13:53 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-08-25 17:28 UTC (permalink / raw) To: binutils Don't load the archive element after seeing a definition in a shared object. bfd/ PR ld/26530 * elflink.c (elf_link_add_object_symbols): Also preserve the dynamic_def bit for --as-needed. (elf_link_add_archive_symbols): Don't load the archive element after seeing a definition in a shared object. ld/ PR ld/26530 * testsuite/ld-plugin/lto.exp: Run PR ld/26530 tests. * testsuite/ld-plugin/pr26530a.c: New file. * testsuite/ld-plugin/pr26530b.c: Likewise. * testsuite/ld-plugin/pr26530c.c: Likewise. --- bfd/elflink.c | 20 ++++++++++++++------ ld/testsuite/ld-plugin/lto.exp | 20 ++++++++++++++++++++ ld/testsuite/ld-plugin/pr26530a.c | 5 +++++ ld/testsuite/ld-plugin/pr26530b.c | 7 +++++++ ld/testsuite/ld-plugin/pr26530c.c | 12 ++++++++++++ 5 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 ld/testsuite/ld-plugin/pr26530a.c create mode 100644 ld/testsuite/ld-plugin/pr26530b.c create mode 100644 ld/testsuite/ld-plugin/pr26530c.c diff --git a/bfd/elflink.c b/bfd/elflink.c index 17a423270e..97e6ece97e 100644 --- a/bfd/elflink.c +++ b/bfd/elflink.c @@ -5340,7 +5340,7 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info) struct elf_link_hash_entry *h; bfd_size_type size; unsigned int alignment_power; - unsigned int non_ir_ref_dynamic; + struct elf_link_hash_entry preserved_h; for (p = htab->root.table.table[i]; p != NULL; p = p->next) { @@ -5362,10 +5362,11 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info) size = 0; alignment_power = 0; } - /* Preserve non_ir_ref_dynamic so that this symbol - will be exported when the dynamic lib becomes needed - in the second pass. */ - non_ir_ref_dynamic = h->root.non_ir_ref_dynamic; + /* Preserve some bits so that this symbol will be exported + when the dynamic lib becomes needed in the second pass. + and to avoid linking in an archive after the dynamic + lib. */ + preserved_h = *h; memcpy (p, old_ent, htab->root.table.entsize); old_ent = (char *) old_ent + htab->root.table.entsize; h = (struct elf_link_hash_entry *) p; @@ -5382,7 +5383,9 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info) if (alignment_power > h->root.u.c.p->alignment_power) h->root.u.c.p->alignment_power = alignment_power; } - h->root.non_ir_ref_dynamic = non_ir_ref_dynamic; + h->root.non_ir_ref_dynamic + = preserved_h.root.non_ir_ref_dynamic; + h->dynamic_def = preserved_h.dynamic_def; } } @@ -5819,6 +5822,11 @@ elf_link_add_archive_symbols (bfd *abfd, struct bfd_link_info *info) if (h == NULL) continue; + /* Don't load the archive element after seeing a definition + in a shared object. */ + if (h->dynamic_def) + continue; + if (h->root.type == bfd_link_hash_undefined) { /* If the archive element has already been loaded then one diff --git a/ld/testsuite/ld-plugin/lto.exp b/ld/testsuite/ld-plugin/lto.exp index 6a37025db5..8331500b33 100644 --- a/ld/testsuite/ld-plugin/lto.exp +++ b/ld/testsuite/ld-plugin/lto.exp @@ -419,6 +419,18 @@ set lto_link_elf_tests [list \ "-shared -Wl,--exclude-libs,ALL tmpdir/pr25618a.o tmpdir/pr25618.a" \ "-fpic" \ {dummy.c} {{readelf {--dyn-syms --wide} pr25618.d}} "pr25618.so" "c++"] \ + [list "Build pr26530a.so" \ + "-shared" "-O2 -fpic $no_lto" \ + {pr26530a.c} {} "pr26530a.so" "c"] \ + [list "Build pr26530b.so" \ + "-shared" "-O2 -fpic $no_lto" \ + {pr26530b.c} {} "pr26530b.so" "c"] \ + [list "Build pr26530a.a" \ + "" "-O2 $no_lto" \ + {pr26530a.c} {} "pr26530a.a" "c"] \ + [list "Build pr26530b.a" \ + "" "-O2 $no_lto" \ + {pr26530b.c} {} "pr26530b.a" "c"] \ ] # PR 14918 checks that libgcc is not spuriously included in a shared link of @@ -582,6 +594,14 @@ set lto_run_elf_shared_tests [list \ [list {pr22220b} \ {-flto -fuse-linker-plugin -Wl,--no-as-needed tmpdir/pr22220lib.so tmpdir/pr22220main.o} {} \ {dummy.c} {pr22220b.exe} {pass.out} {} {c++}] \ + [list "pr26530a" \ + "-O2 -flto -fuse-linker-plugin" "" \ + {pr26530c.c} "pr26530a.exe" "pass.out" "-O2 -flto" "c" {} \ + "-Wl,--as-needed tmpdir/pr26530a.so tmpdir/pr26530b.a"] \ + [list "pr26530b" \ + "-O2 -flto -fuse-linker-plugin" "" \ + {pr26530c.c} "pr26530b.exe" "pass.out" "-O2 -flto" "c" {} \ + "-Wl,--as-needed tmpdir/pr26530a.a tmpdir/pr26530b.so"] \ ] # LTO run-time tests for ELF diff --git a/ld/testsuite/ld-plugin/pr26530a.c b/ld/testsuite/ld-plugin/pr26530a.c new file mode 100644 index 0000000000..08e746911e --- /dev/null +++ b/ld/testsuite/ld-plugin/pr26530a.c @@ -0,0 +1,5 @@ +const char * +lrealpath (const char *a) +{ + return a; +} diff --git a/ld/testsuite/ld-plugin/pr26530b.c b/ld/testsuite/ld-plugin/pr26530b.c new file mode 100644 index 0000000000..0eb003f470 --- /dev/null +++ b/ld/testsuite/ld-plugin/pr26530b.c @@ -0,0 +1,7 @@ +#include <stdlib.h> + +void +lrealpath (void) +{ + abort (); +} diff --git a/ld/testsuite/ld-plugin/pr26530c.c b/ld/testsuite/ld-plugin/pr26530c.c new file mode 100644 index 0000000000..c8ef23675e --- /dev/null +++ b/ld/testsuite/ld-plugin/pr26530c.c @@ -0,0 +1,12 @@ +#include <stdio.h> + +const char *lrealpath(const char *a); + +int +main(int argc, char **argv) +{ + const char *path = lrealpath (argv[0]); + if (path[0] != '\0') + printf ("PASS\n"); + return 0; +} -- 2.26.2 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-25 17:28 [PATCH] elf: Don't load archive element after dynamic definition H.J. Lu @ 2020-08-27 13:53 ` Alan Modra 2020-08-27 14:05 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-08-27 13:53 UTC (permalink / raw) To: H.J. Lu; +Cc: binutils On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote: > Don't load the archive element after seeing a definition in a shared > object. > > bfd/ > > PR ld/26530 > * elflink.c (elf_link_add_object_symbols): Also preserve the > dynamic_def bit for --as-needed. > (elf_link_add_archive_symbols): Don't load the archive element > after seeing a definition in a shared object. This seems over complicated to me. Why don't we just load the as-needed shared library on the first pass? -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-27 13:53 ` Alan Modra @ 2020-08-27 14:05 ` H.J. Lu 2020-08-28 1:58 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-08-27 14:05 UTC (permalink / raw) To: Alan Modra; +Cc: Binutils On Thu, Aug 27, 2020 at 6:53 AM Alan Modra <amodra@gmail.com> wrote: > > On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote: > > Don't load the archive element after seeing a definition in a shared > > object. > > > > bfd/ > > > > PR ld/26530 > > * elflink.c (elf_link_add_object_symbols): Also preserve the > > dynamic_def bit for --as-needed. > > (elf_link_add_archive_symbols): Don't load the archive element > > after seeing a definition in a shared object. > > This seems over complicated to me. Why don't we just load the > as-needed shared library on the first pass? > In the first pass, when we are loading a shared object, do we have the IR symbol resolution? -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-27 14:05 ` H.J. Lu @ 2020-08-28 1:58 ` Alan Modra 2020-08-28 12:53 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-08-28 1:58 UTC (permalink / raw) To: H.J. Lu; +Cc: Binutils On Thu, Aug 27, 2020 at 07:05:15AM -0700, H.J. Lu wrote: > On Thu, Aug 27, 2020 at 6:53 AM Alan Modra <amodra@gmail.com> wrote: > > > > On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote: > > > Don't load the archive element after seeing a definition in a shared > > > object. > > > > > > bfd/ > > > > > > PR ld/26530 > > > * elflink.c (elf_link_add_object_symbols): Also preserve the > > > dynamic_def bit for --as-needed. > > > (elf_link_add_archive_symbols): Don't load the archive element > > > after seeing a definition in a shared object. > > > > This seems over complicated to me. Why don't we just load the > > as-needed shared library on the first pass? > > > > In the first pass, when we are loading a shared object, do we have > the IR symbol resolution? No, but why should that matter? Adding DT_NEEDED for an as-needed library that turns out to not be needed after LTO is not a problem IMO. Also I think it may be necessary to load shared libraries on the first pass for LTO to work properly in corner cases. See the comment in git commit a896df97b952. The condition we use to add --as-needed libraries is: if (!add_needed && matched && definition && ((dynsym && h->ref_regular_nonweak) || (h->ref_dynamic_nonweak && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0 && !on_needed_list (elf_dt_name (abfd), htab->needed, NULL)))) Note the test of dynsym, but earlier /* Nor should we make plugin symbols dynamic. */ if ((abfd->flags & BFD_PLUGIN) != 0) dynsym = FALSE; So I'm thinking perhaps we should delete the above lines and instead add the BFD_PLUGIN condition where we make symbols dynamic. if (dynsym && (abfd->flags & BFD_PLUGIN) == 0 && h->dynindx == -1) { if (! bfd_elf_link_record_dynamic_symbol (info, h)) I haven't tested the idea.. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-28 1:58 ` Alan Modra @ 2020-08-28 12:53 ` H.J. Lu 2020-08-28 14:49 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-08-28 12:53 UTC (permalink / raw) To: Alan Modra; +Cc: Binutils On Thu, Aug 27, 2020 at 6:59 PM Alan Modra <amodra@gmail.com> wrote: > > On Thu, Aug 27, 2020 at 07:05:15AM -0700, H.J. Lu wrote: > > On Thu, Aug 27, 2020 at 6:53 AM Alan Modra <amodra@gmail.com> wrote: > > > > > > On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote: > > > > Don't load the archive element after seeing a definition in a shared > > > > object. > > > > > > > > bfd/ > > > > > > > > PR ld/26530 > > > > * elflink.c (elf_link_add_object_symbols): Also preserve the > > > > dynamic_def bit for --as-needed. > > > > (elf_link_add_archive_symbols): Don't load the archive element > > > > after seeing a definition in a shared object. > > > > > > This seems over complicated to me. Why don't we just load the > > > as-needed shared library on the first pass? > > > > > > > In the first pass, when we are loading a shared object, do we have > > the IR symbol resolution? > > No, but why should that matter? Adding DT_NEEDED for an as-needed > library that turns out to not be needed after LTO is not a problem > IMO. Also I think it may be necessary to load shared libraries on the > first pass for LTO to work properly in corner cases. See the comment > in git commit a896df97b952. > > The condition we use to add --as-needed libraries is: > if (!add_needed > && matched > && definition > && ((dynsym > && h->ref_regular_nonweak) > || (h->ref_dynamic_nonweak > && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0 > && !on_needed_list (elf_dt_name (abfd), > htab->needed, NULL)))) > Note the test of dynsym, but earlier > /* Nor should we make plugin symbols dynamic. */ > if ((abfd->flags & BFD_PLUGIN) != 0) > dynsym = FALSE; > > So I'm thinking perhaps we should delete the above lines and instead > add the BFD_PLUGIN condition where we make symbols dynamic. > > if (dynsym && (abfd->flags & BFD_PLUGIN) == 0 && h->dynindx == -1) > { > if (! bfd_elf_link_record_dynamic_symbol (info, h)) > > I haven't tested the idea.. > I tested the patch enclosed here and it doesn't work. In the first pass, when we are loading a shared object, we can't assume that the shared object is needed simply because it satisfies a reference from an IR object without the IR symbol resolution. -- H.J. ---- diff --git a/bfd/elflink.c b/bfd/elflink.c index 5c085b14b7..3c559bafc5 100644 --- a/bfd/elflink.c +++ b/bfd/elflink.c @@ -5162,10 +5162,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 +5188,9 @@ 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; ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-28 12:53 ` H.J. Lu @ 2020-08-28 14:49 ` Alan Modra 2020-08-28 14:50 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-08-28 14:49 UTC (permalink / raw) To: H.J. Lu; +Cc: Binutils On Fri, Aug 28, 2020 at 05:53:10AM -0700, H.J. Lu wrote: > On Thu, Aug 27, 2020 at 6:59 PM Alan Modra <amodra@gmail.com> wrote: > > > > On Thu, Aug 27, 2020 at 07:05:15AM -0700, H.J. Lu wrote: > > > On Thu, Aug 27, 2020 at 6:53 AM Alan Modra <amodra@gmail.com> wrote: > > > > > > > > On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote: > > > > > Don't load the archive element after seeing a definition in a shared > > > > > object. > > > > > > > > > > bfd/ > > > > > > > > > > PR ld/26530 > > > > > * elflink.c (elf_link_add_object_symbols): Also preserve the > > > > > dynamic_def bit for --as-needed. > > > > > (elf_link_add_archive_symbols): Don't load the archive element > > > > > after seeing a definition in a shared object. > > > > > > > > This seems over complicated to me. Why don't we just load the > > > > as-needed shared library on the first pass? > > > > > > > > > > In the first pass, when we are loading a shared object, do we have > > > the IR symbol resolution? > > > > No, but why should that matter? Adding DT_NEEDED for an as-needed > > library that turns out to not be needed after LTO is not a problem > > IMO. Also I think it may be necessary to load shared libraries on the > > first pass for LTO to work properly in corner cases. See the comment > > in git commit a896df97b952. > > > > The condition we use to add --as-needed libraries is: > > if (!add_needed > > && matched > > && definition > > && ((dynsym > > && h->ref_regular_nonweak) > > || (h->ref_dynamic_nonweak > > && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0 > > && !on_needed_list (elf_dt_name (abfd), > > htab->needed, NULL)))) > > Note the test of dynsym, but earlier > > /* Nor should we make plugin symbols dynamic. */ > > if ((abfd->flags & BFD_PLUGIN) != 0) > > dynsym = FALSE; > > > > So I'm thinking perhaps we should delete the above lines and instead > > add the BFD_PLUGIN condition where we make symbols dynamic. > > > > if (dynsym && (abfd->flags & BFD_PLUGIN) == 0 && h->dynindx == -1) > > { > > if (! bfd_elf_link_record_dynamic_symbol (info, h)) > > > > I haven't tested the idea.. > > > > I tested the patch enclosed here and it doesn't work. Please explain "doesn't work". > In the first pass, when we are loading a shared object, we can't assume > that the shared object is needed simply because it satisfies a reference > from an IR object without the IR symbol resolution. > > -- > H.J. > ---- > diff --git a/bfd/elflink.c b/bfd/elflink.c > index 5c085b14b7..3c559bafc5 100644 > --- a/bfd/elflink.c > +++ b/bfd/elflink.c > @@ -5162,10 +5162,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 +5188,9 @@ 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; -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-28 14:49 ` Alan Modra @ 2020-08-28 14:50 ` H.J. Lu 2020-09-02 6:52 ` Martin Liška 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-08-28 14:50 UTC (permalink / raw) To: Alan Modra; +Cc: Binutils On Fri, Aug 28, 2020 at 7:49 AM Alan Modra <amodra@gmail.com> wrote: > > On Fri, Aug 28, 2020 at 05:53:10AM -0700, H.J. Lu wrote: > > On Thu, Aug 27, 2020 at 6:59 PM Alan Modra <amodra@gmail.com> wrote: > > > > > > On Thu, Aug 27, 2020 at 07:05:15AM -0700, H.J. Lu wrote: > > > > On Thu, Aug 27, 2020 at 6:53 AM Alan Modra <amodra@gmail.com> wrote: > > > > > > > > > > On Tue, Aug 25, 2020 at 10:28:42AM -0700, H.J. Lu via Binutils wrote: > > > > > > Don't load the archive element after seeing a definition in a shared > > > > > > object. > > > > > > > > > > > > bfd/ > > > > > > > > > > > > PR ld/26530 > > > > > > * elflink.c (elf_link_add_object_symbols): Also preserve the > > > > > > dynamic_def bit for --as-needed. > > > > > > (elf_link_add_archive_symbols): Don't load the archive element > > > > > > after seeing a definition in a shared object. > > > > > > > > > > This seems over complicated to me. Why don't we just load the > > > > > as-needed shared library on the first pass? > > > > > > > > > > > > > In the first pass, when we are loading a shared object, do we have > > > > the IR symbol resolution? > > > > > > No, but why should that matter? Adding DT_NEEDED for an as-needed > > > library that turns out to not be needed after LTO is not a problem > > > IMO. Also I think it may be necessary to load shared libraries on the > > > first pass for LTO to work properly in corner cases. See the comment > > > in git commit a896df97b952. > > > > > > The condition we use to add --as-needed libraries is: > > > if (!add_needed > > > && matched > > > && definition > > > && ((dynsym > > > && h->ref_regular_nonweak) > > > || (h->ref_dynamic_nonweak > > > && (elf_dyn_lib_class (abfd) & DYN_AS_NEEDED) != 0 > > > && !on_needed_list (elf_dt_name (abfd), > > > htab->needed, NULL)))) > > > Note the test of dynsym, but earlier > > > /* Nor should we make plugin symbols dynamic. */ > > > if ((abfd->flags & BFD_PLUGIN) != 0) > > > dynsym = FALSE; > > > > > > So I'm thinking perhaps we should delete the above lines and instead > > > add the BFD_PLUGIN condition where we make symbols dynamic. > > > > > > if (dynsym && (abfd->flags & BFD_PLUGIN) == 0 && h->dynindx == -1) > > > { > > > if (! bfd_elf_link_record_dynamic_symbol (info, h)) > > > > > > I haven't tested the idea.. > > > > > > > I tested the patch enclosed here and it doesn't work. > > Please explain "doesn't work". PR ld/26530 test still failed. > > In the first pass, when we are loading a shared object, we can't assume > > that the shared object is needed simply because it satisfies a reference > > from an IR object without the IR symbol resolution. > > > > -- > > H.J. > > ---- > > diff --git a/bfd/elflink.c b/bfd/elflink.c > > index 5c085b14b7..3c559bafc5 100644 > > --- a/bfd/elflink.c > > +++ b/bfd/elflink.c > > @@ -5162,10 +5162,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 +5188,9 @@ 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; > > -- > Alan Modra > Australia Development Lab, IBM -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-08-28 14:50 ` H.J. Lu @ 2020-09-02 6:52 ` Martin Liška 2020-09-02 8:12 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: Martin Liška @ 2020-09-02 6:52 UTC (permalink / raw) To: H.J. Lu, Alan Modra; +Cc: Binutils On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote: > PR ld/26530 test still failed. Hello. Is there any progress on this please? Thanks, Martin ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 6:52 ` Martin Liška @ 2020-09-02 8:12 ` Alan Modra 2020-09-02 11:56 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-02 8:12 UTC (permalink / raw) To: Martin Liška; +Cc: H.J. Lu, Binutils On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote: > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote: > > PR ld/26530 test still failed. > > Hello. > > Is there any progress on this please? I think what we want is the following, and some tweaking of the testsuite to remove FAIL: PR ld/15146 (1). That test relied on not loading a shared lib due to an IR symbol reference, but the logic added by git commit 3d5bef4c0871 has already been reverted. diff --git a/bfd/elflink.c b/bfd/elflink.c index 5c085b14b7..346f534911 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; -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 8:12 ` Alan Modra @ 2020-09-02 11:56 ` H.J. Lu 2020-09-02 13:05 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-09-02 11:56 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils On Wed, Sep 2, 2020 at 1:12 AM Alan Modra <amodra@gmail.com> wrote: > > On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote: > > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote: > > > PR ld/26530 test still failed. > > > > Hello. > > > > Is there any progress on this please? > > I think what we want is the following, and some tweaking of the > testsuite to remove FAIL: PR ld/15146 (1). That test relied on not > loading a shared lib due to an IR symbol reference, but the logic > added by git commit 3d5bef4c0871 has already been reverted. > > diff --git a/bfd/elflink.c b/bfd/elflink.c > index 5c085b14b7..346f534911 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; Will it add a shared library to DT_NEEDED even if the IR symbol reference is removed by LTO? -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 11:56 ` H.J. Lu @ 2020-09-02 13:05 ` Alan Modra 2020-09-02 13:22 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-02 13:05 UTC (permalink / raw) To: H.J. Lu; +Cc: Martin Liška, Binutils On Wed, Sep 02, 2020 at 04:56:48AM -0700, H.J. Lu wrote: > On Wed, Sep 2, 2020 at 1:12 AM Alan Modra <amodra@gmail.com> wrote: > > > > On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote: > > > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote: > > > > PR ld/26530 test still failed. > > > > > > Hello. > > > > > > Is there any progress on this please? > > > > I think what we want is the following, and some tweaking of the > > testsuite to remove FAIL: PR ld/15146 (1). That test relied on not > > loading a shared lib due to an IR symbol reference, but the logic > > added by git commit 3d5bef4c0871 has already been reverted. > > > > diff --git a/bfd/elflink.c b/bfd/elflink.c > > index 5c085b14b7..346f534911 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; > > Will it add a shared library to DT_NEEDED even if the IR symbol reference is > removed by LTO? Yes. That can't be helped. I know we worried about unnecessary --as-needed shared libraries in the past when IR symbols disappear after LTO, but I can't see a simple and reliable way for the linker to be correct. 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. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 13:05 ` Alan Modra @ 2020-09-02 13:22 ` H.J. Lu 2020-09-02 14:29 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-09-02 13:22 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils On Wed, Sep 2, 2020 at 6:05 AM Alan Modra <amodra@gmail.com> wrote: > > On Wed, Sep 02, 2020 at 04:56:48AM -0700, H.J. Lu wrote: > > On Wed, Sep 2, 2020 at 1:12 AM Alan Modra <amodra@gmail.com> wrote: > > > > > > On Wed, Sep 02, 2020 at 08:52:06AM +0200, Martin Liška wrote: > > > > On 8/28/20 4:50 PM, H.J. Lu via Binutils wrote: > > > > > PR ld/26530 test still failed. > > > > > > > > Hello. > > > > > > > > Is there any progress on this please? > > > > > > I think what we want is the following, and some tweaking of the > > > testsuite to remove FAIL: PR ld/15146 (1). That test relied on not > > > loading a shared lib due to an IR symbol reference, but the logic > > > added by git commit 3d5bef4c0871 has already been reverted. > > > > > > diff --git a/bfd/elflink.c b/bfd/elflink.c > > > index 5c085b14b7..346f534911 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; > > > > Will it add a shared library to DT_NEEDED even if the IR symbol reference is > > removed by LTO? > > Yes. That can't be helped. I know we worried about unnecessary > --as-needed shared libraries in the past when IR symbols disappear > after LTO, but I can't see a simple and reliable way for the linker to > be correct. My patch isn't very complicated. > 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. -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 13:22 ` H.J. Lu @ 2020-09-02 14:29 ` Alan Modra 2020-09-02 14:35 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-02 14:29 UTC (permalink / raw) To: H.J. Lu; +Cc: Martin Liška, Binutils 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. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 14:29 ` Alan Modra @ 2020-09-02 14:35 ` H.J. Lu 2020-09-03 1:31 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-09-02 14:35 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils 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? -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-02 14:35 ` H.J. Lu @ 2020-09-03 1:31 ` Alan Modra 2020-09-03 2:16 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-03 1:31 UTC (permalink / raw) To: H.J. Lu; +Cc: Martin Liška, Binutils 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. That's critical in understanding why LTO needs to know about symbols from shared libraries before LTO recompilation. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-03 1:31 ` Alan Modra @ 2020-09-03 2:16 ` H.J. Lu 2020-09-03 6:07 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-09-03 2:16 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils 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? > That's critical in understanding why LTO needs to know about symbols > from shared libraries before LTO recompilation. > Linker is complicated and LTO makes it even more complicated. Without a testcase, we can't be sure that the bug is really fixed nor we can be sure it won't regress. -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-03 2:16 ` H.J. Lu @ 2020-09-03 6:07 ` Alan Modra 2020-09-03 11:34 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-03 6:07 UTC (permalink / raw) To: H.J. Lu; +Cc: Martin Liška, Binutils 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. > > > That's critical in understanding why LTO needs to know about symbols > > from shared libraries before LTO recompilation. > > > > Linker is complicated and LTO makes it even more complicated. > Without a testcase, we can't be sure that the bug is really fixed > nor we can be sure it won't regress. > > > -- > H.J. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-03 6:07 ` Alan Modra @ 2020-09-03 11:34 ` H.J. Lu 2020-09-04 7:25 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-09-03 11:34 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils 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. -- H.J. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-03 11:34 ` H.J. Lu @ 2020-09-04 7:25 ` Alan Modra 2020-09-04 11:06 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-04 7:25 UTC (permalink / raw) To: H.J. Lu; +Cc: Martin Liška, Binutils 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-04 7:25 ` Alan Modra @ 2020-09-04 11:06 ` H.J. Lu 2020-09-08 5:42 ` Alan Modra 0 siblings, 1 reply; 22+ messages in thread From: H.J. Lu @ 2020-09-04 11:06 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-04 11:06 ` H.J. Lu @ 2020-09-08 5:42 ` Alan Modra 2020-09-08 12:43 ` H.J. Lu 0 siblings, 1 reply; 22+ messages in thread From: Alan Modra @ 2020-09-08 5:42 UTC (permalink / raw) To: H.J. Lu; +Cc: Martin Liška, Binutils 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] elf: Don't load archive element after dynamic definition 2020-09-08 5:42 ` Alan Modra @ 2020-09-08 12:43 ` H.J. Lu 0 siblings, 0 replies; 22+ messages in thread From: H.J. Lu @ 2020-09-08 12:43 UTC (permalink / raw) To: Alan Modra; +Cc: Martin Liška, Binutils 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-09-08 12:43 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-25 17:28 [PATCH] elf: Don't load archive element after dynamic definition H.J. Lu 2020-08-27 13:53 ` Alan Modra 2020-08-27 14:05 ` H.J. Lu 2020-08-28 1:58 ` Alan Modra 2020-08-28 12:53 ` H.J. Lu 2020-08-28 14:49 ` Alan Modra 2020-08-28 14:50 ` H.J. Lu 2020-09-02 6:52 ` Martin Liška 2020-09-02 8:12 ` Alan Modra 2020-09-02 11:56 ` H.J. Lu 2020-09-02 13:05 ` Alan Modra 2020-09-02 13:22 ` H.J. Lu 2020-09-02 14:29 ` Alan Modra 2020-09-02 14:35 ` H.J. Lu 2020-09-03 1:31 ` Alan Modra 2020-09-03 2:16 ` H.J. Lu 2020-09-03 6:07 ` Alan Modra 2020-09-03 11:34 ` H.J. Lu 2020-09-04 7:25 ` Alan Modra 2020-09-04 11:06 ` H.J. Lu 2020-09-08 5:42 ` Alan Modra 2020-09-08 12:43 ` H.J. Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).