[dsymutil] Share one BinaryHolder between debug map parsing & linking #113234
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@llvm/pr-subscribers-debuginfo Author: Jonas Devlieghere (JDevlieghere) ChangesI (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. Full diff: https://github.com/llvm/llvm-project/pull/113234.diff 5 Files Affected:
diff --git a/llvm/tools/dsymutil/DebugMap.cpp b/llvm/tools/dsymutil/DebugMap.cpp
index 8724b70422f326..b38d502dda7c97 100644
--- a/llvm/tools/dsymutil/DebugMap.cpp
+++ b/llvm/tools/dsymutil/DebugMap.cpp
@@ -126,6 +126,9 @@ void DebugMap::dump() const { print(errs()); }
namespace {
struct YAMLContext {
+ YAMLContext(BinaryHolder &BinHolder, StringRef PrependPath)
+ : BinHolder(BinHolder), PrependPath(PrependPath) {}
+ BinaryHolder &BinHolder;
StringRef PrependPath;
Triple BinaryTriple;
};
@@ -133,15 +136,13 @@ struct YAMLContext {
} // 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 << " "
diff --git a/llvm/tools/dsymutil/DebugMap.h b/llvm/tools/dsymutil/DebugMap.h
index 9c3a698fa1191d..8e2a4de94c89ef 100644
--- a/llvm/tools/dsymutil/DebugMap.h
+++ b/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
diff --git a/llvm/tools/dsymutil/MachODebugMapParser.cpp b/llvm/tools/dsymutil/MachODebugMapParser.cpp
index 4a06731946b97d..a0ba2512e12f20 100644
--- a/llvm/tools/dsymutil/MachODebugMapParser.cpp
+++ b/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();
}
diff --git a/llvm/tools/dsymutil/dsymutil.cpp b/llvm/tools/dsymutil/dsymutil.cpp
index 364a7d63d486e1..f510fa7f75bd87 100644
--- a/llvm/tools/dsymutil/dsymutil.cpp
+++ b/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);
diff --git a/llvm/tools/dsymutil/dsymutil.h b/llvm/tools/dsymutil/dsymutil.h
index 5504dd57c7e558..7b97b8bcd3a250 100644
--- a/llvm/tools/dsymutil/dsymutil.h
+++ b/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 = "");
|
adrian-prantl
approved these changes
Oct 22, 2024
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 22, 2024
…llvm#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. (cherry picked from commit 2ccbea1)
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Dec 16, 2024
…llvm#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. (cherry picked from commit 2ccbea1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.