[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/Driver.cpp:113
+    if (!file->isEmpty() && !file->hasSymbolTable()) {
+      error(mbref.getBufferIdentifier() +
+            ": archive has no index; run ranlib to add one");
----------------
s/mbref.getBufferIdentifier()/file->getName()/

nit: drop `if` curly braces


================
Comment at: lld/MachO/InputFiles.h:79
+private:
+  llvm::DenseSet<uint64_t> seen;
+};
----------------
would be nice to have a comment here about what the uint64_t represents


================
Comment at: lld/test/MachO/symbol-order.s:2-5
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/g.s -o %tg.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/f1.s -o %tf1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/f2.s -o %tf2.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/f2g.s -o %tf2g.o
----------------
nit: the `Inputs` folder is shared by all tests, so it'll be cleaner if we namespace them. I.e. `symbol-order-g.s` instead of `g.s`

also personally I like to run tests with multiple outputs in their own directory (i.e. `mkdir -p %t` would be the first run command and all files would be of the form %t/foo.o) but either way is fine


================
Comment at: lld/test/MachO/symbol-order.s:16-17
+
+# RUN: lld -flavor darwinnew %tlibf1.dylib %tlibf2_g.a %t.o -lSystem -o %t.out
+# RUN: ./%t.out; echo $? | FileCheck %s --check-prefix DYLIB-FIRST
+# DYLIB-FIRST: 1
----------------
lld's tests don't typically involve executing the linked binaries (plus it wouldn't work anyway on non-OSX CI machines). `-lSystem` isn't portable either.

Instead of using constant literals to distinguish where a symbol came from, I'd suggest using custom section names instead. That is, you can have each object file in the archive have the same symbol name under a different section, then see which section it appears under in the final output. `llvm-objdump --syms --macho --lazy-bind` should cover that, plus show if a symbol is coming from the dylib instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342