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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 4 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:
----------------
MaskRay wrote:
> Ktwu wrote:
> > int3 wrote:
> > > are these two relocations used in this diff's tests?
> > Yup.
> Don't change X86_64_RELOC_SIGNED_1 which is unrelated to the main purpose of the patch.
> 
> Use other existing relocation types.
Sure, I'll rebase this on top of D79311 (thanks!)


================
Comment at: lld/MachO/SymbolTable.cpp:77
+  std::tie(s, wasInserted) = insert(name);
+
+  if (wasInserted)
----------------
MaskRay wrote:
> How does an archive symbol interact with a DylibSymbol?
By "interact" do you mean if there's a conflict -- a dylib and an archive both expose a symbol with the same name? I assume there shouldn't be issues if symbol names are unique...

@int3 you know more about dylibs; do you see potential issues?


================
Comment at: lld/test/MachO/bad-archive.s:1
+# REQUIRES: x86
+# RUN: echo "!<arch>" > %t.a
----------------
MaskRay wrote:
> You could also create `invalid/` and place such tests there.
A place to stick tests for known failures? I like that a lot!

I see now that ELF has the same (although the tests I found didn't live within it...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342