[lld/mac] Don't load DylibFiles from the DylibFile constructor

loadDylib() keeps a name->DylibFile cache, but it only writes
to the cache once the DylibFile constructor has completed.
So dylib loads done recursively from the DylibFile constructor
wouldn't use the cache.

Now, we load additional dylibs after writing to the cache,
which means the cache now gets used for dylibs loaded because
they're referenced from other dylibs.

Related to PR49514 and PR50101, but no dramatic behavior change in itself.
(Technically we no longer crash when a tbd file reexports itself,
but that doesn't happen in practice. We now accept it silently instead
of crashing; ld64 has a diag for the reexport cycle.)

Differential Revision: https://reviews.llvm.org/D103423

lld/MachO/DriverUtils.cpp

@@ -201,10 +201,11 @@ Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
                                        DylibFile *umbrella,
                                        bool isBundleLoader) {
   CachedHashStringRef path(mbref.getBufferIdentifier());
-  DylibFile *file = loadedDylibs[path];
+  DylibFile *&file = loadedDylibs[path];
   if (file)
     return file;
 
+  DylibFile *newFile;
   file_magic magic = identify_magic(mbref.getBuffer());
   if (magic == file_magic::tapi_file) {
     Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
@@ -214,19 +215,27 @@ Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
       return {};
     }
     file = make<DylibFile>(**result, umbrella, isBundleLoader);
+
+    // parseReexports() can recursively call loadDylib(). That's fine since
+    // we wrote DylibFile we just loaded to the loadDylib cache via the `file`
+    // reference. But the recursive load can grow loadDylibs, so the `file`
+    // reference might become invalid after parseReexports() -- so copy the
+    // pointer it refers to before going on.
+    newFile = file;
+    newFile->parseReexports(**result);
   } else {
     assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
            magic == file_magic::macho_dynamically_linked_shared_lib_stub ||
            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;
+    newFile->parseLoadCommands(mbref, umbrella);
   }
-  // Note that DylibFile's ctor may recursively invoke loadDylib(), which can
-  // cause loadedDylibs to get resized and its iterators invalidated. As such,
-  // we redo the key lookup here instead of caching an iterator from our earlier
-  // lookup at the start of the function.
-  loadedDylibs[path] = file;
-  return file;
+  return newFile;
 }
 
 Optional<StringRef>

lld/MachO/InputFiles.cpp

@@ -832,7 +832,7 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
     return;
 
   // Initialize symbols.
-  DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
+  exportingFile = isImplicitlyLinked(dylibName) ? 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,
@@ -846,9 +846,12 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
     error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
+}
 
-  const uint8_t *p =
-      reinterpret_cast<const uint8_t *>(hdr) + target->headerSize;
+void DylibFile::parseLoadCommands(MemoryBufferRef mb, DylibFile *umbrella) {
+  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;
@@ -877,6 +880,13 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
   }
 }
 
+// Some versions of XCode ship with .tbd files that don't have the right
+// platform settings.
+static constexpr std::array<StringRef, 3> skipPlatformChecks{
+    "/usr/lib/system/libsystem_kernel.dylib",
+    "/usr/lib/system/libsystem_platform.dylib",
+    "/usr/lib/system/libsystem_pthread.dylib"};
+
 DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
                      bool isBundleLoader)
     : InputFile(DylibKind, interface), refState(RefState::Unreferenced),
@@ -890,13 +900,6 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
   compatibilityVersion = interface.getCompatibilityVersion().rawValue();
   currentVersion = interface.getCurrentVersion().rawValue();
 
-  // Some versions of XCode ship with .tbd files that don't have the right
-  // platform settings.
-  static constexpr std::array<StringRef, 3> skipPlatformChecks{
-      "/usr/lib/system/libsystem_kernel.dylib",
-      "/usr/lib/system/libsystem_platform.dylib",
-      "/usr/lib/system/libsystem_pthread.dylib"};
-
   if (!is_contained(skipPlatformChecks, dylibName) &&
       !is_contained(interface.targets(), config->platformInfo.target)) {
     error(toString(this) + " is incompatible with " +
@@ -904,7 +907,7 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
     return;
   }
 
-  DylibFile *exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
+  exportingFile = isImplicitlyLinked(dylibName) ? this : umbrella;
   auto addSymbol = [&](const Twine &name) -> void {
     symbols.push_back(symtab->addDylib(saver.save(name), exportingFile,
                                        /*isWeakDef=*/false,
@@ -934,10 +937,11 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
       break;
     }
   }
+}
 
+void DylibFile::parseReexports(const llvm::MachO::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()) ||

lld/MachO/InputFiles.h

@@ -130,13 +130,6 @@ public:
 // .dylib file
 class DylibFile : public InputFile {
 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.
   explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
                      bool isBundleLoader = false);
 
@@ -144,9 +137,20 @@ public:
                      DylibFile *umbrella = nullptr,
                      bool isBundleLoader = false);
 
+  // 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 parseReexports(const llvm::MachO::InterfaceFile &interface);
+
   static bool classof(const InputFile *f) { return f->kind() == DylibKind; }
 
   StringRef dylibName;
+  DylibFile *exportingFile;
   uint32_t compatibilityVersion = 0;
   uint32_t currentVersion = 0;
   int64_t ordinal = 0; // Ordinal numbering starts from 1, so 0 is a sentinel