[lld-macho] Parse re-exports of nested TAPI documents

[int3]

Summary

D103423 neglected to call parseReexports() for nested TBD
documents, leading to symbol resolution failures when trying to look up
a symbol nested more than one level deep in a TBD file. This fixes the
regression and adds a test.

It also appears that umbrella wasn't being set properly when calling
parseLoadCommands -- it's supposed to resolve to this if nullptr
is passed. I didn't write a failing test case for this but I've made
umbrella a member so the previous behavior should be preserved.

[int3]

[thakis]

Whoops!

Thanks for fixing :)

[thakis]

Shower thought: Does ld64 print these nested docs with -t? It's a separate library, but it's the same file, so it's not 100% clear to me. If it doesn't print them, maybe we should move the note printing back to where it was. (But probably also doesn't matter all that much either way :) )

lld/MachO/DriverUtils.cpp

assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
            magic == file_magic::macho_executable ||
            magic == file_magic::macho_bundle);
     file = make<DylibFile>(mbref, umbrella, isBundleLoader);
 
     // parseLoadCommands() can also recursively call loadDylib(). See comment
     // in previous block for why this means we must copy `file` here.
     newFile = file;
     if (newFile->exportingFile)
-      newFile->parseLoadCommands(mbref, umbrella);
+      newFile->parseLoadCommands(mbref);
   }
   return newFile;
 }
 
 Optional<StringRef>
 macho::findPathCombination(const Twine &name,
                            const std::vector<StringRef> &roots,
                            ArrayRef<StringRef> extensions) {

lld/MachO/InputFiles.h

public:
 
   // Mach-O dylibs can re-export other dylibs as sub-libraries, meaning that the
   // symbols in those sub-libraries will be available under the umbrella
   // library's namespace. Those sub-libraries can also have their own
   // re-exports. When loading a re-exported dylib, `umbrella` should be set to
   // the root dylib to ensure symbols in the child library are correctly bound
   // to the root. On the other hand, if a dylib is being directly loaded
   // (through an -lfoo flag), then `umbrella` should be a nullptr.
-  void parseLoadCommands(MemoryBufferRef mb, DylibFile *umbrella);
+  void parseLoadCommands(MemoryBufferRef mb);
   void parseReexports(const llvm::MachO::InterfaceFile &interface);
 
   static bool classof(const InputFile *f) { return f->kind() == DylibKind; }
 
   StringRef dylibName;
   DylibFile *exportingFile = nullptr;
+  DylibFile *umbrella;
   uint32_t compatibilityVersion = 0;
   uint32_t currentVersion = 0;
   int64_t ordinal = 0; // Ordinal numbering starts from 1, so 0 is a sentinel
   RefState refState;
   bool reexport = false;
   bool forceNeeded = false;
   bool forceWeakImport = false;
   bool deadStrippable = false;

lld/MachO/InputFiles.cpp

for (StringRef root : config->systemLibraryRoots)
         return loadDylib(*dylibPath, umbrella);
 
   // TODO: Expand @loader_path, @executable_path, @rpath etc, handle -dylib_path
 
   if (currentTopLevelTapi) {
     for (InterfaceFile &child :
          make_pointee_range(currentTopLevelTapi->documents())) {
       assert(child.documents().empty());
-      if (path == child.getInstallName())
-        return make<DylibFile>(child, umbrella);
+      if (path == child.getInstallName()) {
+        auto file = make<DylibFile>(child, umbrella);
+        file->parseReexports(child);
+        return file;
+      }
     }
   }
 
   if (Optional<std::string> dylibPath = resolveDylibPath(path))
     return loadDylib(*dylibPath, umbrella);
 
   return nullptr;
 }

 
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
                      bool isBundleLoader)
     : InputFile(DylibKind, mb), refState(RefState::Unreferenced),
       isBundleLoader(isBundleLoader) {
   assert(!isBundleLoader || !umbrella);
   if (umbrella == nullptr)
     umbrella = this;
+  this->umbrella = umbrella;
 
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   auto *hdr = reinterpret_cast<const mach_header *>(mb.getBufferStart());
 
   // Initialize dylibName.
   if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     auto *c = reinterpret_cast<const dylib_command *>(cmd);
     currentVersion = read32le(&c->dylib.current_version);
if (config->printEachFile)
     message(toString(this));
 
   deadStrippable = hdr->flags & MH_DEAD_STRIPPABLE_DYLIB;
 
   if (!checkCompatibility(this))
     return;
 
   // Initialize symbols.
-  exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
+  exportingFile = isImplicitlyLinked(dylibName) ? this : this->umbrella;
   if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
     auto *c = reinterpret_cast<const dyld_info_command *>(cmd);
     parseTrie(buf + c->export_off, c->export_size,
               [&](const Twine &name, uint64_t flags) {
                 bool isWeakDef = flags & EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
                 bool isTlv = flags & EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
                 symbols.push_back(symtab->addDylib(
                     saver.save(name), exportingFile, isWeakDef, isTlv));
               });
   } else {
     error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
 }
 
-void DylibFile::parseLoadCommands(MemoryBufferRef mb, DylibFile *umbrella) {
+void DylibFile::parseLoadCommands(MemoryBufferRef mb) {
   auto *hdr = reinterpret_cast<const mach_header *>(mb.getBufferStart());
   const uint8_t *p = reinterpret_cast<const uint8_t *>(mb.getBufferStart()) +
                      target->headerSize;
   for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
     auto *cmd = reinterpret_cast<const load_command *>(p);
     p += cmd->cmdsize;
 
     if (!(hdr->flags & MH_NO_REEXPORTED_DYLIBS) &&

 DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
                      bool isBundleLoader)
     : InputFile(DylibKind, interface), refState(RefState::Unreferenced),
       isBundleLoader(isBundleLoader) {
   // FIXME: Add test for the missing TBD code path.
 
   if (umbrella == nullptr)
     umbrella = this;
+  this->umbrella = umbrella;
 
   dylibName = saver.save(interface.getInstallName());
   compatibilityVersion = interface.getCompatibilityVersion().rawValue();
   currentVersion = interface.getCurrentVersion().rawValue();
 
   if (config->printEachFile)
     message(toString(this));
 
case SymbolKind::ObjectiveCClassEHType:
       break;
     case SymbolKind::ObjectiveCInstanceVariable:
       addSymbol(objc::ivar + symbol->getName());
       break;
     }
   }
 }
 
-void DylibFile::parseReexports(const llvm::MachO::InterfaceFile &interface) {
+void DylibFile::parseReexports(const InterfaceFile &interface) {
   const InterfaceFile *topLevel =
       interface.getParent() == nullptr ? &interface : interface.getParent();
   for (InterfaceFileRef intfRef : interface.reexportedLibraries()) {
     InterfaceFile::const_target_range targets = intfRef.targets();
     if (is_contained(skipPlatformChecks, intfRef.getInstallName()) ||
         is_contained(targets, config->platformInfo.target))
       loadReexport(intfRef.getInstallName(), exportingFile, topLevel);
   }

lld/test/MachO/tapi-link.s

 # REQUIRES: x86
 
-# RUN: mkdir -p %t
-#
-# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/test.o
+# RUN: split-file %s %t --no-leading-lines
 
-# RUN: %lld -o %t/test -lSystem -lc++ -framework CoreFoundation %t/test.o
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %t/test.s -o %t/test.o
+
+# RUN: %lld -o %t/test -lSystem -lc++ -framework CoreFoundation %t/libNested.tbd %t/test.o
 # RUN: llvm-objdump --bind --no-show-raw-insn -d -r %t/test | FileCheck %s
 
 ## libReexportSystem.tbd tests that we can reference symbols from a dylib,
 ## re-exported by a top-level tapi document, which itself is re-exported by
 ## another top-level tapi document.
-# RUN: %lld -o %t/with-reexport %S/Inputs/libReexportSystem.tbd -lc++ -framework CoreFoundation %t/test.o
+# RUN: %lld -o %t/with-reexport %S/Inputs/libReexportSystem.tbd -lc++ -framework CoreFoundation %t/libNested.tbd %t/test.o
 # RUN: llvm-objdump --bind --no-show-raw-insn -d -r %t/with-reexport | FileCheck %s
 
 # CHECK: Disassembly of section __TEXT,__text:
 # CHECK: movq {{.*}} # [[ADDR:[0-9a-f]+]]
 
 # CHECK: Bind table:
 # CHECK-DAG: __DATA_CONST __got 0x[[ADDR]] pointer 0 libSystem ___nan
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_CLASS_$_NSObject
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_METACLASS_$_NSObject
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_IVAR_$_NSConstantArray._count
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 CoreFoundation _OBJC_EHTYPE_$_NSException
 # CHECK-DAG: __DATA __data {{.*}} pointer 0 libc++abi      ___gxx_personality_v0
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 libNested3     _deeply_nested
 
 # RUN: llvm-objdump --macho --all-headers %t/test | \
 # RUN:     FileCheck --check-prefix=LOAD %s
 
 # RUN: llvm-objdump --macho --all-headers %t/with-reexport | \
 # RUN:     FileCheck --check-prefixes=LOAD,LOAD-REEXPORT %s
 
 # LOAD:          cmd LC_LOAD_DYLIB
 # LOAD-NEXT:               cmdsize
 # LOAD-NEXT:                  name /usr/lib/libSystem.dylib
 # LOAD-NEXT:            time stamp
 # LOAD-NEXT:       current version 1.1.1
 # LOAD-NEXT: compatibility version
 
 # LOAD-REEXPORT:          cmd LC_LOAD_DYLIB
 # LOAD-REEXPORT-NEXT:               cmdsize
 # LOAD-REEXPORT-NEXT:                  name /usr/lib/libReexportSystem.dylib
 # LOAD-REEXPORT-NEXT:            time stamp
 # LOAD-REEXPORT-NEXT:       current version 1.0.0
 # LOAD-REEXPORT-NEXT: compatibility version
 
+#--- test.s
 .section __TEXT,__text
 .global _main
 
 _main:
 ## This symbol is defined in an inner TAPI document within libSystem.tbd.
   movq ___nan@GOTPCREL(%rip), %rax
   ret
 
 .data
   .quad _OBJC_CLASS_$_NSObject
   .quad _OBJC_METACLASS_$_NSObject
   .quad _OBJC_IVAR_$_NSConstantArray._count
   .quad _OBJC_EHTYPE_$_NSException
+  .quad _deeply_nested
 
 ## This symbol is defined in libc++abi.tbd, but we are linking test.o against
 ## libc++.tbd (which re-exports libc++abi). Linking against this symbol verifies
 ## that .tbd file re-exports can refer not just to TAPI documents within the
 ## same .tbd file, but to other on-disk files as well.
   .quad ___gxx_personality_v0
+
+## This tests that we can locate a symbol re-exported by a child of a TAPI
+## document.
+#--- libNested.tbd
+--- !tapi-tbd-v3
+archs:            [ x86_64 ]
+uuids:            [ 'x86_64: 00000000-0000-0000-0000-000000000000' ]
+platform:         macosx
+install-name:     '/usr/lib/libNested.dylib'
+exports:
+  - archs:      [ x86_64 ]
+    re-exports: [ '/usr/lib/libNested2.dylib' ]
+--- !tapi-tbd-v3
+archs:            [ x86_64 ]
+uuids:            [ 'x86_64: 00000000-0000-0000-0000-000000000001' ]
+platform:         macosx
+install-name:     '/usr/lib/libNested2.dylib'
+exports:
+  - archs:      [ x86_64 ]
+    re-exports: [ '/usr/lib/libNested3.dylib' ]
+--- !tapi-tbd-v3
+archs:            [ x86_64 ]
+uuids:            [ 'x86_64: 00000000-0000-0000-0000-000000000002' ]
+platform:         macosx
+install-name:     '/usr/lib/libNested3.dylib'
+exports:
+  - archs:      [ x86_64 ]
+    symbols:    [ _deeply_nested ]
+...