[PATCH] D50133: [LLD] Do not overwrite LMAOffset of PT_LOAD header

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/LinkerScript.cpp:780
+    if ((Sec == L->FirstSec) ||
+        ((L->FirstSec == Out::ElfHeader) && (L->LastSec == Sec)))
+      L->LMAOffset = Ctx->LMAOffset;
----------------
kschwarz wrote:
> grimar wrote:
> > kschwarz wrote:
> > > grimar wrote:
> > > > kschwarz wrote:
> > > > > grimar wrote:
> > > > > > kschwarz wrote:
> > > > > > > grimar wrote:
> > > > > > > > I think the comment above should be updated to reflect this change.
> > > > > > > > 
> > > > > > > > But also: your test pass if I reduce this to
> > > > > > > > 
> > > > > > > > ```
> > > > > > > >   if (PhdrEntry *L = Ctx->OutSec->PtLoad)
> > > > > > > >     if (Sec == L->FirstSec)
> > > > > > > >       L->LMAOffset = Ctx->LMAOffset;
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > Can you add a test/update it accordingly to check the rest?
> > > > > > > > (Alternatively, we can go with the reduced code in this patch I think, and leave the rest to follow up patch).
> > > > > > > > 
> > > > > > > > Also, I think that `(L->FirstSec == Out::ElfHeader)` condition is not correct. Because
> > > > > > > > `FirstSec` can be not the `ElfHeader`, but `ProgramHeaders`:
> > > > > > > > https://github.com/llvm-mirror/lld/blob/master/ELF/LinkerScript.cpp#L1073
> > > > > > > > 
> > > > > > > > 
> > > > > > > The following tests rely on the additional check and fail if it is removed:
> > > > > > >     lld :: ELF/linkerscript/at-addr.s
> > > > > > >     lld :: ELF/linkerscript/at.s
> > > > > > >     lld :: ELF/linkerscript/loadaddr.s
> > > > > > >     lld :: ELF/linkerscript/merge-header-load.s
> > > > > > >     lld :: ELF/linkerscript/overlay.test
> > > > > > >     lld :: ELF/linkerscript/segment-headers.s
> > > > > > >     lld :: ELF/note-loadaddr.s
> > > > > > > 
> > > > > > > The ELF header section is added to the first PT_LOAD header unconditionally here: https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1807
> > > > > > > 
> > > > > > > I think we need to check for both ElfHeader and ProgramHeaders and will try to come up with an additional test case for that.
> > > > > > Ah I see, I did not check the other tests.
> > > > > > 
> > > > > > >> I think we need to check for both ElfHeader and ProgramHeaders and will try to come up with an additional test case for that.
> > > > > > 
> > > > > > Yes. I think we might need some little helper function so that it can be reused in places like
> > > > > > https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1824
> > > > > > 
> > > > > > >> The ELF header section is added to the first PT_LOAD header unconditionally here: https://github.com/llvm-mirror/lld/blob/master/ELF/Writer.cpp#L1807
> > > > > > 
> > > > > > Yeah, that is for the case when PHDRS command is not used in the linker script. But if it is used, it is not added unconditionally then.
> > > > > I don't think the `createPhdrs()` functions in Writer.cpp and Linkerscript.cpp share a lot of common functionality. What kind of helper function do you have in mind?
> > > > > 
> > > > > With the PHDRS linker script command, the user is in charge of reasonably adding output sections to PT_LOAD segments.
> > > > > It is possible to create an PT_LOAD header which contains the ProgramHeaders section and multiple other output sections. 
> > > > > Currently we only store the first and last output section of each program header, thus we cannot easily find the first output section after the ELF file header (without iterating over all the section commands once again). A check like `(L->FirstSec == Out::ProgramHeaders) && (L->LastSec == Sec)` would fail in this case. Is there a better way to find the first section behind elf/program header?
> > > > I was thinking about something very trivial. Like:
> > > > 
> > > > ```
> > > > static bool isHeaderSection(InputSection *IS) {
> > > >   return IS == Out::ElfHeader || IS == Out::ProgramHeaders;
> > > > }
> > > > 
> > > > ```
> > > > 
> > > > So you could perhaps write:
> > > > 
> > > > ```
> > > >   if (PhdrEntry *L = Ctx->OutSec->PtLoad)
> > > >     if ((Sec == L->FirstSec) || (isHeaderSection(L->FirstSec) && (L->LastSec == Sec)))
> > > > ```
> > > > 
> > > > Does it make sense?
> > > > 
> > > It would certainly help the readability of the code. I can add that function.
> > > 
> > > However, this case `(FirstSec == Out::ProgramHeaders)` can only occur if the PHDRS command is used, and for that case this patch does not provide a solution (see my comment above). Do you have an idea for that problem?
> > I only can think of helper function that would iterate over `OutputSections` to find the first non-header section for the PT_LOAD given. This should solve the problem, right?
> Yes, that should solve the problem for PHDRS command.
> 
> Would you agree to have that in a follow-up patch or should it already go into this one?
I would perhaps include it into this one because adding the helper discussed looks like a correct way to go forward here.

I think you can leave the test case as is though (since we already have a number of the tests failing without the new code).


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50133