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

[thakis]

Summary

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.)

[thakis]

clang-format

[tschuett]

lld/MachO/DriverUtils.cpp

204

Do you really the cute & hack? Can't you always do loadedLibs[Path] = file;

[thakis]

lld/MachO/DriverUtils.cpp

204

It's the canonical way to write a caching function. So my thinking was we need to have a comment about this so that nobody changes this to using a reference naively, and if we have to add the comment anyways we might as well take what the comment pays for. But I don't feel terribly strongly about it.

[int3]

lgtm

lld/MachO/DriverUtils.cpp

 // It's not uncommon to have multiple attempts to load a single dylib,
 // especially if it's a commonly re-exported core library.
 static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
 
 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);
     if (!result) {
       error("could not load TAPI file at " + mbref.getBufferIdentifier() +
             ": " + toString(result.takeError()));
       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>
 macho::findPathCombination(const Twine &name,
                            const std::vector<StringRef> &roots,
                            ArrayRef<StringRef> extensions) {
   SmallString<261> base;
   for (StringRef dir : roots) {

lld/MachO/InputFiles.h

 public:
   OpaqueFile(MemoryBufferRef mb, StringRef segName, StringRef sectName);
   static bool classof(const InputFile *f) { return f->kind() == OpaqueKind; }
 };
 
 // .dylib file
 class DylibFile : public InputFile {
 public:
+  explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
+                     bool isBundleLoader = false);
+
+  explicit DylibFile(const llvm::MachO::InterfaceFile &interface,
+                     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.
-  explicit DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
-                     bool isBundleLoader = false);
-
-  explicit DylibFile(const llvm::MachO::InterfaceFile &interface,
-                     DylibFile *umbrella = nullptr,
-                     bool isBundleLoader = false);
+  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
   RefState refState;
   bool reexport = false;
   bool forceWeakImport = false;
 
   // An executable can be used as a bundle loader that will load the output

lld/MachO/InputFiles.cpp

if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     error("dylib " + toString(this) + " missing LC_ID_DYLIB load command");
     return;
   }
 
   if (!checkCompatibility(this))
     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,
               [&](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;
   }
+}
 
-  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;
 
     if (!(hdr->flags & MH_NO_REEXPORTED_DYLIBS) &&
         cmd->cmd == LC_REEXPORT_DYLIB) {
       const auto *c = reinterpret_cast<const dylib_command *>(cmd);
       StringRef reexportPath =
if (config->namespaceKind == NamespaceKind::flat &&
       Optional<DylibFile *> dylib = findDylib(dylibPath, umbrella, nullptr);
       if (!dylib)
         error(Twine("unable to locate library '") + dylibPath +
               "' loaded from '" + toString(this) + "' for -flat_namespace");
     }
   }
 }
 
+// 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),
       isBundleLoader(isBundleLoader) {
   // FIXME: Add test for the missing TBD code path.
 
   if (umbrella == nullptr)
     umbrella = this;
 
   dylibName = saver.save(interface.getInstallName());
   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 " +
           std::string(config->platformInfo.target));
     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,
                                        /*isTlv=*/false));
   };
   // TODO(compnerd) filter out symbols based on the target platform
   // TODO: handle weak defs, thread locals
   for (const auto *symbol : interface.symbols()) {
for (const auto *symbol : interface.symbols()) {
     case SymbolKind::ObjectiveCClassEHType:
       addSymbol(objc::ehtype + symbol->getName());
       break;
     case SymbolKind::ObjectiveCInstanceVariable:
       addSymbol(objc::ivar + symbol->getName());
       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()) ||
         is_contained(targets, config->platformInfo.target))
       loadReexport(intfRef.getInstallName(), exportingFile, topLevel);
   }
 }