[dsymutil] Introduce a new CachedBinaryHolder

[JDevlieghere]

Summary

The original binary holder has an optimization where it caches a static
library (archive) between consecutive calls to GetObjects. However, the
actual memory buffer wasn't cached between calls.

This made sense when dsymutil was processing objects one after each
other, but when processing them in parallel, several binaries have to be
in memory at the same time. For this reason, every link context
contained a binary holder.

Having one binary holder per context is problematic, because the same
static archive was cached for every object file. Luckily, when the file
is mmap'ed, this was only costing us virtual memory.

This patch introduces a new BinaryHolder variant that is fully cached,
for all the object files it load, as well as the static archives. This
way, we don't have to give up on this optimization of bypassing the
file system. Currently I only updated the uses in the DwarfLinker, but
my goal is to phase out the exisiting BinaryHolder (let's call it the
LegacyBinaryHolder ;-) in favor of this one.

[JDevlieghere]

I want to do some more extensive testing before landing this, but I think it's at a point where it is ready to be reviewed.

[aprantl]

llvm/tools/dsymutil/BinaryHolder.h

32

Could you add a Doxygen comment explaining what this class is for?

38

comment?

45

comment?

46

Could this be a std::vector<object::ObjectFile> instead?

60

As a casual reader it's no-obvious to me what a "derived version" could be.

89

comment? :-)

[friss]

While you're rewriting this part of the code, why not make the new cache actually thread-safe and share it between the threads handling different architectures. They will open the same files. This way you could also use it in loadClangModule.

In the initial design, the BinaryHolder would have only one file open at a time, now we're keeping them alive basically forever. Any concerns with this?

llvm/tools/dsymutil/DwarfLinker.cpp

4000–4002

It's not clear from reading the comment why it's not thread-safe here. I *think* I know why (this is called by the cloning thread, which is not the thread updating the BinaryHolder), but it would be good to be explicit about it so that others can understand it too.
It would be good to document in another comment somewhere why the other CachedBinaryHolder is used in a thread-safe way. I suppose there's only ever one thread writing to it, but that's not stated anywhere. It would be even better to have a dynamic check that this is actually the case.

[JDevlieghere]

• Shared CachedBinaryHolder between architectures as suggested by Fred.
• Address other CR feedback

[JDevlieghere]

llvm/tools/dsymutil/BinaryHolder.h

46

No, we move it into the unique pointer from the loading logic.

llvm/tools/dsymutil/DwarfLinker.cpp

4000–4002

You are indeed correct. I didn't add the comment as I implemented your suggestion and made the cached binary holder thread safe.

[JDevlieghere]

• Fallback to loading file as an object if loading of archive fails.

[friss]

Why not transition completely to the new implementation and remove the old code in one step?

Should we have a limited number of files that we keep open? Some systems might run with a low number of allowed open files per-process.

This code is supposed to handle a lot of weird cases, can you make sure that all of this still works:

• fat object files
• thin archive containing fat object-files
• fat archive containing thin objectfiles

(I think the current code handles all of them, but I might be wrong. Please double-check)

llvm/tools/dsymutil/BinaryHolder.cpp

254–259

Do we need a clear() method as this object basically never dies?

[JDevlieghere]

In D48501#1145789, @friss wrote:

Why not transition completely to the new implementation and remove the old code in one step?

I wanted to do that in a separate patch: https://reviews.llvm.org/D48770

Should we have a limited number of files that we keep open? Some systems might run with a low number of allowed open files per-process.

The files are closed immediately after being read by the MemoryBuffer. While we keep the files in memory the FD is already closed.

This code is supposed to handle a lot of weird cases, can you make sure that all of this still works:

• fat object files
• thin archive containing fat object-files
• fat archive containing thin objectfiles

(I think the current code handles all of them, but I might be wrong. Please double-check)

This logic didn't change, all of them are still supported. (I paid special attention to this while doing the refactoring)

[JDevlieghere]

• Rebase

[friss]

This looks good to me, modulo my question in D48770.

[JDevlieghere]

• Move lifetime fix into this patch.

llvm/tools/dsymutil/BinaryHolder.h

 //===-- BinaryHolder.h - Utility class for accessing binaries -------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This program is a utility that aims to be a dropin replacement for
 // Darwin's dsymutil.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_TOOLS_DSYMUTIL_BINARYHOLDER_H
 #define LLVM_TOOLS_DSYMUTIL_BINARYHOLDER_H
 
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorOr.h"
 
+#include <mutex>
+
 namespace llvm {
 namespace dsymutil {
 
+/// The CachedBinaryHolder class is responsible for creating and owning
+/// ObjectFiles and their underlying MemoryBuffers. It differs from a simple
+/// OwningBinary in that it handles accessing and caching of archives and its
+/// members.
+class CachedBinaryHolder {
+public:
+  using TimestampTy = sys::TimePoint<std::chrono::seconds>;
+
+  CachedBinaryHolder(bool Verbose = false) : Verbose(Verbose) {}
+
+  // Forward declarations for friend declaration.
+  class ObjectEntry;
+  class ArchiveEntry;
+
+  /// Base class shared by cached entries, representing objects and archives.
+  class EntryBase {
+  protected:
+    std::unique_ptr<MemoryBuffer> MemoryBuffer;
+    std::unique_ptr<object::MachOUniversalBinary> FatBinary;
+    std::string FatBinaryName;
+  };
+
+  /// Cached entry holding one or more (in case of a fat binary) object files.
+  class ObjectEntry : public EntryBase {
+  public:
+    /// Load the given object binary in memory.
+    Error load(StringRef Filename, bool Verbose = false);
+
+    /// Access all owned ObjectFiles.
+    std::vector<const object::ObjectFile *> getObjects() const;
+
+    /// Access to a derived version of all the currently owned ObjectFiles. The
+    /// conversion might be invalid, in which case an Error is returned.
+    template <typename ObjectFileType>
+    Expected<std::vector<const ObjectFileType *>> getObjectsAs() const {
+      std::vector<const object::ObjectFile *> Result;
+      Result.reserve(Objects.size());
+      for (auto &Object : Objects) {
+        const auto *Derived = dyn_cast<ObjectFileType>(Object.get());
+        if (!Derived)
+          return errorCodeToError(object::object_error::invalid_file_type);
+        Result.push_back(Derived);
+      }
+      return Result;
+    }
+
+    /// Access the owned ObjectFile with architecture \p T.
+    Expected<const object::ObjectFile &> getObject(const Triple &T) const;
+
+    /// Access to a derived version of the currently owned ObjectFile with
+    /// architecture \p T. The conversion must be known to be valid.
+    template <typename ObjectFileType>
+    Expected<const ObjectFileType &> getObjectAs(const Triple &T) const {
+      auto Object = getObject(T);
+      if (!Object)
+        return Object.takeError();
+      return cast<ObjectFileType>(*Object);
+    }
+
+  private:
+    std::vector<std::unique_ptr<object::ObjectFile>> Objects;
+    friend ArchiveEntry;
+  };
+
+  /// Cached entry holding one or more (in the of a fat binary) archive files.
+  class ArchiveEntry : public EntryBase {
+  public:
+    struct KeyTy {
+      StringRef Filename;
+      TimestampTy Timestamp;
+    };
+
+    /// Load the given object binary in memory.
+    Error load(StringRef Filename, TimestampTy Timestamp, bool Verbose = false);
+
+    Expected<const ObjectEntry &> getObjectEntry(StringRef Filename,
+                                                 TimestampTy Timestamp,
+                                                 bool Verbose = false);
+
+  private:
+    std::vector<std::unique_ptr<object::Archive>> Archives;
+    DenseMap<KeyTy, ObjectEntry> MemberCache;
+    std::mutex MemberCacheMutex;
+  };
+
+  Expected<const ObjectEntry &> getObjectEntry(StringRef Filename,
+                                               TimestampTy Timestamp);
+
+  void clear();
+
+private:
+  /// Cache of static archives. Objects that are part of a static archive are
+  /// stored under this object, rather than in the map below.
+  StringMap<ArchiveEntry> ArchiveCache;
+  std::mutex ArchiveCacheMutex;
+
+  /// Object entries for objects that are not in a static archive.
+  StringMap<ObjectEntry> ObjectCache;
+  std::mutex ObjectCacheMutex;
+
+  bool Verbose;
+};
+
 /// The BinaryHolder class is responsible for creating and owning ObjectFile
 /// objects and their underlying MemoryBuffer. This is different from a simple
 /// OwningBinary in that it handles accessing to archive members.
 ///
 /// As an optimization, this class will reuse an already mapped and parsed
 /// Archive object if 2 successive requests target the same archive file (Which
 /// is always the case in debug maps).
 /// Currently it only owns one memory buffer at any given time, meaning that a
public:
 
   /// Access the currently owned ObjectFile with architecture \p T. As
   /// successful call to GetObjectFiles() or GetFilesAs() must have
   /// been performed before calling this.
   ErrorOr<const object::ObjectFile &> Get(const Triple &T) {
     return getObjfileForArch(T);
   }
 
-  /// Access to a derived version of the currently owned
-  /// ObjectFile. The conversion must be known to be valid.
+  /// Get and cast to a subclass of the currently owned ObjectFile. The
+  /// conversion must be known to be valid.
   template <typename ObjectFileType>
   ErrorOr<const ObjectFileType &> GetAs(const Triple &T) {
     auto ErrOrObj = Get(T);
     if (auto Err = ErrOrObj.getError())
       return Err;
     return cast<ObjectFileType>(*ErrOrObj);
   }
 };
 } // namespace dsymutil
+
+template <>
+struct DenseMapInfo<dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy> {
+
+  static inline dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy
+  getEmptyKey() {
+    return dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy();
+  }
+
+  static inline dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy
+  getTombstoneKey() {
+    return dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy();
+  }
+
+  static unsigned
+  getHashValue(const dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy &K) {
+    return hash_combine(DenseMapInfo<StringRef>::getHashValue(K.Filename),
+                        DenseMapInfo<unsigned>::getHashValue(
+                            K.Timestamp.time_since_epoch().count()));
+  }
+
+  static bool
+  isEqual(const dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy &LHS,
+          const dsymutil::CachedBinaryHolder::ArchiveEntry::KeyTy &RHS) {
+    return LHS.Filename == RHS.Filename && LHS.Timestamp == RHS.Timestamp;
+  }
+};
+
 } // namespace llvm
 #endif

llvm/tools/dsymutil/BinaryHolder.cpp

 //===-- BinaryHolder.cpp --------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This program is a utility that aims to be a dropin replacement for
 // Darwin's dsymutil.
 //
 //===----------------------------------------------------------------------===//
 
 #include "BinaryHolder.h"
 #include "llvm/Object/MachO.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 namespace dsymutil {
 
+static std::pair<StringRef, StringRef>
+getArchiveAndObjectName(StringRef Filename) {
+  StringRef Archive = Filename.substr(0, Filename.find('('));
+  StringRef Object = Filename.substr(Archive.size() + 1).drop_back();
+  return {Archive, Object};
+}
+
+static bool isArchive(StringRef Filename) { return Filename.endswith(")"); }
+
 static std::vector<MemoryBufferRef>
 getMachOFatMemoryBuffers(StringRef Filename, MemoryBuffer &Mem,
                          object::MachOUniversalBinary &Fat) {
   std::vector<MemoryBufferRef> Buffers;
   StringRef FatData = Fat.getData();
   for (auto It = Fat.begin_objects(), End = Fat.end_objects(); It != End;
        ++It) {
     StringRef ObjData = FatData.substr(It->getOffset(), It->getSize());
     Buffers.emplace_back(ObjData, Filename);
   }
   return Buffers;
 }
 
+Error CachedBinaryHolder::ArchiveEntry::load(StringRef Filename,
+                                             TimestampTy Timestamp,
+                                             bool Verbose) {
+  StringRef ArchiveFilename = getArchiveAndObjectName(Filename).first;
+
+  // Try to load archive and force it to be memory mapped.
+  auto ErrOrBuff = MemoryBuffer::getFileOrSTDIN(ArchiveFilename, -1, false);
+  if (auto Err = ErrOrBuff.getError())
+    return errorCodeToError(Err);
+
+  MemoryBuffer = std::move(*ErrOrBuff);
+
+  if (Verbose)
+    WithColor::note() << "opened archive '" << ArchiveFilename << "'\n";
+
+  // Load one or more archive buffers, depending on whether we're dealing with
+  // a fat binary.
+  std::vector<MemoryBufferRef> ArchiveBuffers;
+
+  auto ErrOrFat =
+      object::MachOUniversalBinary::create(MemoryBuffer->getMemBufferRef());
+  if (!ErrOrFat) {
+    consumeError(ErrOrFat.takeError());
+    ArchiveBuffers.push_back(MemoryBuffer->getMemBufferRef());
+  } else {
+    FatBinary = std::move(*ErrOrFat);
+    FatBinaryName = ArchiveFilename;
+    ArchiveBuffers =
+        getMachOFatMemoryBuffers(FatBinaryName, *MemoryBuffer, *FatBinary);
+  }
+
+  // Finally, try to load the archives.
+  Archives.reserve(ArchiveBuffers.size());
+  for (auto MemRef : ArchiveBuffers) {
+    auto ErrOrArchive = object::Archive::create(MemRef);
+    if (!ErrOrArchive)
+      return ErrOrArchive.takeError();
+    Archives.push_back(std::move(*ErrOrArchive));
+  }
+
+  return Error::success();
+}
+
+Error CachedBinaryHolder::ObjectEntry::load(StringRef Filename, bool Verbose) {
+  // Try to load regular binary and force it to be memory mapped.
+  auto ErrOrBuff = MemoryBuffer::getFileOrSTDIN(Filename, -1, false);
+  if (auto Err = ErrOrBuff.getError())
+    return errorCodeToError(Err);
+
+  MemoryBuffer = std::move(*ErrOrBuff);
+
+  if (Verbose)
+    WithColor::note() << "opened object.\n";
+
+  // Load one or more object buffers, depending on whether we're dealing with a
+  // fat binary.
+  std::vector<MemoryBufferRef> ObjectBuffers;
+
+  auto ErrOrFat =
+      object::MachOUniversalBinary::create(MemoryBuffer->getMemBufferRef());
+  if (!ErrOrFat) {
+    consumeError(ErrOrFat.takeError());
+    ObjectBuffers.push_back(MemoryBuffer->getMemBufferRef());
+  } else {
+    FatBinary = std::move(*ErrOrFat);
+    FatBinaryName = Filename;
+    ObjectBuffers =
+        getMachOFatMemoryBuffers(FatBinaryName, *MemoryBuffer, *FatBinary);
+  }
+
+  Objects.reserve(ObjectBuffers.size());
+  for (auto MemRef : ObjectBuffers) {
+    auto ErrOrObjectFile = object::ObjectFile::createObjectFile(MemRef);
+    if (!ErrOrObjectFile)
+      return ErrOrObjectFile.takeError();
+    Objects.push_back(std::move(*ErrOrObjectFile));
+  }
+
+  return Error::success();
+}
+
+std::vector<const object::ObjectFile *>
+CachedBinaryHolder::ObjectEntry::getObjects() const {
+  std::vector<const object::ObjectFile *> Result;
+  Result.reserve(Objects.size());
+  for (auto &Object : Objects) {
+    Result.push_back(Object.get());
+  }
+  return Result;
+}
+Expected<const object::ObjectFile &>
+CachedBinaryHolder::ObjectEntry::getObject(const Triple &T) const {
+  for (const auto &Obj : Objects) {
+    if (const auto *MachO = dyn_cast<object::MachOObjectFile>(Obj.get())) {
+      if (MachO->getArchTriple().str() == T.str())
+        return *MachO;
+    } else if (Obj->getArch() == T.getArch())
+      return *Obj;
+  }
+  return errorCodeToError(object::object_error::arch_not_found);
+}
+
+Expected<const CachedBinaryHolder::ObjectEntry &>
+CachedBinaryHolder::ArchiveEntry::getObjectEntry(StringRef Filename,
+                                                 TimestampTy Timestamp,
+                                                 bool Verbose) {
+  StringRef ArchiveFilename;
+  StringRef ObjectFilename;
+  std::tie(ArchiveFilename, ObjectFilename) = getArchiveAndObjectName(Filename);
+
+  // Try the cache first.
+  KeyTy Key = {ObjectFilename, Timestamp};
+
+  {
+    std::lock_guard<std::mutex> Lock(MemberCacheMutex);
+    if (MemberCache.count(Key))
+      return MemberCache[Key];
+  }
+
+  // Create a new ObjectEntry, but don't add it to the cache yet. Loading of
+  // the archive members might fail and we don't want to lock the whole archive
+  // during this operation.
+  ObjectEntry OE;
+
+  for (const auto &Archive : Archives) {
+    Error Err = Error::success();
+    for (auto Child : Archive->children(Err)) {
+      if (auto NameOrErr = Child.getName()) {
+        if (*NameOrErr == ObjectFilename) {
+          auto ModTimeOrErr = Child.getLastModified();
+          if (!ModTimeOrErr)
+            return ModTimeOrErr.takeError();
+
+          if (Timestamp != sys::TimePoint<>() &&
+              Timestamp != ModTimeOrErr.get()) {
+            if (Verbose)
+              WithColor::warning() << "member has timestamp mismatch.\n";
+            continue;
+          }
+
+          if (Verbose)
+            WithColor::note() << "found member in current archive.\n";
+
+          auto ErrOrMem = Child.getMemoryBufferRef();
+          if (!ErrOrMem)
+            return ErrOrMem.takeError();
+
+          auto ErrOrObjectFile =
+              object::ObjectFile::createObjectFile(*ErrOrMem);
+          if (!ErrOrObjectFile)
+            return ErrOrObjectFile.takeError();
+
+          OE.Objects.push_back(std::move(*ErrOrObjectFile));
+        }
+      }
+    }
+    if (Err)
+      return std::move(Err);
+  }
+
+  if (OE.Objects.empty())
+    return errorCodeToError(errc::no_such_file_or_directory);
+
+  std::lock_guard<std::mutex> Lock(MemberCacheMutex);
+  MemberCache.try_emplace(Key, std::move(OE));
+  return MemberCache[Key];
+}
+
+Expected<const CachedBinaryHolder::ObjectEntry &>
+CachedBinaryHolder::getObjectEntry(StringRef Filename, TimestampTy Timestamp) {
+  if (Verbose)
+    WithColor::note() << "trying to open '" << Filename << "'\n";
+
+  // If this is an archive, we might have either the object or the archive
+  // cached. In this case we can load it without accessing the file system.
+  if (isArchive(Filename)) {
+    StringRef ArchiveFilename = getArchiveAndObjectName(Filename).first;
+    std::lock_guard<std::mutex> Lock(ArchiveCacheMutex);
+    if (ArchiveCache.count(ArchiveFilename)) {
+      return ArchiveCache[ArchiveFilename].getObjectEntry(Filename, Timestamp);
+    } else {
+      ArchiveEntry &AE = ArchiveCache[ArchiveFilename];
+      auto Err = AE.load(Filename, Timestamp, Verbose);
+      if (Err) {
+        ArchiveCache.erase(ArchiveFilename);
+        // Don't return the error here: maybe the file wasn't an archive.
+        llvm::consumeError(std::move(Err));
+      } else {
+        return ArchiveCache[ArchiveFilename].getObjectEntry(Filename,
+                                                            Timestamp);
+      }
+    }
+  }
+
+  // If this is an object, we might have it cached. If not we'll have to load
+  // it from the file system and cache it now.
+  std::lock_guard<std::mutex> Lock(ObjectCacheMutex);
+  if (!ObjectCache.count(Filename)) {
+    ObjectEntry &OE = ObjectCache[Filename];
+    auto Err = OE.load(Filename);
+    if (Err) {
+      ObjectCache.erase(Filename);
+      return std::move(Err);
+    }
+  }
+
+  return ObjectCache[Filename];
+}
+
+void CachedBinaryHolder::clear() {
+  std::lock_guard<std::mutex> ArchiveLock(ArchiveCacheMutex);
+  std::lock_guard<std::mutex> ObjectLock(ObjectCacheMutex);
+  ArchiveCache.clear();
+  ObjectCache.clear();
+}
+
 void BinaryHolder::changeBackingMemoryBuffer(
     std::unique_ptr<MemoryBuffer> &&Buf) {
   CurrentArchives.clear();
   CurrentObjectFiles.clear();
   CurrentFatBinary.reset();
 
   CurrentMemoryBuffer = std::move(Buf);
 }

llvm/tools/dsymutil/DwarfLinker.cpp

 ///
 /// The root DIEs are identified because they contain relocations that
 /// correspond to a debug map entry at specific places (the low_pc for
 /// a function, the location for a variable). These relocations are
 /// called ValidRelocs in the DwarfLinker and are gathered as a very
 /// first step when we start processing a DebugMapObject.
 class DwarfLinker {
 public:
-  DwarfLinker(raw_fd_ostream &OutFile, const LinkOptions &Options)
-      : OutFile(OutFile), Options(Options) {}
+  DwarfLinker(raw_fd_ostream &OutFile, CachedBinaryHolder &BinHolder,
+              const LinkOptions &Options)
+      : OutFile(OutFile), BinHolder(BinHolder), Options(Options) {}
 
   /// Link the contents of the DebugMap.
   bool link(const DebugMap &);
 
   void reportWarning(const Twine &Warning, const DebugMapObject &DMO,
                      const DWARFDie *DIE = nullptr) const;
 
 private:
public:
 
     bool applyValidRelocs(MutableArrayRef<char> Data, uint32_t BaseOffset,
                           bool isLittleEndian);
   };
 
   /// Keeps track of data associated with one object during linking.
   struct LinkContext {
     DebugMapObject &DMO;
-    BinaryHolder BinHolder;
     const object::ObjectFile *ObjectFile;
     RelocationManager RelocMgr;
     std::unique_ptr<DWARFContext> DwarfContext;
     RangesTy Ranges;
     UnitListTy CompileUnits;
 
-    LinkContext(const DebugMap &Map, DwarfLinker &Linker, DebugMapObject &DMO,
-                bool Verbose = false)
-        : DMO(DMO), BinHolder(Verbose), RelocMgr(Linker) {
+    LinkContext(const DebugMap &Map, DwarfLinker &Linker, DebugMapObject &DMO)
+        : DMO(DMO), RelocMgr(Linker) {
       // Swift ASTs are not object files.
       if (DMO.getType() == MachO::N_AST) {
         ObjectFile = nullptr;
         return;
       }
-      auto ErrOrObj = Linker.loadObject(BinHolder, DMO, Map);
+      auto ErrOrObj = Linker.loadObject(DMO, Map);
       ObjectFile = ErrOrObj ? &*ErrOrObj : nullptr;
       DwarfContext = ObjectFile ? DWARFContext::create(*ObjectFile) : nullptr;
     }
 
     /// Clear compile units and ranges.
     void Clear() {
       CompileUnits.clear();
       Ranges.clear();
private:
   /// @}
 
   /// \defgroup Helpers Various helper methods.
   ///
   /// @{
   bool createStreamer(const Triple &TheTriple, raw_fd_ostream &OutFile);
 
   /// Attempt to load a debug object from disk.
-  ErrorOr<const object::ObjectFile &> loadObject(BinaryHolder &BinaryHolder,
-                                                 const DebugMapObject &Obj,
+  ErrorOr<const object::ObjectFile &> loadObject(const DebugMapObject &Obj,
                                                  const DebugMap &Map);
   /// @}
 
   raw_fd_ostream &OutFile;
+  CachedBinaryHolder &BinHolder;
   LinkOptions Options;
   std::unique_ptr<DwarfStreamer> Streamer;
   uint64_t OutputDebugInfoSize;
   unsigned MaxDwarfVersion = 0;
 
   /// The CIEs that have been emitted in the output section. The actual CIE
   /// data serves a the key to this StringMap, this takes care of comparing the
   /// semantics of CIEs defined in different object files.
if (Error E = loadClangModule(PCMfile, PCMpath, Name, DwoId, ModuleMap, DMO,
                                 ODRContexts, UnitID, Indent + 2)) {
     consumeError(std::move(E));
     return false;
   }
   return true;
 }
 
 ErrorOr<const object::ObjectFile &>
-DwarfLinker::loadObject(BinaryHolder &BinaryHolder, const DebugMapObject &Obj,
-                        const DebugMap &Map) {
-  auto ErrOrObjs =
-      BinaryHolder.GetObjectFiles(Obj.getObjectFilename(), Obj.getTimestamp());
-  if (std::error_code EC = ErrOrObjs.getError()) {
-    reportWarning(Twine(Obj.getObjectFilename()) + ": " + EC.message(), Obj);
-    return EC;
-  }
-  auto ErrOrObj = BinaryHolder.Get(Map.getTriple());
-  if (std::error_code EC = ErrOrObj.getError())
-    reportWarning(Twine(Obj.getObjectFilename()) + ": " + EC.message(), Obj);
-  return ErrOrObj;
+DwarfLinker::loadObject(const DebugMapObject &Obj, const DebugMap &Map) {
+  auto ObjectEntry =
+      BinHolder.getObjectEntry(Obj.getObjectFilename(), Obj.getTimestamp());
+  if (!ObjectEntry) {
+    auto Err = ObjectEntry.takeError();
+    reportWarning(
+        Twine(Obj.getObjectFilename()) + ": " + toString(std::move(Err)), Obj);
+    return errorToErrorCode(std::move(Err));
+  }
+
+  auto Object = ObjectEntry->getObject(Map.getTriple());
+  if (!Object) {
+    auto Err = Object.takeError();
+    reportWarning(
+        Twine(Obj.getObjectFilename()) + ": " + toString(std::move(Err)), Obj);
+    return errorToErrorCode(std::move(Err));
+  }
+
+  return *Object;
 }
 
 Error DwarfLinker::loadClangModule(StringRef Filename, StringRef ModulePath,
                                    StringRef ModuleName, uint64_t DwoId,
                                    DebugMap &ModuleMap,
                                    const DebugMapObject &DMO, RangesTy &Ranges,
                                    OffsetsStringPool &StringPool,
                                    UniquingStringPool &UniquingStringPool,
                                    DeclContextTree &ODRContexts,
                                    unsigned &UnitID, unsigned Indent) {
   SmallString<80> Path(Options.PrependPath);
   if (sys::path::is_relative(Filename))
     sys::path::append(Path, ModulePath, Filename);
   else
     sys::path::append(Path, Filename);
-  BinaryHolder ObjHolder(Options.Verbose);
+  // Don't use the cached binary holder because we have no thread-safety
+  // guarantee and the lifetime is limited.
   auto &Obj = ModuleMap.addDebugMapObject(
       Path, sys::TimePoint<std::chrono::seconds>(), MachO::N_OSO);
-  auto ErrOrObj = loadObject(ObjHolder, Obj, ModuleMap);
+  auto ErrOrObj = loadObject(Obj, ModuleMap);
   if (!ErrOrObj) {
     // Try and emit more helpful warnings by applying some heuristics.
     StringRef ObjFile = DMO.getObjectFilename();
     bool isClangModule = sys::path::extension(Filename).equals(".pcm");
     bool isArchive = ObjFile.endswith(")");
     if (isClangModule) {
       StringRef ModuleCacheDir = sys::path::parent_path(Path);
       if (sys::fs::exists(ModuleCacheDir)) {
bool DwarfLinker::link(const DebugMap &Map) {
   DebugMap ModuleMap(Map.getTriple(), Map.getBinaryPath());
 
   // First populate the data structure we need for each iteration of the
   // parallel loop.
   unsigned NumObjects = Map.getNumberOfObjects();
   std::vector<LinkContext> ObjectContexts;
   ObjectContexts.reserve(NumObjects);
   for (const auto &Obj : Map.objects())
-    ObjectContexts.emplace_back(Map, *this, *Obj.get(), Options.Verbose);
+    ObjectContexts.emplace_back(Map, *this, *Obj.get());
 
   // This Dwarf string pool which is only used for uniquing. This one should
   // never be used for offsets as its not thread-safe or predictable.
   UniquingStringPool UniquingStringPool;
 
   // This Dwarf string pool which is used for emission. It must be used
   // serially as the order of calling getStringOffset matters for
   // reproducibility.
if (Options.Threads == 1) {
     pool.async(AnalyzeLambda);
     pool.async(CloneLambda);
     pool.wait();
   }
 
   return Options.NoOutput ? true : Streamer->finish(Map);
 }
 
-bool linkDwarf(raw_fd_ostream &OutFile, const DebugMap &DM,
-               const LinkOptions &Options) {
-  DwarfLinker Linker(OutFile, Options);
+bool linkDwarf(raw_fd_ostream &OutFile, CachedBinaryHolder &BinHolder,
+               const DebugMap &DM, const LinkOptions &Options) {
+  DwarfLinker Linker(OutFile, BinHolder, Options);
   return Linker.link(DM);
 }
 
 } // namespace dsymutil
 } // namespace llvm

llvm/tools/dsymutil/dsymutil.h

 #include "llvm/Support/ErrorOr.h"
 #include <memory>
 #include <string>
 #include <vector>
 
 namespace llvm {
 namespace dsymutil {
 
+class CachedBinaryHolder;
+
 struct LinkOptions {
   /// Verbosity
   bool Verbose = false;
 
   /// Skip emitting output
   bool NoOutput = false;
 
   /// Do not unique types according to ODR
parseDebugMap(StringRef InputFile, ArrayRef<std::string> Archs,
               bool InputIsYAML);
 
 /// Dump the symbol table
 bool dumpStab(StringRef InputFile, ArrayRef<std::string> Archs,
               StringRef PrependPath = "");
 
 /// Link the Dwarf debug info as directed by the passed DebugMap \p DM into a
 /// DwarfFile named \p OutputFilename. \returns false if the link failed.
-bool linkDwarf(raw_fd_ostream &OutFile, const DebugMap &DM,
-               const LinkOptions &Options);
+bool linkDwarf(raw_fd_ostream &OutFile, CachedBinaryHolder &BinHolder,
+               const DebugMap &DM, const LinkOptions &Options);
 
 void warn(Twine Warning, Twine Context = {});
 bool error(Twine Error, Twine Context = {});
 
 } // end namespace dsymutil
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_DSYMUTIL_DSYMUTIL_H

llvm/tools/dsymutil/dsymutil.cpp

 //===- dsymutil.cpp - Debug info dumping utility for llvm -----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This program is a utility that aims to be a dropin replacement for
 // Darwin's dsymutil.
 //
 //===----------------------------------------------------------------------===//
 
 #include "dsymutil.h"
+#include "BinaryHolder.h"
 #include "CFBundle.h"
 #include "DebugMap.h"
 #include "MachOUtils.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
for (auto &InputFile : *InputsOrErr) {
     }
 
     // Ensure that the debug map is not empty (anymore).
     if (DebugMapPtrsOrErr->empty()) {
       WithColor::error() << "no architecture to link\n";
       return 1;
     }
 
+    // Shared a single binary holder for all the link steps.
+    CachedBinaryHolder BinHolder;
+
     NumThreads =
         std::min<unsigned>(OptionsOrErr->Threads, DebugMapPtrsOrErr->size());
     llvm::ThreadPool Threads(NumThreads);
 
     // If there is more than one link to execute, we need to generate
     // temporary files.
     bool NeedsTempFiles =
         !DumpDebugMap && (OutputFileOpt != "-") &&
for (auto &Map : *DebugMapPtrsOrErr) {
         if (EC) {
           errs() << OutputFile << ": " << EC.message();
           return 1;
         }
       }
 
       auto LinkLambda = [&,
                          OutputFile](std::shared_ptr<raw_fd_ostream> Stream) {
-        AllOK.fetch_and(linkDwarf(*Stream, *Map, *OptionsOrErr));
+        AllOK.fetch_and(linkDwarf(*Stream, BinHolder, *Map, *OptionsOrErr));
         Stream->flush();
         if (Verify && !NoOutput)
           AllOK.fetch_and(verify(OutputFile, Map->getTriple().getArchName()));
       };
 
       // FIXME: The DwarfLinker can have some very deep recursion that can max
       // out the (significantly smaller) stack when using threads. We don't
       // want this limitation when we only have a single thread.