[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:
> > > > 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.
I think that my point is, if a section is written to output, it should be marked as "live" whatever its type is. If non-alloc sections are copied to output unconditionally, they should be marked as "live" by default from the beginning.

Doing something "non-live sections are sometimes actually live under some condition, so handle that special case here" is not a good idea.


https://reviews.llvm.org/D27415