[lld-macho] Change loadReexport to handle the case where a TAPI re-exports to reference documents nested within other TBD.

[oontvoo]

Summary

Currently, it was delibrately impleneted to not handle this case, but as it has turnt out, we need this feature.
The concrete use case is

`System/Library/Frameworks/Cocoa.framework/Versions/A/Cocoa` reexports
        /System/Library/Frameworks/AppKit.framework/Versions/C/AppKit , which then rexports
             /System/Library/PrivateFrameworks/UIFoundation.framework/Versions/A/UIFoundation`

The current implemention uses a global currentTopLevelTapi, which is not reset until it finishes loading the whole tree.
This is a problem because if the top-level is set to Cocoa, then when we get to UIFoundation, it will try to find UIFoundation in the current top level, which is Cocoa and will not find it.

The rules should be:

• When loading a library from a TBD file, re-exports need to be looked up in the auxiliary documents within the same TBD.
• When loading from an actual dylib, no additional TBD documents need to be examined.
• In no case does a re-export mentioned in one TBD file need to be looked up in a document in an auxiliary document from a different TBD file

OPEN-ISSUE:

Haven't been able to write a good test for this. The test checked in here, ideally, should have failed with errors like lld: error: unable to locate re-export with install name <path to the second-level dylib> before this patch. Unfortunately, it kept failing with "undefined symbols". Can someone tell me what I did wrong in the test?

[oontvoo]

<cosmetic changes only>

[jyknight]

IIUC, the requirement is:

• When loading a library from a TBD file, re-exports need to be looked up in the auxiliary documents within the same TBD. (even if no file on disk exists for the referenced library).
• When loading from an actual dylib, no additional TBD documents need to be examined.
• In no case does a re-export mentioned in one TBD file need to be looked up in a document in an auxiliary document from a different TBD file

So, with the addition of getParent() to InterfaceFile, I think this logic can be fully within within the Dylib(InterfaceFile*, ...) constructor, no external info required. It just needs to pass "interface" down to loadReexport/loadReexportHelper (which can call getParent() before accessing documents()).

So, I think all the other API changes can be reverted as unnecessary.

As for the test: Looks like you have only one TBD file, rather than the two in the original example: Cocoa (in Cocoa.tbd) re-exports AppKit (in AppKit.tbd) re-exports UIFoundation (also in AppKit.tbd). For your test, if you add a new "SystemReexport.tbd" which re-exports libSystem, I think that'd show the problem.

lld/MachO/Driver.h

18

LLVM doesn't require C++17 yet, so you can't use this syntax.

[int3]

Like the bundle_loader diff, this one needs more motivating detail in the commit message. It doesn't help that the Cocoa / AppKit libraries on my system don't include TBDs (perhaps because I'm not yet on Big Sur), so I'm not entirely sure what your commit message example is supposed to show. (And we should not assume that future readers will have a local system that matches yours, either.)

@jyknight's comment helps clarify things a bit, but I'm still not sure why we need a parent pointer instead of the global currentTopLevelTapi pointer that we currently have. Is it because we might have a TBD file with child documents that re-exports another TBD file, also with child documents?

lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libSystem.tbd

7–9

how is this different from the re-exports list on line 12 below?

(Scanning through TextStub.cpp, it seems like reexported-libraries is a TBD v4 thing, but this file is TBD v3, so I'm not sure it's doing anything. Are the system TBDs on your machine v4?)

60

nit: I would prefer we keep libSystem.tbd looking like a subset of the real libSystem, i.e. ideally this symbol would be something that's actually defined in libsystem_sim_pthread_host

[int3]

lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libSystem.tbd

7–9

(perhaps this is why your test wasn't failing as expected)

[oontvoo]

In D97438#2588902, @jyknight wrote:

As for the test: Looks like you have only one TBD file, rather than the two in the original example: Cocoa (in Cocoa.tbd) re-exports AppKit (in AppKit.tbd) re-exports UIFoundation (also in AppKit.tbd). For your test, if you add a new "SystemReexport.tbd" which re-exports libSystem, I think that'd show the problem.

I've added a libReexportSystem.tbd, which reexport libSystem.tbd but still coulnd't get it to reproduce the re-export error ... :(
I think we have to tell the test to say load the libReexportSystem right?

In D97438#2590738, @int3 wrote:

Like the bundle_loader diff, this one needs more motivating detail in the commit message. It doesn't help that the Cocoa / AppKit libraries on my system don't include TBDs (perhaps because I'm not yet on Big Sur), so I'm not entirely sure what your commit message example is supposed to show. (And we should not assume that future readers will have a local system that matches yours, either.)

@jyknight's comment helps clarify things a bit, but I'm still not sure why we need a parent pointer instead of the global currentTopLevelTapi pointer that we currently have. Is it because we might have a TBD file with child documents that re-exports another TBD file, also with child documents?

Yes, that's correct. Say you have A re-exports B, B (which has some child docs) reexports c (where c is defined in B)
If you keep the globalTopLevelTapi as A, then in reexportHelper, you can't find c in there.

lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libSystem.tbd

7–9

Yes, good point. This is TBD v4.
I've reverted changes to this file and added a new libReexportSystem (as suggested by jyknight)

[oontvoo]

reverted unneccesary API changes.

[oontvoo]

clang-format

[oontvoo]

updated commit message with more details

[int3]

lld/MachO/Driver.h

21

lld/MachO/InputFiles.cpp

543–544

since this comment no longer precedes a variable declaration, it's not entirely clear what it's referring to any more :)

546–551

this comment still seems relevant

707–708

naming convention is camelCase

708–709

Isn't the current interface having no parent the same as a TAPI being the top-level one?

710–713

nit: this could be a ternary

also, since there are only two levels in the document tree, I guess this should just be "second level exporters", no "+"

lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libReexportSystem.tbd

7

usually the install name matches the tbd name

9

lld/test/MachO/reexport-nested-lib.s

3

6

I like to

6

lt doesn't seem like libReexportSystem is actually being used in this test

Also, there doesn't seem to be a need to use the iPhoneSimulator SDK over the macOS SDK here -- %lld defaults to include the macOS SDK as syslibroot, so it's a bit more convenient to use that.

Another nit: I would rather libReexportSystem to not be placed under /usr/lib -- but I acknowledge there's no strong rationale here, I just like our fake /usr/lib to look as much like a realistic /usr/lib as possible. Personally I would place that tbd directly under %S/Inputs.

Finally, I think it's nice to test that the binary contains the right symbol binding via llvm-objdump --macho --bind, rather than just assuming it from a successful LLD exit code.

15

I would add a comment explaining why __crashreporter_info__ is an interesting symbol to test here

[int3]

lld/test/MachO/reexport-nested-lib.s

6

I like to

sorry, stray comment, ignore it

[oontvoo]

lld/test/MachO/reexport-nested-lib.s

6

lt doesn't seem like libReexportSystem is actually being used in this test

Also, there doesn't seem to be a need to use the iPhoneSimulator SDK over the macOS SDK here -- %lld defaults to include the macOS SDK as syslibroot, so it's a bit more convenient to use that.

Another nit: I would rather libReexportSystem to not be placed under /usr/lib -- but I acknowledge there's no strong rationale here, I just like our fake /usr/lib to look as much like a realistic /usr/lib as possible. Personally I would place that tbd directly under %S/Inputs.

Finally, I think it's nice to test that the binary contains the right symbol binding via llvm-objdump --macho --bind, rather than just assuming it from a successful LLD exit code.

[oontvoo]

Updated diff:

• fixed styling/comment issues
• updated test

[int3]

lld/test/MachO/reexport-nested-lib.s

6

Did you forget to type something? You just quoted my comment in its entirety :P

[oontvoo]

updated test to check for the symbols in result binary

[oontvoo]

lld/test/MachO/reexport-nested-lib.s

6

Did you forget to type something? You just quoted my comment in its entirety :P

Yes, sorry! I thought I typed the responses in-line. not sure why it's not showing up.

Anyway, re-posting it again:

lt doesn't seem like libReexportSystem is actually being used in this test

Fixed (updated the test to reference symbols from both the exporter and the exportee.

Also, there doesn't seem to be a need to use the iPhoneSimulator SDK over the macOS SDK here -- %lld defaults to include the macOS SDK as syslibroot, so it's a bit more convenient to use that.

The macOS SDK's libSystem.tbd doesn't have a "benign" symbol (like crash_reporter symbol) I could use, so I thought it was easier to use the iOS.

Another nit: I would rather libReexportSystem to not be placed under /usr/lib -- but I acknowledge there's no strong rationale here, I just like our fake /usr/lib to look as much like a realistic /usr/lib as possible. Personally I would place that tbd directly under %S/Inputs.
Finally, I think it's nice to test that the binary contains the right symbol binding via llvm-objdump --macho --bind, rather than just

assuming it from a successful LLD exit code.

Added the objdump CHECKs

[int3]

lld/test/MachO/reexport-nested-lib.s

20–22

I don't think this is testing the right thing. (The test passes for me even without your code changes applied.)

I believe the case we are trying to fix is as follows:

• libFooBar.tbd contains libFoo.dylib and libBar.dylib. libFoo re-exports libBar. libBar contains some symbol Bar.
• libReexportsFoo.tbd contains libReexportsFoo.dylib which re-exports libFoo. It doesn't matter whether it re-exports anything else.

Prior to this fix, we would have failed to look up libBar because we would be looking inside libReexportsFoo.tbd, instead of libFoo.tbd.

Right now we are testing that a symbol in libFoo can be resolved by the linker, but we actually need to test for a symbol that's in a library that libFoo (libSystem in your case) re-exports. And FWIW, the fake macOS libSystem has __nan which should fit the bill :)

P.S. it would be good to have a comment that explains the motivation behind the test. Right now the comment talks about re-exports but doesn't touch on the TBD files that contain them, which I think is the key bit.

[oontvoo]

Updated test. Also added a brief description on what it's testing

[oontvoo]

lld/test/MachO/reexport-nested-lib.s

20–22

Yeah, that's the right summary of it. :) Thanks.

Updated the test. Confirmed that w/o the patch, it'd fail with something like this:

--
lld: error: unable to locate re-export with install name /usr/lib/system/libdyld.dylib
lld: error: unable to locate re-export with install name /usr/lib/system/libsystem_c.dylib
lld: error: unable to locate re-export with install name /usr/lib/system/libsystem_m.dylib

[oontvoo]

fixed stale comment

[int3]

lgtm

lld/test/MachO/reexport-nested-lib.s

10

[oontvoo]

updated diff

lld/MachO/Driver.h

 #define LLD_MACHO_DRIVER_H
 
 #include "lld/Common/LLVM.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Support/MemoryBuffer.h"
 
+namespace llvm {
+namespace MachO {
+class InterfaceFile;
+} // namespace MachO
+} // namespace llvm
+
 namespace lld {
 namespace macho {
 
 class DylibFile;
 class InputFile;
 
 class MachOOptTable : public llvm::opt::OptTable {
 public:

lld/MachO/InputFiles.cpp

static Optional<DylibFile *> loadDylib(StringRef path, DylibFile *umbrella) {
   if (!mbref) {
     error("could not read dylib file at " + path);
     return {};
   }
   return loadDylib(*mbref, umbrella);
 }
 
 // TBD files are parsed into a series of TAPI documents (InterfaceFiles), with
 // the first document storing child pointers to the rest of them. When we are
-// processing a given TBD file, we store that top-level document here. When
-// processing re-exports, we search its children for potentially matching
-// documents in the same TBD file. Note that the children themselves don't
-// point to further documents, i.e. this is a two-level tree.
-//
-// ld64 allows a TAPI re-export to reference documents nested within other TBD
-// files, but that seems like a strange design, so this is an intentional
-// deviation.
-const InterfaceFile *currentTopLevelTapi = nullptr;
-
+// processing a given TBD file, we store that top-level document in
+// currentTopLevelTapi. When processing re-exports, we search its children for
+// potentially matching documents in the same TBD file. Note that the children
+// themselves don't point to further documents, i.e. this is a two-level tree.
+//
 // Re-exports can either refer to on-disk files, or to documents within .tbd
 // files.
-static Optional<DylibFile *> loadReexportHelper(StringRef path,
-                                                DylibFile *umbrella) {
+static Optional<DylibFile *>
+loadReexportHelper(StringRef path, DylibFile *umbrella,
+                   const InterfaceFile *currentTopLevelTapi) {
   if (path::is_absolute(path, path::Style::posix))
     for (StringRef root : config->systemLibraryRoots)
       if (Optional<std::string> dylibPath =
               resolveDylibPath((root + path).str()))
         return loadDylib(*dylibPath, umbrella);
 
   // TODO: Expand @loader_path, @executable_path etc
 
static bool isImplicitlyLinked(StringRef path) {
   if (path.consume_front("/System/Library/Frameworks/")) {
     StringRef frameworkName = path.take_until([](char c) { return c == '.'; });
     return path::filename(path) == frameworkName;
   }
 
   return false;
 }
 
-void loadReexport(StringRef path, DylibFile *umbrella) {
-  Optional<DylibFile *> reexport = loadReexportHelper(path, umbrella);
+void loadReexport(StringRef path, DylibFile *umbrella,
+                  const InterfaceFile *currentTopLevelTapi) {
+  Optional<DylibFile *> reexport =
+      loadReexportHelper(path, umbrella, currentTopLevelTapi);
   if (reexport && isImplicitlyLinked(path))
     inputFiles.insert(*reexport);
 }
 
 DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
                      bool isBundleLoader)
     : InputFile(DylibKind, mb), refState(RefState::Unreferenced),
       isBundleLoader(isBundleLoader) {
for (uint32_t i = 0, n = hdr->ncmds; i < n; ++i) {
     auto *cmd = reinterpret_cast<const load_command *>(p);
     p += cmd->cmdsize;
     if (cmd->cmd != LC_REEXPORT_DYLIB)
       continue;
 
     auto *c = reinterpret_cast<const dylib_command *>(cmd);
     StringRef reexportPath =
         reinterpret_cast<const char *>(c) + read32le(&c->dylib.name);
-    loadReexport(reexportPath, umbrella);
+    loadReexport(reexportPath, umbrella, nullptr);
   }
 }
 
 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.
case SymbolKind::ObjectiveCClassEHType:
       addSymbol(objc::ehtype + symbol->getName());
       break;
     case SymbolKind::ObjectiveCInstanceVariable:
       addSymbol(objc::ivar + symbol->getName());
       break;
     }
   }
 
-  bool isTopLevelTapi = false;
-  if (currentTopLevelTapi == nullptr) {
-    currentTopLevelTapi = &interface;
-    isTopLevelTapi = true;
-  }
+  const InterfaceFile *topLevel =
+      interface.getParent() == nullptr ? &interface : interface.getParent();
 
   for (InterfaceFileRef intfRef : interface.reexportedLibraries())
-    loadReexport(intfRef.getInstallName(), umbrella);
-
-  if (isTopLevelTapi)
-    currentTopLevelTapi = nullptr;
+    loadReexport(intfRef.getInstallName(), umbrella, topLevel);
 }
 
 ArchiveFile::ArchiveFile(std::unique_ptr<object::Archive> &&f)
     : InputFile(ArchiveKind, f->getMemoryBufferRef()), file(std::move(f)) {
   for (const object::Archive::Symbol &sym : file->symbols())
     symtab->addLazy(sym.getName(), this, sym);
 }
 
 void ArchiveFile::fetch(const object::Archive::Symbol &sym) {
   object::Archive::Child c =

lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libReexportSystem.tbd

+--- !tapi-tbd-v3
+archs:            [ i386, x86_64 ]
+uuids:            [ 'i386: 00000000-0000-0000-0000-000000000000', 'x86_64: 00000000-0000-0000-0000-000000000001' ]
+platform:         ios
+install-name:     '/usr/lib/libReexportSystem.dylib'
+exports:
+  - archs:      [ i386, x86_64 ]
+    re-exports: [ '/usr/lib/libSystem' ]
+    symbols:    [ __crashreporter_info__, _cache_create ]
+

lld/test/MachO/Inputs/iPhoneSimulator.sdk/usr/lib/libSystem.tbd

 --- !tapi-tbd-v3
 archs:            [ i386, x86_64 ]
 uuids:            [ 'i386: 00000000-0000-0000-0000-000000000000', 'x86_64: 00000000-0000-0000-0000-000000000001' ]
 platform:         ios
 install-name:     '/usr/lib/libSystem.B.dylib'
 current-version:  1281
 exports:
   - archs:      [ i386, x86_64 ]
-    re-exports: [ '/usr/lib/system/libcache.dylib' ]
-    symbols:    [ __crashreporter_info__ ]
+    re-exports: [ '/usr/lib/system/libcache.dylib', ]
+    symbols:    [ __crashreporter_info__, _cache_create ]
 --- !tapi-tbd-v3
 archs:            [ i386, x86_64 ]
 uuids:            [ 'i386: 00000000-0000-0000-0000-000000000002', 'x86_64: 00000000-0000-0000-0000-000000000003' ]
 platform:         ios
 install-name:     '/usr/lib/system/libcache.dylib'
 current-version:  83
 parent-umbrella:  System
 exports:
   - archs:      [ i386 ]
     symbols:    [ __cache_handle_memory_pressure_event ]
   - archs:      [ i386, x86_64 ]
     symbols:    [ _cache_create, _cache_destroy, _cache_get ]
 
 # The following TAPI document is not re-exported by any other document in this
 # TBD file, and should therefore be inaccessible.
 --- !tapi-tbd-v3
 archs:            [ i386, x86_64 ]
 uuids:            [ 'i386: 00000000-0000-0000-0000-000000000003', 'x86_64: 00000000-0000-0000-0000-000000000004' ]
 platform:         ios
 install-name:     '/usr/lib/libnotreexported.dylib'
 exports:
   - archs:      [ i386, x86_64 ]
     symbols:    [ _from_non_reexported_tapi_dylib ]
 ...

lld/test/MachO/reexport-nested-lib.s

+# REQUIRES: x86
+#
+# This 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: rm -rf %t; mkdir -p %t
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/test.o
+# RUN: %lld -o %t/test  -syslibroot %S/Inputs/iPhoneSimulator.sdk -lReexportSystem  %t/test.o
+# RUN: llvm-objdump %t/test --macho --bind %t/test | FileCheck %s
+
+# CHECK: segment  section   address            type        addend  dylib               symbol
+# CHECK: __DATA   __data    0x{{[0-9a-f]*}}    pointer     0       libReexportSystem __crashreporter_info__
+# CHECK: __DATA   __data    0x{{[0-9a-f]*}}    pointer     0       libReexportSystem _cache_create
+
+.text
+.globl _main
+
+_main:
+  ret
+
+.data
+// This symbol is from libSystem, which is re-exported by libReexportSystem.
+// Reference it here to verify that it is visible.
+.quad __crashreporter_info__
+
+// This symbol is from reexported fake_lib.dylib.
+.quad _cache_create

llvm/include/llvm/TextAPI/MachO/InterfaceFile.h

const std::vector<std::pair<Target, std::string>> &uuids() const {
     return UUIDs;
   }
 
   /// Add a library for inlining to top level library.
   ///
   ///\param Document The library to inline with top level library.
   void addDocument(std::shared_ptr<InterfaceFile> &&Document);
 
+  /// Returns the pointer to parent document if exists or nullptr otherwise.
+  InterfaceFile *getParent() const { return Parent; }
+
   /// Get the list of inlined libraries.
   ///
   /// \return Returns a list of the inlined frameworks.
   const std::vector<std::shared_ptr<InterfaceFile>> &documents() const {
     return Documents;
   }
 
   /// Add a symbol to the symbols list or extend an existing one.
private:
   bool IsInstallAPI{false};
   ObjCConstraintType ObjcConstraint = ObjCConstraintType::None;
   std::vector<std::pair<Target, std::string>> ParentUmbrellas;
   std::vector<InterfaceFileRef> AllowableClients;
   std::vector<InterfaceFileRef> ReexportedLibraries;
   std::vector<std::shared_ptr<InterfaceFile>> Documents;
   std::vector<std::pair<Target, std::string>> UUIDs;
   SymbolMapType Symbols;
+  InterfaceFile *Parent = nullptr;
 };
 
 template <typename DerivedT, typename KeyInfoT, typename BucketT>
 bool operator==(const DenseMapBase<DerivedT, SymbolsMapKey, MachO::Symbol *,
                                    KeyInfoT, BucketT> &LHS,
                 const DenseMapBase<DerivedT, SymbolsMapKey, MachO::Symbol *,
                                    KeyInfoT, BucketT> &RHS) {
   if (LHS.size() != RHS.size())

llvm/lib/TextAPI/MachO/InterfaceFile.cpp

 }
 
 void InterfaceFile::addDocument(std::shared_ptr<InterfaceFile> &&Document) {
   auto Pos = llvm::lower_bound(Documents, Document,
                                [](const std::shared_ptr<InterfaceFile> &LHS,
                                   const std::shared_ptr<InterfaceFile> &RHS) {
                                  return LHS->InstallName < RHS->InstallName;
                                });
+  Document->Parent = this;
   Documents.insert(Pos, Document);
 }
 
 bool InterfaceFile::operator==(const InterfaceFile &O) const {
     if (Targets != O.Targets)
       return false;
     if (InstallName != O.InstallName)
       return false;