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

[Peter Smith via Phabricator via llvm-commits]

psmith added a comment.

Apologies for the delay in responding. As I understand it, this would only cause a problem with PHDRS? Our implicit creation of program headers should already be in OutputSection order.

Personally I'd be in favour of making our PHDRS support more closely match ld.bfd. Both Arm and RiscV are making more of an effort to push LLVM rather than GCC in the embedded space (https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm) and LLD is the linker of choice. Avoiding migration problems for embedded developers is helpful. Yes it is true that people can rewrite their linker scripts in most cases, however it normally needs an expert to debug the limitation from an overlap error message.

For this particular change, I've made a few comments and I would like an explanation for some of the file-offset differences and the addition of foo implicit-program-header.test? I can't immediately see from the script why there would be any change in output section order.



================
Comment at: lld/ELF/Writer.cpp:2627
 
-  // Layout SHF_ALLOC sections before non-SHF_ALLOC sections. A non-SHF_ALLOC
-  // will not occupy file offsets contained by a PT_LOAD.
-  for (OutputSection *sec : outputSections) {
-    if (!(sec->flags & SHF_ALLOC))
-      continue;
-    off = setFileOffset(sec, off);
-
-    // If this is a last section of the last executable segment and that
-    // segment is the last loadable segment, align the offset of the
-    // following section to avoid loading non-segments parts of the file.
-    if (config->zSeparate != SeparateSegmentKind::None && lastRX &&
-        lastRX->lastSec == sec)
-      off = alignTo(off, config->commonPageSize);
+  // First place output sections in load segment order
+  for (Partition &part : partitions) {
----------------
I'd prefer a comment that explains why we are doing this. As it stands it is just a 1 line summary of the code.




================
Comment at: lld/ELF/Writer.cpp:2633
+      for (OutputSection *sec : outputSections) {
+        if (sec->ptLoad != p)
+          continue;
----------------
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.


================
Comment at: lld/ELF/Writer.cpp:2646
   }
+
+  // Then place non load sections
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95199