[Reproducers] Refactor reproducer info

[JDevlieghere]

Summary

In the original reproducer design, I expected providers to be more dynamic than they turned out. For example, we don't have any instances where one provider has multiple files. Additionally, I expected there to be less locality between capture and replay, with the provider being defined in one place and the replay code to live in another. Both contributed to the design of the provider info.

This patch refactors the reproducer info to be something static. This means less magic strings and better type checking. The new design still allows for the capture and replay code to live in different places as long as they both have access to the new statically defined info class.

I didn't completely get rid of the index, because it is useful for (1) sanity checking and (2) knowing what files are used by the reproducer.

[labath]

This looks much better. LGTM, just make sure to do something with the lower_bound search, as I don't think that's right.

include/lldb/Utility/Reproducer.h

58–59 ↗

(On Diff #182159)

return StringRef ?

164 ↗

(On Diff #182159)

This might even be a vector of StringRefs, if you're ok with saying that the Provider class is responsible for these strings with sufficient lifetime. In the current setup it kind of already is because there's no telling who will use the returned c strings and when.

source/Utility/Reproducer.cpp

218–219 ↗

(On Diff #182159)

This doesn't seem right. This will only return false if file is lexicographically after all files in the m_files array. Perhaps you meant return it != m_files.end() && *it == file; ?

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

 class Reproducer;
 
 enum class ReproducerMode {
   Capture,
   Replay,
   Off,
 };
 
-/// Abstraction for information associated with a provider. This information
-/// is serialized into an index which is used by the loader.
-struct ProviderInfo {
-  std::string name;
-  std::vector<std::string> files;
-};
-
 /// 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.
+/// reproducing.
 ///
 /// Different components will implement different providers.
 class ProviderBase {
 public:
   virtual ~ProviderBase() = default;
 
-  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;
 
+  virtual llvm::StringRef GetName() const = 0;
+  virtual llvm::StringRef GetFile() const = 0;
+
 protected:
   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_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; }
 
+  llvm::StringRef GetName() const override { return ThisProviderT::info::name; }
+  llvm::StringRef GetFile() const override { return ThisProviderT::info::file; }
+
 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 {
private:
   /// Flag to ensure that we never call both keep and discard.
   bool m_done;
 };
 
 class Loader final {
 public:
   Loader(const FileSpec &root);
 
-  llvm::Optional<ProviderInfo> GetProviderInfo(llvm::StringRef name);
+  template <typename T> FileSpec GetFile() {
+    if (!HasFile(T::file))
+      return {};
+
+    return GetRoot().CopyByAppendingPathComponent(T::file);
+  }
+
   llvm::Error LoadIndex();
 
   const FileSpec &GetRoot() const { return m_root; }
 
 private:
-  llvm::StringMap<ProviderInfo> m_provider_info;
+  bool HasFile(llvm::StringRef file);
+
   FileSpec m_root;
+  std::vector<std::string> m_files;
   bool m_loaded;
 };
 
 /// The reproducer enables clients to obtain access to the Generator and
 /// Loader.
 class Reproducer {
 public:
   static Reproducer &Instance();
private:
   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)
-
-namespace llvm {
-namespace yaml {
-
-template <> struct MappingTraits<lldb_private::repro::ProviderInfo> {
-  static void mapping(IO &io, lldb_private::repro::ProviderInfo &info) {
-    io.mapRequired("name", info.name);
-    io.mapOptional("files", info.files);
-  }
-};
-} // namespace yaml
-} // namespace llvm
-
 #endif // LLDB_UTILITY_REPRODUCER_H

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;
 }
 
+struct ProcessGDBRemoteInfo {
+  static const char *name;
+  static const char *file;
+};
+
+const char *ProcessGDBRemoteInfo::name = "gdb-remote";
+const char *ProcessGDBRemoteInfo::file = "gdb-remote.yaml";
+
 class ProcessGDBRemoteProvider
     : public repro::Provider<ProcessGDBRemoteProvider> {
 public:
+  typedef ProcessGDBRemoteInfo info;
+
   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 =
-        GetRoot().CopyByAppendingPathComponent("gdb-remote.yaml");
+    FileSpec history_file = GetRoot().CopyByAppendingPathComponent(info::file);
 
     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) {
if (!m_gdb_comm.SendAsyncSignal(signo))
     error.SetErrorStringWithFormat("failed to send signal %i", signo);
   return error;
 }
 
 Status ProcessGDBRemote::ConnectToReplayServer(repro::Loader *loader) {
   if (!loader)
     return Status("No loader provided.");
 
-  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->GetRoot().CopyByAppendingPathComponent(
-      provider_info->files.front());
+  FileSpec history_file = loader->GetFile<ProcessGDBRemoteInfo>();
+  if (!history_file)
+    return Status("No provider for gdb-remote.");
 
   // 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

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);
 
+  std::vector<std::string> files;
+  files.reserve(m_providers.size());
   for (auto &provider : m_providers) {
-    auto &provider_info = provider.second->GetInfo();
-    yout << const_cast<ProviderInfo &>(provider_info);
+    files.emplace_back(provider.second->GetFile());
   }
+
+  yout << files;
 }
 
 Loader::Loader(const FileSpec &root) : m_root(root), m_loaded(false) {}
 
 llvm::Error Loader::LoadIndex() {
   if (m_loaded)
     return llvm::Error::success();
 
   FileSpec index = m_root.CopyByAppendingPathComponent("index.yaml");
 
   auto error_or_file = MemoryBuffer::getFile(index.GetPath());
   if (auto err = error_or_file.getError())
     return make_error<StringError>("unable to load reproducer index", err);
 
-  std::vector<ProviderInfo> provider_info;
   yaml::Input yin((*error_or_file)->getBuffer());
-  yin >> provider_info;
-
+  yin >> m_files;
   if (auto err = yin.error())
     return make_error<StringError>("unable to read reproducer index", err);
 
-  for (auto &info : provider_info)
-    m_provider_info[info.name] = info;
+  // Sort files to speed up search.
+  llvm::sort(m_files);
 
+  // Remember that we've loaded the index.
   m_loaded = true;
 
   return llvm::Error::success();
 }
 
-llvm::Optional<ProviderInfo> Loader::GetProviderInfo(StringRef name) {
+bool Loader::HasFile(StringRef file) {
   assert(m_loaded);
-
-  auto it = m_provider_info.find(name);
-  if (it == m_provider_info.end())
-    return llvm::None;
-
-  return it->second;
+  auto it = std::lower_bound(m_files.begin(), m_files.end(), file.str());
+  return (it != m_files.end()) && (*it == file);
 }
 
 void ProviderBase::anchor() {}
 char ProviderBase::ID = 0;