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

[Nico Weber via Phabricator via llvm-commits]

thakis 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);
 }
----------------
int3 wrote:
> int3 wrote:
> > same for the rest of the calls
> actually, I guess this would make for rather verbose code. How about making the parameter default to `false` instead?
I think lld code likes to be explicit about arguments -- cf LinkerDriver::enqueuePath() in lld/COFF/Driver.cpp which looks very similar to this.

If you hate the bool I can invent a two-state enum for this so that it's addFile(path, kForceLoadArchive), addFile(path, kRegularFile). But given that there are just a few callers, all in one file, and COFF doesn't do it, I'm not sure it's worth it?


================
Comment at: lld/MachO/Symbols.cpp:22
+  return std::string(symName);
+}
+std::string lld::toString(const Symbol &sym) {
----------------
int3 wrote:
> nit: can we have a blank line between functions? my vim habits depend on it :p
Done. Off-topic: Do you use `{` / `}` or `[]` / `][`, `[[`, `]]`? I like the spirit behind the latter but they don't seem to work well for me for C++ code. And `{` / `}` movements are often to granular, so I usually navigate with ctrl-f / ctrl-b / search / marks.


================
Comment at: lld/MachO/Symbols.h:238
 std::string toString(const macho::Symbol &);
+std::string toMachOString(const llvm::object::Archive::Symbol &);
+
----------------
int3 wrote:
> why not call this `toString` too? since everything here is `MachO` anyway
Because the argument is of type `llvm::object::Archive::Symbol` and we have toXString(llvm::object::Archive::Symbol) in COFF and ELF too (and so normal overloading based on different types in different ports doesn't work). We can't (easily) put toString() in the macho namespace since some of its overloads are in lld/Common. IIRC the change where I added toCOFFString() had some discussion about the various tradeoffs; we settled on not using toString() for the archive symbol.


================
Comment at: lld/test/MachO/Inputs/mangled-symbols.s:1
+        .section        __TEXT,__text,regular,pure_instructions
+        .macosx_version_min 10, 4
----------------
int3 wrote:
> 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`.)
Yes, `split-file` is much nicer, I'm just not used to it. Done :)


================
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
----------------
int3 wrote:
> can we minimize the assembly? I assume CFI directives aren't needed for demangling...
Done. The cfi stuff isn't needed; I just pasted the output of `clang -S` for `void f() {}`.


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

https://reviews.llvm.org/D92360