[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:
> > 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.
> 
But that is a bug of LLD, no? The invariant is that all relocations from live sections points to  live sections. I think handling non-live sections like this is wrong -- you should fix the bug that live sections are marked as non-live instead.


https://reviews.llvm.org/D27415