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

[Konstantin Schwarz via Phabricator via llvm-commits]

kschwarz added inline comments.


================
Comment at: ELF/LinkerScript.cpp:780
+    if ((Sec == L->FirstSec) ||
+        ((L->FirstSec == Out::ElfHeader) && (L->LastSec == Sec)))
+      L->LMAOffset = Ctx->LMAOffset;
----------------
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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D50133