[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:
> > > 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.
I do not think it is a bug. This was implemented in r282444 by Rafael.
It is fine that non-alloc debug sections can point to non-live allocatable sections I think.


https://reviews.llvm.org/D27415