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

[Rui Ueyama via llvm-commits]

ruiu 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()))
----------------
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.

================
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())
----------------
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.

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

================
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;
----------------
Ditto


https://reviews.llvm.org/D22617