[PATCH] D31888: [LLD][ELF] Always use Script::assignAddresses()

[bd1976 llvm via llvm-commits]

Hi Rafael,

Thanks for dong this work. I think this is an improvement over the previous
situation.

I have tried to grok the code in order to do an informed review.
Unfortunately, I still do not have a comprehensive knowledge of the LLD
codebase so please excuse any questions asked in ignorance :)

My comments are:

If there is no space for the headers then allocateHeaders removes the
Out::ElfHeader section from the first PT_LOAD. However, you don't
explicitly remove the Out::ProgramHeaders section which was also added to
the same PT_LOAD (in createPhdrs). Is this a mistake or am I not
understanding something here?

In general, I dislike the idea of adding and subsequently possibly removing
PHDRS/sections from PHDRS. I think that an immutable PHDRS/program header
table or at least data structures that are "add only" would make for a
codebase that is easier to reason about. I don't understand why we need to
create the PHDRS and add the headers to them so early on. Is it possible to
wait until after assignAddresses has run and then create the required elf
headers and PHDRS at that point?

In LLD if there is not enough VA space for the elf headers then these are
not allocated and the PT_PHDR is removed. This is not behaviour I was
expecting. I was going to suggest that it would simplify the code to just
error here rather than have this behaviour. However, I saw that you added
this behaviour in r282750 to match bfd linker behaviour. Now that linker
script support is more mature, maybe we could go back to emitting an error
and tell the user to use a linker script if they want more control over the
elf header placement?

There appear to be at least a couple of people maintaining downstream tools
that would benefit from support for having elf headers that are not
allocated. Would you object to a patch to lld that allowed this behaviour?
It would need to be controlled by a new linker switch.

Sorry for the delay in looking at this!

On Thu, May 4, 2017 at 8:48 PM, Rafael Avila de Espindola via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> ben via Phabricator <reviews at reviews.llvm.org> writes:
>
> > The question I am struggling with is: Why is allocateHeaders called
> twice? Could the function fixHeaders() be removed?
>
> I think I managed to fix this in r302186. Let me know if that was what
> you had in mind.
>
> Cheers,
> Rafael
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170512/ade08bb8/attachment.html>