Refactor/simplify llvm-symbolizer code.

[samsonov]

Summary

This change rewrites significant part of llvm-symbolizer code,
as it has grown convoluted after a number of recent changes.

Most significantly, it simplifies ownership strategy: parsed binaries
and objects are now wrapped in OwningBinary<> or std::unique_ptr and
are always owned by LLVMSymbolizer object (so there's no need for explicit
destructor), and LLVMSymbolizer::loadObject / loadBinary methods
encapsulate calls to methods of LLVMObject and return raw pointers,
to objects owned by LLVMSymbolizer.

[glider]

Looks mostly good

tools/llvm-symbolizer/LLVMSymbolize.cpp

339

I suggest to move the .gnu_debuglink part into a helper function so that findDbgObj is more readable.

347

"getBinaryPointer", maybe?

[samsonov]

Address comments.

tools/llvm-symbolizer/LLVMSymbolize.cpp

339

Done

347

Done

[samsonov]

Remove irrelevant file.

[glider]

LGTM

[samsonov]

Rebase to trunk.

[Eugene.Zelenko]

Looks like patch was not committed. Does it still make sense?

test/tools/llvm-symbolizer/errors.test

+RUN: echo "nonexisting 0x100" > %t.input
+RUN: echo "%s 0x100" >> %t.input
+
+RUN: llvm-symbolizer < %t.input 2>&1 | FileCheck %s
+
+CHECK: LLVMSymbolizer: error reading file: No such file or directory.
+CHECK: LLVMSymbolizer: error reading file: The file was not recognized as a valid object file.
+

tools/llvm-symbolizer/LLVMSymbolize.h

Options(bool UseSymbolTable = true,
             bool PrintInlining = true, bool Demangle = true,
             std::string DefaultArch = "")
         : UseSymbolTable(UseSymbolTable),
           PrintFunctions(PrintFunctions), PrintInlining(PrintInlining),
           Demangle(Demangle), DefaultArch(DefaultArch) {}
   };
 
   LLVMSymbolizer(const Options &Opts = Options()) : Opts(Opts) {}
-  ~LLVMSymbolizer() {
-    flush();
-  }
 
   // Returns the result of symbolization for module name/offset as
   // a string (possibly containing newlines).
-  std::string
-  symbolizeCode(const std::string &ModuleName, uint64_t ModuleOffset);
-  std::string
-  symbolizeData(const std::string &ModuleName, uint64_t ModuleOffset);
+  std::string symbolizeCode(const std::string &ModuleName,
+                            uint64_t ModuleOffset);
+  std::string symbolizeData(const std::string &ModuleName,
+                            uint64_t ModuleOffset);
   void flush();
   static std::string DemangleName(const std::string &Name);
-private:
-  typedef std::pair<ObjectFile*, ObjectFile*> ObjectPair;
 
+private:
   ModuleInfo *getOrCreateModuleInfo(const std::string &ModuleName);
-  ObjectFile *lookUpDsymFile(const std::string &Path, const MachOObjectFile *ExeObj,
-                             const std::string &ArchName);
-
-  /// \brief Returns pair of pointers to object and debug object.
-  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).
-  ObjectFile *getObjectFileFromBinary(Binary *Bin, const std::string &ArchName);
 
   std::string printDILineInfo(DILineInfo LineInfo) const;
 
-  // 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));
-  }
+  ErrorOr<const Binary *> loadBinary(const std::string &Path);
+  ErrorOr<const ObjectFile *> loadObjectFile(const std::string &Path,
+                                             const std::string &ArchName);
+
+  const ObjectFile *findDbgObj(const ObjectFile *Obj, const std::string &Path,
+                               const std::string &ArchName);
+  const ObjectFile *findDsymFile(const MachOObjectFile *ExeObj,
+                                 const std::string &Path,
+                                 const std::string &ArchName);
+  const ObjectFile *findDebuglinkFile(const ObjectFile *Obj,
+                                      const std::string &Path,
+                                      const std::string &ArchName);
 
-  // Owns module info objects.
-  std::map<std::string, ModuleInfo *> Modules;
-  std::map<std::pair<MachOUniversalBinary *, std::string>, ObjectFile *>
-      ObjectFileForArch;
-  std::map<std::pair<std::string, std::string>, ObjectPair>
-      ObjectPairForPathArch;
+  // Stores parsed binaries for each filepath.
+  std::map<std::string, ErrorOr<OwningBinary<Binary>>> LoadedBinaries;
+  // MachO object files, which are contained inside parsed MachO universal
+  // binaries.
+  SmallVector<std::unique_ptr<MachOObjectFile>, 4> ParsedMachOObjects;
+  // Stores ModuleInfo objects corresponding to filepath/architecture pair.
+  std::map<std::pair<std::string, std::string>, std::unique_ptr<ModuleInfo>>
+      ModuleForPathArch;
 
   Options Opts;
   static const char kBadString[];
 };
 
 class ModuleInfo {
 public:
-  ModuleInfo(ObjectFile *Obj, DIContext *DICtx);
+  ModuleInfo(const ObjectFile *Obj, const ObjectFile *DbgObj);
 
   DILineInfo symbolizeCode(uint64_t ModuleOffset,
                            const LLVMSymbolizer::Options &Opts) const;
   DIInliningInfo symbolizeInlinedCode(
       uint64_t ModuleOffset, const LLVMSymbolizer::Options &Opts) const;
   bool symbolizeData(uint64_t ModuleOffset, std::string &Name, uint64_t &Start,
                      uint64_t &Size) const;
 
 private:
   bool getNameFromSymbolTable(SymbolRef::Type Type, uint64_t Address,
                               std::string &Name, uint64_t &Addr,
                               uint64_t &Size) const;
   void addSymbol(const SymbolRef &Symbol);
-  ObjectFile *Module;
+  const ObjectFile *Module;
   std::unique_ptr<DIContext> DebugInfoContext;
 
   struct SymbolDesc {
     uint64_t Addr;
     // If size is 0, assume that symbol occupies the whole memory range up to
     // the following symbol.
     uint64_t Size;
     friend bool operator<(const SymbolDesc &s1, const SymbolDesc &s2) {

tools/llvm-symbolizer/LLVMSymbolize.cpp

 
 static DILineInfoSpecifier
 getDILineInfoSpecifier(const LLVMSymbolizer::Options &Opts) {
   return DILineInfoSpecifier(
       DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath,
       Opts.PrintFunctions);
 }
 
-ModuleInfo::ModuleInfo(ObjectFile *Obj, DIContext *DICtx)
-    : Module(Obj), DebugInfoContext(DICtx) {
+ModuleInfo::ModuleInfo(const ObjectFile *Obj, const ObjectFile *DbgObj)
+    : Module(Obj), DebugInfoContext(DIContext::getDWARFContext(*DbgObj)) {
+  assert(DebugInfoContext);
   for (const SymbolRef &Symbol : Module->symbols()) {
     addSymbol(Symbol);
   }
   bool NoSymbolTable = (Module->symbol_begin() == Module->symbol_end());
   if (NoSymbolTable && Module->isELF()) {
     // Fallback to dynamic symbol table, if regular symbol table is stripped.
     std::pair<symbol_iterator, symbol_iterator> IDyn =
         getELFDynamicSymbolIterators(Module);
bool ModuleInfo::getNameFromSymbolTable(SymbolRef::Type Type, uint64_t Address,
   Name = SymbolIterator->second.str();
   Addr = SymbolIterator->first.Addr;
   Size = SymbolIterator->first.Size;
   return true;
 }
 
 DILineInfo ModuleInfo::symbolizeCode(
     uint64_t ModuleOffset, const LLVMSymbolizer::Options &Opts) const {
-  DILineInfo LineInfo;
-  if (DebugInfoContext) {
-    LineInfo = DebugInfoContext->getLineInfoForAddress(
-        ModuleOffset, getDILineInfoSpecifier(Opts));
-  }
+  DILineInfo LineInfo = DebugInfoContext->getLineInfoForAddress(
   // Override function name from symbol table if necessary.
   if (Opts.PrintFunctions != FunctionNameKind::None && Opts.UseSymbolTable) {
     std::string FunctionName;
     uint64_t Start, Size;
     if (getNameFromSymbolTable(SymbolRef::ST_Function, ModuleOffset,
                                FunctionName, Start, Size)) {
       LineInfo.FunctionName = FunctionName;
     }
   }
   return LineInfo;
 }
 
 DIInliningInfo ModuleInfo::symbolizeInlinedCode(
     uint64_t ModuleOffset, const LLVMSymbolizer::Options &Opts) const {
-  DIInliningInfo InlinedContext;
-  if (DebugInfoContext) {
-    InlinedContext = DebugInfoContext->getInliningInfoForAddress(
-        ModuleOffset, getDILineInfoSpecifier(Opts));
-  }
+  DIInliningInfo InlinedContext = DebugInfoContext->getInliningInfoForAddress(
   // Make sure there is at least one frame in context.
   if (InlinedContext.getNumberOfFrames() == 0) {
     InlinedContext.addFrame(DILineInfo());
   }
   // Override the function name in lower frame with name from symbol table.
   if (Opts.PrintFunctions != FunctionNameKind::None && Opts.UseSymbolTable) {
     DIInliningInfo PatchedInlinedContext;
     for (uint32_t i = 0, n = InlinedContext.getNumberOfFrames(); i < n; i++) {
if (Opts.UseSymbolTable) {
     }
   }
   std::stringstream ss;
   ss << Name << "\n" << Start << " " << Size << "\n";
   return ss.str();
 }
 
 void LLVMSymbolizer::flush() {
-  DeleteContainerSeconds(Modules);
-  ObjectPairForPathArch.clear();
-  ObjectFileForArch.clear();
+  ModuleForPathArch.clear();
+  ParsedMachOObjects.clear();
+  LoadedBinaries.clear();
 }
 
 // For Path="/path/to/foo" and Basename="foo" assume that debug info is in
 // /path/to/foo.dSYM/Contents/Resources/DWARF/foo.
 // For Path="/path/to/bar.dSYM" and Basename="foo" assume that debug info is in
 // /path/to/bar.dSYM/Contents/Resources/DWARF/foo.
 static
 std::string getDarwinDWARFResourceForPath(
if (checkFileCRC(DebugPath, CRCHash)) {
     Result = DebugPath.str();
     return true;
   }
   return false;
 }
 
 static bool getGNUDebuglinkContents(const ObjectFile *Obj, std::string &DebugName,
                                     uint32_t &CRCHash) {
-  if (!Obj)
-    return false;
   for (const SectionRef &Section : Obj->sections()) {
     StringRef Name;
     Section.getName(Name);
     Name = Name.substr(Name.find_first_not_of("._"));
     if (Name == "gnu_debuglink") {
       StringRef Data;
       Section.getContents(Data);
       DataExtractor DE(Data, Obj->isLittleEndian(), 0);
bool darwinDsymMatchesBinary(const MachOObjectFile *DbgObj,
                              const MachOObjectFile *Obj) {
   ArrayRef<uint8_t> dbg_uuid = DbgObj->getUuid();
   ArrayRef<uint8_t> bin_uuid = Obj->getUuid();
   if (dbg_uuid.empty() || bin_uuid.empty())
     return false;
   return !memcmp(dbg_uuid.data(), bin_uuid.data(), dbg_uuid.size());
 }
 
-ObjectFile *LLVMSymbolizer::lookUpDsymFile(const std::string &ExePath,
-    const MachOObjectFile *MachExeObj, const std::string &ArchName) {
+const ObjectFile *
+LLVMSymbolizer::findDsymFile(const MachOObjectFile *MachExeObj,
+                             const std::string &ExePath,
+                             const std::string &ArchName) {
   // On Darwin we may find DWARF in separate object file in
   // resource directory.
   std::vector<std::string> DsymPaths;
   StringRef Filename = sys::path::filename(ExePath);
   DsymPaths.push_back(getDarwinDWARFResourceForPath(ExePath, Filename));
   for (const auto &Path : Opts.DsymHints) {
     DsymPaths.push_back(getDarwinDWARFResourceForPath(Path, Filename));
   }
-  for (const auto &path : DsymPaths) {
-    ErrorOr<OwningBinary<Binary>> BinaryOrErr = createBinary(path);
-    std::error_code EC = BinaryOrErr.getError();
-    if (EC != errc::no_such_file_or_directory && !error(EC)) {
-      OwningBinary<Binary> B = std::move(BinaryOrErr.get());
-      ObjectFile *DbgObj =
-          getObjectFileFromBinary(B.getBinary(), ArchName);
-      const MachOObjectFile *MachDbgObj =
-          dyn_cast<const MachOObjectFile>(DbgObj);
-      if (!MachDbgObj) continue;
-      if (darwinDsymMatchesBinary(MachDbgObj, MachExeObj)) {
-        addOwningBinary(std::move(B));
-        return DbgObj; 
-      }
-    }
+  for (const auto &Path : DsymPaths) {
+    auto DbgObjOrErr = loadObjectFile(Path, ArchName);
+    if (!DbgObjOrErr)
+      continue;
+        dyn_cast<const MachOObjectFile>(DbgObjOrErr.get());
+    if (MachDbgObj && darwinDsymMatchesBinary(MachDbgObj, MachExeObj))
+      return MachDbgObj;
   }
   return nullptr;
 }
 
-LLVMSymbolizer::ObjectPair
-LLVMSymbolizer::getOrCreateObjects(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;
-  ObjectFile *Obj = nullptr;
-  ObjectFile *DbgObj = nullptr;
-  ErrorOr<OwningBinary<Binary>> BinaryOrErr = createBinary(Path);
-  if (!error(BinaryOrErr.getError())) {
-    OwningBinary<Binary> &B = BinaryOrErr.get();
-    Obj = getObjectFileFromBinary(B.getBinary(), ArchName);
-    if (!Obj) {
-      ObjectPair Res = std::make_pair(nullptr, nullptr);
-      ObjectPairForPathArch[std::make_pair(Path, ArchName)] = Res;
-      return Res;
-    }
-    addOwningBinary(std::move(B));
-    if (auto MachObj = dyn_cast<const MachOObjectFile>(Obj))
-      DbgObj = lookUpDsymFile(Path, MachObj, ArchName);
-    // Try to locate the debug binary using .gnu_debuglink section.
-    if (!DbgObj) {
-      std::string DebuglinkName;
-      uint32_t CRCHash;
-      std::string DebugBinaryPath;
-      if (getGNUDebuglinkContents(Obj, DebuglinkName, CRCHash) &&
-          findDebugBinary(Path, DebuglinkName, CRCHash, DebugBinaryPath)) {
-        BinaryOrErr = createBinary(DebugBinaryPath);
-        if (!error(BinaryOrErr.getError())) {
-          OwningBinary<Binary> B = std::move(BinaryOrErr.get());
-          DbgObj = getObjectFileFromBinary(B.getBinary(), ArchName);
-          addOwningBinary(std::move(B));
-        }
-      }
-    }
+const ObjectFile *
+LLVMSymbolizer::findDebuglinkFile(const ObjectFile *Obj,
+                                  const std::string &Path,
+    if (auto DbgObjOrErr = loadObjectFile(DebugBinaryPath, ArchName))
+      return DbgObjOrErr.get();
+  }
+  return nullptr;
+}
+
+const ObjectFile *LLVMSymbolizer::findDbgObj(const ObjectFile *Obj,
+                                             const std::string &Path,
+                                             const std::string &ArchName) {
+  assert(Obj);
+  // Try to find .dSYM bundle for the file.
+  if (auto MachObj = dyn_cast<MachOObjectFile>(Obj)) {
+    if (const ObjectFile *DbgObj = findDsymFile(MachObj, Path, ArchName))
+      return DbgObj;
+  // Try to find debug binary from .gnu_debuglink section.
+  if (const ObjectFile *DbgObj = findDebuglinkFile(Obj, Path, ArchName))
+    return DbgObj;
+  // If no external object is found, return the original object itself.
+  return Obj;
+
+// Extracts Binary* from ErrorOr<OwningBinary<Binary>>, or propagates error.
+static ErrorOr<const Binary *>
+getPointerToBinary(const ErrorOr<OwningBinary<Binary>> &BinaryOrErr) {
+  if (BinaryOrErr)
+    return BinaryOrErr.get().getBinary();
+  return BinaryOrErr.getError();
+
+ErrorOr<const Binary *> LLVMSymbolizer::loadBinary(const std::string &Path) {
+  const auto &I = LoadedBinaries.find(Path);
+  if (I != LoadedBinaries.end()) {
+    return getPointerToBinary(I->second);
   }
-  if (!DbgObj)
-    DbgObj = Obj;
-  ObjectPair Res = std::make_pair(Obj, DbgObj);
-  ObjectPairForPathArch[std::make_pair(Path, ArchName)] = Res;
-  return Res;
+  const auto &InsertPt =
+      LoadedBinaries.insert(std::make_pair(Path, createBinary(Path)));
+  assert(InsertPt.second);
+  return getPointerToBinary(InsertPt.first->second);
 }
 
-ObjectFile *
-LLVMSymbolizer::getObjectFileFromBinary(Binary *Bin,
-                                        const std::string &ArchName) {
-  if (!Bin)
-    return nullptr;
-  ObjectFile *Res = 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(Triple(ArchName).getArch());
-    if (ParsedObj) {
-      Res = ParsedObj.get().get();
-      ParsedBinariesAndObjects.push_back(std::move(ParsedObj.get()));
-    }
-    ObjectFileForArch[std::make_pair(UB, ArchName)] = Res;
+ErrorOr<const ObjectFile *>
+LLVMSymbolizer::loadObjectFile(const std::string &Path,
+  ErrorOr<const Binary *> BinaryOrErr = loadBinary(Path);
+  if (!BinaryOrErr)
+    return BinaryOrErr.getError();
+  const Binary *Bin = BinaryOrErr.get();
+
+  if (const auto *UB = dyn_cast<MachOUniversalBinary>(Bin)) {
+    auto Res = UB->getObjectForArch(Triple(ArchName).getArch());
+    if (!Res)
+      return Res.getError();
+    ParsedMachOObjects.push_back(std::move(Res.get()));
+    return ParsedMachOObjects.back().get();
   } else if (Bin->isObject()) {
-    Res = cast<ObjectFile>(Bin);
+    return cast<ObjectFile>(Bin);
   }
-  return Res;
+  return object_error::invalid_file_type;
 }
 
-ModuleInfo *
-LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) {
-  const auto &I = Modules.find(ModuleName);
-  if (I != Modules.end())
-    return I->second;
-  std::string BinaryName = ModuleName;
-  std::string ArchName = Opts.DefaultArch;
-  size_t ColonPos = ModuleName.find_last_of(':');
-  // Verify that substring after colon form a valid arch name.
+static std::pair<std::string, std::string>
+getPathArchPair(const std::string &Path, const std::string &DefaultArch) {
+  size_t ColonPos = Path.find_last_of(':');
   if (ColonPos != std::string::npos) {
-    std::string ArchStr = ModuleName.substr(ColonPos + 1);
-    if (Triple(ArchStr).getArch() != Triple::UnknownArch) {
-      BinaryName = ModuleName.substr(0, ColonPos);
-      ArchName = ArchStr;
-    }
-  }
-  ObjectPair Objects = getOrCreateObjects(BinaryName, ArchName);
+    std::string ArchStr = Path.substr(ColonPos + 1);
+    // Verify that substring after colon form a valid arch name.
+    if (Triple(ArchStr).getArch() != Triple::UnknownArch)
+      return std::make_pair(Path.substr(0, ColonPos), ArchStr);
+  return std::make_pair(Path, DefaultArch);
 
-  if (!Objects.first) {
-    // Failed to find valid object file.
-    Modules.insert(make_pair(ModuleName, (ModuleInfo *)nullptr));
+ModuleInfo *
+LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) {
+  auto PathArchPair = getPathArchPair(ModuleName, Opts.DefaultArch);
+  const auto &I = ModuleForPathArch.find(PathArchPair);
+  if (I != ModuleForPathArch.end())
+    return I->second.get();
+
+  // Get obj/debug_obj pair.
+  auto ObjOrError = loadObjectFile(PathArchPair.first, PathArchPair.second);
+  if (!ObjOrError) {
+    error(ObjOrError.getError());
+    ModuleForPathArch.insert(
+        std::make_pair(PathArchPair, std::unique_ptr<ModuleInfo>()));
     return nullptr;
   }
-  DIContext *Context = DIContext::getDWARFContext(*Objects.second);
-  assert(Context);
-  ModuleInfo *Info = new ModuleInfo(Objects.first, Context);
-  Modules.insert(make_pair(ModuleName, Info));
-  return Info;
+  const ObjectFile *Obj = ObjOrError.get();
+  const ObjectFile *DbgObj =
+      findDbgObj(Obj, PathArchPair.first, PathArchPair.second);
+  assert(DbgObj);
+
+  const auto &II = ModuleForPathArch.insert(
+      std::make_pair(PathArchPair, make_unique<ModuleInfo>(Obj, DbgObj)));
+  assert(II.second);
+  return II.first->second.get();
 }
 
 std::string LLVMSymbolizer::printDILineInfo(DILineInfo LineInfo) const {
   // By default, DILineInfo contains "<invalid>" for function/filename it
   // cannot fetch. We replace it to "??" to make our output closer to addr2line.
   static const std::string kDILineInfoBadString = "<invalid>";
   std::stringstream Result;
   if (Opts.PrintFunctions != FunctionNameKind::None) {