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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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

The main observation of the change is that if we can drop the `-T<section>` special rule (as we can see, ld.bfd's rule is quite involved; our current behavior already diverges; lld users can get similar behaviors with explicit `--image-base`), we can skip the whole `allocateHeaders` logic for the !hasSectionsCommand case.

For the default !hasSectionsCommand case, assignAddresses takes care of assigning addresses to headers, and there should be no case that the ELF header or program headers can be discarded. ld.bfd can discard them for the cases like `-Ttext=0` but we can argue they are involved cases, and users should use a linker script to get more predictable behavior. (I can hardly find `-Ttext=0` cases in the wild. I think they are either 1) `--oformat binary` (unaffected by this change) 2) used with SECTIONS command (unaffected by this change) 3) we fail to meet their requirement because our different layout. So the behavior for 3) may not matter much.



================
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
----------------
peter.smith wrote:
> I think this comment would be stale with your proposed change.
> When using a linker script, we also check if the headers are covered by the output section. This allows omitting the headers by not leaving enough space for them in the linker script; this pattern is common in embedded systems.

I think the two sentences were stale before this change. I'll delete them.


================
Comment at: ELF/LinkerScript.cpp:1079
+  } else {
+    // Assign addresses to headers right now.
+    dot = target->getImageBase();
----------------
peter.smith wrote:
> 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. 
In the nmagic and omagic cases, PT_PHDR and PT_INTERP are not allocated, but other program headers can still exist.

Several tests check their behaviors, e.g. nmagic.s, segments.s, magic-page-combo-warn.s. This change does not need an update of them.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67325