[dsymutil] Share one BinaryHolder between debug map parsing & linking (#113234)

I (re)discovered that dsymutil was instantiating two BinaryHolders: one
for parsing the debug map and one for linking. That really defeats the
purpose of the BinaryHolder as it serves as a cache. Fix the issue and
remove an old FIXME.

Author: JDevlieghere

llvm/tools/dsymutil/DebugMap.cpp

@@ -126,22 +126,23 @@ void DebugMap::dump() const { print(errs()); }
 namespace {
 
 struct YAMLContext {
+  YAMLContext(BinaryHolder &BinHolder, StringRef PrependPath)
+      : BinHolder(BinHolder), PrependPath(PrependPath) {}
+  BinaryHolder &BinHolder;
   StringRef PrependPath;
   Triple BinaryTriple;
 };
 
 } // end anonymous namespace
 
 ErrorOr<std::vector<std::unique_ptr<DebugMap>>>
-DebugMap::parseYAMLDebugMap(StringRef InputFile, StringRef PrependPath,
-                            bool Verbose) {
+DebugMap::parseYAMLDebugMap(BinaryHolder &BinHolder, StringRef InputFile,
+                            StringRef PrependPath, bool Verbose) {
   auto ErrOrFile = MemoryBuffer::getFileOrSTDIN(InputFile);
   if (auto Err = ErrOrFile.getError())
     return Err;
 
-  YAMLContext Ctxt;
-
-  Ctxt.PrependPath = PrependPath;
+  YAMLContext Ctxt(BinHolder, PrependPath);
 
   std::unique_ptr<DebugMap> Res;
   yaml::Input yin((*ErrOrFile)->getBuffer(), &Ctxt);
@@ -244,14 +245,13 @@ MappingTraits<dsymutil::DebugMapObject>::YamlDMO::YamlDMO(
 
 dsymutil::DebugMapObject
 MappingTraits<dsymutil::DebugMapObject>::YamlDMO::denormalize(IO &IO) {
-  BinaryHolder BinHolder(vfs::getRealFileSystem(), /* Verbose =*/false);
   const auto &Ctxt = *reinterpret_cast<YAMLContext *>(IO.getContext());
   SmallString<80> Path(Ctxt.PrependPath);
   StringMap<uint64_t> SymbolAddresses;
 
   sys::path::append(Path, Filename);
 
-  auto ObjectEntry = BinHolder.getObjectEntry(Path);
+  auto ObjectEntry = Ctxt.BinHolder.getObjectEntry(Path);
   if (!ObjectEntry) {
     auto Err = ObjectEntry.takeError();
     WithColor::warning() << "Unable to open " << Path << " "

llvm/tools/dsymutil/DebugMap.h

@@ -21,6 +21,7 @@
 #ifndef LLVM_TOOLS_DSYMUTIL_DEBUGMAP_H
 #define LLVM_TOOLS_DSYMUTIL_DEBUGMAP_H
 
+#include "BinaryHolder.h"
 #include "RelocationMap.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
@@ -127,7 +128,8 @@ class DebugMap {
 
   /// Read a debug map for \a InputFile.
   static ErrorOr<std::vector<std::unique_ptr<DebugMap>>>
-  parseYAMLDebugMap(StringRef InputFile, StringRef PrependPath, bool Verbose);
+  parseYAMLDebugMap(BinaryHolder &BinHolder, StringRef InputFile,
+                    StringRef PrependPath, bool Verbose);
 };
 
 /// The DebugMapObject represents one object file described by the DebugMap. It

llvm/tools/dsymutil/MachODebugMapParser.cpp

@@ -27,14 +27,14 @@ using namespace llvm::object;
 
 class MachODebugMapParser {
 public:
-  MachODebugMapParser(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-                      StringRef BinaryPath, ArrayRef<std::string> Archs,
+  MachODebugMapParser(BinaryHolder &BinHolder, StringRef BinaryPath,
+                      ArrayRef<std::string> Archs,
                       ArrayRef<std::string> DSYMSearchPaths,
                       StringRef PathPrefix = "", StringRef VariantSuffix = "",
                       bool Verbose = false)
       : BinaryPath(std::string(BinaryPath)), Archs(Archs),
         DSYMSearchPaths(DSYMSearchPaths), PathPrefix(std::string(PathPrefix)),
-        VariantSuffix(std::string(VariantSuffix)), BinHolder(VFS, Verbose),
+        VariantSuffix(std::string(VariantSuffix)), BinHolder(BinHolder),
         CurrentDebugMapObject(nullptr), SkipDebugMapObject(false) {}
 
   /// Parses and returns the DebugMaps of the input binary. The binary contains
@@ -56,7 +56,7 @@ class MachODebugMapParser {
   std::string VariantSuffix;
 
   /// Owns the MemoryBuffer for the main binary.
-  BinaryHolder BinHolder;
+  BinaryHolder &BinHolder;
   /// Map of the binary symbol addresses.
   StringMap<uint64_t> MainBinarySymbolAddresses;
   StringRef MainBinaryStrings;
@@ -854,24 +854,25 @@ void MachODebugMapParser::loadMainBinarySymbols(
 namespace llvm {
 namespace dsymutil {
 llvm::ErrorOr<std::vector<std::unique_ptr<DebugMap>>>
-parseDebugMap(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-              StringRef InputFile, ArrayRef<std::string> Archs,
+parseDebugMap(BinaryHolder &BinHolder, StringRef InputFile,
+              ArrayRef<std::string> Archs,
               ArrayRef<std::string> DSYMSearchPaths, StringRef PrependPath,
               StringRef VariantSuffix, bool Verbose, bool InputIsYAML) {
   if (InputIsYAML)
-    return DebugMap::parseYAMLDebugMap(InputFile, PrependPath, Verbose);
+    return DebugMap::parseYAMLDebugMap(BinHolder, InputFile, PrependPath,
+                                       Verbose);
 
-  MachODebugMapParser Parser(VFS, InputFile, Archs, DSYMSearchPaths,
+  MachODebugMapParser Parser(BinHolder, InputFile, Archs, DSYMSearchPaths,
                              PrependPath, VariantSuffix, Verbose);
 
   return Parser.parse();
 }
 
-bool dumpStab(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-              StringRef InputFile, ArrayRef<std::string> Archs,
+bool dumpStab(BinaryHolder &BinHolder, StringRef InputFile,
+              ArrayRef<std::string> Archs,
               ArrayRef<std::string> DSYMSearchPaths, StringRef PrependPath,
               StringRef VariantSuffix) {
-  MachODebugMapParser Parser(VFS, InputFile, Archs, DSYMSearchPaths,
+  MachODebugMapParser Parser(BinHolder, InputFile, Archs, DSYMSearchPaths,
                              PrependPath, VariantSuffix, false);
   return Parser.dumpStab();
 }

llvm/tools/dsymutil/dsymutil.cpp

@@ -180,17 +180,6 @@ static Error verifyOptions(const DsymutilOptions &Options) {
                                    errc::invalid_argument);
   }
 
-  if (Options.LinkOpts.Update && llvm::is_contained(Options.InputFiles, "-")) {
-    // FIXME: We cannot use stdin for an update because stdin will be
-    // consumed by the BinaryHolder during the debugmap parsing, and
-    // then we will want to consume it again in DwarfLinker. If we
-    // used a unique BinaryHolder object that could cache multiple
-    // binaries this restriction would go away.
-    return make_error<StringError>(
-        "standard input cannot be used as input for a dSYM update.",
-        errc::invalid_argument);
-  }
-
   if (!Options.Flat && Options.OutputFile == "-")
     return make_error<StringError>(
         "cannot emit to standard output without --flat.",
@@ -674,9 +663,12 @@ int dsymutil_main(int argc, char **argv, const llvm::ToolContext &) {
     }
 
   for (auto &InputFile : Options.InputFiles) {
+    // Shared a single binary holder for all the link steps.
+    BinaryHolder BinHolder(Options.LinkOpts.VFS, Options.LinkOpts.Verbose);
+
     // Dump the symbol table for each input file and requested arch
     if (Options.DumpStab) {
-      if (!dumpStab(Options.LinkOpts.VFS, InputFile, Options.Archs,
+      if (!dumpStab(BinHolder, InputFile, Options.Archs,
                     Options.LinkOpts.DSYMSearchPaths,
                     Options.LinkOpts.PrependPath,
                     Options.LinkOpts.BuildVariantSuffix))
@@ -685,10 +677,9 @@ int dsymutil_main(int argc, char **argv, const llvm::ToolContext &) {
     }
 
     auto DebugMapPtrsOrErr = parseDebugMap(
-        Options.LinkOpts.VFS, InputFile, Options.Archs,
-        Options.LinkOpts.DSYMSearchPaths, Options.LinkOpts.PrependPath,
-        Options.LinkOpts.BuildVariantSuffix, Options.LinkOpts.Verbose,
-        Options.InputIsYAMLDebugMap);
+        BinHolder, InputFile, Options.Archs, Options.LinkOpts.DSYMSearchPaths,
+        Options.LinkOpts.PrependPath, Options.LinkOpts.BuildVariantSuffix,
+        Options.LinkOpts.Verbose, Options.InputIsYAMLDebugMap);
 
     if (auto EC = DebugMapPtrsOrErr.getError()) {
       WithColor::error() << "cannot parse the debug map for '" << InputFile
@@ -714,9 +705,6 @@ int dsymutil_main(int argc, char **argv, const llvm::ToolContext &) {
       return EXIT_FAILURE;
     }
 
-    // Shared a single binary holder for all the link steps.
-    BinaryHolder BinHolder(Options.LinkOpts.VFS);
-
     // Compute the output location and update the resource directory.
     Expected<OutputLocation> OutputLocationOrErr =
         getOutputFileName(InputFile, Options);

llvm/tools/dsymutil/dsymutil.h

@@ -16,6 +16,7 @@
 #ifndef LLVM_TOOLS_DSYMUTIL_DSYMUTIL_H
 #define LLVM_TOOLS_DSYMUTIL_DSYMUTIL_H
 
+#include "BinaryHolder.h"
 #include "DebugMap.h"
 #include "LinkUtils.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -33,14 +34,14 @@ namespace dsymutil {
 /// The file has to be a MachO object file. Multiple debug maps can be
 /// returned when the file is universal (aka fat) binary.
 ErrorOr<std::vector<std::unique_ptr<DebugMap>>>
-parseDebugMap(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-              StringRef InputFile, ArrayRef<std::string> Archs,
+parseDebugMap(BinaryHolder &BinHolder, StringRef InputFile,
+              ArrayRef<std::string> Archs,
               ArrayRef<std::string> DSYMSearchPaths, StringRef PrependPath,
               StringRef VariantSuffix, bool Verbose, bool InputIsYAML);
 
 /// Dump the symbol table.
-bool dumpStab(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
-              StringRef InputFile, ArrayRef<std::string> Archs,
+bool dumpStab(BinaryHolder &BinHolder, StringRef InputFile,
+              ArrayRef<std::string> Archs,
               ArrayRef<std::string> DSYMSearchPaths, StringRef PrependPath = "",
               StringRef VariantSuffix = "");