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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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:
> 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.


https://reviews.llvm.org/D27415