[PATCH] D75382: [lld] Initial commit for new Mach-O backend

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:50
+  default:
+    error("TODO: Unhandled relocation type");
+  }
----------------
getImplicitAddend errors for an unknown relocation type. Is here unreachable?


================
Comment at: lld/MachO/Options.td:11
+
+def version: Flag<["--"], "version">, HelpText<"Display the version number and exit">;
+
----------------
--version uses a double-dash form while other long options use single-dash form. Can you confirm?


================
Comment at: lld/MachO/Target.h:18
+enum {
+  PageSize = 4096,
+  ImageBase = 4096,
----------------
`pageSize`
`imageBase`


================
Comment at: lld/MachO/Target.h:27
+                                     uint8_t type) const = 0;
+  virtual void relocateOne(uint8_t *loc, uint8_t type, uint64_t val) const = 0;
+
----------------
In ELF, D73254 renamed relocateOne to relocate and passed in `Relocation`. You may think whether Mach-O will need other fields of `Reloc`.


================
Comment at: lld/MachO/Writer.cpp:38
+  virtual ~LoadCommand() = default;
+  virtual uint64_t getSize() = 0;
+  virtual void writeTo(uint8_t *buf) = 0;
----------------
`virtual uint64_t getSize() const = 0;`


================
Comment at: lld/MachO/Writer.cpp:49
+  std::unique_ptr<FileOutputBuffer> &buffer;
+  uint64_t fileSize;
+
----------------
Why are the two variables not placed with other member variables?


================
Comment at: lld/MachO/Writer.cpp:94
+
+  uint64_t getOffset() { return fileOff + contents.size(); }
+
----------------
const


================
Comment at: lld/MachO/Writer.cpp:99
+  SmallVector<char, 128> contents;
+  raw_svector_ostream os{contents};
+};
----------------
Can `os` be local to createDyldInfoContents?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75382