Bug 26256 - Support mixed SHF_LINK_ORDER & non-SHF_LINK_ORDER components in an output section
Summary: Support mixed SHF_LINK_ORDER & non-SHF_LINK_ORDER components in an output sec...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.36
: P2 normal
Target Milestone: 2.36
Assignee: H.J. Lu
URL:
Keywords:
: 27098 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-07-17 04:14 UTC by Fangrui Song
Modified: 2021-01-09 05:41 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-12-19 00:00:00


Attachments
A patch (1.09 KB, patch)
2020-12-19 21:21 UTC, H.J. Lu
Details | Diff
An updated patch (2.88 KB, patch)
2020-12-20 15:21 UTC, H.J. Lu
Details | Diff
A patch with tests (4.15 KB, patch)
2020-12-20 16:21 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2020-07-17 04:14:40 UTC
See https://groups.google.com/forum/#!topic/generic-abi/hgx_m1aXqUo
"SHF_LINK_ORDER's original semantics make upgrade difficult"

Mixed SHF_LINK_ORDER and non-SHF_LINK_ORDER components are not allowed. This makes it infeasible to add SHF_LINK_ORDER to an existing metadata section if backward compatibility with older object files are concerned.

It would be nice if the two different semantics (ordering requirement & garbage
collection) were not overloaded on one section flag, however, it is probably
difficult to obtain a generic flag at this point
(https://groups.google.com/forum/#!topic/generic-abi/hgx_m1aXqUo
"SHF_LINK_ORDER's original semantics make upgrade difficult").

On the other hand, defining an order is not going to be troublesome, and will help metadata sections.

cat > a.s <<e
.section .text.bar,"a",@progbits
.byte 2
.section .text.foo,"a",@progbits
.byte 1
.section .rodata.foo,"ao",@progbits,.text.foo
.byte 1
## If the two .rodata.bar sections are in the same input section description,
## 03 (sh_link!=0) will be ordered before 02 (sh_link=0).
.section .rodata.bar,"a",@progbits
.byte 2
.section .rodata.bar,"ao",@progbits,.text.bar
.byte 3
e
as-new a.s -o a.o
ld-new a.o  # .rodata has both ordered [`.rodata.foo' in a.o] and unordered [`.rodata.bar' in a.o] sections


I am going to change LLD to perform ordering within an input section description.

For example, .rodata : {*(.rodata.foo) *(.rodata.bar)}, has two InputSectionDescription's. If there is at least one SHF_LINK_ORDER and at least one non-SHF_LINK_ORDER in .rodata.foo, they are ordered within *(.rodata.foo): we arbitrarily place SHF_LINK_ORDER components before non-SHF_LINK_ORDER components (like Solaris ld).

*(.rodata.bar) is ordered similarly, but the two input section descriptions don't interact.

Section content of .rodata for a few different linker scripts:

# RUN: echo 'SECTIONS { .rodata : {BYTE(0) *(.rodata*) BYTE(4)} \                                                                                     
# RUN:  .text : {*(.text.foo) *(.text.bar)} }' > %t1.lds                                                                                              
# RUN: ld.lld -T %t1.lds %t.o -o %t1

0003010204

# RUN: echo 'SECTIONS { .rodata : {*(.rodata.foo) *(.rodata.bar)} }' > %t2.lds                                                                        
# RUN: ld.lld -T %t2.lds %t.o -o %t2

010302

## 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                                       
# RUN: llvm-readelf -S -x .rodata %t5 | FileCheck --check-prefix=CHECK2 %s
Comment 1 Fangrui Song 2020-07-17 04:16:49 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.
Comment 2 Fangrui Song 2020-08-14 18:35:44 UTC
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.
Comment 4 H.J. Lu 2020-12-19 18:12:57 UTC
*** Bug 27098 has been marked as a duplicate of this bug. ***
Comment 5 H.J. Lu 2020-12-19 21:21:21 UTC
Created attachment 13066 [details]
A patch

Please try this.
Comment 6 Fangrui Song 2020-12-20 06:07:45 UTC
(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);` ...
Comment 7 H.J. Lu 2020-12-20 15:21:51 UTC
Created attachment 13069 [details]
An updated patch

Please try this.
Comment 8 H.J. Lu 2020-12-20 16:21:18 UTC
Created attachment 13070 [details]
A patch with tests

Try this.
Comment 9 Herton R. Krzesinski 2020-12-21 21:35:38 UTC
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.
Comment 10 Fangrui Song 2020-12-21 23:06:11 UTC
(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.
Comment 11 Fangrui Song 2020-12-21 23:13:44 UTC
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)
Comment 12 H.J. Lu 2020-12-22 04:16:20 UTC
(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.
Comment 13 Fangrui Song 2020-12-22 05:21:39 UTC
(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.
Comment 14 H.J. Lu 2020-12-22 13:22:03 UTC
(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++;
+  }
     }
Comment 15 Fangrui Song 2020-12-23 00:56:31 UTC
(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.
Comment 16 Sourceware Commits 2021-01-04 20:44:42 UTC
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.
Comment 17 H.J. Lu 2021-01-04 20:49:06 UTC
Fixed for 2.36.
Comment 18 Sourceware Commits 2021-01-05 05:31:00 UTC
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.
Comment 19 Sourceware Commits 2021-01-09 05:41:55 UTC
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.