[PATCH] D76839: [lld-macho] Extend SyntheticSections to cover all segment load commands

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall I'd like to see more comments. All functions and classes that are not trivial should have a comment explaining what they represent or what they are supposed to do.



================
Comment at: lld/MachO/InputSection.cpp:24
 
+uint32_t InputSection::getFileOffset() const {
+  return parent->fileOff + addr - parent->firstSection()->addr;
----------------
It is safer to use uint64_t to represent file offsets. At least on Linux, we actually create binaries that are larger than 4 GiB.


================
Comment at: lld/MachO/OutputSegment.cpp:25
+    return VM_PROT_READ | VM_PROT_EXECUTE;
+  else if (name == segment_names::pageZero)
+    return 0;
----------------
nit: we generally omit `else` after `return`, so

  if (...)
    return ...;
  if (...)
    return ...;
  return ...;


================
Comment at: lld/MachO/OutputSegment.h:38
+
+  void addSection(InputSection *);
+  const llvm::MapVector<StringRef, std::vector<InputSection *>> &
----------------
nit: add a blank line after this line


================
Comment at: lld/MachO/SyntheticSections.h:23-24
+
+constexpr const char *binding = "__binding";
+constexpr const char *header = "__mach_header";
+constexpr const char *pageZero = "__pagezero";
----------------
int3 wrote:
> The names of hidden sections don't make it to the final executable; they're just for distinguishing the InputSections within lld code and so can be any unique string. But I've picked the same names as the corresponding Atoms in ld64 for easy reference.
Actually this kind of one-shot temporary can be inlined, and in particular for these names, we don't have to have then as global constants because we are not really referring them.


================
Comment at: lld/MachO/SyntheticSections.h:34
+
+class MachHeaderSection : public InputSection {
+public:
----------------
Please add a class comment to explain what this class represents.


================
Comment at: lld/MachO/SyntheticSections.h:76
 
+// Stores bind opcodes, which tell dyld which dylib symbols to load non-lazily.
+class BindingSection : public InputSection {
----------------
Can you expand the comment a bit? I'd explain what the bind opcode in more details.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76839