[PATCH] D76252: [lld-macho] Add basic support for linking against dylibs

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:215
+
+  if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
+    auto *c = (const dylib_command *)cmd;
----------------
Can you add a brief comment here that we initialize `dylibName`.


================
Comment at: lld/MachO/InputFiles.cpp:223
+
+  if (const load_command *cmd = findCommand(hdr, LC_SYMTAB)) {
+    auto *c = (const symtab_command *)cmd;
----------------
Ditto -- we are initializing `symbols`.


================
Comment at: lld/MachO/Symbols.h:79
+  DylibFile *file;
+  uint64_t gotIndex = UINT64_MAX;
+};
----------------
Isn't uint32_t enough? It's not a big deal, but I'd use uint32_t for an index of GOT.


================
Comment at: lld/MachO/SyntheticSections.h:33
+  void writeTo(uint8_t *buf) override {
+    // Nothing to write, GOT starts as all zeroes
+  }
----------------
So GOT has no data but dynamic relocations? If so, I'd leave a brief comment here.


================
Comment at: lld/MachO/SyntheticSections.h:35
+  }
+private:
+  llvm::SetVector<const DylibSymbol *> entries;
----------------
nit: add a blank line before a label.


================
Comment at: lld/MachO/Writer.cpp:300
+void Writer::scanRelocations() {
+  for (auto &sect : inputSections)
+    for (Reloc &r : sect->relocs)
----------------
Replace auto with a concrete type.


================
Comment at: lld/MachO/Writer.cpp:357-360
+      os << (char)BIND_OPCODE_SET_SYMBOL_TRAILING_FLAGS_IMM;
+      os << sym->getName() << '\0';
+      os << (char)(BIND_OPCODE_SET_TYPE_IMM | BIND_TYPE_POINTER);
+      os << (char)BIND_OPCODE_DO_BIND;
----------------
nit: we usually do

  os << foo << bar
     << baz << fizz;

instead of

  os << foo << bar;
  os << baz << fizz;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76252