[PATCH] D91127: [ELF] Use input order instead of pattern order within an input section description

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

> This patch also fixes 2 bugs:
> The second SORT in *(SORT(...) SORT(...)) is incorrectly parsed as a file pattern
> A section may be added multiple times if it multiple patterns match it

Can these bug fixes be separated to their own reviews (at least the first one seems can be independent)?



================
Comment at: lld/ELF/LinkerScript.cpp:435
+               [&](InputSectionBase *l, InputSectionBase *r) {
+                 return pos.find(l)->second < pos.find(r)->second;
+               });
----------------
I am not sure that doing 2 lookups in the DenseMap on each predicate call is a good idea.


================
Comment at: lld/ELF/LinkerScript.cpp:445
     for (InputSectionBase *sec : sections) {
-      if (!sec->isLive() || sec->parent)
+      if (!sec->isLive() || sec->parent || pos.find(sec)->second % 2)
         continue;
----------------
So, with it it is also possible to do lookup in pos twice:
one here and one with `pos[sec] |= 1;`

It looks a bit dirty. I think usually double lookup situation is avoided.


================
Comment at: lld/ELF/LinkerScript.cpp:468
+      // Prevent duplicate elements in ret.
+      pos[sec] |= 1;
     }
----------------
Looks a bit hacky approach to me.


================
Comment at: lld/ELF/LinkerScript.cpp:473
+      continue;
+    sortByPosition(sizeLastSort);
     sortInputSections(
----------------
You don't need to call `sortByPosition` here? It is always sorted I believe.


================
Comment at: lld/ELF/LinkerScript.cpp:484
   return ret;
 }
 
----------------
I've experimented with the implementation and got the following
version that addresses all my concerns/comments. What do you think?


```
// Compute and remember which sections the InputSectionDescription matches.
std::vector<InputSectionBase *>
LinkerScript::computeInputSections(const InputSectionDescription *cmd,
                                   ArrayRef<InputSectionBase *> sections) {
  std::vector<InputSectionBase *> ret;
  SetVector<size_t> indices; // Parallel to ret.

  // Collects all sections that satisfy constraints of Cmd.
  size_t sizeLastSort = 0;
  for (const SectionPattern &pat : cmd->sectionPatterns) {
    size_t sizeBefore = ret.size();

    for (size_t I = 0, E = sections.size(); I != E; ++I) {
      InputSectionBase *sec = sections[I];
      if (!sec->isLive() || sec->parent || indices.contains(I))
        continue;

      // For -emit-relocs we have to ignore entries like
      //   .rela.dyn : { *(.rela.data) }
      // which are common because they are in the default bfd script.
      // We do not ignore SHT_REL[A] linker-synthesized sections here because
      // want to support scripts that do custom layout for them.
      if (isa<InputSection>(sec) &&
          cast<InputSection>(sec)->getRelocatedSection())
        continue;

      // Check the name early to improve performance in the common case.
      if (!pat.sectionPat.match(sec->name))
        continue;

      if (!cmd->matchesFile(sec->file) || pat.excludesFile(sec->file) ||
          (sec->flags & cmd->withFlags) != cmd->withFlags ||
          (sec->flags & cmd->withoutFlags) != 0)
        continue;

      indices.insert(I);
      ret.push_back(sec);
    }

    if (pat.sortOuter == SortSectionPolicy::Default)
      continue;

    sortInputSections(
        MutableArrayRef<InputSectionBase *>(ret).slice(sizeBefore),
        pat.sortOuter, pat.sortInner);
    sizeLastSort = ret.size();
  }

  std::vector<size_t> indicesV = indices.takeVector();
  llvm::sort(MutableArrayRef<size_t>(indicesV).slice(sizeLastSort),
             [&](size_t l, size_t r) { return l < r; });

  for (size_t I = sizeLastSort, E = ret.size(); I != E; ++I)
    ret[I] = sections[indicesV[I]];

  sortInputSections(
      MutableArrayRef<InputSectionBase *>(ret).slice(sizeLastSort),
      config->sortSection, SortSectionPolicy::None);
  return ret;
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91127/new/

https://reviews.llvm.org/D91127