Bug 27565 - ld: Support input section description keyword: REVERSE
Summary: ld: Support input section description keyword: REVERSE
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Nick Clifton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-03-11 21:25 UTC by Fangrui Song
Modified: 2023-11-01 15:52 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2023-03-22 00:00:00


Attachments
Proposed patch (3.55 KB, patch)
2023-03-22 16:29 UTC, Nick Clifton
Details | Diff
Proposed patch (3.56 KB, patch)
2023-03-27 10:21 UTC, Nick Clifton
Details | Diff
Proposed patch (3.69 KB, patch)
2023-10-18 12:47 UTC, Nick Clifton
Details | Diff
Proposed patch (4.00 KB, patch)
2023-10-20 11:38 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2021-03-11 21:25:07 UTC
REVERSE complements SORT, e.g.

  SECTIONS { .aaa : { *(REVERSE(SORT_BY_NAME(.aaa.*))) } }

If the user wants to detect some static initialization order fiasco issues, they can specify:

  # If https://sourceware.org/bugzilla/show_bug.cgi?id=26404 is supported,
  # the user can provide a fragment instead of a full linker script

  .init_array : {
     *(SORT_BY_INIT_PRIORITY(REVERSE(.init_array.* .ctors.*)))
     *(REVERSE(.init_array EXCLUDE_FILE (*crtbegin.o *crtbegin?.o *crtend.o *crtend?.o ) .ctors))
   }

which runs static constructors in a reversed order.
Comment 1 Fangrui Song 2023-03-06 21:30:13 UTC
Justin Cady wants to add REVERSE to ld.lld in https://reviews.llvm.org/D145381

The semantics seem pretty clear, so ld.lld adopting the feature first may be fine :)
Comment 2 Nick Clifton 2023-03-22 16:29:06 UTC
Created attachment 14772 [details]
Proposed patch

Hi Fanguri,

  What do you think of this patch ?  Does it do what you need ?

Cheers
  Nick
Comment 3 Fangrui Song 2023-03-24 22:01:46 UTC
(In reply to Nick Clifton from comment #2)
> Created attachment 14772 [details]
> Proposed patch
> 
> Hi Fanguri,
> 
>   What do you think of this patch ?  Does it do what you need ?
> 
> Cheers
>   Nick

Hi Nick, thanks for working on this feature! Do you have some examples how this keyword can be used? If I try (https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/linkerscript/sort-init.s#L53):


SECTIONS {
  .init_array : {
    *(REVERSE(SORT_BY_INIT_PRIORITY(.init_array.* .ctors.*)) SORT_BY_INIT_PRIORITY(foo*))
    *(REVERSE(.init_array .ctors))
  }
}

ld reports `syntax error`.
Comment 4 Nick Clifton 2023-03-27 10:15:17 UTC
(In reply to Fangrui Song from comment #3)

> Hi Nick, thanks for working on this feature! Do you have some examples how
> this keyword can be used?

The patch does include some new test cases which show the keyword being used.


> If I try
> (https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/linkerscript/
> sort-init.s#L53):
> 
> 
> SECTIONS {
>   .init_array : {
>     *(REVERSE(SORT_BY_INIT_PRIORITY(.init_array.* .ctors.*))
> SORT_BY_INIT_PRIORITY(foo*))
>     *(REVERSE(.init_array .ctors))
>   }
> }
> 
> ld reports `syntax error`.

Darn!   I included support for "REVERSE(SORT_BY_NAME(..." but not for "REVERSE(SORT_BY_INIT_PRIORITY(..."  

I will update the patch...
Comment 5 Nick Clifton 2023-03-27 10:21:44 UTC
Created attachment 14784 [details]
Proposed patch

Please try this revised patch.
Comment 6 Alexey 2023-10-18 08:24:00 UTC
I would appreciate having it in the upcoming release. Is it possible to submit it?
Comment 7 Sam James 2023-10-18 11:12:24 UTC
(In reply to Alexey from comment #6)
> I would appreciate having it in the upcoming release. Is it possible to
> submit it?

Could you try it and confirm it does what you want at least? Although ideally Fangrui would try it too
Comment 8 Nick Clifton 2023-10-18 12:47:46 UTC
Created attachment 15180 [details]
Proposed patch

And here is a reformatted version of the patch.

I realised that I had not included the filenames of the new test files in the v2 patch, so it did not apply cleanly.  This should be fixed in this version.
Comment 9 Alexey 2023-10-19 16:49:38 UTC
That is what I found.

Issue 1:

Can not pass multiple sections to REVERSE, e.g:
*(REVERSE(.init_array .ctors))

Can only with combining:
*(REVERSE(.init_array))
*(REVERSE(.ctors))

Issue 2:

Reverse does not apply for:

*(REVERSE(EXCLUDE_FILE (*crtend.* *crtbegin.*) .ctors))

? Issue 3:

The first element is 0xffffffff for:
*(REVERSE(.ctors))

I would like to not change the place of terminating 0xffffffff ctors.
Could not check how it works with clang linker, because getting this:
ld.lld: error: script.ld:112: expected filename pattern
>>>     KEEP (*(REVERSE(.ctors)))
>>>                             ^
Comment 10 Nick Clifton 2023-10-20 11:38:12 UTC
Hi Alexey,

(In reply to Alexey from comment #9)
> That is what I found.

Thanks for reviewing the patch.
 
> Issue 1:
> 
> Can not pass multiple sections to REVERSE, e.g:
> *(REVERSE(.init_array .ctors))

This is in line with the other sorting directives.  For example this is illegal too:

  *(SORT_BY_NAME (.text .data))

This behaviour is even documented in the linker manual:

  SORT_BY_NAME
  Normally, the linker will place files and sections matched 
  by wildcards in the order in which they are seen during the 
  link.  You can change this by using the SORT_BY_NAME
  keyword, which appears before *a* wildcard pattern in 
  parentheses (e.g., SORT_BY_NAME(.text*)}.

(I added the emphasis).

> Can only with combining:
> *(REVERSE(.init_array))
> *(REVERSE(.ctors))

Right - this is the way that script writers are expected to solve this problem.

However I accept that this might be confusing to users, so I am going to extend the documentation of the sorting commands to include an extra paragraph:

  Note - the sorting commands only accept a single wildcard pattern.
  So for example the following will not work:
       *(REVERSE(.text* .init*))
  To resolve this problem list the patterns individually, like this:
       *(REVERSE(.text*))
       *(REVERSE(.init*))


> Issue 2:
> 
> Reverse does not apply for:
> 
> *(REVERSE(EXCLUDE_FILE (*crtend.* *crtbegin.*) .ctors))

Yes - there was another bug in the changes to the ldgram.y file.  The REVERSE command is meant to imply SORT_BY_NAME if no sorting command is used, but this was only happening for file names and not for section names.

Note - you may have also noticed that the following is not accepted:

  *(EXCLUDE_FILE (*crtend.* *crtbegin.*) REVERSE(.ctors))

Supporting this form would mean even more changes to the grammar, which I would rather avoid unless really necessary.  Instead I would prefer to add another paragraph to the documentation:

  Note - you can put the 'EXCLUDE_FILE' command inside a sorting
  command, but not the other way around.  So for example:
       *(SORT_BY_NAME(EXCLUDE_FILE(foo) .text*))
   will work, but:
       *(EXCLUDE_FILE(foo) SORT_BY_NAME(.text*))
   will not.


> ? Issue 3:
> 
> The first element is 0xffffffff for:
> *(REVERSE(.ctors))
> 
> I would like to not change the place of terminating 0xffffffff ctors.
 
No, I think that this is not something that should be supported.  Changing the order of the constructors is a very bad idea, and legitimizing it by adding code to handle the terminating -1 is not something that I think we should do.

Please try out the revised patch and let me know if you are happy with it.

Cheers
  Nick
Comment 11 Nick Clifton 2023-10-20 11:38:31 UTC
Created attachment 15185 [details]
Proposed patch
Comment 12 Sourceware Commits 2023-11-01 13:52:44 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=85921e9a2588bf4820b827fc1630f5d7da22cb1c

commit 85921e9a2588bf4820b827fc1630f5d7da22cb1c
Author: Nick Clifton <nickc@redhat.com>
Date:   Wed Nov 1 13:51:17 2023 +0000

    ld: Support input section description keyword: REVERSE
    
      PR 27565
      * ldlex.l: Add REVERSE.
      * ldgram.y: Allow REVERSE to be used wherever a sorting command can be used.
      * ld.h (struct wildcard_spec): Add 'reversed' field.
      * ldlang.h (lang_wild_statement_struct): Add 'filenames_reversed' field.
      * ldlang.c (compare_sections): Add reversed parameter. (wild_sort): Reverse the comparison if requested. (print_wild_statement): Handle the reversed field.
      * ld.texi: Document the new feature.
      * NEWS: Mention the new feature.
      * testsuite/ld-scripts/sort-file-reversed-1.d: New test driver.
      * testsuite/ld-scripts/sort-file-reversed-1.t: New test source.
      * testsuite/ld-scripts/sort-file-reversed-2.t: New test source.
      * testsuite/ld-scripts/sort-file-reversed-2.d: New test driver.
      * testsuite/ld-scripts/sort-sections-reversed-1.d: New test driver.
      * testsuite/ld-scripts/sort-sections-reversed-1.t: New test source.
      * testsuite/ld-scripts/sort-sections-reversed-2.t: New test source.
      * testsuite/ld-scripts/sort-sections-reversed-2.d: New test driver.
      * testsuite/ld-scripts/sort-sections-reversed-3.d: New test driver.
      * testsuite/ld-scripts/sort-sections-reversed-3.t: New test source.
Comment 13 Nick Clifton 2023-11-01 13:53:41 UTC
OK, I have gone ahead and applied the fourth revision of the patch.

I am going to close this PR for now.  If any problems arise in the future it can be reopened or another PR filed.
Comment 14 Eric Botcazou 2023-11-01 15:24:51 UTC
There is a "may" missing in the NEWS blurb.
Comment 15 Nick Clifton 2023-11-01 15:52:57 UTC
(In reply to Eric Botcazou from comment #14)
> There is a "may" missing in the NEWS blurb.

Doh!  So there is.  I have pushed a simple commit to add the missing word.