[PATCH] D85867: [ELF] Assign file offsets of non-SHF_ALLOC after SHF_ALLOC and set sh_addr=0 to non-SHF_ALLOC

[Peter Smith via Phabricator via llvm-commits]

psmith added a comment.

Just to check I understand.

In D85100 <https://reviews.llvm.org/D85100> we moved non SHF_ALLOC orphans after all other SHF_ALLOC sections so that they didn't affect address assignment of SHF_ALLOC sections. Now we don't move the SHF_ALLOC section positions but instead we do not move the location counter, and also move their file offset after all SHF_ALLOC sections so that they have no affect on addresses of SHF_ALLOC sections and cannot be part of a PT_LOAD section.

This is related to pr47094.

I don't have any objections, in the general case I think compatibility with BFD is worthwhile.



================
Comment at: lld/ELF/LinkerScript.cpp:858
+  // Non-SHF_ALLOC sections have zero addresses.
+  if (!(sec->flags & SHF_ALLOC)) {
+    dot = 0;
----------------
May be worth writing the condition the other way around as it is the most common case. Only a weak preference though.
```
if (sec->flags & SHF_ALLOC) {
...
} else {
  // Non-SHF_ALLOC OutputSections start at address 0
  ...
}
```


================
Comment at: lld/ELF/LinkerScript.cpp:925
   }
+
+  if (!(sec->flags & SHF_ALLOC))
----------------
I think it's worth adding a comment. Suggest something like:
// Non-SHF_ALLOC sections do not affect the addresses of other OutputSections as they are not part of the process image.


================
Comment at: lld/ELF/Writer.cpp:1236
 findOrphanPos(std::vector<BaseCommand *>::iterator b,
               std::vector<BaseCommand *>::iterator e) {
   OutputSection *sec = cast<OutputSection>(*e);
----------------
I guess we've removed this as it is not strictly necessary any more?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85867