[Reproducers] Improve reproducer API and add unit tests.

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.

Differential revision: https://reviews.llvm.org/D54616

llvm-svn: 347716
This commit is contained in:
Jonas Devlieghere 2018-11-27 22:11:02 +00:00
parent a116602475
commit 68ed93d252
12 changed files with 284 additions and 138 deletions

View File

@ -228,7 +228,7 @@ public:
const char *GetReproducerPath() const;
void SetReproducerPath(const char *reproducer);
lldb::SBError ReplayReproducer(const char *path);
lldb::ScriptLanguage GetScriptLanguage() const;

View File

@ -263,8 +263,8 @@ public:
llvm::StringRef GetReproducerPath() const;
void SetReproducerPath(llvm::StringRef p);
void SetReproducerPath(const char *) = delete;
llvm::Error SetReproducerReplay(llvm::StringRef p);
llvm::Error SetReproducerReplay(const char *) = delete;
llvm::Error SetReproducerCapture(bool b);

View File

@ -12,7 +12,7 @@
#include "lldb/Utility/FileSpec.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/YAMLTraits.h"
@ -38,12 +38,12 @@ struct ProviderInfo {
/// i.e. in the constructor.
///
/// Different components will implement different providers.
class Provider {
class ProviderBase {
public:
virtual ~Provider() = default;
virtual ~ProviderBase() = default;
const ProviderInfo &GetInfo() { return m_info; }
const FileSpec &GetDirectory() { return m_directory; }
const ProviderInfo &GetInfo() const { return m_info; }
const FileSpec &GetRoot() const { return m_root; }
/// The Keep method is called when it is decided that we need to keep the
/// data in order to provide a reproducer.
@ -53,15 +53,33 @@ public:
/// keep any information and will not generate a reproducer.
virtual void Discard(){};
// Returns the class ID for this type.
static const void *ClassID() { return &ID; }
// Returns the class ID for the dynamic type of this Provider instance.
virtual const void *DynamicClassID() const = 0;
protected:
Provider(const FileSpec &directory) : m_directory(directory) {}
ProviderBase(const FileSpec &root) : m_root(root) {}
/// Every provider keeps track of its own files.
ProviderInfo m_info;
private:
/// Every provider knows where to dump its potential files.
FileSpec m_directory;
FileSpec m_root;
virtual void anchor();
static char ID;
};
template <typename ThisProviderT> class Provider : public ProviderBase {
public:
static const void *ClassID() { return &ThisProviderT::ID; }
const void *DynamicClassID() const override { return &ThisProviderT::ID; }
protected:
using ProviderBase::ProviderBase; // Inherit constructor.
};
/// The generator is responsible for the logic needed to generate a
@ -69,7 +87,7 @@ private:
/// is necessary for reproducing a failure.
class Generator final {
public:
Generator();
Generator(const FileSpec &root);
~Generator();
/// Method to indicate we want to keep the reproducer. If reproducer
@ -81,31 +99,44 @@ public:
/// might need to clean up files already written to disk.
void Discard();
/// Providers are registered at creating time.
template <typename T> T &CreateProvider() {
std::unique_ptr<T> provider = llvm::make_unique<T>(m_directory);
return static_cast<T &>(Register(std::move(provider)));
/// Create and register a new provider.
template <typename T> T *Create() {
std::unique_ptr<ProviderBase> provider = llvm::make_unique<T>(m_root);
return static_cast<T *>(Register(std::move(provider)));
}
void ChangeDirectory(const FileSpec &directory);
const FileSpec &GetDirectory() const;
/// Get an existing provider.
template <typename T> T *Get() {
auto it = m_providers.find(T::ClassID());
if (it == m_providers.end())
return nullptr;
return static_cast<T *>(it->second.get());
}
/// Get a provider if it exists, otherwise create it.
template <typename T> T &GetOrCreate() {
auto *provider = Get<T>();
if (provider)
return *provider;
return *Create<T>();
}
const FileSpec &GetRoot() const;
private:
friend Reproducer;
void SetEnabled(bool enabled) { m_enabled = enabled; }
Provider &Register(std::unique_ptr<Provider> provider);
void AddProviderToIndex(const ProviderInfo &provider_info);
ProviderBase *Register(std::unique_ptr<ProviderBase> provider);
std::vector<std::unique_ptr<Provider>> m_providers;
/// Builds and index with provider info.
void AddProvidersToIndex();
/// Map of provider IDs to provider instances.
llvm::DenseMap<const void *, std::unique_ptr<ProviderBase>> m_providers;
std::mutex m_providers_mutex;
/// The reproducer root directory.
FileSpec m_directory;
/// Flag for controlling whether we generate a reproducer when Keep is
/// called.
bool m_enabled;
FileSpec m_root;
/// Flag to ensure that we never call both keep and discard.
bool m_done;
@ -113,16 +144,16 @@ private:
class Loader final {
public:
Loader();
Loader(const FileSpec &root);
llvm::Optional<ProviderInfo> GetProviderInfo(llvm::StringRef name);
llvm::Error LoadIndex(const FileSpec &directory);
llvm::Error LoadIndex();
const FileSpec &GetDirectory() { return m_directory; }
const FileSpec &GetRoot() const { return m_root; }
private:
llvm::StringMap<ProviderInfo> m_provider_info;
FileSpec m_directory;
FileSpec m_root;
bool m_loaded;
};
@ -139,18 +170,14 @@ public:
const Generator *GetGenerator() const;
const Loader *GetLoader() const;
llvm::Error SetGenerateReproducer(bool value);
llvm::Error SetReplayReproducer(bool value);
llvm::Error SetCapture(llvm::Optional<FileSpec> root);
llvm::Error SetReplay(llvm::Optional<FileSpec> root);
llvm::Error SetReproducerPath(const FileSpec &path);
FileSpec GetReproducerPath() const;
private:
Generator m_generator;
Loader m_loader;
bool m_generate_reproducer = false;
bool m_replay_reproducer = false;
llvm::Optional<Generator> m_generator;
llvm::Optional<Loader> m_loader;
mutable std::mutex m_mutex;
};

View File

@ -376,8 +376,8 @@ public:
const char *
GetReproducerPath() const;
void
SetReproducerPath (const char *path);
lldb::SBError
ReplayReproducer (const char *path);
lldb::ScriptLanguage
GetScriptLanguage() const;

View File

@ -1057,9 +1057,15 @@ const char *SBDebugger::GetReproducerPath() const {
: nullptr);
}
void SBDebugger::SetReproducerPath(const char *p) {
if (m_opaque_sp)
m_opaque_sp->SetReproducerPath(llvm::StringRef::withNullAsEmpty(p));
SBError SBDebugger::ReplayReproducer(const char *p) {
SBError sb_error;
if (m_opaque_sp) {
auto error =
m_opaque_sp->SetReproducerReplay(llvm::StringRef::withNullAsEmpty(p));
std::string error_str = llvm::toString(std::move(error));
sb_error.ref().SetErrorString(error_str);
}
return sb_error;
}
ScriptLanguage SBDebugger::GetScriptLanguage() const {

View File

@ -143,25 +143,13 @@ protected:
auto &r = repro::Reproducer::Instance();
if (auto e = r.SetReplayReproducer(true)) {
const char *repro_path = command.GetArgumentAtIndex(0);
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;
}
if (auto loader = r.GetLoader()) {
const char *repro_path = command.GetArgumentAtIndex(0);
if (auto e = loader->LoadIndex(FileSpec(repro_path))) {
std::string error_str = llvm::toString(std::move(e));
result.AppendErrorWithFormat("Unable to load reproducer: %s",
error_str.c_str());
return false;
}
} else {
result.AppendErrorWithFormat("Unable to get the reproducer loader");
return false;
}
result.SetStatus(eReturnStatusSuccessFinishNoResult);
return result.Succeeded();
}

View File

@ -418,17 +418,22 @@ llvm::StringRef Debugger::GetReproducerPath() const {
return r.GetReproducerPath().GetCString();
}
void Debugger::SetReproducerPath(llvm::StringRef p) {
auto &r = repro::Reproducer::Instance();
if (auto e = r.SetReproducerPath(FileSpec(p)))
llvm::consumeError(std::move(e));
llvm::Error Debugger::SetReproducerReplay(llvm::StringRef p) {
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();
if (auto e = r.SetGenerateReproducer(b))
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 {

View File

@ -157,7 +157,8 @@ static const ProcessKDPPropertiesSP &GetGlobalPluginProperties() {
return g_settings_sp;
}
class ProcessGDBRemoteProvider : public repro::Provider {
class ProcessGDBRemoteProvider
: public repro::Provider<ProcessGDBRemoteProvider> {
public:
ProcessGDBRemoteProvider(const FileSpec &directory) : Provider(directory) {
m_info.name = "gdb-remote";
@ -166,7 +167,7 @@ public:
raw_ostream *GetHistoryStream() {
FileSpec history_file =
GetDirectory().CopyByAppendingPathComponent("gdb-remote.yaml");
GetRoot().CopyByAppendingPathComponent("gdb-remote.yaml");
std::error_code EC;
m_stream_up = llvm::make_unique<raw_fd_ostream>(history_file.GetPath(), EC,
@ -182,11 +183,15 @@ public:
void Discard() override { m_callback(); }
static char ID;
private:
std::function<void()> m_callback;
std::unique_ptr<raw_fd_ostream> m_stream_up;
};
char ProcessGDBRemoteProvider::ID = 0;
} // namespace
// TODO Randomly assigning a port is unsafe. We should get an unused
@ -297,10 +302,9 @@ ProcessGDBRemote::ProcessGDBRemote(lldb::TargetSP target_sp,
m_async_broadcaster.SetEventName(eBroadcastBitAsyncThreadDidExit,
"async thread did exit");
repro::Generator *generator = repro::Reproducer::Instance().GetGenerator();
if (generator) {
if (repro::Generator *g = repro::Reproducer::Instance().GetGenerator()) {
ProcessGDBRemoteProvider &provider =
generator->CreateProvider<ProcessGDBRemoteProvider>();
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.
@ -3436,8 +3440,8 @@ Status ProcessGDBRemote::ConnectToReplayServer(repro::Loader *loader) {
return Status("Provider for gdb-remote contains no files.");
// Construct replay history path.
FileSpec history_file(loader->GetDirectory());
history_file.AppendPathComponent(provider_info->files.front());
FileSpec history_file = loader->GetRoot().CopyByAppendingPathComponent(
provider_info->files.front());
// Enable replay mode.
m_replay_mode = true;

View File

@ -8,7 +8,6 @@
//===----------------------------------------------------------------------===//
#include "lldb/Utility/Reproducer.h"
#include "lldb/Host/HostInfo.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Threading.h"
@ -26,145 +25,131 @@ Reproducer &Reproducer::Instance() {
const Generator *Reproducer::GetGenerator() const {
std::lock_guard<std::mutex> guard(m_mutex);
if (m_generate_reproducer)
return &m_generator;
if (m_generator)
return &(*m_generator);
return nullptr;
}
const Loader *Reproducer::GetLoader() const {
std::lock_guard<std::mutex> guard(m_mutex);
if (m_replay_reproducer)
return &m_loader;
if (m_loader)
return &(*m_loader);
return nullptr;
}
Generator *Reproducer::GetGenerator() {
std::lock_guard<std::mutex> guard(m_mutex);
if (m_generate_reproducer)
return &m_generator;
if (m_generator)
return &(*m_generator);
return nullptr;
}
Loader *Reproducer::GetLoader() {
std::lock_guard<std::mutex> guard(m_mutex);
if (m_replay_reproducer)
return &m_loader;
if (m_loader)
return &(*m_loader);
return nullptr;
}
llvm::Error Reproducer::SetGenerateReproducer(bool value) {
llvm::Error Reproducer::SetCapture(llvm::Optional<FileSpec> root) {
std::lock_guard<std::mutex> guard(m_mutex);
if (value && m_replay_reproducer)
if (root && m_loader)
return make_error<StringError>(
"cannot generate a reproducer when replay one",
inconvertibleErrorCode());
m_generate_reproducer = value;
m_generator.SetEnabled(value);
if (root)
m_generator.emplace(*root);
else
m_generator.reset();
return Error::success();
}
llvm::Error Reproducer::SetReplayReproducer(bool value) {
llvm::Error Reproducer::SetReplay(llvm::Optional<FileSpec> root) {
std::lock_guard<std::mutex> guard(m_mutex);
if (value && m_generate_reproducer)
if (root && m_generator)
return make_error<StringError>(
"cannot replay a reproducer when generating one",
inconvertibleErrorCode());
m_replay_reproducer = value;
if (root) {
m_loader.emplace(*root);
if (auto e = m_loader->LoadIndex())
return e;
} else {
m_loader.reset();
}
return Error::success();
}
llvm::Error Reproducer::SetReproducerPath(const FileSpec &path) {
// Setting the path implies using the reproducer.
if (auto e = SetReplayReproducer(true))
return e;
// Tell the reproducer to load the index form the given path.
if (auto loader = GetLoader()) {
if (auto e = loader->LoadIndex(path))
return e;
}
return make_error<StringError>("unable to get loader",
inconvertibleErrorCode());
}
FileSpec Reproducer::GetReproducerPath() const {
if (auto g = GetGenerator())
return g->GetDirectory();
return g->GetRoot();
if (auto l = GetLoader())
return l->GetRoot();
return {};
}
Generator::Generator() : m_enabled(false), m_done(false) {
m_directory = HostInfo::GetReproducerTempDir();
}
Generator::Generator(const FileSpec &root) : m_root(root), m_done(false) {}
Generator::~Generator() {}
Provider &Generator::Register(std::unique_ptr<Provider> provider) {
ProviderBase *Generator::Register(std::unique_ptr<ProviderBase> provider) {
std::lock_guard<std::mutex> lock(m_providers_mutex);
AddProviderToIndex(provider->GetInfo());
m_providers.push_back(std::move(provider));
return *m_providers.back();
std::pair<const void *, std::unique_ptr<ProviderBase>> key_value(
provider->DynamicClassID(), std::move(provider));
auto e = m_providers.insert(std::move(key_value));
return e.first->getSecond().get();
}
void Generator::Keep() {
assert(!m_done);
m_done = true;
if (!m_enabled)
return;
for (auto &provider : m_providers)
provider->Keep();
provider.second->Keep();
AddProvidersToIndex();
}
void Generator::Discard() {
assert(!m_done);
m_done = true;
if (!m_enabled)
return;
for (auto &provider : m_providers)
provider->Discard();
provider.second->Discard();
llvm::sys::fs::remove_directories(m_directory.GetPath());
llvm::sys::fs::remove_directories(m_root.GetPath());
}
void Generator::ChangeDirectory(const FileSpec &directory) {
assert(m_providers.empty() && "Changing the directory after providers have "
"been registered would invalidate the index.");
m_directory = directory;
}
const FileSpec &Generator::GetRoot() const { return m_root; }
const FileSpec &Generator::GetDirectory() const { return m_directory; }
void Generator::AddProviderToIndex(const ProviderInfo &provider_info) {
FileSpec index = m_directory;
void Generator::AddProvidersToIndex() {
FileSpec index = m_root;
index.AppendPathComponent("index.yaml");
std::error_code EC;
auto strm = llvm::make_unique<raw_fd_ostream>(index.GetPath(), EC,
sys::fs::OpenFlags::F_None);
yaml::Output yout(*strm);
yout << const_cast<ProviderInfo &>(provider_info);
for (auto &provider : m_providers) {
auto &provider_info = provider.second->GetInfo();
yout << const_cast<ProviderInfo &>(provider_info);
}
}
Loader::Loader() : m_loaded(false) {}
Loader::Loader(const FileSpec &root) : m_root(root), m_loaded(false) {}
llvm::Error Loader::LoadIndex(const FileSpec &directory) {
llvm::Error Loader::LoadIndex() {
if (m_loaded)
return llvm::Error::success();
FileSpec index = directory.CopyByAppendingPathComponent("index.yaml");
FileSpec index = m_root.CopyByAppendingPathComponent("index.yaml");
auto error_or_file = MemoryBuffer::getFile(index.GetPath());
if (auto err = error_or_file.getError())
@ -180,7 +165,6 @@ llvm::Error Loader::LoadIndex(const FileSpec &directory) {
for (auto &info : provider_info)
m_provider_info[info.name] = info;
m_directory = directory;
m_loaded = true;
return llvm::Error::success();
@ -195,3 +179,6 @@ llvm::Optional<ProviderInfo> Loader::GetProviderInfo(StringRef name) {
return it->second;
}
void ProviderBase::anchor() {}
char ProviderBase::ID = 0;

View File

@ -368,7 +368,9 @@ SBError Driver::ProcessArgs(const opt::InputArgList &args, FILE *out_fh,
auto optarg = arg->getValue();
SBFileSpec file(optarg);
if (file.Exists()) {
m_debugger.SetReproducerPath(optarg);
SBError repro_error = m_debugger.ReplayReproducer(optarg);
if (repro_error.Fail())
return repro_error;
} else {
error.SetErrorStringWithFormat("file specified in --reproducer "
"(-z) option doesn't exist: '%s'",

View File

@ -15,6 +15,7 @@ add_lldb_unittest(UtilityTests
NameMatchesTest.cpp
PredicateTest.cpp
RegisterValueTest.cpp
ReproducerTest.cpp
ScalarTest.cpp
StateTest.cpp
StatusTest.cpp

View File

@ -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<DummyProvider> {
public:
static constexpr const char *NAME = "dummy";
DummyProvider(const FileSpec &directory) : 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 &generator = *reproducer.GetGenerator();
auto *provider = generator.Create<DummyProvider>();
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 &generator = *reproducer.GetGenerator();
auto *provider = generator.Create<DummyProvider>();
EXPECT_NE(nullptr, provider);
auto *provider_alt = generator.Get<DummyProvider>();
EXPECT_EQ(provider, provider_alt);
}
TEST(GeneratorTest, GetOrCreate) {
Reproducer reproducer;
EXPECT_THAT_ERROR(reproducer.SetCapture(FileSpec("/bogus/path")),
Succeeded());
auto &generator = *reproducer.GetGenerator();
auto &provider = generator.GetOrCreate<DummyProvider>();
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());
auto &provider_alt = generator.GetOrCreate<DummyProvider>();
EXPECT_EQ(&provider, &provider_alt);
}