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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 11 inline comments as done.
Ktwu 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:
----------------
int3 wrote:
> are these two relocations used in this diff's tests?
Yup.


================
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;
----------------
int3 wrote:
> 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...
re: const, yup, thanks!


================
Comment at: lld/MachO/InputFiles.cpp:259-261
+      CHECK(sym.getMember(), toString(this) +
+                                 ": could not get the member for symbol " +
+                                 sym.getName());
----------------
int3 wrote:
> When does this happen? Does it indicate input corruption?
This is code copied from the initial implementation of MachO lld we inherited. However, looking at the implementation of getMember(), it looks like it represents a malformed archive (and hence a parse error).


================
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; }
----------------
int3 wrote:
> 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
This is part of that lld corpus we inherited, so I have no opinion on this. I assume it's just because archive creation returns a unique pointer.

Your convention seems reasonable to me so I'm happy to adapt that here.


================
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
----------------
int3 wrote:
> 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 ...`
I'll definitely get rid of archive.s (this was back when I didn't know how Filecheck worked).

I like having explicit test files (although knowing about passing stuff straight to llvm-mc is a neat trick), so I'd prefer to keep the archive#.s if that's OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342