[PATCH] D95199: [ELF] Write output sections in PT_LOAD segment order

[Patrick Oppenlander via Phabricator via llvm-commits]

pattop added inline comments.


================
Comment at: lld/ELF/Writer.cpp:2633
+      for (OutputSection *sec : outputSections) {
+        if (sec->ptLoad != p)
+          continue;
----------------
psmith wrote:
> Can we guarantee that all sections with SHF_ALLOC are assigned to a loadable program header? From a brief scan of the code I think so.
> 
> Can we guarantee that all sections assigned to SHF_ALLOC have SHF_ALLOC? I'm not sure we can guarantee it if PHDRS command is used, but I think we can if PHDRS is not used (linker creates them implicitly), would be good to check that this is the case with an assertion.
> Can we guarantee that all sections with SHF_ALLOC are assigned to a loadable program header? From a brief scan of the code I think so.

I don't think this is the case for tbss.

> Can we guarantee that all sections assigned to SHF_ALLOC have SHF_ALLOC? I'm not sure we can guarantee it if PHDRS command is used, but I think we can if PHDRS is not used (linker creates them implicitly), would be good to check that this is the case with an assertion.

Did you mean all sections assigned to a loadable segment have SHF_ALLOC? If so, yes, I think this can be asserted.


================
Comment at: lld/ELF/Writer.cpp:2646
   }
+
+  // Then place non load sections
----------------
psmith wrote:
> Is there a chance that we've not written a SHF_ALLOC output section. I think this would only be possible if the section were not assigned to a program header, which I don't think is possible if our PHDRS or implicit program header implementation is correct. Is there any way we can check for this.
I think tbss is a special case section which is allocated but not assigned to a load segment. That makes adding a check more difficult.


================
Comment at: lld/ELF/Writer.cpp:2649
   for (OutputSection *sec : outputSections)
-    if (!(sec->flags & SHF_ALLOC))
+    if (!sec->ptLoad)
       off = setFileOffset(sec, off);
----------------
psmith wrote:
> Similarly we may be able to assert that (sec->flags & SHF_ALLOC) == 0 here.
Unfortunately we can't assert this because:

1. tbss is a special case which is allocated but not part of a load segment
2. sometimes we don't generate load segments (just a file header, no program headers) in which case all output is written from this loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95199