JDevlieghere created this revision.
JDevlieghere added reviewers: labath, teemperor, vsk, jingham.
Herald added a subscriber: dang.
JDevlieghere requested review of this revision.
For performance reasons the reproducers don't copy the files captured by the
file collector until we decide that the reproducer needs to be kept (i.e.
generated). This is a problematic when LLDB crashes and we need to do this
signal-unsafe work in the signal handler. This patch uses the same trick as
clang and reinvokes itself to copy the files out of process.
https://reviews.llvm.org/D89600
Files:
lldb/include/lldb/API/SBReproducer.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/Utility/Reproducer.cpp
lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
lldb/tools/driver/Driver.cpp
lldb/tools/driver/Options.td
Index: lldb/tools/driver/Options.td
===================================================================
--- lldb/tools/driver/Options.td
+++ lldb/tools/driver/Options.td
@@ -232,6 +232,9 @@
def replay: Separate<["--", "-"], "replay">,
MetaVarName<"<filename>">,
HelpText<"Tells the debugger to replay a reproducer from <filename>.">;
+def reproducer_finalize: Separate<["--", "-"], "reproducer-finalize">,
+ MetaVarName<"<filename>">,
+ HelpText<"Finalize the reproducer by copying files into reproducer root.">;
def no_version_check: F<"reproducer-no-version-check">,
HelpText<"Disable the reproducer version check.">;
def no_verification: F<"reproducer-no-verify">,
Index: lldb/tools/driver/Driver.cpp
===================================================================
--- lldb/tools/driver/Driver.cpp
+++ lldb/tools/driver/Driver.cpp
@@ -729,25 +729,10 @@
signal(signo, sigcont_handler);
}
-void reproducer_handler(void *argv0) {
+void reproducer_handler(void *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 *>(cmd));
+ fflush(stdout);
}
}
@@ -799,6 +784,31 @@
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 @@
}
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,18 @@
}
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 *str = new std::string(argv0);
+ str->append(" --reproducer-finalize ");
+ str->append(reproducer_path);
+ llvm::sys::AddSignalHandler(reproducer_handler,
+ const_cast<char *>(str->c_str()));
+ }
+ }
}
return llvm::None;
Index: lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
===================================================================
--- lldb/test/Shell/Reproducer/TestCaptureEnvOverride.test
+++ 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
Index: lldb/source/Utility/Reproducer.cpp
===================================================================
--- lldb/source/Utility/Reproducer.cpp
+++ lldb/source/Utility/Reproducer.cpp
@@ -176,10 +176,12 @@
Generator::~Generator() {
if (!m_done) {
- if (m_auto_generate)
+ if (m_auto_generate) {
+ GetOrCreate<repro::FileProvider>().SetCopyInProcess(true);
Keep();
- else
+ } else {
Discard();
+ }
}
}
@@ -359,3 +361,58 @@
}
}
}
+
+llvm::Error Finalizer::Finalize() const {
+ FileSpec vfs_mapping = m_loader->GetFile<FileProvider::Info>();
+ ErrorOr<std::unique_ptr<MemoryBuffer>> buffer =
+ vfs::getRealFileSystem()->getBufferForFile(vfs_mapping.GetPath());
+ if (!buffer)
+ return errorCodeToError(buffer.getError());
+
+ IntrusiveRefCntPtr<vfs::FileSystem> vfs = vfs::getVFSFromYAML(
+ std::move(buffer.get()), nullptr, vfs_mapping.GetPath());
+ if (!vfs)
+ return make_error<StringError>(
+ "unable to initialize the virtual file system",
+ inconvertibleErrorCode());
+
+ Expected<std::string> working_dir =
+ GetDirectoryFrom<WorkingDirectoryProvider>(m_loader);
+ if (!working_dir)
+ return working_dir.takeError();
+
+ if (!vfs->exists(*working_dir))
+ return make_error<StringError>(
+ "working directory not in the virtual file system",
+ inconvertibleErrorCode());
+
+ vfs->setCurrentWorkingDirectory(*working_dir);
+
+ auto &redirecting_vfs = static_cast<vfs::RedirectingFileSystem &>(*vfs);
+ redirecting_vfs.setFallthrough(false);
+
+ FileSpec reproducer_root = m_loader->GetRoot();
+
+ FileCollector collector(
+ reproducer_root.CopyByAppendingPathComponent("root").GetPath(),
+ reproducer_root.GetPath());
+
+ std::vector<llvm::StringRef> roots = redirecting_vfs.getRoots();
+ for (llvm::StringRef root : roots) {
+ std::error_code ec;
+ vfs::recursive_directory_iterator iter(*vfs, root, ec);
+ vfs::recursive_directory_iterator end;
+ for (; iter != end && !ec; iter.increment(ec)) {
+ StringRef path = iter->path();
+ collector.addFile(path);
+ }
+ }
+
+ 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();
+}
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===================================================================
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -191,6 +191,7 @@
auto &r = Reproducer::Instance();
if (auto generator = r.GetGenerator()) {
+ generator->GetOrCreate<repro::FileProvider>().SetCopyInProcess(true);
generator->Keep();
} else if (r.IsReplaying()) {
// Make this operation a NO-OP in replay mode.
Index: lldb/source/API/SBReproducer.cpp
===================================================================
--- lldb/source/API/SBReproducer.cpp
+++ lldb/source/API/SBReproducer.cpp
@@ -266,6 +266,28 @@
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();
+ }
+
+ Finalizer finalizer(loader);
+ if (auto e = finalizer.Finalize()) {
+ 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 +307,11 @@
}
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) {
Index: lldb/include/lldb/Utility/ReproducerProvider.h
===================================================================
--- lldb/include/lldb/Utility/ReproducerProvider.h
+++ lldb/include/lldb/Utility/ReproducerProvider.h
@@ -101,20 +101,26 @@
: Provider(directory),
m_collector(std::make_shared<llvm::FileCollector>(
directory.CopyByAppendingPathComponent("root").GetPath(),
- directory.GetPath())) {}
+ directory.GetPath())),
+ m_copy_in_process(false) {}
std::shared_ptr<llvm::FileCollector> GetFileCollector() {
return m_collector;
}
+ void SetCopyInProcess(bool b) { m_copy_in_process = b; }
+
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;
+ if (m_copy_in_process) {
+ // 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());
}
@@ -122,6 +128,7 @@
private:
std::shared_ptr<llvm::FileCollector> m_collector;
+ bool m_copy_in_process;
};
/// Provider for the LLDB version number.
Index: lldb/include/lldb/Utility/Reproducer.h
===================================================================
--- lldb/include/lldb/Utility/Reproducer.h
+++ lldb/include/lldb/Utility/Reproducer.h
@@ -238,6 +238,15 @@
Loader *m_loader;
};
+class Finalizer {
+public:
+ Finalizer(Loader *loader) : m_loader(loader) {}
+ llvm::Error Finalize() const;
+
+private:
+ Loader *m_loader;
+};
+
struct ReplayOptions {
bool verify = true;
bool check_version = true;
Index: lldb/include/lldb/API/SBReproducer.h
===================================================================
--- lldb/include/lldb/API/SBReproducer.h
+++ lldb/include/lldb/API/SBReproducer.h
@@ -48,6 +48,7 @@
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();
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
