[PATCH] D67325: [ELF] Map the ELF header at imageBase

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I'm struggling a bit with this one at the moment. I've put some comments in but I still need to work this one through. As I understand it we have today:

- allocateHeaders() that decides whether the program headers are allocated or not and assigns the headers addresses. This is the same for both linkerscripts and non-linkerscripts.
- fixSectionAlignments is performed only for the non-linkerscript case just prior to the final address allocation. It adds expressions to the OutputSections so that it will be aligned properly.

There is a clash between allocateHeaders and fixSectionAlignments and your change resolves this by only running allocateHeaders in the linkerscript. I can see that this would work, but I think that this could be at the expense of missing some of the logic in allocateHeaders, and leaving a point to diverge in the future. As this is making me a bit nervous at the moment I'd like to try and understand the conflict a bit better and whether there is a better way of doing this (there may not be).



================
Comment at: ELF/LinkerScript.cpp:1013
 //
 // When using the default layout, we check if the headers fit below the first
 // allocated section. When using a linker script, we also check if the headers
----------------
I think this comment would be stale with your proposed change.


================
Comment at: ELF/LinkerScript.cpp:1022
 void LinkerScript::allocateHeaders(std::vector<PhdrEntry *> &phdrs) {
+  if (!script->hasSectionsCommand)
+    return;
----------------
Given that allocateHeaders is only called once, it may be worth moving to the callsite in Writer.cpp

```
if (script->hasSectionsCommand)
    script->allocateHeaders(mainPart->phdrs)
```


================
Comment at: ELF/LinkerScript.cpp:1076
+  if (script->hasSectionsCommand) {
+    // Delay the allocation of headers to allocateHeaders().
+    dot = config->imageBase ? *config->imageBase : 0;
----------------
Delay may not be the right word as assignAddresses() is called multiple times and I think allocateHeaders() is called early, most likely before assignAddresses().

Perhaps:
// With a linker script assignment of addresses to headers is covered by allocateHeaders().


================
Comment at: ELF/LinkerScript.cpp:1079
+  } else {
+    // Assign addresses to headers right now.
+    dot = target->getImageBase();
----------------
I think that this would get nmagic and omagic wrong. In this case the ELF Headers and Program Headers are not allocated, even with the default linker script. I'd expect a linker script to be used with nmagic and omagic, but this would be a change in behaviour from ld.bfd and older versions of LLD. 


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67325/new/

https://reviews.llvm.org/D67325