[llvm-addr2line] Replace checkFileExists with getOrCreateModuleInfo

[MaskRay]

Summary

GNU addr2line exits immediately if -e (default to a.out) specifies a file that
cannot be open or a directory. llvm-addr2line used to wait for input on if the
input file cannot be open and addresses are not specified in command line.
Replace the D147652 checkFileExists with getOrCreateModuleInfo to avoid
a separate sys::fs::status operation.

[jhenderson]

Looks good, but I'm wondering if it would be just better for @sepavloff to integrate this change into their original patch, as that patch has a fair amount of useful discussion on it.

Regardless, I'd like @sepavloff to sign off on this.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

482

[sepavloff]

The patch requires rebase because D147652 has committed, but the logic is clear and seems correct.

LGTM.

[MaskRay]

rebase to restore some code before D147652

[Jake-Egan]

Hi, on AIX we're getting the error: "The file was not recognized as a valid object file instead of the expected error"

Could you take a look please?

https://lab.llvm.org/buildbot/#/builders/214/builds/8134/steps/6/logs/FAIL__LLVM__input-file-err_test

[sepavloff]

In D153595#4448648, @Jake-Egan wrote:

Hi, on AIX we're getting the error: "The file was not recognized as a valid object file instead of the expected error"

It probably relates to https://reviews.llvm.org/D151567.

[Jake-Egan]

@sepavloff Thanks, that should address it

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

public:
   static std::string
   DemangleName(const std::string &Name,
                const SymbolizableModule *DbiModuleDescriptor);
 
   void setBuildIDFetcher(std::unique_ptr<BuildIDFetcher> Fetcher) {
     BIDFetcher = std::move(Fetcher);
   }
 
-  Error checkFileExists(StringRef Name);
+  /// Returns a SymbolizableModule or an error if loading debug info failed.
+  /// Only one attempt is made to load a module, and errors during loading are
+  /// only reported once. Subsequent calls to get module info for a module that
+  /// failed to load will return nullptr.
+  Expected<SymbolizableModule *>
+  getOrCreateModuleInfo(const std::string &ModuleName);
 
 private:
   // Bundles together object file with code/data and object file with
   // corresponding debug info. These objects can be the same.
   using ObjectPair = std::pair<const ObjectFile *, const ObjectFile *>;
 
   template <typename T>
   Expected<DILineInfo>
   symbolizeCodeCommon(const T &ModuleSpecifier,
                       object::SectionedAddress ModuleOffset);
   template <typename T>
   Expected<DIInliningInfo>
   symbolizeInlinedCodeCommon(const T &ModuleSpecifier,
                              object::SectionedAddress ModuleOffset);
   template <typename T>
   Expected<DIGlobal> symbolizeDataCommon(const T &ModuleSpecifier,
                                          object::SectionedAddress ModuleOffset);
   template <typename T>
   Expected<std::vector<DILocal>>
   symbolizeFrameCommon(const T &ModuleSpecifier,
                        object::SectionedAddress ModuleOffset);
 
-  /// Returns a SymbolizableModule or an error if loading debug info failed.
-  /// Only one attempt is made to load a module, and errors during loading are
-  /// only reported once. Subsequent calls to get module info for a module that
-  /// failed to load will return nullptr.
-  Expected<SymbolizableModule *>
-  getOrCreateModuleInfo(const std::string &ModuleName);
   Expected<SymbolizableModule *> getOrCreateModuleInfo(const ObjectFile &Obj);
 
   /// Returns a SymbolizableModule or an error if loading debug info failed.
   /// Unlike the above, errors are reported each time, since they are more
   /// likely to be transient.
   Expected<SymbolizableModule *>
   getOrCreateModuleInfo(ArrayRef<uint8_t> BuildID);
 
private:
   /// architecture (e.g. if path refers to a Mach-O universal binary, only one
   /// object file from it will be returned).
   Expected<ObjectFile *> getOrCreateObject(const std::string &Path,
                                            const std::string &ArchName);
 
   /// Update the LRU cache order when a binary is accessed.
   void recordAccess(CachedBinary &Bin);
 
-  /// Split binary file name into file and architecture parts. For example,
-  /// the name 'macho-universal:i386', will be split into 'macho-universal' and
-  /// 'i386'.
-  std::pair<std::string, std::string> splitBinaryFileName(StringRef Name);
-
   std::map<std::string, std::unique_ptr<SymbolizableModule>, std::less<>>
       Modules;
   StringMap<std::string> BuildIDPaths;
 
   /// Contains cached results of getOrCreateObjectPair().
   std::map<std::pair<std::string, std::string>, ObjectPair>
       ObjectPairForPathArch;
 

llvm/lib/DebugInfo/Symbolize/Symbolize.cpp

LLVMSymbolizer::createModuleInfo(const ObjectFile *Obj,
   auto InsertResult = Modules.insert(
       std::make_pair(std::string(ModuleName), std::move(SymMod)));
   assert(InsertResult.second);
   if (!InfoOrErr)
     return InfoOrErr.takeError();
   return InsertResult.first->second.get();
 }
 
-std::pair<std::string, std::string>
-LLVMSymbolizer::splitBinaryFileName(StringRef Name) {
-  StringRef BinaryName = Name;
+Expected<SymbolizableModule *>
+LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) {
+  std::string BinaryName = ModuleName;
   std::string ArchName = Opts.DefaultArch;
-  size_t ColonPos = Name.find_last_of(':');
-  if (ColonPos != StringRef::npos) {
-    StringRef ArchStr = Name.substr(ColonPos + 1);
+  size_t ColonPos = ModuleName.find_last_of(':');
+  // Verify that substring after colon form a valid arch name.
+  if (ColonPos != std::string::npos) {
+    std::string ArchStr = ModuleName.substr(ColonPos + 1);
     if (Triple(ArchStr).getArch() != Triple::UnknownArch) {
-      BinaryName = Name.substr(0, ColonPos);
+      BinaryName = ModuleName.substr(0, ColonPos);
       ArchName = ArchStr;
     }
   }
-  return std::make_pair(BinaryName.str(), ArchName);
-}
-
-Expected<SymbolizableModule *>
-LLVMSymbolizer::getOrCreateModuleInfo(const std::string &ModuleName) {
-  auto [BinaryName, ArchName] = splitBinaryFileName(ModuleName);
 
   auto I = Modules.find(ModuleName);
   if (I != Modules.end()) {
     recordAccess(BinaryForPath.find(BinaryName)->second);
     return I->second.get();
   }
 
   auto ObjectsOrErr = getOrCreateObjectPair(BinaryName, ArchName);
if (DbiModuleDescriptor && DbiModuleDescriptor->isWin32Module()) {
     // may also be applied on top of the Itanium or Rust name mangling.
     if (nonMicrosoftDemangle(DemangledCName, Result))
       return Result;
     return DemangledCName;
   }
   return Name;
 }
 
-Error LLVMSymbolizer::checkFileExists(StringRef BinName) {
-  auto [BinaryName, ArchName] = splitBinaryFileName(BinName);
-  sys::fs::file_status Stat;
-  std::error_code EC = sys::fs::status(BinaryName, Stat);
-  if (!EC && sys::fs::is_directory(Stat))
-    EC = errc::is_a_directory;
-  return errorCodeToError(EC);
-}
-
 void LLVMSymbolizer::recordAccess(CachedBinary &Bin) {
   if (Bin->getBinary())
     LRUBinaries.splice(LRUBinaries.end(), LRUBinaries, Bin.getIterator());
 }
 
 void LLVMSymbolizer::pruneCache() {
   // Evict the LRU binary until the max cache size is reached or there's <= 1
   // item in the cache. The MRU binary is always kept to avoid thrashing if it's

llvm/test/tools/llvm-symbolizer/input-file-err.test

 RUN: not llvm-addr2line -e %p/Inputs/nonexistent 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT-A2L -DMSG=%errc_ENOENT
 RUN: not llvm-addr2line -e %p/Inputs/nonexistent 2>&1 | FileCheck %s --check-prefix=CHECK-NONEXISTENT-A2L -DMSG=%errc_ENOENT
 CHECK-NONEXISTENT-A2L: llvm-addr2line{{.*}}: error: '{{.*}}Inputs/nonexistent': [[MSG]]
 
 RUN: not llvm-addr2line -e %p/Inputs 0x12 2>&1 | FileCheck %s --check-prefix=CHECK-DIRECTORY-A2L -DMSG=%errc_EISDIR
 RUN: not llvm-addr2line -e %p/Inputs 2>&1 | FileCheck %s --check-prefix=CHECK-DIRECTORY-A2L -DMSG=%errc_EISDIR
 CHECK-DIRECTORY-A2L: llvm-addr2line{{.*}}: error: '{{.*}}Inputs': [[MSG]]
+
+not llvm-addr2line --output-style=JSON -e %p/Inputs/nonexistent 2>&1 | FileCheck %s --check-prefix=NONEXIST-JSON -DMSG=%errc_ENOENT
+NONEXIST-JSON: {"Address":"0x0","Error":{"Message":"[[MSG]]"},"ModuleName":"{{.*}}nonexistent"}

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

#endif
   std::unique_ptr<DIPrinter> Printer;
   if (Style == OutputStyle::GNU)
     Printer = std::make_unique<GNUPrinter>(outs(), printError, Config);
   else if (Style == OutputStyle::JSON)
     Printer = std::make_unique<JSONPrinter>(outs(), Config);
   else
     Printer = std::make_unique<LLVMPrinter>(outs(), printError, Config);
 
-  StringRef InputFile = Args.getLastArgValue(OPT_obj_EQ);
-  if (!InputFile.empty() && IsAddr2Line) {
-    Error Status = Symbolizer.checkFileExists(InputFile);
-    if (Status) {
-      handleAllErrors(std::move(Status), [&](const ErrorInfoBase &EI) {
-        printError(EI, InputFile);
+  // When an input file is specified, exit immediately if the file cannot be
+  // read. If getOrCreateModuleInfo succeeds, symbolizeInput will reuse the
+  // cached file handle.
+  if (auto *Arg = Args.getLastArg(OPT_obj_EQ); Arg && IsAddr2Line) {
+    auto Status = Symbolizer.getOrCreateModuleInfo(Arg->getValue());
+    if (!Status) {
+      Request SymRequest = {Arg->getValue(), 0};
+      handleAllErrors(Status.takeError(), [&](const ErrorInfoBase &EI) {
+        Printer->printError(SymRequest, EI);
       });
       return EXIT_FAILURE;
     }
   }
 
   std::vector<std::string> InputAddresses = Args.getAllArgValues(OPT_INPUT);
   if (InputAddresses.empty()) {
     const int kMaxInputStringLength = 1024;