[PATCH] D92360: [lld/mac] Fix issues around thin archives

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:306
   for (StringRef path : args::getLines(mbref))
-    addFile(path);
-}
-
-static void forceLoadArchive(StringRef path) {
-  if (Optional<MemoryBufferRef> buffer = readFile(path))
-    for (MemoryBufferRef member : getArchiveMembers(*buffer))
-      inputFiles.push_back(make<ObjFile>(member));
+    addFile(path, false);
 }
----------------
same for the rest of the calls


================
Comment at: lld/MachO/Symbols.cpp:22
+  return std::string(symName);
+}
+std::string lld::toString(const Symbol &sym) {
----------------
nit: can we have a blank line between functions? my vim habits depend on it :p


================
Comment at: lld/MachO/Symbols.h:238
 std::string toString(const macho::Symbol &);
+std::string toMachOString(const llvm::object::Archive::Symbol &);
+
----------------
why not call this `toString` too? since everything here is `MachO` anyway


================
Comment at: lld/test/MachO/Inputs/mangled-symbols.s:1
+        .section        __TEXT,__text,regular,pure_instructions
+        .macosx_version_min 10, 4
----------------
I'd prefer we use `split-file` to have this file inline in the main test file. (The tests that currently use `Inputs/` were written before we had `split-file`.)


================
Comment at: lld/test/MachO/Inputs/mangled-symbols.s:6-15
+        .cfi_startproc
+## %bb.0:                               ## %entry
+        pushq   %rbp
+        .cfi_def_cfa_offset 16
+        .cfi_offset %rbp, -16
+        movq    %rsp, %rbp
+        .cfi_def_cfa_register %rbp
----------------
can we minimize the assembly? I assume CFI directives aren't needed for demangling...


================
Comment at: lld/test/MachO/thin-archive.s:13
+# RUN: %lld %t/main.o %t/lib.a -o %t/out 2>&1 | \
+# RUN:     FileCheck --allow-empty %s
+# RUN: %lld %t/main.o %t/lib_thin.a -o %t/out 2>&1 | \
----------------
is running FileCheck really necessary here if the input is going to be empty? I guess the intent is to verify that the `CHECK-NOT` line doesn't match, but if lld were emitting an error, this line would fail anyway...

might be clearer to just run the commands and check that `out` contains the symbol definition, similar to what `archive.s` is doing


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

https://reviews.llvm.org/D92360