This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Implemented --sort-section cmd line option and SORT_NONE script command.
ClosedPublic

Authored by grimar on Sep 15 2016, 3:05 AM.

Details

Summary

This fixes Bug 30385 - SORT_NONE not implemented,

`SORT_NONE' disables section sorting by ignoring the command line
section sorting option.

That is why this patch also implements --sort-section option.

Full rules of applying sorting (from ld manual) are:

When both command line section sorting option and linker script

section sorting command are used, section sorting command always takes
precedence over the command line option.

If the section sorting command in linker script isn't nested, the

command line option will make the section sorting command to be treated
as nested sorting command.

  1. SORT_BY_NAME' (wildcard section pattern ) with --sort-sections alignment' is equivalent to SORT_BY_NAME' (SORT_BY_ALIGNMENT' (wildcard section pattern)).
  2. SORT_BY_ALIGNMENT' (wildcard section pattern) with --sort-section name' is equivalent to SORT_BY_ALIGNMENT' (SORT_BY_NAME' (wildcard section pattern)).

    If the section sorting command in linker script is nested, the

command line option will be ignored.

Diff Detail

Event Timeline

grimar updated this revision to Diff 71489.Sep 15 2016, 3:05 AM
grimar retitled this revision from to [ELF] - Implemented --sort-section cmd line option and SORT_NONE script command..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777.
rafael added inline comments.Sep 15 2016, 5:34 AM
ELF/LinkerScript.cpp
1010

Please change the Config to store the parsed enum value.
Can you make it an error to pass an unknown value?

grimar added inline comments.Sep 15 2016, 5:34 AM
ELF/LinkerScript.cpp
1010

Ok, sure.

ruiu added inline comments.Sep 15 2016, 11:05 AM
ELF/LinkerScript.cpp
979–981

This naming scheme is confusing. SORT_NONE is mapped to SortDisabled, and SortNone doesn't mean SORT_NONE?

1035

We are reading something here, but you are applying? This function name doesn't feel right.

grimar added inline comments.Sep 15 2016, 11:09 AM
ELF/LinkerScript.cpp
979–981

Yes, a bit confusing, but according to documentation SORT_NONE is used to _disable_ sorting. A agree that probably better to map it to SortNone, but what is better name for absence of sorting (which is none) ? SortEmpty probably ?

ruiu added inline comments.Sep 15 2016, 11:16 AM
ELF/LinkerScript.cpp
979–981

I don't know. Null, Empty, Void, Dont, etc... Nothing feels right. I thought about it for a few minutes but didn't come up with a good name. Maybe just return 0?

grimar added inline comments.Sep 15 2016, 11:27 AM
ELF/LinkerScript.cpp
979–981

It is probably a bit strange to return 0 when we have enum. Also problem is that we use the code like that:

if (Cmd->SortInner)
  std::stable_sort(V.begin(), V.end(), getComparator(Cmd->SortInner));
if (Cmd->SortOuter)
  std::stable_sort(V.begin(), V.end(), getComparator(Cmd->SortOuter));

So in that case for this to work we will need to modify start value of enum:

enum SortKind { SortDisabled = 1, SortByName, SortByAlignment };

What is not very beautiful IMO. I`ll think more about that.

ruiu added inline comments.Sep 15 2016, 4:24 PM
ELF/LinkerScript.cpp
979–981

If you don't come up with a good name, I don't think = 1 is ugly.

grimar updated this revision to Diff 71619.Sep 16 2016, 5:12 AM
  • Addressed review comments.
ELF/LinkerScript.cpp
979–981

I tried this, but result was not very clean: D24655

But then during addressing Rafael's comments, anyways I found that probably anyways we want to have SortSectionPolicy that does not need to duplcate entries of SortKind enum and can be reused for linkerscript, so I ended with current code, I think it is looks much more clear.

1035

I think we read sort kind in readSortKind() above, by "applying" I was mean that we select/calculate/apply final value basing on rules. Renamed to selectSortKind.

rafael accepted this revision.Sep 16 2016, 5:24 AM
rafael edited edge metadata.

I like it. It adds a separation from parsing and deciding which sorting to actually use.

LGTM, but wait to see if Rui agrees.

This revision is now accepted and ready to land.Sep 16 2016, 5:24 AM
ruiu added inline comments.Sep 16 2016, 11:06 AM
ELF/LinkerScript.cpp
130–134

Convert this to a switch with a default statement with llvm_unreacahble, so that it reports an error if SortSectionPolicy is not one of Priority, Name nor Alignment.

988

It's hard to figure out the final output of this function. Can you describe it for me? I think it's hard to understand because you update Cmd and then fix its members in the second pass using this function. Why don't you set the proper values in the first place in one shot?

grimar added inline comments.Sep 16 2016, 11:30 AM
ELF/LinkerScript.cpp
988

In short: you have SORT type specified by --sort-section in Config->SortSection and types that are specified in script. Depending on if you have Outer/Inner rule set by script for sections it should either ignore either use or even mix with rules from --sort-section.
So final output is mixing commandline and script.

This SORT applying logic is described at https://sourceware.org/binutils/docs/ld/Input-Section-Wildcards.html page and it is not just that trivial,
so I think it is much better to split reading of the script and mixing sorting types, like this method do to keep logic simplier.

ruiu accepted this revision.Sep 16 2016, 12:20 PM
ruiu edited edge metadata.

LGTM. I have an idea to simplify but it is probably beyond being a code review comment, so please go ahead. I'll make a patch and send it to you.

In D24604#545191, @ruiu wrote:

LGTM. I have an idea to simplify but it is probably beyond being a code review comment, so please go ahead. I'll make a patch and send it to you.

Thanks ! That have a conflicts now, I am plan to resolve it and commit at my early tomorrow.

This revision was automatically updated to reflect the committed changes.
In D24604#545191, @ruiu wrote:

LGTM. I have an idea to simplify but it is probably beyond being a code review comment, so please go ahead. I'll make a patch and send it to you.

Thanks ! That have a conflicts now, I am plan to resolve it and commit at my early tomorrow.

Conflicts were trivial though, so r281771.