Summary: | Support mixed SHF_LINK_ORDER & non-SHF_LINK_ORDER components in an output section | ||
---|---|---|---|
Product: | binutils | Reporter: | Fangrui Song <i> |
Component: | ld | Assignee: | H.J. Lu <hjl.tools> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | hkrzesin, jakub, pbrobinson |
Priority: | P2 | ||
Version: | 2.36 | ||
Target Milestone: | 2.36 | ||
Host: | Target: | ||
Build: | Last reconfirmed: | 2020-12-19 00:00:00 | |
Attachments: |
A patch
An updated patch A patch with tests |
Description
Fangrui Song
2020-07-17 04:14:40 UTC
The last case has the same section content as the previous one: ## Non-contiguous SHF_LINK_ORDER sections, separated by a symbol assignment. # RUN: echo 'SECTIONS { .rodata : {*(.rodata.foo) a = .; *(.rodata.bar)} }' > %t5.lds # RUN: ld.lld -T %t5.lds %t.o -o %t5 010302 This demonstrates why performing sorting within an input section description makes sense while sorting within an output section may not. There is a related issue. >a.s<<e cat .global _start _start: .section .text.bar,"a",@progbits .byte 2 .section .text.foo,"a",@progbits .byte 1 .section .ro.foo,"ao",@progbits,.text.foo .byte 1 .section .ro.bar,"ao",@progbits,.text.bar .byte 2 e >a.lds echo 'SECTIONS { .ro : {*(.ro.bar) *(.ro.foo)} .text : {*(.text.foo) *(.text.bar)} }' as a.s -o a.o # >=2.35 ld.bfd -T a.lds a.o -o a readelf -W -x .text -x .ro a Hex dump of section '.ro': 0x00000000 0102 .. Hex dump of section '.text': 0x00000038 0102 .. However, I don't expect ld to reorder .ro.foo before .ro.bar because *(.ro.bar) *(.ro.foo) clearly expresses an order. The current asection::map_header.link_order representation does not take into account different input section descriptions. You can use the tests https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/linkerscript/linkorder.s https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/linkorder-mixed.s *** Bug 27098 has been marked as a duplicate of this bug. *** Created attachment 13066 [details]
A patch
Please try this.
(In reply to H.J. Lu from comment #5) > Created attachment 13066 [details] > A patch > > Please try this. If I replace the ld.lld command on https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/linkerscript/linkorder.s#L7 with ld-new, ld-new segfaults on bfd/elflink.c:11863 `asection *asec = elf_linked_to_section (alo->u.indirect.section);` ... Created attachment 13069 [details]
An updated patch
Please try this.
Created attachment 13070 [details]
A patch with tests
Try this.
I tested this last patch (from attachment/comment #8), fixes the kernel/aarch64 with gcc11 link issue reported through bug 27098 and marked as duplicate of this one. (In reply to H.J. Lu from comment #8) > Created attachment 13070 [details] > A patch with tests > > Try this. With a minor change, it'll match LLD (I place ordered sections before unordered sections because Solaris folks said they did so). + /* Place ordered sections before unordered sections. */ + if (bsec != NULL) + return 1; + else if (asec != NULL) + return -1; + return 0; For the test case lld/test/ELF/linkorder-mixed.s , ld-new produced %t, %t1 and %t3 now match LLD. %t2 and %t4 still don't, but they are probably corner cases and don't matter in practice. About -r & SHF_LINK_ORDER. The LLD resolution is that SHF_LINK_ORDER sections of the same name should not be combined. https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/linkerscript/linkorder-linked-to.s demonstrates the behavior. The idea is that the final link decides the section order. A -r link should not make an early decision (original patch is https://reviews.llvm.org/D68094) (In reply to Fangrui Song from comment #10) > (In reply to H.J. Lu from comment #8) > > Created attachment 13070 [details] > > A patch with tests > > > > Try this. > > With a minor change, it'll match LLD (I place ordered sections before > unordered sections because Solaris folks said they did so). > > + /* Place ordered sections before unordered sections. */ > + if (bsec != NULL) > + return 1; > + else if (asec != NULL) > + return -1; > + return 0; > > For the test case lld/test/ELF/linkorder-mixed.s , ld-new produced %t, %t1 > and %t3 now match LLD. > %t2 and %t4 still don't, but they are probably corner cases and don't matter > in practice. %t2 and %t4 are lld specific behavior. (In reply to H.J. Lu from comment #12) > (In reply to Fangrui Song from comment #10) > > (In reply to H.J. Lu from comment #8) > > > Created attachment 13070 [details] > > > A patch with tests > > > > > > Try this. > > > > With a minor change, it'll match LLD (I place ordered sections before > > unordered sections because Solaris folks said they did so). > > > > + /* Place ordered sections before unordered sections. */ > > + if (bsec != NULL) > > + return 1; > > + else if (asec != NULL) > > + return -1; > > + return 0; > > > > For the test case lld/test/ELF/linkorder-mixed.s , ld-new produced %t, %t1 > > and %t3 now match LLD. > > %t2 and %t4 still don't, but they are probably corner cases and don't matter > > in practice. > > %t2 and %t4 are lld specific behavior. I actually think %t2 and %t4 are generic: ordered .rodata.bar (.byte 3) precedes unordered .rodata.bar (.byte 2) SECTIONS { .rodata : {*(.rodata.foo) *(.rodata.bar)} } What LLD does is to perform SHF_LINK_ORDER sorting within the input section description *(.rodata.bar). I agree that this is a corner case which can hardly do harm in practice. The case is somewhat similar to [SORT and multiple patterns in an input section description](https://sourceware.org/pipermail/binutils/2020-November/114098.html). I think the GNU ld behavior is a bit less reasonable but users probably should not use depend on such complex behaviors anyway. Anyway, if the patch prefers ordered sections to unordered sections, I think it is in quite a good shape. Being divergent on %t2 and %t4 should be fine. (In reply to Fangrui Song from comment #13) > (In reply to H.J. Lu from comment #12) > > (In reply to Fangrui Song from comment #10) > > > (In reply to H.J. Lu from comment #8) > > > > Created attachment 13070 [details] > > > > A patch with tests > > > > > > > > Try this. > > > > > > With a minor change, it'll match LLD (I place ordered sections before > > > unordered sections because Solaris folks said they did so). > > > > > > + /* Place ordered sections before unordered sections. */ > > > + if (bsec != NULL) > > > + return 1; > > > + else if (asec != NULL) > > > + return -1; > > > + return 0; > > > > > > For the test case lld/test/ELF/linkorder-mixed.s , ld-new produced %t, %t1 > > > and %t3 now match LLD. > > > %t2 and %t4 still don't, but they are probably corner cases and don't matter > > > in practice. > > > > %t2 and %t4 are lld specific behavior. > > > I actually think %t2 and %t4 are generic: ordered .rodata.bar (.byte 3) > precedes unordered .rodata.bar (.byte 2) > > SECTIONS { .rodata : {*(.rodata.foo) *(.rodata.bar)} } > > What LLD does is to perform SHF_LINK_ORDER sorting within the input section > description *(.rodata.bar). I agree that this is a corner case which can > hardly do harm in practice. It is a bug: diff --git a/bfd/elflink.c b/bfd/elflink.c index a1e4635e96..ddff3bfe7b 100644 --- a/bfd/elflink.c +++ b/bfd/elflink.c @@ -11988,9 +11988,7 @@ elf_fixup_link_order (struct bfd_link_info *info, bfd *abfd, asection *o) for (p = o->map_head.link_order; p != NULL; p = p->next) sections[seen_linkorder++] = p; - for (indirect_sections = sections, n = 0; - n < seen_linkorder; - indirect_sections++, n++) + for (indirect_sections = sections, n = 0; n < seen_linkorder;) { /* Find the first bfd_indirect_link_order section. */ if (indirect_sections[0]->type == bfd_indirect_link_order) @@ -12012,6 +12010,11 @@ elf_fixup_link_order (struct bfd_link_info *info, bfd *abfd, asection *o) indirect_sections += n_indirect; n += n_indirect; } + else + { + indirect_sections++; + n++; + } } (In reply to H.J. Lu from comment #14) > (In reply to Fangrui Song from comment #13) > > (In reply to H.J. Lu from comment #12) > > > (In reply to Fangrui Song from comment #10) > > > > (In reply to H.J. Lu from comment #8) > > > > > Created attachment 13070 [details] > > > > > A patch with tests > > > > > > > > > > Try this. > > > > > > > > With a minor change, it'll match LLD (I place ordered sections before > > > > unordered sections because Solaris folks said they did so). > > > > > > > > + /* Place ordered sections before unordered sections. */ > > > > + if (bsec != NULL) > > > > + return 1; > > > > + else if (asec != NULL) > > > > + return -1; > > > > + return 0; > > > > > > > > For the test case lld/test/ELF/linkorder-mixed.s , ld-new produced %t, %t1 > > > > and %t3 now match LLD. > > > > %t2 and %t4 still don't, but they are probably corner cases and don't matter > > > > in practice. > > > > > > %t2 and %t4 are lld specific behavior. > > > > > > I actually think %t2 and %t4 are generic: ordered .rodata.bar (.byte 3) > > precedes unordered .rodata.bar (.byte 2) > > > > SECTIONS { .rodata : {*(.rodata.foo) *(.rodata.bar)} } > > > > What LLD does is to perform SHF_LINK_ORDER sorting within the input section > > description *(.rodata.bar). I agree that this is a corner case which can > > hardly do harm in practice. > > It is a bug: > > diff --git a/bfd/elflink.c b/bfd/elflink.c > index a1e4635e96..ddff3bfe7b 100644 > --- a/bfd/elflink.c > +++ b/bfd/elflink.c > @@ -11988,9 +11988,7 @@ elf_fixup_link_order (struct bfd_link_info *info, > bfd *abfd, asection *o) > for (p = o->map_head.link_order; p != NULL; p = p->next) > sections[seen_linkorder++] = p; > > - for (indirect_sections = sections, n = 0; > - n < seen_linkorder; > - indirect_sections++, n++) > + for (indirect_sections = sections, n = 0; n < seen_linkorder;) > { > /* Find the first bfd_indirect_link_order section. */ > if (indirect_sections[0]->type == bfd_indirect_link_order) > @@ -12012,6 +12010,11 @@ elf_fixup_link_order (struct bfd_link_info *info, > bfd *abfd, asection *o) > indirect_sections += n_indirect; > n += n_indirect; > } > + else > + { > + indirect_sections++; > + n++; > + } > } Thanks. With this diff added, all the %t* tests are good now. The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=cd6d537c48fa862e8a5888b648102d7b2330ef81 commit cd6d537c48fa862e8a5888b648102d7b2330ef81 Author: H.J. Lu <hjl.tools@gmail.com> Date: Mon Jan 4 12:37:49 2021 -0800 elf: Allow mixed ordered/unordered inputs for non-relocatable link For non-relocatable link with SHF_LINK_ORDER inputs, allow mixed indirect and data inputs with ordered and unordered inputs: 1. Add pattern to bfd_section for the matching section name pattern in linker script and update BFD_FAKE_SECTION. 2. Sort the consecutive bfd_indirect_link_order sections with the same pattern to allow linker script to overdide input section order. 3. Place unordered sections before ordered sections. 4. Change the offsets of the indirect input sections only. bfd/ PR ld/26256 * elflink.c (compare_link_order): Place unordered sections before ordered sections. (elf_fixup_link_order): Add a link info argument. Allow mixed ordered and unordered input sections for non-relocatable link. Sort the consecutive bfd_indirect_link_order sections with the same pattern. Change the offsets of the bfd_indirect_link_order sections only. (bfd_elf_final_link): Pass info to elf_fixup_link_order. * section.c (bfd_section): Add pattern. (BFD_FAKE_SECTION): Initialize pattern to NULL. * bfd-in2.h: Regenerated. gas/ PR ld/26256 * config/obj-elf.c (obj_elf_change_section): Also filter out SHF_LINK_ORDER. ld/ PR ld/26256 * ldlang.c (gc_section_callback): Set pattern. * testsuite/ld-elf/pr26256-1.s: New file. * testsuite/ld-elf/pr26256-1.t: Likewise. * testsuite/ld-elf/pr26256-1a.d: Likewise. * testsuite/ld-elf/pr26256-1b.d: Likewise. * testsuite/ld-elf/pr26256-2.s: Likewise. * testsuite/ld-elf/pr26256-2.t: Likewise. * testsuite/ld-elf/pr26256-2a.d: Likewise. * testsuite/ld-elf/pr26256-2b-alt.d: Likewise. * testsuite/ld-elf/pr26256-2b.d: Likewise. * testsuite/ld-elf/pr26256-3.s: Likewise. * testsuite/ld-elf/pr26256-3a.d: Likewise. * testsuite/ld-elf/pr26256-3a.t: Likewise. * testsuite/ld-elf/pr26256-3b.d: Likewise. * testsuite/ld-elf/pr26256-3b.t: Likewise. Fixed for 2.36. The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=40b119304e2b412975edce421fd7f943a6c24942 commit 40b119304e2b412975edce421fd7f943a6c24942 Author: Alan Modra <amodra@gmail.com> Date: Tue Jan 5 15:43:37 2021 +1030 Re: elf: Allow mixed ordered/unordered inputs for non-relocatable link PR ld/26256 * testsuite/ld-elf/pr26256-1b.d: xfail s12z. * testsuite/ld-scripts/crossref.exp (cross1): Don't xfail ia64. The master branch has been updated by H.J. Lu <hjl@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b1b29aa51a6a67d5e35391ad31a80765839d6eb4 commit b1b29aa51a6a67d5e35391ad31a80765839d6eb4 Author: H.J. Lu <hjl.tools@gmail.com> Date: Fri Jan 8 21:38:39 2021 -0800 elf: Verify section size for mixed ordered/unordered inputs When fixing up SHF_LINK_ORDER, issue a fatal error if the output section size is increased. Otherwise, bfd_set_section_contents will fail later when attempting to write contents past the end of the output section. PR ld/26256 PR ld/27160 * elflink.c (elf_fixup_link_order): Verify that fixing up SHF_LINK_ORDER doesn't increase the output section size. |