[lldb/Reproducers] Support multiple GDB remotes

When running the test suite with always capture on, a handful of tests
are failing because they have multiple targets and therefore multiple
GDB remote connections. The current reproducer infrastructure is capable
of dealing with that.

This patch reworks the GDB remote provider to support multiple GDB
remote connections, similar to how the reproducers support shadowing
multiple command interpreter inputs. The provider now keeps a list of
packet recorders which deal with a single GDB remote connection. During
replay we rely on the order of creation to match the number of packets
to the GDB remote connection.

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

(cherry picked from commit e81268d)

Author: JDevlieghere

lldb/include/lldb/Utility/GDBRemote.h

@@ -9,6 +9,8 @@
 #ifndef liblldb_GDBRemote_h_
 #define liblldb_GDBRemote_h_
 
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-public.h"
@@ -69,7 +71,6 @@ struct GDBRemotePacket {
     std::string data;
   };
 
-  void Serialize(llvm::raw_ostream &strm) const;
   void Dump(Stream &strm) const;
 
   BinaryData packet;
@@ -82,6 +83,46 @@ struct GDBRemotePacket {
   llvm::StringRef GetTypeStr() const;
 };
 
+namespace repro {
+class PacketRecorder : public AbstractRecorder {
+public:
+  PacketRecorder(const FileSpec &filename, std::error_code &ec)
+      : AbstractRecorder(filename, ec) {}
+
+  static llvm::Expected<std::unique_ptr<PacketRecorder>>
+  Create(const FileSpec &filename);
+
+  void Record(const GDBRemotePacket &packet);
+};
+
+class GDBRemoteProvider : public repro::Provider<GDBRemoteProvider> {
+public:
+  struct Info {
+    static const char *name;
+    static const char *file;
+  };
+
+  GDBRemoteProvider(const FileSpec &directory) : Provider(directory) {}
+
+  llvm::raw_ostream *GetHistoryStream();
+  PacketRecorder *GetNewPacketRecorder();
+
+  void SetCallback(std::function<void()> callback) {
+    m_callback = std::move(callback);
+  }
+
+  void Keep() override;
+  void Discard() override;
+
+  static char ID;
+
+private:
+  std::function<void()> m_callback;
+  std::unique_ptr<llvm::raw_fd_ostream> m_stream_up;
+  std::vector<std::unique_ptr<PacketRecorder>> m_packet_recorders;
+};
+
+} // namespace repro
 } // namespace lldb_private
 
 LLVM_YAML_IS_DOCUMENT_LIST_VECTOR(lldb_private::GDBRemotePacket)

lldb/include/lldb/Utility/Reproducer.h

@@ -150,24 +150,13 @@ class WorkingDirectoryProvider : public Provider<WorkingDirectoryProvider> {
   static char ID;
 };
 
-class DataRecorder {
-public:
-  DataRecorder(const FileSpec &filename, std::error_code &ec)
+class AbstractRecorder {
+protected:
+  AbstractRecorder(const FileSpec &filename, std::error_code &ec)
       : m_filename(filename.GetFilename().GetStringRef()),
         m_os(filename.GetPath(), ec, llvm::sys::fs::F_Text), m_record(true) {}
 
-  static llvm::Expected<std::unique_ptr<DataRecorder>>
-  Create(const FileSpec &filename);
-
-  template <typename T> void Record(const T &t, bool newline = false) {
-    if (!m_record)
-      return;
-    m_os << t;
-    if (newline)
-      m_os << '\n';
-    m_os.flush();
-  }
-
+public:
   const FileSpec &GetFilename() { return m_filename; }
 
   void Stop() {
@@ -177,10 +166,30 @@ class DataRecorder {
 
 private:
   FileSpec m_filename;
+
+protected:
   llvm::raw_fd_ostream m_os;
   bool m_record;
 };
 
+class DataRecorder : public AbstractRecorder {
+public:
+  DataRecorder(const FileSpec &filename, std::error_code &ec)
+      : AbstractRecorder(filename, ec) {}
+
+  static llvm::Expected<std::unique_ptr<DataRecorder>>
+  Create(const FileSpec &filename);
+
+  template <typename T> void Record(const T &t, bool newline = false) {
+    if (!m_record)
+      return;
+    m_os << t;
+    if (newline)
+      m_os << '\n';
+    m_os.flush();
+  }
+};
+
 class CommandProvider : public Provider<CommandProvider> {
 public:
   struct Info {
@@ -201,32 +210,6 @@ class CommandProvider : public Provider<CommandProvider> {
   std::vector<std::unique_ptr<DataRecorder>> m_data_recorders;
 };
 
-class ProcessGDBRemoteProvider
-    : public repro::Provider<ProcessGDBRemoteProvider> {
-public:
-  struct Info {
-    static const char *name;
-    static const char *file;
-  };
-
-  ProcessGDBRemoteProvider(const FileSpec &directory) : Provider(directory) {}
-
-  llvm::raw_ostream *GetHistoryStream();
-
-  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<llvm::raw_fd_ostream> m_stream_up;
-};
-
 /// 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.
@@ -356,13 +339,43 @@ class Reproducer {
   mutable std::mutex m_mutex;
 };
 
-/// Helper class for replaying commands through the reproducer.
-class CommandLoader {
+template <typename T> class MultiLoader {
 public:
-  CommandLoader(std::vector<std::string> files) : m_files(files) {}
+  MultiLoader(std::vector<std::string> files) : m_files(files) {}
+
+  static std::unique_ptr<MultiLoader> Create(Loader *loader) {
+    if (!loader)
+      return {};
+
+    FileSpec file = loader->GetFile<typename T::Info>();
+    if (!file)
+      return {};
+
+    auto error_or_file = llvm::MemoryBuffer::getFile(file.GetPath());
+    if (auto err = error_or_file.getError())
+      return {};
 
-  static std::unique_ptr<CommandLoader> Create(Loader *loader);
-  llvm::Optional<std::string> GetNextFile();
+    std::vector<std::string> files;
+    llvm::yaml::Input yin((*error_or_file)->getBuffer());
+    yin >> files;
+
+    if (auto err = yin.error())
+      return {};
+
+    for (auto &file : files) {
+      FileSpec absolute_path =
+          loader->GetRoot().CopyByAppendingPathComponent(file);
+      file = absolute_path.GetPath();
+    }
+
+    return std::make_unique<MultiLoader<T>>(std::move(files));
+  }
+
+  llvm::Optional<std::string> GetNextFile() {
+    if (m_index >= m_files.size())
+      return {};
+    return m_files[m_index++];
+  }
 
 private:
   std::vector<std::string> m_files;

lldb/source/API/SBDebugger.cpp

@@ -304,8 +304,9 @@ void SBDebugger::SetInputFileHandle(FILE *fh, bool transfer_ownership) {
   if (repro::Generator *g = repro::Reproducer::Instance().GetGenerator())
     recorder = g->GetOrCreate<repro::CommandProvider>().GetNewDataRecorder();
 
-  static std::unique_ptr<repro::CommandLoader> loader =
-      repro::CommandLoader::Create(repro::Reproducer::Instance().GetLoader());
+  static std::unique_ptr<repro::MultiLoader<repro::CommandProvider>> loader =
+      repro::MultiLoader<repro::CommandProvider>::Create(
+          repro::Reproducer::Instance().GetLoader());
   if (loader) {
     llvm::Optional<std::string> file = loader->GetNextFile();
     fh = file ? FileSystem::Instance().Fopen(file->c_str(), "r") : nullptr;

lldb/source/Commands/CommandObjectReproducer.cpp

@@ -407,20 +407,18 @@ class CommandObjectReproducerDump : public CommandObjectParsed {
       return true;
     }
     case eReproducerProviderCommands: {
-      // Create a new command loader.
-      std::unique_ptr<repro::CommandLoader> command_loader =
-          repro::CommandLoader::Create(loader);
-      if (!command_loader) {
+      std::unique_ptr<repro::MultiLoader<repro::CommandProvider>> multi_loader =
+          repro::MultiLoader<repro::CommandProvider>::Create(loader);
+      if (!multi_loader) {
         SetError(result,
                  make_error<StringError>(llvm::inconvertibleErrorCode(),
                                          "Unable to create command loader."));
         return false;
       }
 
       // Iterate over the command files and dump them.
-      while (true) {
-        llvm::Optional<std::string> command_file =
-            command_loader->GetNextFile();
+      llvm::Optional<std::string> command_file;
+      while ((command_file = multi_loader->GetNextFile())) {
         if (!command_file)
           break;
 
@@ -436,24 +434,29 @@ class CommandObjectReproducerDump : public CommandObjectParsed {
       return true;
     }
     case eReproducerProviderGDB: {
-      FileSpec gdb_file = loader->GetFile<ProcessGDBRemoteProvider::Info>();
-      auto error_or_file = MemoryBuffer::getFile(gdb_file.GetPath());
-      if (auto err = error_or_file.getError()) {
-        SetError(result, errorCodeToError(err));
-        return false;
-      }
+      std::unique_ptr<repro::MultiLoader<repro::GDBRemoteProvider>>
+          multi_loader =
+              repro::MultiLoader<repro::GDBRemoteProvider>::Create(loader);
+      llvm::Optional<std::string> gdb_file;
+      while ((gdb_file = multi_loader->GetNextFile())) {
+        auto error_or_file = MemoryBuffer::getFile(*gdb_file);
+        if (auto err = error_or_file.getError()) {
+          SetError(result, errorCodeToError(err));
+          return false;
+        }
 
-      std::vector<GDBRemotePacket> packets;
-      yaml::Input yin((*error_or_file)->getBuffer());
-      yin >> packets;
+        std::vector<GDBRemotePacket> packets;
+        yaml::Input yin((*error_or_file)->getBuffer());
+        yin >> packets;
 
-      if (auto err = yin.error()) {
-        SetError(result, errorCodeToError(err));
-        return false;
-      }
+        if (auto err = yin.error()) {
+          SetError(result, errorCodeToError(err));
+          return false;
+        }
 
-      for (GDBRemotePacket &packet : packets) {
-        packet.Dump(result.GetOutputStream());
+        for (GDBRemotePacket &packet : packets) {
+          packet.Dump(result.GetOutputStream());
+        }
       }
 
       result.SetStatus(eReturnStatusSuccessFinishResult);

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp

@@ -31,6 +31,7 @@
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegularExpression.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/Utility/StreamString.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -1242,8 +1243,9 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
 
 void GDBRemoteCommunication::DumpHistory(Stream &strm) { m_history.Dump(strm); }
 
-void GDBRemoteCommunication::SetHistoryStream(llvm::raw_ostream *strm) {
-  m_history.SetStream(strm);
+void GDBRemoteCommunication::SetPacketRecorder(
+    repro::PacketRecorder *recorder) {
+  m_history.SetRecorder(recorder);
 }
 
 llvm::Error

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h

@@ -27,6 +27,9 @@
 #include "lldb/lldb-public.h"
 
 namespace lldb_private {
+namespace repro {
+class PacketRecorder;
+}
 namespace process_gdb_remote {
 
 enum GDBStoppointType {
@@ -133,7 +136,8 @@ class GDBRemoteCommunication : public Communication {
                          // fork/exec to avoid having to connect/accept
 
   void DumpHistory(Stream &strm);
-  void SetHistoryStream(llvm::raw_ostream *strm);
+
+  void SetPacketRecorder(repro::PacketRecorder *recorder);
 
   static llvm::Error ConnectLocally(GDBRemoteCommunication &client,
                                     GDBRemoteCommunication &server);

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationHistory.cpp

@@ -40,8 +40,8 @@ void GDBRemoteCommunicationHistory::AddPacket(char packet_char,
   m_packets[idx].bytes_transmitted = bytes_transmitted;
   m_packets[idx].packet_idx = m_total_packet_count;
   m_packets[idx].tid = llvm::get_threadid();
-  if (m_stream)
-    m_packets[idx].Serialize(*m_stream);
+  if (m_recorder)
+    m_recorder->Record(m_packets[idx]);
 }
 
 void GDBRemoteCommunicationHistory::AddPacket(const std::string &src,
@@ -58,8 +58,8 @@ void GDBRemoteCommunicationHistory::AddPacket(const std::string &src,
   m_packets[idx].bytes_transmitted = bytes_transmitted;
   m_packets[idx].packet_idx = m_total_packet_count;
   m_packets[idx].tid = llvm::get_threadid();
-  if (m_stream)
-    m_packets[idx].Serialize(*m_stream);
+  if (m_recorder)
+    m_recorder->Record(m_packets[idx]);
 }
 
 void GDBRemoteCommunicationHistory::Dump(Stream &strm) const {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationHistory.h

@@ -13,11 +13,15 @@
 #include <vector>
 
 #include "lldb/Utility/GDBRemote.h"
+#include "lldb/Utility/Reproducer.h"
 #include "lldb/lldb-public.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace lldb_private {
+namespace repro {
+class PacketRecorder;
+}
 namespace process_gdb_remote {
 
 /// The history keeps a circular buffer of GDB remote packets. The history is
@@ -41,7 +45,7 @@ class GDBRemoteCommunicationHistory {
   void Dump(Log *log) const;
   bool DidDumpToLog() const { return m_dumped_to_log; }
 
-  void SetStream(llvm::raw_ostream *strm) { m_stream = strm; }
+  void SetRecorder(repro::PacketRecorder *recorder) { m_recorder = recorder; }
 
 private:
   uint32_t GetFirstSavedPacketIndex() const {
@@ -73,7 +77,7 @@ class GDBRemoteCommunicationHistory {
   uint32_t m_curr_idx;
   uint32_t m_total_packet_count;
   mutable bool m_dumped_to_log;
-  llvm::raw_ostream *m_stream = nullptr;
+  repro::PacketRecorder *m_recorder = nullptr;
 };
 
 } // namespace process_gdb_remote