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

[bd1976 llvm via llvm-commits]

Thanks for the detailed response.

On Fri, May 12, 2017 at 3:45 PM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

> 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.
>
>
I was more thinking that we don't need to create the PHDRs early and then
edit them. We only need to know the size of the program header table early
- we can create the actual table later when writing out the elf?

In terms of being pessimistic or iterating, 2 solutions:
- Force the user to be explicit about whether they want the headers
allocated or not.
- Iterate on the layout. Is iterating on the layout a problem? It seems
that adding thunk support will require iteration.


> > 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.
>
>
No problem. The comment for r282750 implies that you didn't have any
example scripts to hand at the time either. I don't want to sound critical
here - I am actually impressed with the documentation in LLD as to why
decisions were made.


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

Historically, this was done as not allocating the headers makes the layout
simpler. We want LLD to match this behavior so that we can more easily
compare the output against the proprietary linker we have.

As for other platforms I'm not sure, It would be interesting to know - on
an embedded system maybe you need the address space?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170515/1abb95a1/attachment.html>