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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I'll take another look tomorrow. Thanks for the clarifications.



================
Comment at: ELF/LinkerScript.cpp:1079
+  } else {
+    // Assign addresses to headers right now.
+    dot = target->getImageBase();
----------------
MaskRay wrote:
> 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.
What I was worrying about was whether the headers and program headers were mapped into the first PT_LOAD as this was something that allocateHeaders() took account of. However it looks like this isn't a problem as createPhdrs() doesn't "unconditionally" add them when either omagic or nmagic is added.


================
Comment at: ELF/LinkerScript.cpp:1011
+// When the SECTIONS command is used, try to find an address for the file and
+// program headers output sections, which were unconditionally added to the
+// first PT_LOAD segment earlier.
----------------
>From a double check, it seems like unconditional isn't true any more. In createPhdrs() nmagic and omagic don't add the ELF Headers into the loadable segment, looks like I missed updating the comment when adding omagic and nmagic.

Perhaps update the comment to:

```
// When the SECTIONS command is used, try to find an address for the file and
// program headers output sections, which can be added to the first PT_LOAD segment
// when program headers are created.
``` 


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D67325