[PATCH] D50133: [LLD] Do not overwrite LMAOffset of PT_LOAD header
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 1 03:44:54 PDT 2018
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
More information about the llvm-commits
mailing list