This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Closed by commit rG73811d32c72d: [lldb] Move copying of files into reproducer
out of process (authored by JDevlieghere).
Herald added a project: LLDB.
Changed prior to commit:
https://reviews.llvm.org/D89600?vs=300051&id=300381#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89600/new/
https://reviews.llvm.org/D89600
Files:
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/test/Shell/Reproducer/TestFinalize.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,8 @@
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">,
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 *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 @@
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,19 @@
}
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;
Index: lldb/test/Shell/Reproducer/TestFinalize.test
===================================================================
--- /dev/null
+++ 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
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/ReproducerProvider.cpp
===================================================================
--- lldb/source/Utility/ReproducerProvider.cpp
+++ 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 @@
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);
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) {
Keep();
- else
+ llvm::cantFail(Finalize(GetRoot()));
+ } else {
Discard();
+ }
}
}
@@ -359,3 +361,58 @@
}
}
}
+
+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);
+}
Index: lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
+++ lldb/source/Plugins/ExpressionParser/Clang/ModuleDependencyCollector.h
@@ -18,7 +18,7 @@
: 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 @@
void writeFileMap() override {}
private:
- std::shared_ptr<llvm::FileCollector> m_file_collector;
+ std::shared_ptr<llvm::FileCollectorBase> m_file_collector;
};
} // namespace lldb_private
Index: lldb/source/Host/common/FileSystem.cpp
===================================================================
--- lldb/source/Host/common/FileSystem.cpp
+++ lldb/source/Host/common/FileSystem.cpp
@@ -49,7 +49,7 @@
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);
}
Index: lldb/source/Commands/CommandObjectReproducer.cpp
===================================================================
--- lldb/source/Commands/CommandObjectReproducer.cpp
+++ lldb/source/Commands/CommandObjectReproducer.cpp
@@ -192,6 +192,10 @@
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);
Index: lldb/source/API/SBReproducer.cpp
===================================================================
--- lldb/source/API/SBReproducer.cpp
+++ 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 @@
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 @@
}
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
@@ -90,6 +90,23 @@
}
};
+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 @@
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.
Index: lldb/include/lldb/Utility/Reproducer.h
===================================================================
--- lldb/include/lldb/Utility/Reproducer.h
+++ lldb/include/lldb/Utility/Reproducer.h
@@ -243,6 +243,9 @@
bool check_version = true;
};
+llvm::Error Finalize(Loader *loader);
+llvm::Error Finalize(const FileSpec &root);
+
} // namespace repro
} // namespace lldb_private
Index: lldb/include/lldb/Host/FileSystem.h
===================================================================
--- lldb/include/lldb/Host/FileSystem.h
+++ lldb/include/lldb/Host/FileSystem.h
@@ -34,7 +34,7 @@
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 @@
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 @@
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;
};
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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
