[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

30

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

36

comment?

43

comment?

44

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

58

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

87

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

3998–3999

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

44

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

llvm/tools/dsymutil/DwarfLinker.cpp

3998–3999

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"
 
 namespace llvm {
 namespace dsymutil {
 
+class CachedBinaryHolder {
+public:
+  using TimestampTy = sys::TimePoint<std::chrono::seconds>;
+
+  CachedBinaryHolder(bool Verbose = false) : Verbose(Verbose) {}
+
+  class EntryBase {
+  protected:
+    std::unique_ptr<MemoryBuffer> MemoryBuffer;
+    std::unique_ptr<object::MachOUniversalBinary> FatBinary;
+    std::string FatBinaryName;
+  };
+
+  struct ObjectEntry : public EntryBase {
+    std::vector<std::unique_ptr<object::ObjectFile>> Objects;
+
+    Error load(StringRef Filename, bool Verbose = false);
+
+    /// Access all owned ObjectFiles.
+    std::vector<const object::ObjectFile *> getObjects() {
+      std::vector<const object::ObjectFile *> Result;
+      Result.reserve(Objects.size());
+      for (auto &Object : Objects) {
+        Result.push_back(Object.get());
+      }
+      return Result;
+    }
+
+    /// 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() {
+      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);
+
+    /// 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) {
+      auto Object = getObject(T);
+      if (!Object)
+        return Object.takeError();
+      return cast<ObjectFileType>(*Object);
+    }
+  };
+
+  struct ArchiveEntry : public EntryBase {
+    struct KeyTy {
+      StringRef Filename;
+      TimestampTy Timestamp;
+    };
+
+    Error load(StringRef Filename, TimestampTy Timestamp, bool Verbose = false);
+
+    Expected<ObjectEntry &> getObjectEntry(StringRef Filename,
+                                           TimestampTy Timestamp,
+                                           bool Verbose = false);
+
+  private:
+    std::vector<std::unique_ptr<object::Archive>> Archives;
+    DenseMap<KeyTy, ObjectEntry> ObjectCache;
+  };
+
+  Expected<ObjectEntry &> getObjectEntry(StringRef Filename,
+                                         TimestampTy Timestamp);
+
+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;
+
+  /// Object entries for objects that are not in a static archive.
+  StringMap<ObjectEntry> ObjectCache;
+
+  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:
   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();
+}
+
+Expected<const object::ObjectFile &>
+CachedBinaryHolder::ObjectEntry::getObject(const Triple &T) {
+  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<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};
+
+  if (ObjectCache.count(Key))
+    return ObjectCache[Key];
+
+  // Create a new ObjectEntry, but don't add it to the cache yet as loading
+  // the archive members might fail.
+  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);
+
+  ObjectCache.try_emplace(Key, std::move(OE));
+  return ObjectCache[Key];
+}
+
+Expected<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;
+    if (!ArchiveCache.count(ArchiveFilename)) {
+      // Create a new ArchiveEntry, but don't add it to the cache yet as
+      // loading might fail.
+      ArchiveEntry AE;
+      auto Err = AE.load(Filename, Timestamp, Verbose);
+      if (Err)
+        return std::move(Err);
+      ArchiveCache[ArchiveFilename] = std::move(AE);
+    }
+    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.
+  if (!ObjectCache.count(Filename)) {
+    // Create a new ObjectEntry, but don't add it to the cache yet as loading
+    // might fail.
+    ObjectEntry OE;
+    auto Err = OE.load(Filename);
+    if (Err)
+      return std::move(Err);
+    ObjectCache.try_emplace(Filename, std::move(OE));
+  }
+
+  return ObjectCache[Filename];
+}
+
 void BinaryHolder::changeBackingMemoryBuffer(
     std::unique_ptr<MemoryBuffer> &&Buf) {
   CurrentArchives.clear();
   CurrentObjectFiles.clear();
   CurrentFatBinary.reset();
 
   CurrentMemoryBuffer = std::move(Buf);
 }
 
 ErrorOr<std::vector<MemoryBufferRef>> BinaryHolder::GetMemoryBuffersForFile(
     StringRef Filename, sys::TimePoint<std::chrono::seconds> Timestamp) {
   if (Verbose)
     outs() << "trying to open '" << Filename << "'\n";
 
   // Try that first as it doesn't involve any filesystem access.
   if (auto ErrOrArchiveMembers = GetArchiveMemberBuffers(Filename, Timestamp))
     return *ErrOrArchiveMembers;
 
   // If the name ends with a closing paren, there is a huge chance
   // it is an archive member specification.
   if (Filename.endswith(")"))
     if (auto ErrOrArchiveMembers =
             MapArchiveAndGetMemberBuffers(Filename, Timestamp))
       return *ErrOrArchiveMembers;
 
   // Otherwise, just try opening a standard file. If this is an
   // archive member specifiaction and any of the above didn't handle it
   // (either because the archive is not there anymore, or because the
   // archive doesn't contain the requested member), this will still
   // provide a sensible error message.
   auto ErrOrFile = MemoryBuffer::getFileOrSTDIN(Filename);
   if (auto Err = ErrOrFile.getError())
     return Err;
 

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) {}
+      : OutFile(OutFile), Options(Options), BinHolder(Options.Verbose) {}
 
   /// 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) {
+                CachedBinaryHolder &BinHolder)
+        : 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);
       ObjectFile = ErrOrObj ? &*ErrOrObj : nullptr;
       DwarfContext = ObjectFile ? DWARFContext::create(*ObjectFile) : nullptr;
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,
-                                                 const DebugMap &Map);
+  ErrorOr<const object::ObjectFile &>
+  loadObject(CachedBinaryHolder &BinaryHolder, const DebugMapObject &Obj,
   /// @}
 
   raw_fd_ostream &OutFile;
   LinkOptions Options;
   std::unique_ptr<DwarfStreamer> Streamer;
+  CachedBinaryHolder BinHolder;
   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.
   StringMap<uint32_t> EmittedCIEs;
 
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(CachedBinaryHolder &BinaryHolder,
+                        const DebugMapObject &Obj, const DebugMap &Map) {
+  auto ObjectEntry =
+      BinaryHolder.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.
+  CachedBinaryHolder ObjHolder(Options.Verbose);
   auto &Obj = ModuleMap.addDebugMapObject(
       Path, sys::TimePoint<std::chrono::seconds>(), MachO::N_OSO);
   auto ErrOrObj = loadObject(ObjHolder, 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(")");
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(), BinHolder);
 
   // 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.