[PATCH] D19976: [ELF, WIP] - Prototype of possible linkerscript redesign.

[Eugene Leviant via llvm-commits]

evgeny777 added a comment.

Though, I'm quite new to lld development, I think this code can be simplified. Here is my version of createSections()
It simply iterates over SectionRule array, selects input sections matching each rule, and adds them output section

  template <class ELFT>
  OutputSectionList LinkerScript<ELFT>::createSections(OutputSectionFactory<ELFT> &Factory) {
     OutputSectionList Result;
    typedef std::unique_ptr<elf::ObjectFile<ELFT>> ObjectFileUP;
  
    // Select input sections matching rule and add them to corresponding 
    // output section. Section rules are processed in order they're listed
    // in script, so correct input section order is maintained by design
    for (SectionRule &R : Opt.Sections) {
      for (const ObjectFileUP &F : Symtab<ELFT>::X->getObjectFiles()) {
        for (InputSectionBase<ELFT> *S : F->getSections()) {
            if (!elf::isDiscarded(S) && globMatch(R.SectionPattern, S->getSectionName())) 
               // Adds input section to output section collection
               addInputSection(Factory, S, R.Dest, Result);
         }
      }
    }
    // Add all other input sections, which are not listed in script
    for (const ObjectFileUP &F : Symtab<ELFT>::X->getObjectFiles()) {
      for (InputSectionBase<ELFT> *C : F->getSections()) {
        if (C->OutSec == nullptr)
          addInputSection(Factory, C, getOutputSectionName(C), Result);
      }
    }
    return Result;
  }

Then in Writer<ELFT> I'm dispatching to correct version of createSections(), just like you're doing.

It's several times shorter and passes all unit tests.
I might be missing some important strategic move, which is introduced by your patch, so let's wait and see what others will say.


================
Comment at: ELF/LinkerScript.cpp:199
@@ -196,4 +198,3 @@
 
-template <class ELFT>
-bool LinkerScript<ELFT>::isDiscarded(InputSectionBase<ELFT> *S) {
-  return getOutputSection(S) == "/DISCARD/";
+size_t LayoutParser::createLayout() {
+  if (!Tokens.size())
----------------
I wonder, if we can live without script parser refactoring for the very first iteration.
May be this can be done as a separate review?

================
Comment at: ELF/LinkerScript.cpp:315
@@ +314,3 @@
+
+    if (Cmd.Kind != InputSectionKind)
+      continue;
----------------
I wonder why do you now mix SectionsRule (which is now InputSectionKind) and SectionsCommand (now OutputSectionKind) into one Command object? Every time you iterate Opt.LayoutParser->Commands, you need either rules or commands, never both. May be it makes sense to have two separate collections, like we had before?

================
Comment at: ELF/LinkerScript.cpp:350
@@ +349,3 @@
+  auto Assign = [&](StringRef Name) {
+    OutputSectionBase<ELFT> *Sec = findSection<ELFT>(Sections, Name);
+
----------------
You seem to ignore comment in original version of assignAddresses:

```
// Find all the sections with required name. There can be more than
// ont section with such name, if the alignment, flags or type
// attribute differs.
```



================
Comment at: ELF/LinkerScript.cpp:386
@@ +385,3 @@
+    if (!Assigned.count(Sec)) {
+      Assigned.insert(Sec);
+      Assign(Sec->getName());
----------------
Do we need to insert to Assigned here? Looks like, we're doing the final processing step

================
Comment at: ELF/LinkerScript.cpp:396
@@ +395,3 @@
+  for (const Command &Cmd : Opt.LayoutParser->Commands) {
+    if (Cmd.Kind != InputSectionKind)
+      continue;
----------------
See my previous comment about mixing section rules and commands.


https://reviews.llvm.org/D19976