[PATCH] D76252: [lld-macho] Add basic support for linking against dylibs

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a subscriber: Ktwu.
smeenai added a comment.

Initial set of comments. I'm still going through the rest.



================
Comment at: lld/MachO/Arch/X86_64.cpp:47
+  case X86_64_RELOC_GOT_LOAD:
+    // These types are  only used for pc-relative relocations, so offset by 4
+    // since the RIP has advanced by 4 at this point.
----------------
Nit: change the two spaces between "are" and "only" to one


================
Comment at: lld/MachO/InputFiles.cpp:78
+
+  // If this is a regular non-fat file, return it.
+  const char *buf = mbref.getBufferStart();
----------------
Universal binary support could technically be a diff by itself. I'd prefer to do so, although it's small enough that I'm not super opposed to folding it into this diff. We definitely need to add tests for it either way. llvm-lipo is complete enough to be used for the tests.

I'd also note in the commit message that some of this functionality is being restored from @pcc and @ruiu's initial commit, for completeness.


================
Comment at: lld/MachO/InputFiles.cpp:89
+  auto *arch = reinterpret_cast<const MachO::fat_arch *>(buf + sizeof(*hdr));
+  for (size_t i = 0; i < hdr->nfat_arch; ++i) {
+    if (read32be(&arch[i].cputype) != target->cpuType ||
----------------
We should add some sort of checking here to prevent going out of bounds of the file (if you have a ridiculously large `nfat_arch` in a malformed file, for example). We should also add tests for the error conditions here if possible (assuming yaml2obj lets us construct malformed universal binaries that would trigger the conditions).


================
Comment at: lld/MachO/InputFiles.cpp:99
+    return MemoryBufferRef(StringRef(buf + offset, size), path);
+  }
+
----------------
I believe ld64 warns if you give it a universal binary input file and it can't find any slices for the architecture being linked. We should do the same.


================
Comment at: lld/MachO/InputFiles.cpp:223
+
+  if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
+    auto *c = (const dylib_command *)cmd;
----------------
Is it valid to not have an `LC_ID_DYLIB` load command? If it is, should we figure out a fallback `dylibName`? If not, we should error. (Also, we should add tests for this either way if possible.)


================
Comment at: lld/MachO/InputFiles.cpp:228
+
+  if (const load_command *cmd = findCommand(hdr, LC_SYMTAB)) {
+    auto *c = (const symtab_command *)cmd;
----------------
Is it valid to not have a symtab? If not, we should error.


================
Comment at: lld/MachO/InputFiles.cpp:243
+
+// Returns "<internal>", "foo.a(bar.o)" or "baz.o".
 std::string lld::toString(const InputFile *file) {
----------------
Unnecessary comment change? We aren't adding support for archive files yet. (CC @Ktwu, this'll need to be adjusted when archive file support is added.)


================
Comment at: lld/test/MachO/Inputs/goodbye-dylib.yaml:1
+# This yaml file was originally generated from linking the following source
+# input with ld64:
----------------
Description for hello and goodbye seems swapped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76252