Index: include/lldb/Core/Debugger.h =================================================================== --- include/lldb/Core/Debugger.h +++ include/lldb/Core/Debugger.h @@ -263,8 +263,8 @@ 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); Index: include/lldb/Utility/Reproducer.h =================================================================== --- include/lldb/Utility/Reproducer.h +++ include/lldb/Utility/Reproducer.h @@ -42,8 +42,8 @@ public: virtual ~Provider() = 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. @@ -54,14 +54,21 @@ virtual void Discard(){}; protected: - Provider(const FileSpec &directory) : m_directory(directory) {} + Provider(const FileSpec &root) : m_root(root) {} + + void InitializeFileInfo(llvm::StringRef name, + llvm::ArrayRef files) { + m_info.name = name; + for (auto file : files) + m_info.files.push_back(file); + } /// 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; }; /// The generator is responsible for the logic needed to generate a @@ -69,7 +76,7 @@ /// 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,27 +88,46 @@ /// might need to clean up files already written to disk. void Discard(); - /// Providers are registered at creating time. - template T &CreateProvider() { - std::unique_ptr provider = llvm::make_unique(m_directory); - return static_cast(Register(std::move(provider))); + /// Create and register a new provider. + template T *Create() { + std::unique_ptr provider = llvm::make_unique(m_root); + return static_cast(Register(std::move(provider))); + } + + /// Get an existing provider. + template T *Get() { + auto it = m_providers.find(T::NAME); + if (it == m_providers.end()) + return nullptr; + return static_cast(it->second.get()); } - void ChangeDirectory(const FileSpec &directory); - const FileSpec &GetDirectory() const; + /// Get a provider if it exists, otherwise create it. + template T *GetOrCreate() { + auto *provider = Get(); + if (provider) + return provider; + return Create(); + } + + void ChangeRoot(const FileSpec &root); + const FileSpec &GetRoot() const; private: friend Reproducer; void SetEnabled(bool enabled) { m_enabled = enabled; } - Provider &Register(std::unique_ptr provider); - void AddProviderToIndex(const ProviderInfo &provider_info); + Provider *Register(std::unique_ptr provider); - std::vector> m_providers; + /// Builds and index with provider info. + void AddProvidersToIndex(); + + /// List of providers indexed by their name for easy access. + llvm::StringMap> m_providers; std::mutex m_providers_mutex; /// The reproducer root directory. - FileSpec m_directory; + FileSpec m_root; /// Flag for controlling whether we generate a reproducer when Keep is /// called. @@ -113,16 +139,16 @@ class Loader final { public: - Loader(); + Loader(const FileSpec &root); llvm::Optional 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 m_provider_info; - FileSpec m_directory; + FileSpec m_root; bool m_loaded; }; @@ -139,18 +165,14 @@ const Generator *GetGenerator() const; const Loader *GetLoader() const; - llvm::Error SetGenerateReproducer(bool value); - llvm::Error SetReplayReproducer(bool value); + llvm::Error SetCapture(bool value, FileSpec root = {}); + llvm::Error SetReplay(bool value, 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 m_generator; + llvm::Optional m_loader; mutable std::mutex m_mutex; }; Index: source/API/SBDebugger.cpp =================================================================== --- source/API/SBDebugger.cpp +++ source/API/SBDebugger.cpp @@ -1058,8 +1058,11 @@ } void SBDebugger::SetReproducerPath(const char *p) { - if (m_opaque_sp) - m_opaque_sp->SetReproducerPath(llvm::StringRef::withNullAsEmpty(p)); + if (m_opaque_sp) { + auto error = + m_opaque_sp->SetReproducerReplay(llvm::StringRef::withNullAsEmpty(p)); + llvm::consumeError(std::move(error)); + } } ScriptLanguage SBDebugger::GetScriptLanguage() const { Index: source/Commands/CommandObjectReproducer.cpp =================================================================== --- source/Commands/CommandObjectReproducer.cpp +++ source/Commands/CommandObjectReproducer.cpp @@ -143,25 +143,13 @@ auto &r = repro::Reproducer::Instance(); - if (auto e = r.SetReplayReproducer(true)) { + const char *repro_path = command.GetArgumentAtIndex(0); + if (auto e = r.SetReplay(true, 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(); } Index: source/Core/Debugger.cpp =================================================================== --- source/Core/Debugger.cpp +++ source/Core/Debugger.cpp @@ -418,15 +418,17 @@ return r.GetReproducerPath().GetCString(); } -void Debugger::SetReproducerPath(llvm::StringRef p) { +llvm::Error Debugger::SetReproducerReplay(llvm::StringRef p) { auto &r = repro::Reproducer::Instance(); - if (auto e = r.SetReproducerPath(FileSpec(p))) - llvm::consumeError(std::move(e)); + if (auto e = r.SetReplay(true, FileSpec(p))) + return e; + return llvm::Error::success(); } llvm::Error Debugger::SetReproducerCapture(bool b) { auto &r = repro::Reproducer::Instance(); - if (auto e = r.SetGenerateReproducer(b)) + auto root = HostInfo::GetReproducerTempDir(); + if (auto e = r.SetCapture(b, root)) return e; return llvm::Error::success(); } Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp =================================================================== --- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -159,14 +159,15 @@ class ProcessGDBRemoteProvider : public repro::Provider { public: + static constexpr const char *NAME = "gdb-remote"; + ProcessGDBRemoteProvider(const FileSpec &directory) : Provider(directory) { - m_info.name = "gdb-remote"; - m_info.files.push_back("gdb-remote.yaml"); + InitializeFileInfo(NAME, {"gdb-remote.yaml"}); } 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(history_file.GetPath(), EC, @@ -297,14 +298,14 @@ m_async_broadcaster.SetEventName(eBroadcastBitAsyncThreadDidExit, "async thread did exit"); - repro::Generator *generator = repro::Reproducer::Instance().GetGenerator(); - if (generator) { - ProcessGDBRemoteProvider &provider = - generator->CreateProvider(); - // 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); }); + if (repro::Generator *g = repro::Reproducer::Instance().GetGenerator()) { + if (ProcessGDBRemoteProvider *provider = + g->GetOrCreate()) { + // 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)); @@ -3436,7 +3437,7 @@ return Status("Provider for gdb-remote contains no files."); // Construct replay history path. - FileSpec history_file(loader->GetDirectory()); + FileSpec history_file = (loader->GetRoot()); history_file.AppendPathComponent(provider_info->files.front()); // Enable replay mode. Index: source/Utility/Reproducer.cpp =================================================================== --- source/Utility/Reproducer.cpp +++ source/Utility/Reproducer.cpp @@ -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,93 +25,89 @@ const Generator *Reproducer::GetGenerator() const { std::lock_guard 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 guard(m_mutex); - if (m_replay_reproducer) - return &m_loader; + if (m_loader) + return &(*m_loader); return nullptr; } Generator *Reproducer::GetGenerator() { std::lock_guard guard(m_mutex); - if (m_generate_reproducer) - return &m_generator; + if (m_generator) + return &(*m_generator); return nullptr; } Loader *Reproducer::GetLoader() { std::lock_guard 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(bool value, FileSpec root) { std::lock_guard guard(m_mutex); - if (value && m_replay_reproducer) + if (value && m_loader) return make_error( "cannot generate a reproducer when replay one", inconvertibleErrorCode()); - m_generate_reproducer = value; - m_generator.SetEnabled(value); + if (value) { + m_generator.emplace(root); + m_generator->SetEnabled(value); + } else { + m_generator.reset(); + } return Error::success(); } -llvm::Error Reproducer::SetReplayReproducer(bool value) { +llvm::Error Reproducer::SetReplay(bool value, FileSpec root) { std::lock_guard guard(m_mutex); - if (value && m_generate_reproducer) + if (value && m_generator) return make_error( "cannot replay a reproducer when generating one", inconvertibleErrorCode()); - m_replay_reproducer = value; - - 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)) + if (value) { + m_loader.emplace(root); + if (auto e = m_loader->LoadIndex()) return e; + } else { + m_loader.reset(); } - return make_error("unable to get loader", - inconvertibleErrorCode()); + return Error::success(); } 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_enabled(false), m_done(false) {} Generator::~Generator() {} -Provider &Generator::Register(std::unique_ptr provider) { +Provider *Generator::Register(std::unique_ptr provider) { std::lock_guard lock(m_providers_mutex); - - AddProviderToIndex(provider->GetInfo()); - - m_providers.push_back(std::move(provider)); - return *m_providers.back(); + std::string provider_name = provider->GetInfo().name; + std::pair> key_value( + provider_name, std::move(provider)); + auto e = m_providers.insert(std::move(key_value)); + return e.first->getValue().get(); } void Generator::Keep() { @@ -123,7 +118,9 @@ return; for (auto &provider : m_providers) - provider->Keep(); + provider.second->Keep(); + + AddProvidersToIndex(); } void Generator::Discard() { @@ -134,37 +131,42 @@ 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; +void Generator::ChangeRoot(const FileSpec &root) { + assert(m_providers.empty() && + "Changing the root directory after providers have " + "been registered would invalidate the index."); + m_root = root; } -const FileSpec &Generator::GetDirectory() const { return m_directory; } +const FileSpec &Generator::GetRoot() const { return m_root; } -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(index.GetPath(), EC, sys::fs::OpenFlags::F_None); yaml::Output yout(*strm); - yout << const_cast(provider_info); + + for (auto &provider : m_providers) { + auto &provider_info = provider.second->GetInfo(); + yout << const_cast(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 +182,6 @@ for (auto &info : provider_info) m_provider_info[info.name] = info; - m_directory = directory; m_loaded = true; return llvm::Error::success(); Index: unittests/Utility/CMakeLists.txt =================================================================== --- unittests/Utility/CMakeLists.txt +++ unittests/Utility/CMakeLists.txt @@ -15,6 +15,7 @@ NameMatchesTest.cpp PredicateTest.cpp RegisterValueTest.cpp + ReproducerTest.cpp ScalarTest.cpp StateTest.cpp StatusTest.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 &directory) : 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 &generator = *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 &generator = *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; + + auto error = reproducer.SetCapture(true, FileSpec("/bogus/path")); + EXPECT_FALSE(static_cast(error)); + auto &generator = *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; + + auto error = reproducer.SetCapture(true, FileSpec("/bogus/path")); + EXPECT_FALSE(static_cast(error)); + auto &generator = *reproducer.GetGenerator(); + + auto *provider = generator.GetOrCreate(); + 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()); + + auto *provider_alt = generator.GetOrCreate(); + EXPECT_EQ(provider, provider_alt); +}