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

[Rafael Avila de Espindola via llvm-commits]

bd1976 llvm <bd1976llvm at gmail.com> writes:

> 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?

The representation of a PT_LOAD is the first and last section. So
changing the first from ElfHeader to a regular section also removes ProgramHeaders.


> 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?

We need to either be "pessimistic" or iterate. If we start without
allocating the headers and then find out that there is space, we will
add a PT_PHDR, making the headers bigger and will have to assign
addresses again.

> 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?

Possibly. I can't remember which script wanted a section at address 0.

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

I don't think such an option would be too invasive. If that is the case,
I would not object to it.

BTW, what is the use case you have for not allocating the header?

Cheers,
Rafael