JDevlieghere updated this revision to Diff 174407.
JDevlieghere added a comment.
Simplify code a little, as per Stefan's (offline) suggestion.
https://reviews.llvm.org/D54616
Files:
include/lldb/API/SBDebugger.h
include/lldb/Utility/Reproducer.h
scripts/interface/SBDebugger.i
source/API/SBDebugger.cpp
source/Commands/CommandObjectReproducer.cpp
source/Core/Debugger.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Utility/Reproducer.cpp
tools/driver/Driver.cpp
unittests/Utility/ReproducerTest.cpp
Index: unittests/Utility/ReproducerTest.cpp
===================================================================
--- unittests/Utility/ReproducerTest.cpp
+++ unittests/Utility/ReproducerTest.cpp
@@ -33,22 +33,24 @@
// Enable capture and check that means we have a generator.
{
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast<bool>(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
EXPECT_NE(nullptr, reproducer.GetGenerator());
-
- // Make sure the bogus path is correctly set.
EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetGenerator()->GetRoot());
EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
}
// Ensure that we cannot enable replay.
{
- auto error = reproducer.SetReplay(true);
+ auto error = reproducer.SetReplay(FileSpec("/bogus/path"));
EXPECT_TRUE(static_cast<bool>(error));
llvm::consumeError(std::move(error));
EXPECT_EQ(nullptr, reproducer.GetLoader());
}
+
+ // Ensure we can disable the generator again.
+ llvm::cantFail(reproducer.SetCapture(llvm::None));
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
}
TEST(ReproducerTest, SetReplay) {
@@ -60,7 +62,7 @@
// Enable capture and check that means we have a generator.
{
- auto error = reproducer.SetReplay(true, FileSpec("/bogus/path"));
+ auto error = reproducer.SetReplay(FileSpec("/bogus/path"));
// Expected to fail because we can't load the index.
EXPECT_TRUE(static_cast<bool>(error));
llvm::consumeError(std::move(error));
@@ -74,7 +76,7 @@
// Ensure that we cannot enable replay.
{
- auto error = reproducer.SetCapture(true);
+ auto error = reproducer.SetCapture(FileSpec("/bogus/path"));
EXPECT_TRUE(static_cast<bool>(error));
llvm::consumeError(std::move(error));
EXPECT_EQ(nullptr, reproducer.GetGenerator());
@@ -84,8 +86,7 @@
TEST(GeneratorTest, ChangeRoot) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast<bool>(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto &generator = *reproducer.GetGenerator();
// As long as we haven't registered any providers this should work.
@@ -99,8 +100,7 @@
TEST(GeneratorTest, Create) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast<bool>(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto &generator = *reproducer.GetGenerator();
auto *provider = generator.Create<DummyProvider>();
@@ -114,8 +114,7 @@
TEST(GeneratorTest, Get) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast<bool>(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto &generator = *reproducer.GetGenerator();
auto *provider = generator.Create<DummyProvider>();
@@ -128,8 +127,7 @@
TEST(GeneratorTest, GetOrCreate) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast<bool>(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto &generator = *reproducer.GetGenerator();
auto *provider = generator.GetOrCreate<DummyProvider>();
Index: tools/driver/Driver.cpp
===================================================================
--- tools/driver/Driver.cpp
+++ tools/driver/Driver.cpp
@@ -734,7 +734,9 @@
case 'z': {
SBFileSpec file(optarg);
if (file.Exists()) {
- m_debugger.SetReproducerPath(optarg);
+ SBError repro_error = m_debugger.ReplayReproducer(optarg);
+ if (repro_error.Fail())
+ error = repro_error;
} else
error.SetErrorStringWithFormat("file specified in --reproducer "
"(-z) option doesn't exist: '%s'",
Index: source/Utility/Reproducer.cpp
===================================================================
--- source/Utility/Reproducer.cpp
+++ source/Utility/Reproducer.cpp
@@ -51,34 +51,32 @@
return nullptr;
}
-llvm::Error Reproducer::SetCapture(bool value, FileSpec root) {
+llvm::Error Reproducer::SetCapture(llvm::Optional<FileSpec> root) {
std::lock_guard<std::mutex> guard(m_mutex);
- if (value && m_loader)
+ if (root && m_loader)
return make_error<StringError>(
"cannot generate a reproducer when replay one",
inconvertibleErrorCode());
- if (value) {
- m_generator.emplace(root);
- m_generator->SetEnabled(value);
- } else {
+ if (root)
+ m_generator.emplace(*root);
+ else
m_generator.reset();
- }
return Error::success();
}
-llvm::Error Reproducer::SetReplay(bool value, FileSpec root) {
+llvm::Error Reproducer::SetReplay(llvm::Optional<FileSpec> root) {
std::lock_guard<std::mutex> guard(m_mutex);
- if (value && m_generator)
+ if (root && m_generator)
return make_error<StringError>(
"cannot replay a reproducer when generating one",
inconvertibleErrorCode());
- if (value) {
- m_loader.emplace(root);
+ if (root) {
+ m_loader.emplace(*root);
if (auto e = m_loader->LoadIndex())
return e;
} else {
@@ -96,8 +94,7 @@
return {};
}
-Generator::Generator(const FileSpec &root)
- : m_root(root), m_enabled(false), m_done(false) {}
+Generator::Generator(const FileSpec &root) : m_root(root), m_done(false) {}
Generator::~Generator() {}
@@ -114,9 +111,6 @@
assert(!m_done);
m_done = true;
- if (!m_enabled)
- return;
-
for (auto &provider : m_providers)
provider.second->Keep();
@@ -127,9 +121,6 @@
assert(!m_done);
m_done = true;
- if (!m_enabled)
- return;
-
for (auto &provider : m_providers)
provider.second->Discard();
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -299,13 +299,13 @@
"async thread did exit");
if (repro::Generator *g = repro::Reproducer::Instance().GetGenerator()) {
- if (ProcessGDBRemoteProvider *provider =
- g->GetOrCreate<ProcessGDBRemoteProvider>()) {
- // Set the history stream to the stream owned by the provider.
- m_gdb_comm.SetHistoryStream(provider->GetHistoryStream());
- // Make sure to clear the stream again when we're finished.
- provider->SetCallback([&]() { m_gdb_comm.SetHistoryStream(nullptr); });
- }
+ // Guaranteed to be not null.
+ ProcessGDBRemoteProvider *provider =
+ g->GetOrCreate<ProcessGDBRemoteProvider>();
+ // Set the history stream to the stream owned by the provider.
+ m_gdb_comm.SetHistoryStream(provider->GetHistoryStream());
+ // Make sure to clear the stream again when we're finished.
+ provider->SetCallback([&]() { m_gdb_comm.SetHistoryStream(nullptr); });
}
Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_ASYNC));
@@ -3437,8 +3437,8 @@
return Status("Provider for gdb-remote contains no files.");
// Construct replay history path.
- FileSpec history_file = (loader->GetRoot());
- history_file.AppendPathComponent(provider_info->files.front());
+ FileSpec history_file = loader->GetRoot().CopyByAppendingPathComponent(
+ provider_info->files.front());
// Enable replay mode.
m_replay_mode = true;
Index: source/Core/Debugger.cpp
===================================================================
--- source/Core/Debugger.cpp
+++ source/Core/Debugger.cpp
@@ -419,18 +419,21 @@
}
llvm::Error Debugger::SetReproducerReplay(llvm::StringRef p) {
- auto &r = repro::Reproducer::Instance();
- if (auto e = r.SetReplay(true, FileSpec(p)))
- return e;
- return llvm::Error::success();
+ llvm::Optional<FileSpec> arg = llvm::None;
+
+ if (!p.empty())
+ arg = FileSpec(p);
+
+ return repro::Reproducer::Instance().SetReplay(arg);
}
llvm::Error Debugger::SetReproducerCapture(bool b) {
- auto &r = repro::Reproducer::Instance();
- auto root = HostInfo::GetReproducerTempDir();
- if (auto e = r.SetCapture(b, root))
- return e;
- return llvm::Error::success();
+ llvm::Optional<FileSpec> arg = llvm::None;
+
+ if (b)
+ arg = HostInfo::GetReproducerTempDir();
+
+ return repro::Reproducer::Instance().SetCapture(arg);
}
const FormatEntity::Entry *Debugger::GetThreadFormat() const {
Index: source/Commands/CommandObjectReproducer.cpp
===================================================================
--- source/Commands/CommandObjectReproducer.cpp
+++ source/Commands/CommandObjectReproducer.cpp
@@ -144,7 +144,7 @@
auto &r = repro::Reproducer::Instance();
const char *repro_path = command.GetArgumentAtIndex(0);
- if (auto e = r.SetReplay(true, FileSpec(repro_path))) {
+ if (auto e = r.SetReplay(FileSpec(repro_path))) {
std::string error_str = llvm::toString(std::move(e));
result.AppendErrorWithFormat("%s", error_str.c_str());
return false;
Index: source/API/SBDebugger.cpp
===================================================================
--- source/API/SBDebugger.cpp
+++ source/API/SBDebugger.cpp
@@ -1057,12 +1057,15 @@
: nullptr);
}
-void SBDebugger::SetReproducerPath(const char *p) {
+SBError SBDebugger::ReplayReproducer(const char *p) {
+ SBError sb_error;
if (m_opaque_sp) {
auto error =
m_opaque_sp->SetReproducerReplay(llvm::StringRef::withNullAsEmpty(p));
- llvm::consumeError(std::move(error));
+ std::string error_str = llvm::toString(std::move(error));
+ sb_error.ref().SetErrorString(error_str);
}
+ return sb_error;
}
ScriptLanguage SBDebugger::GetScriptLanguage() const {
Index: scripts/interface/SBDebugger.i
===================================================================
--- scripts/interface/SBDebugger.i
+++ scripts/interface/SBDebugger.i
@@ -376,8 +376,8 @@
const char *
GetReproducerPath() const;
- void
- SetReproducerPath (const char *path);
+ lldb::SBError
+ ReplayReproducer (const char *path);
lldb::ScriptLanguage
GetScriptLanguage() const;
Index: include/lldb/Utility/Reproducer.h
===================================================================
--- include/lldb/Utility/Reproducer.h
+++ include/lldb/Utility/Reproducer.h
@@ -116,7 +116,6 @@
private:
friend Reproducer;
- void SetEnabled(bool enabled) { m_enabled = enabled; }
Provider *Register(std::unique_ptr<Provider> provider);
/// Builds and index with provider info.
@@ -129,10 +128,6 @@
/// The reproducer root directory.
FileSpec m_root;
- /// Flag for controlling whether we generate a reproducer when Keep is
- /// called.
- bool m_enabled;
-
/// Flag to ensure that we never call both keep and discard.
bool m_done;
};
@@ -165,8 +160,8 @@
const Generator *GetGenerator() const;
const Loader *GetLoader() const;
- llvm::Error SetCapture(bool value, FileSpec root = {});
- llvm::Error SetReplay(bool value, FileSpec root = {});
+ llvm::Error SetCapture(llvm::Optional<FileSpec> root);
+ llvm::Error SetReplay(llvm::Optional<FileSpec> root);
FileSpec GetReproducerPath() const;
Index: include/lldb/API/SBDebugger.h
===================================================================
--- include/lldb/API/SBDebugger.h
+++ include/lldb/API/SBDebugger.h
@@ -228,7 +228,7 @@
const char *GetReproducerPath() const;
- void SetReproducerPath(const char *reproducer);
+ lldb::SBError ReplayReproducer(const char *path);
lldb::ScriptLanguage GetScriptLanguage() const;
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
