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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/InputSection.cpp:88
+  if (!Config->Relocatable && !isa<MergeInputSection<ELFT>>(this) &&
+      !isa<MergeSection<ELFT>>(this))
     this->Flags &= ~(SHF_MERGE | SHF_STRINGS);
----------------
ruiu wrote:
> How does this new condition happen?
I am creating MergeSection:
```
MergeSection<ELFT> *Syn = 
  make<MergeSection<ELFT>>(MS->Name, MS->Type, Flags, Alignment);
```

And should not drop flags for it, because otherwise next condition works wrong later:
```
template <class ELFT> bool MergeSection<ELFT>::shouldTailMerge() const {
  return (this->Flags & SHF_STRINGS) && Config->Optimize >= 2;
}
```



================
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;
+  }
----------------
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;
```


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


================
Comment at: ELF/OutputSections.h:75
+  // not hold a table of fixed-size entries.
+  void updateEntsize(uint64_t Size) {
+    if (!Size || Entsize == (uint64_t)-1)
----------------
ruiu wrote:
> I do not understand why we need to care about sh_entsize of our output files. Why can't you just leave it 0?
Well, that is actually the question I wish to find answer and mentioned in description of patch.
We set sh_entsize and checked it in testcases before, when each different mergeable section was a single output.
If we can ignore it and set to 0, that makes patch a bit easier. Looking on gold output which is inconsistent,
looks that value makes no sence (if not -r elocatable).


https://reviews.llvm.org/D27415