[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL347716: [Reproducers] Improve reproducer API and add unit
tests. (authored by JDevlieghere, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54616?vs=175293=175578#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54616/new/
https://reviews.llvm.org/D54616
Files:
lldb/trunk/include/lldb/API/SBDebugger.h
lldb/trunk/include/lldb/Core/Debugger.h
lldb/trunk/include/lldb/Utility/Reproducer.h
lldb/trunk/scripts/interface/SBDebugger.i
lldb/trunk/source/API/SBDebugger.cpp
lldb/trunk/source/Commands/CommandObjectReproducer.cpp
lldb/trunk/source/Core/Debugger.cpp
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/trunk/source/Utility/Reproducer.cpp
lldb/trunk/tools/driver/Driver.cpp
lldb/trunk/unittests/Utility/CMakeLists.txt
lldb/trunk/unittests/Utility/ReproducerTest.cpp
Index: lldb/trunk/unittests/Utility/ReproducerTest.cpp
===
--- lldb/trunk/unittests/Utility/ReproducerTest.cpp
+++ lldb/trunk/unittests/Utility/ReproducerTest.cpp
@@ -0,0 +1,126 @@
+//===-- ReproducerTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Reproducer.h"
+
+using namespace llvm;
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+class DummyProvider : public repro::Provider {
+public:
+ static constexpr const char *NAME = "dummy";
+
+ DummyProvider(const FileSpec ) : Provider(directory) {
+m_info.name = "dummy";
+m_info.files.push_back("dummy.yaml");
+ }
+
+ static char ID;
+};
+
+char DummyProvider::ID = 0;
+
+TEST(ReproducerTest, SetCapture) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Enable capture and check that means we have a generator.
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ EXPECT_NE(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetGenerator()->GetRoot());
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
+
+ // Ensure that we cannot enable replay.
+ EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), Failed());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Ensure we can disable the generator again.
+ EXPECT_THAT_ERROR(reproducer.SetCapture(llvm::None), Succeeded());
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+}
+
+TEST(ReproducerTest, SetReplay) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Expected to fail because we can't load the index.
+ EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), Failed());
+ // However the loader should still be set, which we check here.
+ EXPECT_NE(nullptr, reproducer.GetLoader());
+
+ // Make sure the bogus path is correctly set.
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetLoader()->GetRoot());
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
+
+ // Ensure that we cannot enable replay.
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")), Failed());
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+}
+
+TEST(GeneratorTest, Create) {
+ Reproducer reproducer;
+
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+ EXPECT_EQ(FileSpec("/bogus/path"), provider->GetRoot());
+ EXPECT_EQ(std::string("dummy"), provider->GetInfo().name);
+ EXPECT_EQ((size_t)1, provider->GetInfo().files.size());
+ EXPECT_EQ(std::string("dummy.yaml"), provider->GetInfo().files.front());
+}
+
+TEST(GeneratorTest, Get) {
+ Reproducer reproducer;
+
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+
+ auto *provider_alt = generator.Get();
+ EXPECT_EQ(provider, provider_alt);
+}
+
+TEST(GeneratorTest, GetOrCreate) {
+ Reproducer reproducer;
+
+
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54616/new/ https://reviews.llvm.org/D54616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
labath added a comment.
I haven't been following the reproducer work in detail, but this seems
reasonable to me. Thanks for incorporating my drive-by suggestions.
Comment at: include/lldb/Utility/Reproducer.h:104
+ template T *Create() {
+std::unique_ptr provider(new T(m_root));
+return static_cast(Register(std::move(provider)));
You should still be able to use make_unique here.
Comment at: include/lldb/Utility/Reproducer.h:134
- std::vector> m_providers;
+ /// List of providers indexed by their name for easy access.
+ llvm::DenseMap> m_providers;
Out of date comment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54616/new/
https://reviews.llvm.org/D54616
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
JDevlieghere added a comment. Does anybody want to have another look or can I land this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54616/new/ https://reviews.llvm.org/D54616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
JDevlieghere updated this revision to Diff 175293.
JDevlieghere marked 6 inline comments as done.
JDevlieghere added a comment.
Thanks Pavel, really useful feedback, I learned a few new things :-)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54616/new/
https://reviews.llvm.org/D54616
Files:
include/lldb/API/SBDebugger.h
include/lldb/Core/Debugger.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/CMakeLists.txt
unittests/Utility/ReproducerTest.cpp
Index: unittests/Utility/ReproducerTest.cpp
===
--- /dev/null
+++ unittests/Utility/ReproducerTest.cpp
@@ -0,0 +1,126 @@
+//===-- ReproducerTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "llvm/Support/Error.h"
+#include "llvm/Testing/Support/Error.h"
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Reproducer.h"
+
+using namespace llvm;
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+class DummyProvider : public repro::Provider {
+public:
+ static constexpr const char *NAME = "dummy";
+
+ DummyProvider(const FileSpec ) : Provider(directory) {
+m_info.name = "dummy";
+m_info.files.push_back("dummy.yaml");
+ }
+
+ static char ID;
+};
+
+char DummyProvider::ID = 0;
+
+TEST(ReproducerTest, SetCapture) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Enable capture and check that means we have a generator.
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ EXPECT_NE(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetGenerator()->GetRoot());
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
+
+ // Ensure that we cannot enable replay.
+ EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), Failed());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Ensure we can disable the generator again.
+ EXPECT_THAT_ERROR(reproducer.SetCapture(llvm::None), Succeeded());
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+}
+
+TEST(ReproducerTest, SetReplay) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Expected to fail because we can't load the index.
+ EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), Failed());
+ // However the loader should still be set, which we check here.
+ EXPECT_NE(nullptr, reproducer.GetLoader());
+
+ // Make sure the bogus path is correctly set.
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetLoader()->GetRoot());
+ EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
+
+ // Ensure that we cannot enable replay.
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")), Failed());
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+}
+
+TEST(GeneratorTest, Create) {
+ Reproducer reproducer;
+
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+ EXPECT_EQ(FileSpec("/bogus/path"), provider->GetRoot());
+ EXPECT_EQ(std::string("dummy"), provider->GetInfo().name);
+ EXPECT_EQ((size_t)1, provider->GetInfo().files.size());
+ EXPECT_EQ(std::string("dummy.yaml"), provider->GetInfo().files.front());
+}
+
+TEST(GeneratorTest, Get) {
+ Reproducer reproducer;
+
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+
+ auto *provider_alt = generator.Get();
+ EXPECT_EQ(provider, provider_alt);
+}
+
+TEST(GeneratorTest, GetOrCreate) {
+ Reproducer reproducer;
+
+ EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
+Succeeded());
+ auto = *reproducer.GetGenerator();
+
+ auto = generator.GetOrCreate();
+ EXPECT_EQ(FileSpec("/bogus/path"), provider.GetRoot());
+ EXPECT_EQ(std::string("dummy"), provider.GetInfo().name);
+ EXPECT_EQ((size_t)1,
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
labath added inline comments.
Comment at: include/lldb/Utility/Reproducer.h:59-64
+ void InitializeFileInfo(llvm::StringRef name,
+ llvm::ArrayRef files) {
+m_info.name = name;
+for (auto file : files)
+ m_info.files.push_back(file);
+ }
Could this be replaced by additional arguments to the constructor (avoiding
two-stage initialization)?
Comment at: include/lldb/Utility/Reproducer.h:99
+ template T *Get() {
+auto it = m_providers.find(T::NAME);
+if (it == m_providers.end())
Is the value of the `NAME` used anywhere? If not, you could just have each
class define a `char` variable (like `llvm::ErrorInfo` subclasses do) and then
use its address as a key, making everything faster.
(Even if name is useful (for printing to the user or whatever), it might be
worth to switch to using the char-address for lookup and then just have
`getName` method for other things.)
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:304
+ProcessGDBRemoteProvider *provider =
+g->GetOrCreate();
// Set the history stream to the stream owned by the provider.
could `GetOrCreate` keep returning a reference (to avoid spurious `guaranteed
to be not null` comments)?
Comment at: source/Utility/Reproducer.cpp:131-133
+ assert(m_providers.empty() &&
+ "Changing the root directory after providers have "
+ "been registered would invalidate the index.");
Given these limitations, would it be possible to set the root once in the
constructor and keep it immutable since then?
Comment at: unittests/Utility/ReproducerTest.cpp:44-46
+auto error = reproducer.SetReplay(FileSpec("/bogus/path"));
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
Shorter and with better error messages:
`EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")),
llvm::Succeeded());`
Comment at: unittests/Utility/ReproducerTest.cpp:79-81
+auto error = reproducer.SetCapture(FileSpec("/bogus/path"));
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
`EXPECT_THAT_ERROR(..., llvm::Failed());`
https://reviews.llvm.org/D54616
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
JDevlieghere updated this revision to Diff 174416.
https://reviews.llvm.org/D54616
Files:
include/lldb/API/SBDebugger.h
include/lldb/Core/Debugger.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/CMakeLists.txt
unittests/Utility/ReproducerTest.cpp
Index: unittests/Utility/ReproducerTest.cpp
===
--- /dev/null
+++ unittests/Utility/ReproducerTest.cpp
@@ -0,0 +1,142 @@
+//===-- ReproducerTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Reproducer.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+class DummyProvider : public repro::Provider {
+public:
+ static constexpr const char *NAME = "dummy";
+
+ DummyProvider(const FileSpec ) : Provider(directory) {
+InitializeFileInfo(NAME, {"dummy.yaml"});
+ }
+};
+
+TEST(ReproducerTest, SetCapture) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Enable capture and check that means we have a generator.
+ {
+llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
+EXPECT_NE(nullptr, reproducer.GetGenerator());
+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(FileSpec("/bogus/path"));
+EXPECT_TRUE(static_cast(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) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Enable capture and check that means we have a generator.
+ {
+auto error = reproducer.SetReplay(FileSpec("/bogus/path"));
+// Expected to fail because we can't load the index.
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
+// However the loader should still be set, which we check here.
+EXPECT_NE(nullptr, reproducer.GetLoader());
+
+// Make sure the bogus path is correctly set.
+EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetLoader()->GetRoot());
+EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
+ }
+
+ // Ensure that we cannot enable replay.
+ {
+auto error = reproducer.SetCapture(FileSpec("/bogus/path"));
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
+EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ }
+}
+
+TEST(GeneratorTest, ChangeRoot) {
+ Reproducer reproducer;
+
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
+ auto = *reproducer.GetGenerator();
+
+ // As long as we haven't registered any providers this should work.
+ generator.ChangeRoot(FileSpec("/other/bogus/path"));
+
+ EXPECT_EQ(FileSpec("/other/bogus/path"),
+reproducer.GetGenerator()->GetRoot());
+ EXPECT_EQ(FileSpec("/other/bogus/path"), reproducer.GetReproducerPath());
+}
+
+TEST(GeneratorTest, Create) {
+ Reproducer reproducer;
+
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+ EXPECT_EQ(FileSpec("/bogus/path"), provider->GetRoot());
+ EXPECT_EQ(std::string("dummy"), provider->GetInfo().name);
+ EXPECT_EQ((size_t)1, provider->GetInfo().files.size());
+ EXPECT_EQ(std::string("dummy.yaml"), provider->GetInfo().files.front());
+}
+
+TEST(GeneratorTest, Get) {
+ Reproducer reproducer;
+
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+
+ auto *provider_alt = generator.Get();
+ EXPECT_EQ(provider, provider_alt);
+}
+
+TEST(GeneratorTest, GetOrCreate) {
+ Reproducer reproducer;
+
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
+ auto =
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
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(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(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(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(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(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *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(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.Create();
@@ -114,8 +114,7 @@
TEST(GeneratorTest, Get) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.Create();
@@ -128,8 +127,7 @@
TEST(GeneratorTest, GetOrCreate) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.GetOrCreate();
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 root) {
std::lock_guard guard(m_mutex);
- if (value && m_loader)
+ if (root && m_loader)
return make_error(
"cannot generate a reproducer when replay
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
JDevlieghere updated this revision to Diff 174399.
JDevlieghere marked an inline comment as done.
JDevlieghere added a comment.
Missed a comment; fixed now.
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(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(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(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(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(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *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(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.Create();
@@ -114,8 +114,7 @@
TEST(GeneratorTest, Get) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.Create();
@@ -128,8 +127,7 @@
TEST(GeneratorTest, GetOrCreate) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.GetOrCreate();
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 root) {
std::lock_guard guard(m_mutex);
- if (value && m_loader)
+ if (root && m_loader)
return make_error(
"cannot generate a
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
JDevlieghere updated this revision to Diff 174398.
JDevlieghere marked 4 inline comments as done.
JDevlieghere added a comment.
Address Stefan's review feedback.
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(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(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(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(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(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *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(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.Create();
@@ -114,8 +114,7 @@
TEST(GeneratorTest, Get) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.Create();
@@ -128,8 +127,7 @@
TEST(GeneratorTest, GetOrCreate) {
Reproducer reproducer;
- auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
- EXPECT_FALSE(static_cast(error));
+ llvm::cantFail(reproducer.SetCapture(FileSpec("/bogus/path")));
auto = *reproducer.GetGenerator();
auto *provider = generator.GetOrCreate();
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 root) {
std::lock_guard guard(m_mutex);
- if (value && m_loader)
+ if (root && m_loader)
return make_error(
"cannot generate a
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
sgraenitz added a comment.
Looks pretty good to me, but added my 2¢ on minor things.
Comment at: include/lldb/Utility/Reproducer.h:169
+ llvm::Error SetCapture(bool value, FileSpec root = {});
+ llvm::Error SetReplay(bool value, FileSpec root = {});
Are the default-value-cases necessary?
Comment at: source/API/SBDebugger.cpp:1064
+m_opaque_sp->SetReproducerReplay(llvm::StringRef::withNullAsEmpty(p));
+llvm::consumeError(std::move(error));
+ }
Will the error be passed up the stack soon? Otherwise maybe add FIXME comment?
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:303
+if (ProcessGDBRemoteProvider *provider =
+g->GetOrCreate()) {
+ // Set the history stream to the stream owned by the provider.
In which case would `GetOrCreate()` fail?
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3440
// Construct replay history path.
- FileSpec history_file(loader->GetDirectory());
+ FileSpec history_file = (loader->GetRoot());
history_file.AppendPathComponent(provider_info->files.front());
Brackets
Comment at: unittests/Utility/ReproducerTest.cpp:37
+auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
+EXPECT_FALSE(static_cast(error));
+EXPECT_NE(nullptr, reproducer.GetGenerator());
It may be good to know what went wrong if this fails. And: are the other
`EXPECT`s still relevant in the error-case?
If `SetCapture()` can fail in this particular situation, you may want to do
something like:
```
if (error)
FAIL() << llvm::toString(std::move(error));
```
Otherwise, if this was only to prepare the test and you wanted to indicate that
it's not supposed to fail, then you could do:
```
llvm::cantFail(reproducer.SetCapture(true, FileSpec("/bogus/path"));
```
Repository:
rLLDB LLDB
https://reviews.llvm.org/D54616
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D54616: [Reproducers] Improve reproducer API and add unit tests.
JDevlieghere created this revision.
JDevlieghere added reviewers: davide, shafik, friss, jingham.
JDevlieghere added a project: LLDB.
Herald added subscribers: abidh, mgorny.
When I landed the initial reproducer framework I knew there were some things
that needed improvement. Rather than bundling it with a patch that adds more
functionality I split it off into this patch. I also think the API is stable
enough to add unit testing, which is included in this patch as well.
Other improvements include:
- Refactor how we initialize the loader and generator.
- Improve naming consistency: capture and replay seems the least ambiguous.
- Index providers by name and make sure there's only one of each.
- Add convenience methods for creating and accessing providers.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D54616
Files:
include/lldb/Core/Debugger.h
include/lldb/Utility/Reproducer.h
source/API/SBDebugger.cpp
source/Commands/CommandObjectReproducer.cpp
source/Core/Debugger.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
source/Utility/Reproducer.cpp
unittests/Utility/CMakeLists.txt
unittests/Utility/ReproducerTest.cpp
Index: unittests/Utility/ReproducerTest.cpp
===
--- /dev/null
+++ unittests/Utility/ReproducerTest.cpp
@@ -0,0 +1,144 @@
+//===-- ReproducerTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Reproducer.h"
+
+using namespace lldb_private;
+using namespace lldb_private::repro;
+
+class DummyProvider : public repro::Provider {
+public:
+ static constexpr const char *NAME = "dummy";
+
+ DummyProvider(const FileSpec ) : Provider(directory) {
+InitializeFileInfo(NAME, {"dummy.yaml"});
+ }
+};
+
+TEST(ReproducerTest, SetCapture) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Enable capture and check that means we have a generator.
+ {
+auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
+EXPECT_FALSE(static_cast(error));
+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);
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
+EXPECT_EQ(nullptr, reproducer.GetLoader());
+ }
+}
+
+TEST(ReproducerTest, SetReplay) {
+ Reproducer reproducer;
+
+ // Initially both generator and loader are unset.
+ EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ EXPECT_EQ(nullptr, reproducer.GetLoader());
+
+ // Enable capture and check that means we have a generator.
+ {
+auto error = reproducer.SetReplay(true, FileSpec("/bogus/path"));
+// Expected to fail because we can't load the index.
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
+// However the loader should still be set, which we check here.
+EXPECT_NE(nullptr, reproducer.GetLoader());
+
+// Make sure the bogus path is correctly set.
+EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetLoader()->GetRoot());
+EXPECT_EQ(FileSpec("/bogus/path"), reproducer.GetReproducerPath());
+ }
+
+ // Ensure that we cannot enable replay.
+ {
+auto error = reproducer.SetCapture(true);
+EXPECT_TRUE(static_cast(error));
+llvm::consumeError(std::move(error));
+EXPECT_EQ(nullptr, reproducer.GetGenerator());
+ }
+}
+
+TEST(GeneratorTest, ChangeRoot) {
+ Reproducer reproducer;
+
+ auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
+ EXPECT_FALSE(static_cast(error));
+ auto = *reproducer.GetGenerator();
+
+ // As long as we haven't registered any providers this should work.
+ generator.ChangeRoot(FileSpec("/other/bogus/path"));
+
+ EXPECT_EQ(FileSpec("/other/bogus/path"),
+reproducer.GetGenerator()->GetRoot());
+ EXPECT_EQ(FileSpec("/other/bogus/path"), reproducer.GetReproducerPath());
+}
+
+TEST(GeneratorTest, Create) {
+ Reproducer reproducer;
+
+ auto error = reproducer.SetCapture(true, FileSpec("/bogus/path"));
+ EXPECT_FALSE(static_cast(error));
+ auto = *reproducer.GetGenerator();
+
+ auto *provider = generator.Create();
+ EXPECT_NE(nullptr, provider);
+ EXPECT_EQ(FileSpec("/bogus/path"), provider->GetRoot());
+ EXPECT_EQ(std::string("dummy"), provider->GetInfo().name);
+ EXPECT_EQ((size_t)1,
