[PATCH] D94935: [lld-macho] Fix alignment & layout to match ld64 and satisfy codesign

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added a comment.

Could you please add a test case for verifying the output order of the load commands?  It seems that the load commands are expected to be in a certain order according to your change - it makes sense to have a test which validates that ordering.



================
Comment at: lld/MachO/SyntheticSections.cpp:396
+          MachO::S_ATTR_PURE_INSTRUCTIONS;
+  align = 4;
   reserved2 = target->stubSize;
----------------
Hmm, this is `4` and not `WordSize`?  It is plausible, but might be reasonable to add a comment explaining why `4` on 64-bit systems is needed/correct.


================
Comment at: lld/MachO/SyntheticSections.cpp:422
+  flags = MachO::S_ATTR_SOME_INSTRUCTIONS | MachO::S_ATTR_PURE_INSTRUCTIONS;
+  align = 4;
+}
----------------
Similar.


================
Comment at: lld/MachO/Target.h:30
   WordSize = 8,
-  PageSize = 4096,
+  PageSize = 0x4000,         // 16 KiB
   PageZeroSize = 1ull << 32, // XXX should be 4096 for 32-bit targets
----------------
Isn't the page size 4K on x64 and 16K on ARM64?


================
Comment at: lld/MachO/UnwindInfoSection.cpp:90
     : SyntheticSection(segment_names::text, section_names::unwindInfo) {
-  align = WordSize; // TODO(gkm): make this 4 KiB ?
+  align = 4;
 }
----------------
IIRC, 4 is correct for the `__unwind_info` section, but this probably deserves a comment (unless Im mixing up the `__compact_unwind_entries`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94935