<div dir="ltr"><div style="font-size:12.8px">Hi Rafael,<br></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">Thanks for dong this work. I think this is an improvement over the previous situation.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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 :)</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">My comments are:</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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?</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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?</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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?</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">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.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">Sorry for the delay in looking at this!</div><div class="gmail-yj6qo gmail-ajU" style="font-size:12.8px"></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 4, 2017 at 8:48 PM, Rafael Avila de Espindola via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">ben via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> writes:<br>
<br>
> The question I am struggling with is: Why is allocateHeaders called twice? Could the function fixHeaders() be removed?<br>
<br>
</span>I think I managed to fix this in r302186. Let me know if that was what<br>
you had in mind.<br>
<br>
Cheers,<br>
Rafael<br>
<div class="HOEnZb"><div class="h5">______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>