[PATCH] D85404: [lld-macho] Handle TAPI and regular re-exports uniformly

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/Driver.cpp:132
 
-    if (Optional<std::string> path = findWithExtension(symlink, {".tbd", ""}))
+    if (Optional<std::string> path = resolveDylibPath(symlink))
       return path;
----------------
IIUC, the previous version would prefer the tbd to the dylib, and `resolveDylibPath` will prefer the dylib to the TBD.

IIRC, if ld64 finds both a TBD and a dylib for the same file, it verifies the TBD against the dylib and falls back to the dylib in case of a mismatch. I don't think we need to implement that (and I don't think LLVM's version of TextAPI has the equivalence checking logic yet anyway), so perhaps always preferring the dylib in that case (as you're doing now) is reasonable.


================
Comment at: lld/MachO/Driver.cpp:181
     }
+    if (!found && isDirectory(optionLetter, path))
+      paths.push_back(path);
----------------
This matches ld64's behavior as far as I can tell, so it looks good, but can you add a test? Both for this and for the standard search paths **only** being searched for under the syslibroots?

(This and the tests should also probably be their own diff.)


================
Comment at: lld/MachO/Driver.cpp:354-355
     if (auto *dylibFile = dyn_cast<DylibFile>(file)) {
-      StringRef filename = path::filename(dylibFile->getName());
-      if (filename.consume_front(searchName) && filename == ".dylib") {
+      StringRef name = path::filename(dylibFile->dylibName);
+      if (name.consume_front(searchName) && name == ".dylib") {
         dylibFile->reexport = true;
----------------
Can we get a test for this fix as well? (It should also probably be its own diff.)


================
Comment at: lld/MachO/InputFiles.cpp:375
+
+  for (InterfaceFile &intf : make_pointee_range(inlineDocuments))
+    if (path == intf.getInstallName())
----------------
If I'm understanding correctly, this will only be able to find any inline documents we've seen in our input files thus far. Does this match ld64, or is it able to find inline documents from later files?


================
Comment at: lld/MachO/InputFiles.cpp:432
         reinterpret_cast<const char *>(c) + read32le(&c->dylib.name);
-    // TODO: Expand @loader_path, @executable_path etc in reexportPath
-    Optional<MemoryBufferRef> buffer = readFile(reexportPath);
-    if (!buffer) {
-      error("unable to read re-exported dylib at " + reexportPath);
-      return;
-    }
-    reexported.push_back(make<DylibFile>(*buffer, umbrella));
+    if (Optional<DylibFile *> reexport = loadReExport(reexportPath, umbrella))
+      reexported.push_back(*reexport);
----------------
Nit: we're inconsistent about reexport vs reExport


================
Comment at: lld/test/MachO/Inputs/MacOSX.sdk/usr/lib/libc++abi.tbd:5
+platform:         macosx
+install-name:     '/usr/lib/libc++.dylib'
+current-version:  1281
----------------
Should this be libc++abi?


================
Comment at: lld/test/MachO/reexport-stub.s:16
+# CHECK: Bind table:
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libreexporter ___gxx_personality_v0
+
----------------
I thought that for sub-libraries, the bind information reflected the actual sub-library the symbol was coming from, and that seems to be the case in a basic test I did with ld64. Can you confirm how this is supposed to behave?

(IIRC there's options to re-export specific symbols, and those bind to the re-exporting library.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85404