[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:135-136
+  case Merge: {
+    const MergeInputSection<ELFT> *MS = cast<MergeInputSection<ELFT>>(this);
+    return MS->MergeSec ? MS->MergeSec->OutSecOff + MS->getOffset(Offset) : 0;
+  }
----------------
ruiu wrote:
> 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.
No. I mean that non-allocatable sections can have relocations against sections that are not LIve.
That happens for .debug sections often I thing.
Just please look at the gc-sections-alloc.s. or gc-sections-non-alloc-to-merge.s.

And actually we have similar pieces of code around to handle that, examples:

```
static typename ELFT::uint getSymVA(const SymbolBody &Body,
                                    typename ELFT::uint &Addend) {
..
uintX_t VA = (SC->OutSec ? SC->OutSec->Addr : 0) + SC->getOffset(Offset);
```

```
typename ELFT::uint MergeInputSection<ELFT>::getOffset(uintX_t Offset) const {
...
  if (!this->Live)
    return 0;
```

I changed that place to be more obious.



================
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.
----------------
ruiu wrote:
> 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.
Done.


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


https://reviews.llvm.org/D27415