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; 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 @@ -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 @@ /// 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 @@ /// 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 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 @@ /// 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 @@ /// 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(new T(m_root)); + return static_cast(Register(std::move(provider))); + } + + /// Get an existing provider. + template T *Get() { + auto it = m_providers.find(T::ClassID()); + if (it == m_providers.end()) + return nullptr; + return static_cast(it->second.get()); + } + + /// Get a provider if it exists, otherwise create it. + template T &GetOrCreate() { + auto *provider = Get(); + if (provider) + return *provider; + return *Create(); } - void ChangeDirectory(const FileSpec &directory); - const FileSpec &GetDirectory() const; + 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); + ProviderBase *Register(std::unique_ptr provider); + + /// Builds and index with provider info. + void AddProvidersToIndex(); - std::vector> m_providers; + /// List of providers indexed by their name for easy access. + llvm::DenseMap> 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 @@ 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 +170,14 @@ const Generator *GetGenerator() const; const Loader *GetLoader() const; - llvm::Error SetGenerateReproducer(bool value); - llvm::Error SetReplayReproducer(bool value); + llvm::Error SetCapture(llvm::Optional root); + llvm::Error SetReplay(llvm::Optional 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: 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: source/API/SBDebugger.cpp =================================================================== --- source/API/SBDebugger.cpp +++ source/API/SBDebugger.cpp @@ -1057,9 +1057,15 @@ : 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 { 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(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,17 +418,22 @@ 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 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 arg = llvm::None; + + if (b) + arg = HostInfo::GetReproducerTempDir(); + + return repro::Reproducer::Instance().SetCapture(arg); } const FormatEntity::Entry *Debugger::GetThreadFormat() const { Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp =================================================================== --- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -157,7 +157,8 @@ return g_settings_sp; } -class ProcessGDBRemoteProvider : public repro::Provider { +class ProcessGDBRemoteProvider + : public repro::Provider { public: ProcessGDBRemoteProvider(const FileSpec &directory) : Provider(directory) { m_info.name = "gdb-remote"; @@ -166,7 +167,7 @@ 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, @@ -182,11 +183,15 @@ void Discard() override { m_callback(); } + static char ID; + private: std::function m_callback; std::unique_ptr m_stream_up; }; +char ProcessGDBRemoteProvider::ID = 0; + } // namespace // TODO Randomly assigning a port is unsafe. We should get an unused @@ -297,10 +302,9 @@ 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(); + 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. @@ -3436,8 +3440,8 @@ 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; 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,145 +25,131 @@ 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(llvm::Optional root) { std::lock_guard guard(m_mutex); - if (value && m_replay_reproducer) + if (root && m_loader) return make_error( "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 root) { std::lock_guard guard(m_mutex); - if (value && m_generate_reproducer) + if (root && 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 (root) { + 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_done(false) {} Generator::~Generator() {} -Provider &Generator::Register(std::unique_ptr provider) { +ProviderBase *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::pair> 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(); - - llvm::sys::fs::remove_directories(m_directory.GetPath()); -} + provider.second->Discard(); -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; + llvm::sys::fs::remove_directories(m_root.GetPath()); } -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 +165,6 @@ 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 @@ return it->second; } + +void ProviderBase::anchor() {} +char ProviderBase::ID = 0; 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: 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,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 &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(); + 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(); + 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 &generator = *reproducer.GetGenerator(); + + auto &provider = generator.GetOrCreate(); + 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); +}