[Symbolize] Improve the ownership of parsed objects.

This code changes the way Symbolize handles parsed binaries: now
parsed OwningBinary<Binary> is not broken into (binary, memory buffer)
pair, and is just stored as-is in a cache. ObjectFile components
of Mach-O universal binaries are also stored explicitly in a
separate cache.

Additionally, this change:
* simplifies the code that parses/caches binaries: it's now done
  in a single place, not three different functions.
* makes flush() method behave as expected, and actually clear
  the cached parsed binaries and objects.
* fixes a dangling pointer issue described in
  http://reviews.llvm.org/D15638

llvm-svn: 256041

llvm/include/llvm/DebugInfo/Symbolize/Symbolize.h

@@ -13,13 +13,9 @@
 #ifndef LLVM_DEBUGINFO_SYMBOLIZE_SYMBOLIZE_H
 #define LLVM_DEBUGINFO_SYMBOLIZE_SYMBOLIZE_H
 
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/DebugInfo/DIContext.h"
 #include "llvm/DebugInfo/Symbolize/SymbolizableModule.h"
-#include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/ErrorOr.h"
-#include "llvm/Support/MemoryBuffer.h"
 #include <map>
 #include <memory>
 #include <string>
@@ -63,6 +59,8 @@ public:
                                   const SymbolizableModule *ModInfo);
 
 private:
+  // Bundles together object file with code/data and object file with
+  // corresponding debug info. These objects can be the same.
   typedef std::pair<ObjectFile*, ObjectFile*> ObjectPair;
 
   ErrorOr<SymbolizableModule *>
@@ -75,30 +73,29 @@ private:
                                     const std::string &ArchName);
 
   /// \brief Returns pair of pointers to object and debug object.
-  ErrorOr<ObjectPair> getOrCreateObjects(const std::string &Path,
-                                         const std::string &ArchName);
-  /// \brief Returns a parsed object file for a given architecture in a
-  /// universal binary (or the binary itself if it is an object file).
-  ErrorOr<ObjectFile *> getObjectFileFromBinary(Binary *Bin,
-                                                const std::string &ArchName);
+  ErrorOr<ObjectPair> getOrCreateObjectPair(const std::string &Path,
+                                            const std::string &ArchName);
 
-  // Owns all the parsed binaries and object files.
-  SmallVector<std::unique_ptr<Binary>, 4> ParsedBinariesAndObjects;
-  SmallVector<std::unique_ptr<MemoryBuffer>, 4> MemoryBuffers;
-  void addOwningBinary(OwningBinary<Binary> OwningBin) {
-    std::unique_ptr<Binary> Bin;
-    std::unique_ptr<MemoryBuffer> MemBuf;
-    std::tie(Bin, MemBuf) = OwningBin.takeBinary();
-    ParsedBinariesAndObjects.push_back(std::move(Bin));
-    MemoryBuffers.push_back(std::move(MemBuf));
-  }
+  /// \brief Return a pointer to object file at specified path, for a specified
+  /// architecture (e.g. if path refers to a Mach-O universal binary, only one
+  /// object file from it will be returned).
+  ErrorOr<ObjectFile *> getOrCreateObject(const std::string &Path,
+                                          const std::string &ArchName);
 
   std::map<std::string, ErrorOr<std::unique_ptr<SymbolizableModule>>> Modules;
-  std::map<std::pair<MachOUniversalBinary *, std::string>,
-           ErrorOr<ObjectFile *>> ObjectFileForArch;
+
+  /// \brief Contains cached results of getOrCreateObjectPair().
   std::map<std::pair<std::string, std::string>, ErrorOr<ObjectPair>>
       ObjectPairForPathArch;
 
+  /// \brief Contains parsed binary for each path, or parsing error.
+  std::map<std::string, ErrorOr<OwningBinary<Binary>>> BinaryForPath;
+
+  /// \brief Parsed object file for path/architecture pair, where "path" refers
+  /// to Mach-O universal binary.
+  std::map<std::pair<std::string, std::string>, ErrorOr<std::unique_ptr<ObjectFile>>>
+      ObjectForUBPathAndArch;
+
   Options Opts;
 };
 

llvm/lib/DebugInfo/Symbolize/Symbolize.cpp

@@ -22,6 +22,7 @@
 #include "llvm/DebugInfo/PDB/PDBContext.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/MachO.h"
+#include "llvm/Object/MachOUniversal.h"
 #include "llvm/Support/COFF.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compression.h"
@@ -109,9 +110,10 @@ ErrorOr<DIGlobal> LLVMSymbolizer::symbolizeData(const std::string &ModuleName,
 }
 
 void LLVMSymbolizer::flush() {
-  Modules.clear();
+  ObjectForUBPathAndArch.clear();
+  BinaryForPath.clear();
   ObjectPairForPathArch.clear();
-  ObjectFileForArch.clear();
+  Modules.clear();
 }
 
 // For Path="/path/to/foo" and Basename="foo" assume that debug info is in
@@ -223,23 +225,16 @@ ObjectFile *LLVMSymbolizer::lookUpDsymFile(const std::string &ExePath,
   for (const auto &Path : Opts.DsymHints) {
     DsymPaths.push_back(getDarwinDWARFResourceForPath(Path, Filename));
   }
-  for (const auto &path : DsymPaths) {
-    ErrorOr<OwningBinary<Binary>> BinaryOrErr = createBinary(path);
-    if (!BinaryOrErr)
-      continue;
-    OwningBinary<Binary> &B = BinaryOrErr.get();
-    auto DbgObjOrErr = getObjectFileFromBinary(B.getBinary(), ArchName);
+  for (const auto &Path : DsymPaths) {
+    auto DbgObjOrErr = getOrCreateObject(Path, ArchName);
     if (!DbgObjOrErr)
       continue;
     ObjectFile *DbgObj = DbgObjOrErr.get();
-    const MachOObjectFile *MachDbgObj =
-        dyn_cast<const MachOObjectFile>(DbgObj);
+    const MachOObjectFile *MachDbgObj = dyn_cast<const MachOObjectFile>(DbgObj);
     if (!MachDbgObj)
       continue;
-    if (darwinDsymMatchesBinary(MachDbgObj, MachExeObj)) {
-      addOwningBinary(std::move(B));
+    if (darwinDsymMatchesBinary(MachDbgObj, MachExeObj))
       return DbgObj;
-    }
   }
   return nullptr;
 }
@@ -254,40 +249,25 @@ ObjectFile *LLVMSymbolizer::lookUpDebuglinkObject(const std::string &Path,
     return nullptr;
   if (!findDebugBinary(Path, DebuglinkName, CRCHash, DebugBinaryPath))
     return nullptr;
-  ErrorOr<OwningBinary<Binary>> DebugBinaryOrErr =
-      createBinary(DebugBinaryPath);
-  if (!DebugBinaryOrErr)
-    return nullptr;
-  OwningBinary<Binary> &DB = DebugBinaryOrErr.get();
-  auto DbgObjOrErr = getObjectFileFromBinary(DB.getBinary(), ArchName);
+  auto DbgObjOrErr = getOrCreateObject(DebugBinaryPath, ArchName);
   if (!DbgObjOrErr)
     return nullptr;
-  addOwningBinary(std::move(DB));
   return DbgObjOrErr.get();
 }
 
 ErrorOr<LLVMSymbolizer::ObjectPair>
-LLVMSymbolizer::getOrCreateObjects(const std::string &Path,
-                                   const std::string &ArchName) {
+LLVMSymbolizer::getOrCreateObjectPair(const std::string &Path,
+                                      const std::string &ArchName) {
   const auto &I = ObjectPairForPathArch.find(std::make_pair(Path, ArchName));
   if (I != ObjectPairForPathArch.end())
     return I->second;
 
-  ErrorOr<OwningBinary<Binary>> BinaryOrErr = createBinary(Path);
-  if (auto EC = BinaryOrErr.getError()) {
-    ObjectPairForPathArch.insert(
-        std::make_pair(std::make_pair(Path, ArchName), EC));
-    return EC;
-  }
-  OwningBinary<Binary> &B = BinaryOrErr.get();
-
-  auto ObjOrErr = getObjectFileFromBinary(B.getBinary(), ArchName);
+  auto ObjOrErr = getOrCreateObject(Path, ArchName);
   if (auto EC = ObjOrErr.getError()) {
     ObjectPairForPathArch.insert(
         std::make_pair(std::make_pair(Path, ArchName), EC));
     return EC;
   }
-  addOwningBinary(std::move(B));
 
   ObjectFile *Obj = ObjOrErr.get();
   assert(Obj != nullptr);
@@ -306,24 +286,43 @@ LLVMSymbolizer::getOrCreateObjects(const std::string &Path,
 }
 
 ErrorOr<ObjectFile *>
-LLVMSymbolizer::getObjectFileFromBinary(Binary *Bin,
-                                        const std::string &ArchName) {
-  assert(Bin != nullptr);
-  if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(Bin)) {
-    const auto &I = ObjectFileForArch.find(
-        std::make_pair(UB, ArchName));
-    if (I != ObjectFileForArch.end())
-      return I->second;
-    ErrorOr<std::unique_ptr<ObjectFile>> ParsedObj =
-        UB->getObjectForArch(ArchName);
-    if (auto EC = ParsedObj.getError()) {
-      ObjectFileForArch.insert(
-          std::make_pair(std::make_pair(UB, ArchName), EC));
+LLVMSymbolizer::getOrCreateObject(const std::string &Path,
+                                  const std::string &ArchName) {
+  const auto &I = BinaryForPath.find(Path);
+  Binary *Bin = nullptr;
+  if (I == BinaryForPath.end()) {
+    ErrorOr<OwningBinary<Binary>> BinOrErr = createBinary(Path);
+    if (auto EC = BinOrErr.getError()) {
+      BinaryForPath.insert(std::make_pair(Path, EC));
       return EC;
     }
-    ObjectFile *Res = ParsedObj.get().get();
-    ParsedBinariesAndObjects.push_back(std::move(ParsedObj.get()));
-    ObjectFileForArch.insert(std::make_pair(std::make_pair(UB, ArchName), Res));
+    Bin = BinOrErr->getBinary();
+    BinaryForPath.insert(std::make_pair(Path, std::move(BinOrErr.get())));
+  } else if (auto EC = I->second.getError()) {
+    return EC;
+  } else {
+    Bin = I->second->getBinary();
+  }
+
+  assert(Bin != nullptr);
+
+  if (MachOUniversalBinary *UB = dyn_cast<MachOUniversalBinary>(Bin)) {
+    const auto &I = ObjectForUBPathAndArch.find(std::make_pair(Path, ArchName));
+    if (I != ObjectForUBPathAndArch.end()) {
+      if (auto EC = I->second.getError())
+        return EC;
+      return I->second->get();
+    }
+    ErrorOr<std::unique_ptr<ObjectFile>> ObjOrErr =
+        UB->getObjectForArch(ArchName);
+    if (auto EC = ObjOrErr.getError()) {
+      ObjectForUBPathAndArch.insert(
+          std::make_pair(std::make_pair(Path, ArchName), EC));
+      return EC;
+    }
+    ObjectFile *Res = ObjOrErr->get();
+    ObjectForUBPathAndArch.insert(std::make_pair(std::make_pair(Path, ArchName),
+                                                 std::move(ObjOrErr.get())));
     return Res;
   }
   if (Bin->isObject()) {
@@ -352,7 +351,7 @@ LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) {
       ArchName = ArchStr;
     }
   }
-  auto ObjectsOrErr = getOrCreateObjects(BinaryName, ArchName);
+  auto ObjectsOrErr = getOrCreateObjectPair(BinaryName, ArchName);
   if (auto EC = ObjectsOrErr.getError()) {
     // Failed to find valid object file.
     Modules.insert(std::make_pair(ModuleName, EC));