[PATCH] D27415: [ELF] - Replace MergeOutputSection with synthetic input section MergeSection.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/InputSection.cpp:135-136
+  case Merge: {
+    const MergeInputSection<ELFT> *MS = cast<MergeInputSection<ELFT>>(this);
+    return MS->MergeSec ? MS->MergeSec->OutSecOff + MS->getOffset(Offset) : 0;
+  }
----------------
grimar wrote:
> ruiu wrote:
> > Is there a reason to return 0? If you shouldn't call this function until an output section is set, you want to add an assert instead of returning a fake value.
> Reason is that this lines can be called when doing rellocateNonAlloc. 
> When a merge section was only referenced from
> non-alloca sections. Like gc-sections-non-alloc-to-merge.s test shows.
> 
> In this case MS was not Live so could not have section MergeSec.
> getOffset by the way contains inside the next code to handle this:
> ```
>   if (!this->Live)
>     return 0;
> ```
Do you mean that sections which is not marked as live are copied to the result? If so, that sounds like a bug. Anyways, adding this obscure code is not a good thing.


================
Comment at: ELF/InputSection.h:218-220
+  // Mergeable input sections with different attributes are placed to
+  // synthetic sections which are parts of regular output section.
+  // That way we can emit a single output section at the end.
----------------
This comment needs refinement. So, MergeInputSections are aggregated to a synthetic input section, MergeSections, and then added to an OutputSection. This pointer points to a MergeSection which this section belongs to.


================
Comment at: ELF/LinkerScript.cpp:351
 
+      combineMergableSections(V);
+
----------------
Why do you have to do this in LinkerScript.cpp? It feels something is not right. Section aggregation should happen before the control is passed to the linker script.


================
Comment at: ELF/LinkerScript.cpp:366
+      std::vector<InputSectionBase<ELFT> *> V = { S };
+      combineMergableSections(V);
+      addSection(Factory, V[0], getOutputSectionName(S->Name));
----------------
This is odd too.


================
Comment at: ELF/OutputSections.cpp:139
+
+  // If we add mergable input section we might want to create or
+  // use existing synthetic MergeSection for each input section group.
----------------
grimar wrote:
> grimar wrote:
> > ruiu wrote:
> > > This is not a right place to add this code. You want to do outside of this function. Instead of hiding this logic inside `addSection`, make a new function that scans `Symtab<ELFT>::X->Sections` vector to merge mergeable input sections into synthetic mergeable sections.
> > This can work for non-script case, but how would you handle linkerscript then ?
> > 
> > This patch already relies on fact that script calls addSection() with merge input sections.
> > It still creates multiple output sections for different attributes because script has own createKey(),
> > but OutputSection::addSection is responsible for merging all what I give it.
> > 
> > I tried to do the suggested change and need to fix script logic too because
> > it still passes MergeInputSections.
> > 
> > Script code for creating the list of sections is next:
> > ```
> >       std::vector<InputSectionBase<ELFT> *> V = createInputSectionList(*Cmd);
> > 
> >       // The output section name `/DISCARD/' is special.
> >       // Any input section assigned to it is discarded.
> >       if (Cmd->Name == "/DISCARD/") {
> >         discard(V);
> >         continue;
> >       }
> > 
> >       // This is for ONLY_IF_RO and ONLY_IF_RW. An output section directive
> >       // ".foo : ONLY_IF_R[OW] { ... }" is handled only if all member input
> >       // sections satisfy a given constraint. If not, a directive is handled
> >       // as if it wasn't present from the beginning.
> >       //
> >       // Because we'll iterate over Commands many more times, the easiest
> >       // way to "make it as if it wasn't present" is to just remove it.
> >       if (!matchConstraints<ELFT>(V, Cmd->Constraint)) {
> >         for (InputSectionBase<ELFT> *S : V)
> >           S->Assigned = false;
> >         Opt.Commands.erase(Iter);
> >         --I;
> >         continue;
> >       }
> > ```
> > 
> > So looks after the last block of code I can take V, create synthetic mergeable sections for sections in V if any
> > and update V and Symtab<ELFT>::X->Sections. 
> > 
> > There is no way to do that once outside like for non-scripted case. And that will complicate the logic.
> > Do you think it is correct direction ?
> So generally I do not see benefits from doing that outside of OutputSection.
> Logic of "scan all sections, create new synthetic sections, remove regular mergable sections from array and do something more on a linker script side" looks more complicated that do everything incapsulated in OutputSection in a short method.
I disagree. This patch adds small piece of code to many different places, and in total it is not clear how mergeable sections are handled. What we want to do to mergeable sections is, well, merging. Just merge them at one place and rest should be processed normally.


================
Comment at: ELF/OutputSections.h:130
 private:
-  void finalizeTailMerge();
-  void finalizeNoTailMerge();
-
-  llvm::StringTableBuilder Builder;
-  std::vector<MergeInputSection<ELFT> *> Sections;
+  std::vector<MergeSection<ELFT> *> MergeSections;
 };
----------------
You don't need this, no?


https://reviews.llvm.org/D27415