[Lldb-commits] [lldb] 73811d3 - [lldb] Move copying of files into reproducer out of process
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Fri Oct 23 12:34:23 PDT 2020
Author: Jonas Devlieghere
Date: 2020-10-23T12:33:54-07:00
New Revision: 73811d32c72d0760c8c2066e4675dd6f1a7bbef7
URL: https://github.com/llvm/llvm-project/commit/73811d32c72d0760c8c2066e4675dd6f1a7bbef7
DIFF: https://github.com/llvm/llvm-project/commit/73811d32c72d0760c8c2066e4675dd6f1a7bbef7.diff
LOG: [lldb] Move copying of files into reproducer out of process
For performance reasons the reproducers don't copy the files captured by
the file collector eagerly, but wait until the reproducer needs to be
generated.
This is a problematic when LLDB crashes and we have to do all this
signal-unsafe work in the signal handler. This patch uses a similar
trick to clang, which has the driver invoke a new cc1 instance to do all
this work out-of-process.
This patch moves the writing of the mapping file as well as copying over
the reproducers into a separate process spawned when lldb crashes.
Differential revision: https://reviews.llvm.org/D89600
Added:
lldb/test/Shell/Reproducer/TestFinalize.test
Modified:
lldb/include/lldb/API/SBReproducer.h
lldb/include/lldb/Host/FileSystem.h
lldb/include/lldb/Utility/Reproducer.h
lldb/include/lldb/Utility/ReproducerProvider.h
lldb/source/API/SBReproducer.cpp
lldb/source/Commands/CommandObjectReproducer.cpp
lldb/source/Host/common/FileSystem.cpp
lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
lldb/source/Utility/Reproducer.cpp
lldb/source/Utility/ReproducerProvider.cpp
lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
lldb/tools/driver/Driver.cpp
lldb/tools/driver/Options.td
Removed:
################################################################################
diff --git a/lldb/include/lldb/API/SBReproducer.h b/lldb/include/lldb/API/SBReproducer.h
index 5578162412c8..ecf28d6f0e11 100644
--- a/lldb/include/lldb/API/SBReproducer.h
+++ b/lldb/include/lldb/API/SBReproducer.h
@@ -48,6 +48,7 @@ class LLDB_API SBReproducer {
static const char *Replay(const char *path, bool skip_version_check);
static const char *Replay(const char *path, const SBReplayOptions &options);
static const char *PassiveReplay(const char *path);
+ static const char *Finalize(const char *path);
static const char *GetPath();
static bool SetAutoGenerate(bool b);
static bool Generate();
diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h
index 811a10e47f1c..02ff5f301336 100644
--- a/lldb/include/lldb/Host/FileSystem.h
+++ b/lldb/include/lldb/Host/FileSystem.h
@@ -34,7 +34,7 @@ class FileSystem {
FileSystem()
: m_fs(llvm::vfs::getRealFileSystem()), m_collector(nullptr),
m_home_directory(), m_mapped(false) {}
- FileSystem(std::shared_ptr<llvm::FileCollector> collector)
+ FileSystem(std::shared_ptr<llvm::FileCollectorBase> collector)
: m_fs(llvm::vfs::getRealFileSystem()), m_collector(std::move(collector)),
m_home_directory(), m_mapped(false) {}
FileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
@@ -48,7 +48,7 @@ class FileSystem {
static FileSystem &Instance();
static void Initialize();
- static void Initialize(std::shared_ptr<llvm::FileCollector> collector);
+ static void Initialize(std::shared_ptr<llvm::FileCollectorBase> collector);
static llvm::Error Initialize(const FileSpec &mapping);
static void Initialize(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs);
static void Terminate();
@@ -199,7 +199,7 @@ class FileSystem {
private:
static llvm::Optional<FileSystem> &InstanceImpl();
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> m_fs;
- std::shared_ptr<llvm::FileCollector> m_collector;
+ std::shared_ptr<llvm::FileCollectorBase> m_collector;
std::string m_home_directory;
bool m_mapped;
};
diff --git a/lldb/include/lldb/Utility/Reproducer.h b/lldb/include/lldb/Utility/Reproducer.h
index 7e5591493d71..4659254e57d6 100644
--- a/lldb/include/lldb/Utility/Reproducer.h
+++ b/lldb/include/lldb/Utility/Reproducer.h
@@ -243,6 +243,9 @@ struct ReplayOptions {
bool check_version = true;
};
+llvm::Error Finalize(Loader *loader);
+llvm::Error Finalize(const FileSpec &root);
+
} // namespace repro
} // namespace lldb_private
diff --git a/lldb/include/lldb/Utility/ReproducerProvider.h b/lldb/include/lldb/Utility/ReproducerProvider.h
index abb13f0edd43..221c0eb9c5bb 100644
--- a/lldb/include/lldb/Utility/ReproducerProvider.h
+++ b/lldb/include/lldb/Utility/ReproducerProvider.h
@@ -90,6 +90,23 @@ class YamlRecorder : public AbstractRecorder {
}
};
+class FlushingFileCollector : public llvm::FileCollectorBase {
+public:
+ FlushingFileCollector(llvm::StringRef files_path, llvm::StringRef dirs_path,
+ std::error_code &ec);
+
+protected:
+ void addFileImpl(llvm::StringRef file) override;
+
+ llvm::vfs::directory_iterator
+ addDirectoryImpl(const llvm::Twine &dir,
+ llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> vfs,
+ std::error_code &dir_ec) override;
+
+ llvm::Optional<llvm::raw_fd_ostream> m_files_os;
+ llvm::Optional<llvm::raw_fd_ostream> m_dirs_os;
+};
+
class FileProvider : public Provider<FileProvider> {
public:
struct Info {
@@ -97,31 +114,26 @@ class FileProvider : public Provider<FileProvider> {
static const char *file;
};
- FileProvider(const FileSpec &directory)
- : Provider(directory),
- m_collector(std::make_shared<llvm::FileCollector>(
- directory.CopyByAppendingPathComponent("root").GetPath(),
- directory.GetPath())) {}
+ FileProvider(const FileSpec &directory) : Provider(directory) {
+ std::error_code ec;
+ m_collector = std::make_shared<FlushingFileCollector>(
+ directory.CopyByAppendingPathComponent("files.txt").GetPath(),
+ directory.CopyByAppendingPathComponent("dirs.txt").GetPath(), ec);
+ if (ec)
+ m_collector.reset();
+ }
- std::shared_ptr<llvm::FileCollector> GetFileCollector() {
+ std::shared_ptr<llvm::FileCollectorBase> GetFileCollector() {
return m_collector;
}
void RecordInterestingDirectory(const llvm::Twine &dir);
void RecordInterestingDirectoryRecursive(const llvm::Twine &dir);
- void Keep() override {
- auto mapping = GetRoot().CopyByAppendingPathComponent(Info::file);
- // Temporary files that are removed during execution can cause copy errors.
- if (auto ec = m_collector->copyFiles(/*stop_on_error=*/false))
- return;
- m_collector->writeMapping(mapping.GetPath());
- }
-
static char ID;
private:
- std::shared_ptr<llvm::FileCollector> m_collector;
+ std::shared_ptr<FlushingFileCollector> m_collector;
};
/// Provider for the LLDB version number.
diff --git a/lldb/source/API/SBReproducer.cpp b/lldb/source/API/SBReproducer.cpp
index ec1c85d24329..4d25fcc4a8f6 100644
--- a/lldb/source/API/SBReproducer.cpp
+++ b/lldb/source/API/SBReproducer.cpp
@@ -8,7 +8,6 @@
#include "SBReproducerPrivate.h"
-#include "SBReproducerPrivate.h"
#include "lldb/API/LLDB.h"
#include "lldb/API/SBAddress.h"
#include "lldb/API/SBAttachInfo.h"
@@ -266,6 +265,27 @@ const char *SBReproducer::Replay(const char *path,
return nullptr;
}
+const char *SBReproducer::Finalize(const char *path) {
+ static std::string error;
+ if (auto e = Reproducer::Initialize(ReproducerMode::Replay, FileSpec(path))) {
+ error = llvm::toString(std::move(e));
+ return error.c_str();
+ }
+
+ repro::Loader *loader = repro::Reproducer::Instance().GetLoader();
+ if (!loader) {
+ error = "unable to get replay loader.";
+ return error.c_str();
+ }
+
+ if (auto e = repro::Finalize(loader)) {
+ error = llvm::toString(std::move(e));
+ return error.c_str();
+ }
+
+ return nullptr;
+}
+
bool SBReproducer::Generate() {
auto &r = Reproducer::Instance();
if (auto generator = r.GetGenerator()) {
@@ -285,10 +305,11 @@ bool SBReproducer::SetAutoGenerate(bool b) {
}
const char *SBReproducer::GetPath() {
- static std::string path;
+ ConstString path;
auto &r = Reproducer::Instance();
- path = r.GetReproducerPath().GetCString();
- return path.c_str();
+ if (FileSpec reproducer_path = Reproducer::Instance().GetReproducerPath())
+ path = ConstString(r.GetReproducerPath().GetCString());
+ return path.GetCString();
}
void SBReproducer::SetWorkingDirectory(const char *path) {
diff --git a/lldb/source/Commands/CommandObjectReproducer.cpp b/lldb/source/Commands/CommandObjectReproducer.cpp
index ae4894009054..55f34c05d11d 100644
--- a/lldb/source/Commands/CommandObjectReproducer.cpp
+++ b/lldb/source/Commands/CommandObjectReproducer.cpp
@@ -192,6 +192,10 @@ class CommandObjectReproducerGenerate : public CommandObjectParsed {
auto &r = Reproducer::Instance();
if (auto generator = r.GetGenerator()) {
generator->Keep();
+ if (llvm::Error e = repro::Finalize(r.GetReproducerPath())) {
+ SetError(result, std::move(e));
+ return result.Succeeded();
+ }
} else if (r.IsReplaying()) {
// Make this operation a NO-OP in replay mode.
result.SetStatus(eReturnStatusSuccessFinishNoResult);
diff --git a/lldb/source/Host/common/FileSystem.cpp b/lldb/source/Host/common/FileSystem.cpp
index b8c962cb182f..8a6c03f72b6f 100644
--- a/lldb/source/Host/common/FileSystem.cpp
+++ b/lldb/source/Host/common/FileSystem.cpp
@@ -49,7 +49,7 @@ void FileSystem::Initialize() {
InstanceImpl().emplace();
}
-void FileSystem::Initialize(std::shared_ptr<FileCollector> collector) {
+void FileSystem::Initialize(std::shared_ptr<FileCollectorBase> collector) {
lldbassert(!InstanceImpl() && "Already initialized.");
InstanceImpl().emplace(collector);
}
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h b/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
index b7b6640c4810..4fe727460fdb 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
@@ -18,7 +18,7 @@ class ModuleDependencyCollectorAdaptor
: public clang::ModuleDependencyCollector {
public:
ModuleDependencyCollectorAdaptor(
- std::shared_ptr<llvm::FileCollector> file_collector)
+ std::shared_ptr<llvm::FileCollectorBase> file_collector)
: clang::ModuleDependencyCollector(""), m_file_collector(file_collector) {
}
@@ -33,7 +33,7 @@ class ModuleDependencyCollectorAdaptor
void writeFileMap() override {}
private:
- std::shared_ptr<llvm::FileCollector> m_file_collector;
+ std::shared_ptr<llvm::FileCollectorBase> m_file_collector;
};
} // namespace lldb_private
diff --git a/lldb/source/Utility/Reproducer.cpp b/lldb/source/Utility/Reproducer.cpp
index 1f9ab8d37174..f238a2fabc39 100644
--- a/lldb/source/Utility/Reproducer.cpp
+++ b/lldb/source/Utility/Reproducer.cpp
@@ -176,10 +176,12 @@ Generator::Generator(FileSpec root) : m_root(MakeAbsolute(std::move(root))) {
Generator::~Generator() {
if (!m_done) {
- if (m_auto_generate)
+ if (m_auto_generate) {
Keep();
- else
+ llvm::cantFail(Finalize(GetRoot()));
+ } else {
Discard();
+ }
}
}
@@ -359,3 +361,58 @@ void Verifier::Verify(
}
}
}
+
+static llvm::Error addPaths(StringRef path,
+ function_ref<void(StringRef)> callback) {
+ auto buffer = llvm::MemoryBuffer::getFile(path);
+ if (!buffer)
+ return errorCodeToError(buffer.getError());
+
+ SmallVector<StringRef, 0> paths;
+ (*buffer)->getBuffer().split(paths, '\0');
+ for (StringRef p : paths) {
+ if (!p.empty())
+ callback(p);
+ }
+
+ return errorCodeToError(llvm::sys::fs::remove(path));
+}
+
+llvm::Error repro::Finalize(Loader *loader) {
+ if (!loader)
+ return make_error<StringError>("invalid loader",
+ llvm::inconvertibleErrorCode());
+
+ FileSpec reproducer_root = loader->GetRoot();
+ std::string files_path =
+ reproducer_root.CopyByAppendingPathComponent("files.txt").GetPath();
+ std::string dirs_path =
+ reproducer_root.CopyByAppendingPathComponent("dirs.txt").GetPath();
+
+ FileCollector collector(
+ reproducer_root.CopyByAppendingPathComponent("root").GetPath(),
+ reproducer_root.GetPath());
+
+ if (Error e =
+ addPaths(files_path, [&](StringRef p) { collector.addFile(p); }))
+ return e;
+
+ if (Error e =
+ addPaths(dirs_path, [&](StringRef p) { collector.addDirectory(p); }))
+ return e;
+
+ FileSpec mapping =
+ reproducer_root.CopyByAppendingPathComponent(FileProvider::Info::file);
+ if (auto ec = collector.copyFiles(/*stop_on_error=*/false))
+ return errorCodeToError(ec);
+ collector.writeMapping(mapping.GetPath());
+
+ return llvm::Error::success();
+}
+
+llvm::Error repro::Finalize(const FileSpec &root) {
+ Loader loader(root);
+ if (Error e = loader.LoadIndex())
+ return e;
+ return Finalize(&loader);
+}
diff --git a/lldb/source/Utility/ReproducerProvider.cpp b/lldb/source/Utility/ReproducerProvider.cpp
index d67c886708a2..ed016820db21 100644
--- a/lldb/source/Utility/ReproducerProvider.cpp
+++ b/lldb/source/Utility/ReproducerProvider.cpp
@@ -8,6 +8,7 @@
#include "lldb/Utility/ReproducerProvider.h"
#include "lldb/Utility/ProcessInfo.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/WithColor.h"
#include "llvm/Support/raw_ostream.h"
@@ -44,6 +45,40 @@ void VersionProvider::Keep() {
os << m_version << "\n";
}
+FlushingFileCollector::FlushingFileCollector(llvm::StringRef files_path,
+ llvm::StringRef dirs_path,
+ std::error_code &ec) {
+ auto clear = llvm::make_scope_exit([this]() {
+ m_files_os.reset();
+ m_dirs_os.reset();
+ });
+ m_files_os.emplace(files_path, ec, llvm::sys::fs::OF_Append);
+ if (ec)
+ return;
+ m_dirs_os.emplace(dirs_path, ec, llvm::sys::fs::OF_Append);
+ if (ec)
+ return;
+ clear.release();
+}
+
+void FlushingFileCollector::addFileImpl(StringRef file) {
+ if (m_files_os) {
+ *m_files_os << file << '\0';
+ m_files_os->flush();
+ }
+}
+
+llvm::vfs::directory_iterator
+FlushingFileCollector::addDirectoryImpl(const Twine &dir,
+ IntrusiveRefCntPtr<vfs::FileSystem> vfs,
+ std::error_code &dir_ec) {
+ if (m_dirs_os) {
+ *m_dirs_os << dir << '\0';
+ m_dirs_os->flush();
+ }
+ return vfs->dir_begin(dir, dir_ec);
+}
+
void FileProvider::RecordInterestingDirectory(const llvm::Twine &dir) {
if (m_collector)
m_collector->addFile(dir);
diff --git a/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test b/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
index ef06bce8983f..122a05980aa7 100644
--- a/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
+++ b/lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
@@ -9,8 +9,6 @@
# RUN: LLDB_CAPTURE_REPRODUCER=0 %lldb -b -o 'reproducer status' --capture --capture-path %t.repro | FileCheck %s --check-prefix OFF
# RUN: LLDB_CAPTURE_REPRODUCER=0 %lldb -b -o 'reproducer status' --capture | FileCheck %s --check-prefix OFF
-# RUN: LLDB_CAPTURE_REPRODUCER=OFF %lldb -b -o 'reproducer status' --capture --capture-path %t.repro | FileCheck %s --check-prefix OFF
-# RUN: LLDB_CAPTURE_REPRODUCER=off %lldb -b -o 'reproducer status' --capture | FileCheck %s --check-prefix OFF
# RUN: LLDB_CAPTURE_REPRODUCER=bogus %lldb -b -o 'reproducer status' --capture | FileCheck %s --check-prefix CAPTURE
# RUN: LLDB_CAPTURE_REPRODUCER=bogus %lldb -b -o 'reproducer status' | FileCheck %s --check-prefix OFF
diff --git a/lldb/test/Shell/Reproducer/TestFinalize.test b/lldb/test/Shell/Reproducer/TestFinalize.test
new file mode 100644
index 000000000000..4bfe82b56f17
--- /dev/null
+++ b/lldb/test/Shell/Reproducer/TestFinalize.test
@@ -0,0 +1,14 @@
+# RUN: mkdir -p %t.repro
+# RUN: touch %t.known.file
+# RUN: mkdir -p %t.known.dir
+# RUN: touch %t.repro/index.yaml
+# RUN: echo -n "%t.known.file" > %t.repro/files.txt
+# RUN: echo -n "%t.known.dir" > %t.repro/dirs.txt
+
+# RUN: %lldb --reproducer-finalize %t.repro 2>&1 | FileCheck %s
+# CHECK-NOT: error
+# CHECK: Reproducer written to
+
+# RUN: echo "CHECK-DAG: %t.known.file" > %t.filecheck
+# RUN: echo "CHECK-DAG %t.known.dir" >> %t.filecheck
+# RUN: cat %t.repro/files.yaml | FileCheck %t.filecheck
diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index 79720ddd1bf6..e6b725d431f1 100644
--- a/lldb/tools/driver/Driver.cpp
+++ b/lldb/tools/driver/Driver.cpp
@@ -729,25 +729,10 @@ void sigcont_handler(int signo) {
signal(signo, sigcont_handler);
}
-void reproducer_handler(void *argv0) {
+void reproducer_handler(void *finalize_cmd) {
if (SBReproducer::Generate()) {
- auto exe = static_cast<const char *>(argv0);
- llvm::outs() << "********************\n";
- llvm::outs() << "Crash reproducer for ";
- llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
- llvm::outs() << '\n';
- llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
- << "'\n";
- llvm::outs() << '\n';
- llvm::outs() << "Before attaching the reproducer to a bug report:\n";
- llvm::outs() << " - Look at the directory to ensure you're willing to "
- "share its content.\n";
- llvm::outs()
- << " - Make sure the reproducer works by replaying the reproducer.\n";
- llvm::outs() << '\n';
- llvm::outs() << "Replay the reproducer with the following command:\n";
- llvm::outs() << exe << " -replay " << SBReproducer::GetPath() << "\n";
- llvm::outs() << "********************\n";
+ std::system(static_cast<const char *>(finalize_cmd));
+ fflush(stdout);
}
}
@@ -799,6 +784,31 @@ static void printHelp(LLDBOptTable &table, llvm::StringRef tool_name) {
llvm::Optional<int> InitializeReproducer(llvm::StringRef argv0,
opt::InputArgList &input_args) {
+ if (auto *finalize_path = input_args.getLastArg(OPT_reproducer_finalize)) {
+ if (const char *error = SBReproducer::Finalize(finalize_path->getValue())) {
+ WithColor::error() << "reproducer finalization failed: " << error << '\n';
+ return 1;
+ }
+
+ llvm::outs() << "********************\n";
+ llvm::outs() << "Crash reproducer for ";
+ llvm::outs() << lldb::SBDebugger::GetVersionString() << '\n';
+ llvm::outs() << '\n';
+ llvm::outs() << "Reproducer written to '" << SBReproducer::GetPath()
+ << "'\n";
+ llvm::outs() << '\n';
+ llvm::outs() << "Before attaching the reproducer to a bug report:\n";
+ llvm::outs() << " - Look at the directory to ensure you're willing to "
+ "share its content.\n";
+ llvm::outs()
+ << " - Make sure the reproducer works by replaying the reproducer.\n";
+ llvm::outs() << '\n';
+ llvm::outs() << "Replay the reproducer with the following command:\n";
+ llvm::outs() << argv0 << " -replay " << finalize_path->getValue() << "\n";
+ llvm::outs() << "********************\n";
+ return 0;
+ }
+
if (auto *replay_path = input_args.getLastArg(OPT_replay)) {
SBReplayOptions replay_options;
replay_options.SetCheckVersion(!input_args.hasArg(OPT_no_version_check));
@@ -821,12 +831,6 @@ llvm::Optional<int> InitializeReproducer(llvm::StringRef argv0,
}
if (capture || capture_path) {
- // Register the reproducer signal handler.
- if (!input_args.hasArg(OPT_no_generate_on_signal)) {
- llvm::sys::AddSignalHandler(reproducer_handler,
- const_cast<char *>(argv0.data()));
- }
-
if (capture_path) {
if (!capture)
WithColor::warning() << "-capture-path specified without -capture\n";
@@ -843,6 +847,19 @@ llvm::Optional<int> InitializeReproducer(llvm::StringRef argv0,
}
if (generate_on_exit)
SBReproducer::SetAutoGenerate(true);
+
+ // Register the reproducer signal handler.
+ if (!input_args.hasArg(OPT_no_generate_on_signal)) {
+ if (const char *reproducer_path = SBReproducer::GetPath()) {
+ // Leaking the string on purpose.
+ std::string *finalize_cmd = new std::string(argv0);
+ finalize_cmd->append(" --reproducer-finalize '");
+ finalize_cmd->append(reproducer_path);
+ finalize_cmd->append("'");
+ llvm::sys::AddSignalHandler(reproducer_handler,
+ const_cast<char *>(finalize_cmd->c_str()));
+ }
+ }
}
return llvm::None;
diff --git a/lldb/tools/driver/Options.td b/lldb/tools/driver/Options.td
index b3ffc2d694ef..8bcb0e7bc52e 100644
--- a/lldb/tools/driver/Options.td
+++ b/lldb/tools/driver/Options.td
@@ -232,6 +232,8 @@ def capture_path: Separate<["--", "-"], "capture-path">,
def replay: Separate<["--", "-"], "replay">,
MetaVarName<"<filename>">,
HelpText<"Tells the debugger to replay a reproducer from <filename>.">;
+def reproducer_finalize: Separate<["--", "-"], "reproducer-finalize">,
+ MetaVarName<"<filename>">;
def no_version_check: F<"reproducer-no-version-check">,
HelpText<"Disable the reproducer version check.">;
def no_verification: F<"reproducer-no-verify">,
More information about the lldb-commits
mailing list