[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:
> > 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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50133