Simplify SORT and --sort-section command line option handling.

[ruiu]

Summary

Simplify SORT and --sort-section command line option handling.

[ruiu]

• update comments

[ruiu]

Rebased.

[grimar]

My comments below, every of them is just an IMHO of course.

ELF/LinkerScript.cpp

169 ↗

(On Diff #71700)

So you can call sortSections() or not call it, and function itself can
do sort or not, while it's name contains "sort" explicitly.

I understand why you had to do all that, but it is much
more confusing than was for me.

189 ↗

(On Diff #71700)

This comment says nothing about how rules are applied,
selectSortKind() had many of them, though I think it could leave fully without,
as SortSectionPolicy had very clear constant names.

I can't say the same for this code though.
I thought you wanted to have more comments generally,
if they helps to avoid reading code too much.
Do you find them excessive in your variant ?

1012 ↗

(On Diff #71700)

By the way this place did not need the comment before renaming IgnoreConfig to None, I think code on the left side (if take version before renaming) is much more readable without need of in-mind-debugging.

[• rafael]

I do like this new one better, but I think it needs a few more comments.

In particular, mention that the command line replaces the inner sort if not set explicitly.

BTW, this and the previous code would crash if SORT_NONE was the inner sort, no?

ELF/LinkerScript.cpp

165 ↗

(On Diff #71700)

static

[ruiu]

• Update comments
• Fix a bug if a SORT_NONE is used as an inner sort directive.

[ruiu]

George, I think this is better as it directly works on data structure we'd like to sort, rather than creating directives, fix it in the second pass, and then apply them. I updated the comment to describe how it works. Don't you still dislike this?

[grimar]

It looks much better when there are comments like that near. Thanks !

lld/trunk/ELF/LinkerScript.cpp

static bool matchConstraints(ArrayRef<InputSectionBase<ELFT> *> Sections,
   if (Kind == ConstraintKind::NoConstraint)
     return true;
   return llvm::all_of(Sections, [=](InputSectionData *Sec2) {
     auto *Sec = static_cast<InputSectionBase<ELFT> *>(Sec2);
     return checkConstraint(Sec->getSectionHdr()->sh_flags, Kind);
   });
 }
 
+static void sortSections(std::vector<InputSectionData *> &Sections,
+                         SortSectionPolicy K) {
+  if (K != SortSectionPolicy::Default && K != SortSectionPolicy::None)
+    std::stable_sort(Sections.begin(), Sections.end(), getComparator(K));
+}
+
 // Compute and remember which sections the InputSectionDescription matches.
 template <class ELFT>
 void LinkerScript<ELFT>::computeInputSections(InputSectionDescription *I) {
   // Collects all sections that satisfy constraints of I
   // and attach them to I.
   for (SectionPattern &Pat : I->SectionPatterns) {
     for (ObjectFile<ELFT> *F : Symtab<ELFT>::X->getObjectFiles()) {
       StringRef Filename = sys::path::filename(F->getName());
       if (!I->FileRe.match(Filename) || Pat.ExcludedFileRe.match(Filename))
         continue;
 
       for (InputSectionBase<ELFT> *S : F->getSections())
         if (!isDiscarded(S) && !S->OutSec && Pat.SectionRe.match(S->Name))
           I->Sections.push_back(S);
       if (Pat.SectionRe.match("COMMON"))
         I->Sections.push_back(CommonInputSection<ELFT>::X);
     }
   }
 
-  // Sort for SORT() commands.
-  if (I->SortInner != SortSectionPolicy::Default)
-    std::stable_sort(I->Sections.begin(), I->Sections.end(),
-                     getComparator(I->SortInner));
-  if (I->SortOuter != SortSectionPolicy::Default)
-    std::stable_sort(I->Sections.begin(), I->Sections.end(),
-                     getComparator(I->SortOuter));
+  // Sort sections as instructed by SORT-family commands and --sort-section
+  // option. Because SORT-family commands can be nested at most two depth
+  // (e.g. SORT_BY_NAME(SORT_BY_ALIGNMENT(.text.*))) and because the command
+  // line option is respected even if a SORT command is given, the exact
+  // behavior we have here is a bit complicated. Here are the rules.
+  //
+  // 1. If two SORT commands are given, --sort-section is ignored.
+  // 2. If one SORT command is given, and if it is not SORT_NONE,
+  //    --sort-section is handled as an inner SORT command.
+  // 3. If one SORT command is given, and if it is SORT_NONE, don't sort.
+  // 4. If no SORT command is given, sort according to --sort-section.
+  if (I->SortOuter != SortSectionPolicy::None) {
+    if (I->SortInner == SortSectionPolicy::Default)
+      sortSections(I->Sections, Config->SortSection);
+    else
+      sortSections(I->Sections, I->SortInner);
+    sortSections(I->Sections, I->SortOuter);
+  }
 
   // We do not add duplicate input sections, so mark them with a dummy output
   // section for now.
   for (InputSectionData *S : I->Sections) {
     auto *S2 = static_cast<InputSectionBase<ELFT> *>(S);
     S2->OutSec = (OutputSectionBase<ELFT> *)-1;
   }
 }
if (skip("SORT_BY_ALIGNMENT"))
     return SortSectionPolicy::Alignment;
   if (skip("SORT_BY_INIT_PRIORITY"))
     return SortSectionPolicy::Priority;
   if (skip("SORT_NONE"))
     return SortSectionPolicy::None;
   return SortSectionPolicy::Default;
 }
 
-static void selectSortKind(InputSectionDescription *Cmd) {
-  if (Cmd->SortOuter == SortSectionPolicy::None) {
-    Cmd->SortOuter = SortSectionPolicy::Default;
-    return;
-  }
-
-  if (Cmd->SortOuter != SortSectionPolicy::Default) {
-    // If the section sorting command in linker script is nested, the command
-    // line option will be ignored.
-    if (Cmd->SortInner != SortSectionPolicy::Default)
-      return;
-    // 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.
-    Cmd->SortInner = Config->SortSection;
-    return;
-  }
-  // If sorting rule not specified, use command line option.
-  Cmd->SortOuter = Config->SortSection;
-}
-
 // Method reads a list of sequence of excluded files and section globs given in
 // a following form: ((EXCLUDE_FILE(file_pattern+))? section_pattern+)+
 // Example: *(.foo.1 EXCLUDE_FILE (*a.o) .foo.2 EXCLUDE_FILE (*b.o) .foo.3)
 // The semantics of that is next:
 // * Include .foo.1 from every file.
 // * Include .foo.2 from every file but a.o
 // * Include .foo.3 from every file but b.o
 void ScriptParser::readSectionExcludes(InputSectionDescription *Cmd) {
if (K2 != SortSectionPolicy::Default) {
       Cmd->SortInner = K2;
       expect("(");
       Cmd->SectionPatterns.push_back({Regex(), readFilePatterns()});
       expect(")");
     } else {
       Cmd->SectionPatterns.push_back({Regex(), readFilePatterns()});
     }
     expect(")");
-    selectSortKind(Cmd);
     return Cmd;
   }
 
-  selectSortKind(Cmd);
   readSectionExcludes(Cmd);
   return Cmd;
 }
 
 InputSectionDescription *
 ScriptParser::readInputSectionDescription(StringRef Tok) {
   // Input section wildcard can be surrounded by KEEP.
   // https://sourceware.org/binutils/docs/ld/Input-Section-Keep.html#Input-Section-Keep

lld/trunk/test/ELF/linkerscript/sort.s

 # RUN: ld.lld --sort-section name -o %t9 --script %t8.script %t1.o %t2.o
 # RUN: llvm-objdump -s %t9 | FileCheck -check-prefix=SORTED_B %s
 
 ## SORT_NONE disables the --sort-section.
 # RUN: echo "SECTIONS { .aaa : { *(SORT_NONE(.aaa.*)) } }" > %t9.script
 # RUN: ld.lld --sort-section name -o %t10 --script %t9.script %t2.o %t1.o
 # RUN: llvm-objdump -s %t10 | FileCheck -check-prefix=UNSORTED %s
 
+## SORT_NONE as a inner sort directive.
+# RUN: echo "SECTIONS { .aaa : { *(SORT_BY_NAME(SORT_NONE(.aaa.*))) } }" > %t10.script
+# RUN: ld.lld -o %t11 --script %t10.script %t2.o %t1.o
+# RUN: llvm-objdump -s %t11 | FileCheck -check-prefix=SORTED_A %s
 
 .global _start
 _start:
  nop
 
 .section .aaa.5, "a"
 .align 32
 .quad 5