[PATCH] D78342: [lld] Add archive file support to Mach-O backend

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:34-37
+  case X86_64_RELOC_BRANCH:
   case X86_64_RELOC_SIGNED:
   case X86_64_RELOC_GOT_LOAD:
+  case X86_64_RELOC_SIGNED_1:
----------------
are these two relocations used in this diff's tests?


================
Comment at: lld/MachO/Arch/X86_64.cpp:52-54
+  case X86_64_RELOC_SIGNED_1:
+    // This type is only used for pc-relative relocations, so offset by 4 since
+    // the RIP has advanced by 4 at this point.
----------------
could just move this case to line 50 above and remove the comment


================
Comment at: lld/MachO/Driver.cpp:107-109
+    std::unique_ptr<object::Archive> file = CHECK(
+        object::Archive::create(mbref), path + ": failed to parse archive");
+    inputFiles.push_back(make<ArchiveFile>(file));
----------------
We should add a test for an erroneous archive.

Would also be nice to explicitly `#include` Archive.h here.

Also, some notes in case there are other reviewers like me who aren't familiar with lld's error handling code:

1. `CHECK` fatals lld if the given value is an error, so no need to worry about returning early
2. The ErrorHandler.h comments say "You should never call fatal() except for reporting a corrupted input file." So this seems like an appropriate use of CHECK/fatal


================
Comment at: lld/MachO/InputFiles.cpp:248-255
+std::vector<InputSection *> const ArchiveFile::getInputSections() {
+  std::vector<InputSection *> sections;
+  for (InputFile *input : resolvedInputs) {
+    auto inputSections = input->getInputSections();
+    sections.insert(sections.end(), inputSections.begin(), inputSections.end());
+  }
+  return sections;
----------------
Kind of unfortunate that we have to return by value here, but I see that lld-ELF's `getInputSections` does something similar.

Also, did you mean to type this method as `std::vector<InputSection *> ArchiveFile::getInputSections() const {`? I *think* the current placement of the `const` is saying that the return value is `const` instead of saying the method is const...


================
Comment at: lld/MachO/InputFiles.cpp:259-261
+      CHECK(sym.getMember(), toString(this) +
+                                 ": could not get the member for symbol " +
+                                 sym.getName());
----------------
When does this happen? Does it indicate input corruption?


================
Comment at: lld/MachO/InputFiles.h:82
+public:
+  explicit ArchiveFile(std::unique_ptr<llvm::object::Archive> &file);
+  static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
----------------
Not sure if LLVM convention says otherwise, but typically I don't pass `unique_ptrs` as arguments unless the callee is changing object ownership. Otherwise pass by reference or raw pointer


================
Comment at: lld/MachO/SymbolTable.h:31
   Symbol *addDylib(StringRef name, DylibFile *file);
+  Symbol *addLazy(StringRef name, ArchiveFile *file,
+                  const llvm::object::Archive::Symbol sym);
----------------
nit: add newline above, or remove lines 27 and 29.

Not really sure what the vertical spacing convention is in the rest of the codebase, idk if @ruiu has an opinion here


================
Comment at: lld/MachO/SymbolTable.h:32
+  Symbol *addLazy(StringRef name, ArchiveFile *file,
+                  const llvm::object::Archive::Symbol sym);
 
----------------
take by const ref


================
Comment at: lld/MachO/Symbols.cpp:18
 
+InputFile *LazySymbol::fetch() { return cast<ArchiveFile>(file)->fetch(sym); }
+
----------------
this method seems currently unused


================
Comment at: lld/test/MachO/archive.s:3
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/archive.s -o %t.main
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/archive2.s -o %t2
----------------
Nits: I don't think having a separate `Inputs/archive.s` is necessary; could just include its contents below and reference it  via `%s`.

Also, I was looking at some of the lld-ELF tests (e.g. `archive-fetch.s`), and it seems that we can create object files / archives with symbols but no corresponding code. So we could define those files inline too via `echo '.globl _boo | llvm-mc ...`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342