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

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

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 ?

1011

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

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 !

ELF/Config.h

 
 // For --strip-{all,debug}.
 enum class StripPolicy { None, All, Debug };
 
 // For --unresolved-symbols.
 enum class UnresolvedPolicy { NoUndef, ReportError, Warn, Ignore };
 
 // For --sort-section and linkerscript sorting rules.
-enum class SortSectionPolicy { None, IgnoreConfig, Alignment, Name, Priority };
+enum class SortSectionPolicy { Default, None, Alignment, Name, Priority };
 
 struct SymbolVersion {
   llvm::StringRef Name;
   bool IsExternCpp;
   bool HasWildcards;
 };
 
 // This struct contains symbols version definition that

ELF/Driver.cpp

 static SortSectionPolicy getSortKind(opt::InputArgList &Args) {
   StringRef S = getString(Args, OPT_sort_section);
   if (S == "alignment")
     return SortSectionPolicy::Alignment;
   if (S == "name")
     return SortSectionPolicy::Name;
   if (!S.empty())
     error("unknown --sort-section rule: " + S);
-  return SortSectionPolicy::None;
+  return SortSectionPolicy::Default;
 }
 
 // Initializes Config members by the command line options.
 void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   for (auto *Arg : Args.filtered(OPT_L))
     Config->SearchPaths.push_back(Arg->getValue());
 
   std::vector<StringRef> RPaths;

ELF/LinkerScript.h

 };
 
 struct InputSectionDescription : BaseCommand {
   InputSectionDescription(StringRef FilePattern)
       : BaseCommand(InputSectionKind),
         FileRe(compileGlobPatterns({FilePattern})) {}
   static bool classof(const BaseCommand *C);
   llvm::Regex FileRe;
-  SortSectionPolicy SortOuter = SortSectionPolicy::None;
-  SortSectionPolicy SortInner = SortSectionPolicy::None;
+  SortSectionPolicy SortOuter = SortSectionPolicy::Default;
+  SortSectionPolicy SortInner = SortSectionPolicy::Default;
   // Pairs of section regex and files excluded.
   std::list<std::pair<llvm::Regex, llvm::Regex>> SectionsVec;
   std::vector<InputSectionData *> Sections;
 };
 
 struct AssertCommand : BaseCommand {
   AssertCommand(Expr E) : BaseCommand(AssertKind), Expression(E) {}
   static bool classof(const BaseCommand *C);

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);
   });
 }
 
+void sortSections(std::vector<InputSectionData *> &Sections,
+                  SortSectionPolicy K) {
+  if (K != SortSectionPolicy::Default)
+    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) {
   for (const std::pair<llvm::Regex, llvm::Regex> &V : I->SectionsVec) {
     for (ObjectFile<ELFT> *F : Symtab<ELFT>::X->getObjectFiles()) {
       if (fileMatches(I->FileRe, V.first, sys::path::filename(F->getName()))) {
         Regex &Re = const_cast<Regex &>(V.second);
         for (InputSectionBase<ELFT> *S : F->getSections())
           if (!isDiscarded(S) && !S->OutSec && Re.match(S->Name))
             I->Sections.push_back(S);
 
         if (Re.match("COMMON"))
           I->Sections.push_back(CommonInputSection<ELFT>::X);
       }
     }
   }
 
-  if (I->SortInner != SortSectionPolicy::None)
-    std::stable_sort(I->Sections.begin(), I->Sections.end(),
-                     getComparator(I->SortInner));
-  if (I->SortOuter != SortSectionPolicy::None)
-    std::stable_sort(I->Sections.begin(), I->Sections.end(),
-                     getComparator(I->SortOuter));
+  // Sort sections by --sort-section and SORT_*() commands.
+  // We do not sort it at all if it was surrounded by SORT_NONE().
+  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;
   }
 }

 
 SortSectionPolicy ScriptParser::readSortKind() {
   if (skip("SORT") || skip("SORT_BY_NAME"))
     return SortSectionPolicy::Name;
   if (skip("SORT_BY_ALIGNMENT"))
     return SortSectionPolicy::Alignment;
   if (skip("SORT_BY_INIT_PRIORITY"))
     return SortSectionPolicy::Priority;
-  // `SORT_NONE' disables section sorting by ignoring the command line
-  // section sorting option.
   if (skip("SORT_NONE"))
-    return SortSectionPolicy::IgnoreConfig;
     return SortSectionPolicy::None;
-}
-
-static void selectSortKind(InputSectionDescription *Cmd) {
-  if (Cmd->SortOuter == SortSectionPolicy::IgnoreConfig) {
-    Cmd->SortOuter = SortSectionPolicy::None;
-    return;
-  }
-
-  if (Cmd->SortOuter != SortSectionPolicy::None) {
-    // If the section sorting command in linker script is nested, the command
-    // line option will be ignored.
-    if (Cmd->SortInner != SortSectionPolicy::None)
-      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;
+  return SortSectionPolicy::Default;
 }
 
 // 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)
 void ScriptParser::readSectionExcludes(InputSectionDescription *Cmd) {
   llvm::Regex ExcludeFileRe;
   std::vector<StringRef> V;

 
 InputSectionDescription *
 ScriptParser::readInputSectionRules(StringRef FilePattern) {
   auto *Cmd = new InputSectionDescription(FilePattern);
   expect("(");
 
   // Read SORT().
   SortSectionPolicy K1 = readSortKind();
-  if (K1 != SortSectionPolicy::None) {
+  if (K1 != SortSectionPolicy::Default) {
     Cmd->SortOuter = K1;
     expect("(");
     SortSectionPolicy K2 = readSortKind();
-    if (K2 != SortSectionPolicy::None) {
+    if (K2 != SortSectionPolicy::Default) {
       Cmd->SortInner = K2;
       expect("(");
       Cmd->SectionsVec.push_back({llvm::Regex(), readFilePatterns()});
       expect(")");
     } else {
       Cmd->SectionsVec.push_back({llvm::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