[Reproducers] Improve reproducer API and add unit tests.

[JDevlieghere]

Summary

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.

[sgraenitz]

Looks pretty good to me, but added my 2¢ on minor things.

include/lldb/Utility/Reproducer.h

169 ↗

(On Diff #174330)

Are the default-value-cases necessary?

source/API/SBDebugger.cpp

1064 ↗

(On Diff #174330)

Will the error be passed up the stack soon? Otherwise maybe add FIXME comment?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

303 ↗

(On Diff #174330)

In which case would GetOrCreate() fail?

3440 ↗

(On Diff #174330)

Brackets

unittests/Utility/ReproducerTest.cpp

37 ↗

(On Diff #174330)

It may be good to know what went wrong if this fails. And: are the other EXPECTs 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"));

[JDevlieghere]

Address Stefan's review feedback.

[JDevlieghere]

Missed a comment; fixed now.

[JDevlieghere]

Simplify code a little, as per Stefan's (offline) suggestion.

[labath]

include/lldb/Utility/Reproducer.h

59–64 ↗

(On Diff #174416)

Could this be replaced by additional arguments to the constructor (avoiding two-stage initialization)?

99 ↗

(On Diff #174416)

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.)

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

304 ↗

(On Diff #174416)

could GetOrCreate keep returning a reference (to avoid spurious guaranteed to be not null comments)?

source/Utility/Reproducer.cpp

131–133 ↗

(On Diff #174416)

Given these limitations, would it be possible to set the root once in the constructor and keep it immutable since then?

unittests/Utility/ReproducerTest.cpp

44–46 ↗

(On Diff #174416)

Shorter and with better error messages:
EXPECT_THAT_ERROR(reproducer.SetReplay(FileSpec("/bogus/path")), llvm::Succeeded());

79–81 ↗

(On Diff #174416)

EXPECT_THAT_ERROR(..., llvm::Failed());

[JDevlieghere]

Thanks Pavel, really useful feedback, I learned a few new things :-)

[JDevlieghere]

Does anybody want to have another look or can I land this?

[labath]

I haven't been following the reproducer work in detail, but this seems reasonable to me. Thanks for incorporating my drive-by suggestions.

include/lldb/Utility/Reproducer.h

104 ↗

(On Diff #175293)

You should still be able to use make_unique here.

134 ↗

(On Diff #175293)

Out of date comment.

[davide]

LGTM

lldb/trunk/include/lldb/API/SBDebugger.h

public:
   lldb::user_id_t GetID();
 
   const char *GetPrompt() const;
 
   void SetPrompt(const char *prompt);
 
   const char *GetReproducerPath() const;
 
-  void SetReproducerPath(const char *reproducer);
+  lldb::SBError ReplayReproducer(const char *path);
 
   lldb::ScriptLanguage GetScriptLanguage() const;
 
   void SetScriptLanguage(lldb::ScriptLanguage script_lang);
 
   bool GetCloseInputOnEOF() const;
 
   void SetCloseInputOnEOF(bool b);

lldb/trunk/include/lldb/Core/Debugger.h

public:
 
   llvm::StringRef GetPrompt() const;
 
   void SetPrompt(llvm::StringRef p);
   void SetPrompt(const char *) = delete;
 
   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);
 
   bool GetUseExternalEditor() const;
 
   bool SetUseExternalEditor(bool use_external_editor_p);
 
   bool GetUseColor() const;

lldb/trunk/include/lldb/Utility/Reproducer.h

 //===-- Reproducer.h --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_REPRODUCER_H
 #define LLDB_UTILITY_REPRODUCER_H
 
 #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"
 
 #include <mutex>
 #include <string>
 #include <vector>
 
 namespace lldb_private {

 };
 
 /// The provider defines an interface for generating files needed for
 /// reproducing. The provider must populate its ProviderInfo to communicate
 /// its name and files to the index, before registering with the generator,
 /// 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.
   virtual void Keep(){};
 
   /// The Discard method is called when it is decided that we do not need to
   /// 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
 /// reproducer. For doing so it relies on providers, who serialize data that
 /// 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
   /// generation is disabled, this does nothing.
   void Keep();
 
   /// Method to indicate we do not want to keep the reproducer. This is
   /// unaffected by whether or not generation reproduction is enabled, as we
   /// 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)));
+  }
+
+  /// 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>();
   }
 
-  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> provider);
-  void AddProviderToIndex(const ProviderInfo &provider_info);
+  ProviderBase *Register(std::unique_ptr<ProviderBase> provider);
+
+  /// Builds and index with provider info.
+  void AddProvidersToIndex();
 
-  std::vector<std::unique_ptr<Provider>> m_providers;
+  /// 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;
 };
 
 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;
 };
 
 /// The reproducer enables clients to obtain access to the Generator and
 /// Loader.
 class Reproducer final {
 
 public:
   static Reproducer &Instance();
 
   Generator *GetGenerator();
   Loader *GetLoader();
 
   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;
 };
 
 } // namespace repro
 } // namespace lldb_private
 
 LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(lldb_private::repro::ProviderInfo)

lldb/trunk/scripts/interface/SBDebugger.i

public:
     GetPrompt() const;
 
     void
     SetPrompt (const char *prompt);
 
     const char *
     GetReproducerPath() const;
 
-    void
-    SetReproducerPath (const char *path);
+    lldb::SBError
+    ReplayReproducer (const char *path);
 
     lldb::ScriptLanguage
     GetScriptLanguage() const;
 
     void
     SetScriptLanguage (lldb::ScriptLanguage script_lang);
 
     bool

lldb/trunk/source/API/SBDebugger.cpp

 }
 
 const char *SBDebugger::GetReproducerPath() const {
   return (m_opaque_sp
               ? ConstString(m_opaque_sp->GetReproducerPath()).GetCString()
               : 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 {
   return (m_opaque_sp ? m_opaque_sp->GetScriptLanguage() : eScriptLanguageNone);
 }
 
 void SBDebugger::SetScriptLanguage(ScriptLanguage script_lang) {
   if (m_opaque_sp) {

lldb/trunk/source/Commands/CommandObjectReproducer.cpp

if (command.empty()) {
       result.AppendErrorWithFormat(
           "'%s' takes a single argument: the reproducer path",
           m_cmd_name.c_str());
       return false;
     }
 
     auto &r = repro::Reproducer::Instance();
 
-    if (auto e = r.SetReplayReproducer(true)) {
-      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");
+    if (auto e = r.SetReplay(FileSpec(repro_path))) {
+      result.AppendErrorWithFormat("%s", error_str.c_str());
       return false;
     }
 
     result.SetStatus(eReturnStatusSuccessFinishNoResult);
     return result.Succeeded();
   }
 };
 

lldb/trunk/source/Core/Debugger.cpp

void Debugger::SetPrompt(llvm::StringRef p) {
   GetCommandInterpreter().UpdatePrompt(new_prompt);
 }
 
 llvm::StringRef Debugger::GetReproducerPath() const {
   auto &r = repro::Reproducer::Instance();
   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 {
   const uint32_t idx = ePropertyThreadFormat;
   return m_collection_sp->GetPropertyAtIndexAsFormatEntity(nullptr, idx);
 }
 
 const FormatEntity::Entry *Debugger::GetThreadStopFormat() const {

lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

 
 static const ProcessKDPPropertiesSP &GetGlobalPluginProperties() {
   static ProcessKDPPropertiesSP g_settings_sp;
   if (!g_settings_sp)
     g_settings_sp.reset(new PluginProperties());
   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";
     m_info.files.push_back("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<raw_fd_ostream>(history_file.GetPath(), EC,
                                                     sys::fs::OpenFlags::F_None);
     return m_stream_up.get();
   }
 
   void SetCallback(std::function<void()> callback) {
     m_callback = std::move(callback);
   }
 
   void Keep() override { m_callback(); }
 
   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
 // ephemeral port from the kernel and make sure we reserve it before passing it
 // to debugserver.
 
 #if defined(__APPLE__)
 #define LOW_PORT (IPPORT_RESERVED)
: Process(target_sp, listener_sp),
       m_allow_flash_writes(false), m_erased_flash_ranges() {
   m_async_broadcaster.SetEventName(eBroadcastBitAsyncThreadShouldExit,
                                    "async thread should exit");
   m_async_broadcaster.SetEventName(eBroadcastBitAsyncContinue,
                                    "async thread continue");
   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.
     provider.SetCallback([&]() { m_gdb_comm.SetHistoryStream(nullptr); });
   }
 
   Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_ASYNC));
 
Status ProcessGDBRemote::ConnectToReplayServer(repro::Loader *loader) {
   auto provider_info = loader->GetProviderInfo("gdb-remote");
   if (!provider_info)
     return Status("No provider for gdb-remote.");
 
   if (provider_info->files.empty())
     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;
 
   // Load replay history.
   if (auto error = m_gdb_replay_server.LoadReplayHistory(history_file))
     return Status("Unable to load replay history");
 

lldb/trunk/source/Utility/Reproducer.cpp

 //===-- Reproducer.cpp ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Utility/Reproducer.h"
-#include "lldb/Host/HostInfo.h"
 
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace lldb_private;
 using namespace lldb_private::repro;
 using namespace llvm;
 using namespace llvm::yaml;
 
 Reproducer &Reproducer::Instance() {
   static Reproducer g_reproducer;
   return g_reproducer;
 }
 
 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;
-
-  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<StringError>("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> 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);
+
+  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())
     return errorCodeToError(err);
 
   std::vector<ProviderInfo> provider_info;
   yaml::Input yin((*error_or_file)->getBuffer());
   yin >> provider_info;
 
   if (auto err = yin.error())
     return errorCodeToError(err);
 
   for (auto &info : provider_info)
     m_provider_info[info.name] = info;
 
-  m_directory = directory;
   m_loaded = true;
 
   return llvm::Error::success();
 }
 
 llvm::Optional<ProviderInfo> Loader::GetProviderInfo(StringRef name) {
   assert(m_loaded);
 
   auto it = m_provider_info.find(name);
   if (it == m_provider_info.end())
     return llvm::None;
 
   return it->second;
 }
+
+void ProviderBase::anchor() {}
+char ProviderBase::ID = 0;

lldb/trunk/tools/driver/Driver.cpp

SBError Driver::ProcessArgs(const opt::InputArgList &args, FILE *out_fh,
   if (args.hasArg(OPT_no_use_colors)) {
     m_option_data.m_debug_mode = true;
   }
 
   if (auto *arg = args.getLastArg(OPT_reproducer)) {
     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'",
                                      optarg);
       return error;
     }
   }
 

lldb/trunk/unittests/Utility/CMakeLists.txt

add_lldb_unittest(UtilityTests
   EnvironmentTest.cpp
   FileSpecTest.cpp
   FlagsTest.cpp
   JSONTest.cpp
   LogTest.cpp
   NameMatchesTest.cpp
   PredicateTest.cpp
   RegisterValueTest.cpp
+  ReproducerTest.cpp
   ScalarTest.cpp
   StateTest.cpp
   StatusTest.cpp
   StreamTeeTest.cpp
   StreamTest.cpp
   StringExtractorTest.cpp
   StringLexerTest.cpp
   StringListTest.cpp

lldb/trunk/unittests/Utility/ReproducerTest.cpp

+//===-- 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);
+}