[PATCH] D22617: [ELF] - Linkerscript: add InputSectionDescription command to LS parser.

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/LinkerScript.cpp:220
@@ -211,4 +219,3 @@
 bool LinkerScript<ELFT>::shouldKeep(InputSectionBase<ELFT> *S) {
-  for (StringRef Pat : Opt.KeptSections)
-    if (globMatch(Pat, S->getSectionName()))
-      return true;
+  for (const std::unique_ptr<BaseCommand> &Base : Opt.Commands)
+    if (auto *OutSec = dyn_cast<OutputSectionCommand>(Base.get()))
----------------
ruiu wrote:
> evgeny777 wrote:
> > grimar wrote:
> > > evgeny777 wrote:
> > > > grimar wrote:
> > > > > evgeny777 wrote:
> > > > > > It looks like a slow-down compared with previous one, no?
> > > > > That fits in new design. I am sure we will spend time on optimization of LS later,
> > > > > when will be able to run benchmarks. And probably many places will need tweaking,
> > > > > for now I don't think it is critical to change until major slowdown is proven.
> > > > > 
> > > > As far as I understand if --gc-sections command line switch is set, then markLive() calls LinkerScript::shouldKeep() for every input section. There could easily be millions of input sections in large projects like WebKit or LLVM itself. It might not be a good idea to sacrifice performance that much in favor of design improvements.
> > > I think the most slow here is the call of globMatch. Amount of calls remained the same.
> > Well, the time complexity in your new code is O(I*R), I - number of sections, R - number of rules
> > Imagine you don't have any kept sections and don't call globMatch. Let's say you have 1 million sections and 10 rules total in script
> > Your for statements are actually for_each, at average you need about 20 instructions (with -O2 and 100 without optimization) for a single iteration (this is just 'for' statement without body).
> > Let's say you'll need 50 instructions for each rule (just assumption - can't tell the exact number), so
> > 
> > 1 000 000 * 10 * 50 = 500 000 000
> > On 2 Ghz processor and 1 clock cycle per instruction
> > 
> > t = (5 * 10^8) / (2*10^9) = 0.25 sec.
> > for doing nothing
> > 
> > Is it worth that?
> > 
> > 
> Yeah, I have a feeling that we should keep KeptSections mostly for simplicity. The reason why we want to construct an AST in a way that that reflects the structure of a given linker script is because we want to construct output sections in the same order as they appear in a linker script. On the other hand, "KEEP" seems to have been just piggybacking the grammar which they already had. It doesn't belong to this much deep data structure.
Reverted to KeptSections.

================
Comment at: ELF/LinkerScript.cpp:250
@@ -236,9 +249,3 @@
   // in script, so correct input section order is maintained by design.
-  for (SectionRule &R : Opt.Sections)
-    for (const std::unique_ptr<ObjectFile<ELFT>> &F :
-         Symtab<ELFT>::X->getObjectFiles())
-      for (InputSectionBase<ELFT> *S : F->getSections())
-        if (!isDiscarded(S) && !S->OutSec &&
-            globMatch(R.SectionPattern, S->getSectionName()))
-          // Add single input section to output section.
-          AddInputSec(S, R.Dest);
+  for (const std::unique_ptr<ObjectFile<ELFT>> &F :
+       Symtab<ELFT>::X->getObjectFiles())
----------------
ruiu wrote:
> I think you still want to iterate over SECTIONS subcommands. Eventually we want to create output sections and add input sections to them while we are visiting SECTIONS subcommands so that the results are naturally sorted by the same order as they appear in a linker script, so that we don't need to sort the results afterwards.
I see. Done. Looks a little bit more complicated now though.

================
Comment at: ELF/LinkerScript.cpp:745
@@ -731,1 +744,3 @@
     if (Tok == "*") {
+      InputSectionDescription *IS = new InputSectionDescription();
+      Cmd->Commands.emplace_back(IS);
----------------
ruiu wrote:
> Use `auto`.
Done.

================
Comment at: ELF/LinkerScript.cpp:754
@@ -738,6 +753,3 @@
       expect("(");
-      while (!Error && !skip(")")) {
-        StringRef Sec = next();
-        Opt.Sections.emplace_back(OutSec, Sec);
-        Opt.KeptSections.push_back(Sec);
-      }
+      InputSectionDescription *IS = new InputSectionDescription();
+      IS->Keep = true;
----------------
ruiu wrote:
> Ditto
Done.

================
Comment at: ELF/LinkerScript.h:45
@@ +44,3 @@
+  OutputSectionKind,
+  InputSectionKind
+};
----------------
evgeny777 wrote:
> See comment above:
> 
> > This enum represents what we can observe in SECTIONS tag of script.
> > (https://sourceware.org/binutils/docs/ld/SECTIONS.html#SECTIONS)
> 
> If you follow the link:
> 
> ```
> Each sections-command may of be one of the following:
> 
> - an ENTRY command (see Entry command)
> - a symbol assignment (see Assignments)
> - an output section description
> - an overlay description
> ```
> 
> 
> 
> 
Well, comment is depricated. I guess we do not want to treat SectionsCommandKind as written anymore, so it can be removed completely,
Rui, what do you think ?


https://reviews.llvm.org/D22617