[lldb] [gdb-remote] Initial support for multiple ContinueDelegates

[mgorny]

Summary

Introduce the initial support for handling multiple ContinueDelegates
in GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(). This
is the first step towards moving to a shared asynchronous thread serving
multiple ProcessGDBRemote instances.

The final goal is that every stop response will be passed through all
ContinueDelegates, and every delegate will decide whether it is
applicable to its process. The additional handled parameter is used
to indicate that the correct delegate has been found and no further
delegates need to be invoked.

The next step is going to involve decoupling the async thread from
ProcessGDBRemote itself, and moving all the logic responsible for
updating the process status into ContinueDelegate API.

Sponsored by: The FreeBSD Foundation

[labath]

This sort of makes sense to me, but it's not clear to me how is this selection logic going to apply to packets other than stop-replies. All of the forked processes are going to share the same pty, so it's going to be literally impossible to distinguish the O packets, for instance. I'm wondering if it would make sense to split this interface into two, and have just one "preferred" recipient of O packets (and possibly others as well -- as we have no plans for supporting them right now), and have a separate list of recipients who would do things with the stop replies?

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

134–140

It would be nice to have a utility function for this loop (opportunity to play with templates and/or member function pointers).

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

23

Why not just make this a regular return value?

[mgorny]

In D131837#3732971, @labath wrote:

This sort of makes sense to me, but it's not clear to me how is this selection logic going to apply to packets other than stop-replies. All of the forked processes are going to share the same pty, so it's going to be literally impossible to distinguish the O packets, for instance. I'm wondering if it would make sense to split this interface into two, and have just one "preferred" recipient of O packets (and possibly others as well -- as we have no plans for supporting them right now), and have a separate list of recipients who would do things with the stop replies?

Specifically about the O packets, I've been thinking that it doesn't really make any difference which process receives them — after all, they will be printed all the same, won't they?

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

23

I've figured out an explicit variable makes its purpose clearer, rather than arbitrary return true/false.

[labath]

In D131837#3734204, @mgorny wrote:

In D131837#3732971, @labath wrote:

This sort of makes sense to me, but it's not clear to me how is this selection logic going to apply to packets other than stop-replies. All of the forked processes are going to share the same pty, so it's going to be literally impossible to distinguish the O packets, for instance. I'm wondering if it would make sense to split this interface into two, and have just one "preferred" recipient of O packets (and possibly others as well -- as we have no plans for supporting them right now), and have a separate list of recipients who would do things with the stop replies?

Specifically about the O packets, I've been thinking that it doesn't really make any difference which process receives them — after all, they will be printed all the same, won't they?

Not exactly. That output gets routed through process-specific (SB)Process:GetSTDOUT functions. Of course, if you're using the command line, then all of these outputs will funnel into the lldb terminal anyway, but a scripting/gui user might be doing something different. I guess one of the goals of a future larger SB API redesign could be refactor this such that it is clear that forked processes will share the same terminal/stdout..

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

23

Maybe sometimes, but I don't think that's the case here. I mean, if you think this is particularly ambiguous, you could make a {handled, not_handled} enum and return that.

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

 
 namespace lldb_private {
 namespace process_gdb_remote {
 
 class GDBRemoteClientBase : public GDBRemoteCommunication {
 public:
   struct ContinueDelegate {
     virtual ~ContinueDelegate();
-    virtual void HandleAsyncStdout(llvm::StringRef out) = 0;
-    virtual void HandleAsyncMisc(llvm::StringRef data) = 0;
-    virtual void HandleStopReply() = 0;
+    virtual void HandleAsyncStdout(llvm::StringRef out, bool &handled) = 0;
+    virtual void HandleAsyncMisc(llvm::StringRef data, bool &handled) = 0;
+    virtual void HandleStopReply(bool &handled) = 0;
 
     /// Process asynchronously-received structured data.
     ///
     /// \param[in] data
     ///   The complete data packet, expected to start with JSON-async.
-    virtual void HandleAsyncStructuredDataPacket(llvm::StringRef data) = 0;
+    virtual void HandleAsyncStructuredDataPacket(llvm::StringRef data,
+                                                 bool &handled) = 0;
   };
 
   GDBRemoteClientBase(const char *comm_name, const char *listener_name);
 
   bool SendAsyncSignal(int signo, std::chrono::seconds interrupt_timeout);
 
   bool Interrupt(std::chrono::seconds interrupt_timeout);
 
   lldb::StateType SendContinuePacketAndWaitForResponse(
-      ContinueDelegate &delegate, const UnixSignals &signals,
+      llvm::ArrayRef<ContinueDelegate *> delegates, const UnixSignals &signals,
       llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
       StringExtractorGDBRemote &response);
 
   // If interrupt_timeout == 0 seconds, don't interrupt the target.
   // Only send the packet if the target is stopped.
   // If you want to use this mode, use the fact that the timeout is defaulted
   // so it's clear from the call-site that you are using no-interrupt.
   // If it is non-zero, interrupt the target if it is running, and

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

 GDBRemoteClientBase::ContinueDelegate::~ContinueDelegate() = default;
 
 GDBRemoteClientBase::GDBRemoteClientBase(const char *comm_name,
                                          const char *listener_name)
     : GDBRemoteCommunication(comm_name, listener_name), m_async_count(0),
       m_is_running(false), m_should_stop(false) {}
 
 StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
-    ContinueDelegate &delegate, const UnixSignals &signals,
+    llvm::ArrayRef<ContinueDelegate *> delegates, const UnixSignals &signals,
     llvm::StringRef payload, std::chrono::seconds interrupt_timeout,
     StringExtractorGDBRemote &response) {
   Log *log = GetLog(GDBRLog::Process);
   response.Clear();
 
   {
     std::lock_guard<std::mutex> lock(m_mutex);
     m_continue_packet = std::string(payload);
case 'E':
       return eStateInvalid;
     default:
       LLDB_LOGF(log, "GDBRemoteClientBase::%s () unrecognized async packet",
                 __FUNCTION__);
       return eStateInvalid;
     case 'O': {
       std::string inferior_stdout;
       response.GetHexByteString(inferior_stdout);
-      delegate.HandleAsyncStdout(inferior_stdout);
+      for (auto delegate : delegates) {
+        bool handled = false;
+        delegate->HandleAsyncStdout(inferior_stdout, handled);
+        if (handled)
+          break;
+      }
       break;
     }
     case 'A':
-      delegate.HandleAsyncMisc(
-          llvm::StringRef(response.GetStringRef()).substr(1));
+      for (auto delegate : delegates) {
+        bool handled = false;
+        delegate->HandleAsyncMisc(
+            llvm::StringRef(response.GetStringRef()).substr(1), handled);
+        if (handled)
+          break;
+      }
       break;
     case 'J':
-      delegate.HandleAsyncStructuredDataPacket(response.GetStringRef());
+      for (auto delegate : delegates) {
+        bool handled = false;
+        delegate->HandleAsyncStructuredDataPacket(response.GetStringRef(),
+                                                  handled);
+        if (handled)
+          break;
+      }
       break;
     case 'T':
     case 'S':
       // Do this with the continue lock held.
       const bool should_stop = ShouldStop(signals, response);
       response.SetFilePos(0);
 
       // The packet we should resume with. In the future we should check our
       // thread list and "do the right thing" for new threads that show up
       // while we stop and run async packets. Setting the packet to 'c' to
       // continue all threads is the right thing to do 99.99% of the time
       // because if a thread was single stepping, and we sent an interrupt, we
       // will notice above that we didn't stop due to an interrupt but stopped
       // due to stepping and we would _not_ continue. This packet may get
       // modified by the async actions (e.g. to send a signal).
       m_continue_packet = 'c';
       cont_lock.unlock();
 
-      delegate.HandleStopReply();
+      for (auto delegate : delegates) {
+        bool handled = false;
+        delegate->HandleStopReply(handled);
+        if (handled)
+          break;
+      }
       if (should_stop)
         return eStateStopped;
 
       switch (cont_lock.lock()) {
       case ContinueLock::LockResult::Success:
         break;
       case ContinueLock::LockResult::Failed:
         return eStateInvalid;

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

private:
   uint64_t m_last_signals_version = 0;
 
   static bool NewThreadNotifyBreakpointHit(void *baton,
                                            StoppointCallbackContext *context,
                                            lldb::user_id_t break_id,
                                            lldb::user_id_t break_loc_id);
 
   // ContinueDelegate interface
-  void HandleAsyncStdout(llvm::StringRef out) override;
-  void HandleAsyncMisc(llvm::StringRef data) override;
-  void HandleStopReply() override;
-  void HandleAsyncStructuredDataPacket(llvm::StringRef data) override;
+  void HandleAsyncStdout(llvm::StringRef out, bool &handled) override;
+  void HandleAsyncMisc(llvm::StringRef data, bool &handled) override;
+  void HandleStopReply(bool &handled) override;
+  void HandleAsyncStructuredDataPacket(llvm::StringRef data,
+                                       bool &handled) override;
 
   void SetThreadPc(const lldb::ThreadSP &thread_sp, uint64_t index);
   using ModuleCacheKey = std::pair<std::string, std::string>;
   // KeyInfo for the cached module spec DenseMap.
   // The invariant is that all real keys will have the file and architecture
   // set.
   // The empty key has an empty file and an empty arch.
   // The tombstone key has an invalid arch and an empty file.

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

if (m_async_listener_sp->GetEvent(event_sp, llvm::None)) {
                       __FUNCTION__, GetID(), continue_cstr);
 
             if (::strstr(continue_cstr, "vAttach") == nullptr)
               SetPrivateState(eStateRunning);
             StringExtractorGDBRemote response;
 
             StateType stop_state =
                 GetGDBRemote().SendContinuePacketAndWaitForResponse(
-                    *this, *GetUnixSignals(),
+                    {this}, *GetUnixSignals(),
                     llvm::StringRef(continue_cstr, continue_cstr_len),
                     GetInterruptTimeout(), response);
 
             // We need to immediately clear the thread ID list so we are sure
             // to get a valid list of threads. The thread ID list might be
             // contained within the "response", or the stop reply packet that
             // caused the stop. So clear it now before we give the stop reply
             // packet to the process using the
void ProcessGDBRemote::ModulesDidLoad(ModuleList &module_list) {
   // do anything
   Process::ModulesDidLoad(module_list);
 
   // After loading shared libraries, we can ask our remote GDB server if it
   // needs any symbols.
   m_gdb_comm.ServeSymbolLookups(this);
 }
 
-void ProcessGDBRemote::HandleAsyncStdout(llvm::StringRef out) {
+void ProcessGDBRemote::HandleAsyncStdout(llvm::StringRef out, bool &handled) {
   AppendSTDOUT(out.data(), out.size());
+  handled = true;
 }
 
 static const char *end_delimiter = "--end--;";
 static const int end_delimiter_len = 8;
 
-void ProcessGDBRemote::HandleAsyncMisc(llvm::StringRef data) {
+void ProcessGDBRemote::HandleAsyncMisc(llvm::StringRef data, bool &handled) {
   std::string input = data.str(); // '1' to move beyond 'A'
   if (m_partial_profile_data.length() > 0) {
     m_partial_profile_data.append(input);
     input = m_partial_profile_data;
     m_partial_profile_data.clear();
   }
 
   size_t found, pos = 0, len = input.length();
   while ((found = input.find(end_delimiter, pos)) != std::string::npos) {
     StringExtractorGDBRemote profileDataExtractor(
         input.substr(pos, found).c_str());
     std::string profile_data =
         HarmonizeThreadIdsForProfileData(profileDataExtractor);
     BroadcastAsyncProfileData(profile_data);
 
     pos = found + end_delimiter_len;
   }
 
   if (pos < len) {
     // Last incomplete chunk.
     m_partial_profile_data = input.substr(pos);
   }
+
+  handled = true;
 }
 
 std::string ProcessGDBRemote::HarmonizeThreadIdsForProfileData(
     StringExtractorGDBRemote &profileDataExtractor) {
   std::map<uint64_t, uint32_t> new_thread_id_to_used_usec_map;
   std::string output;
   llvm::raw_string_ostream output_stream(output);
   llvm::StringRef name, value;
while (profileDataExtractor.GetNameColonValue(name, value)) {
     }
   }
   output_stream << end_delimiter;
   m_thread_id_to_used_usec_map = new_thread_id_to_used_usec_map;
 
   return output_stream.str();
 }
 
-void ProcessGDBRemote::HandleStopReply() {
+void ProcessGDBRemote::HandleStopReply(bool &handled) {
+  handled = true;
   if (GetStopID() != 0)
     return;
 
   if (GetID() == LLDB_INVALID_PROCESS_ID) {
     lldb::pid_t pid = m_gdb_comm.GetCurrentProcessID();
     if (pid != LLDB_INVALID_PROCESS_ID)
       SetID(pid);
   }
if (json_sp) {
                 "() received StructuredData packet:"
                 " parse failure",
                 __FUNCTION__);
     }
   }
   return json_sp;
 }
 
-void ProcessGDBRemote::HandleAsyncStructuredDataPacket(llvm::StringRef data) {
+void ProcessGDBRemote::HandleAsyncStructuredDataPacket(llvm::StringRef data,
+                                                       bool &handled) {
   auto structured_data_sp = ParseStructuredDataPacket(data);
   if (structured_data_sp)
     RouteAsyncStructuredData(structured_data_sp);
+  handled = true;
 }
 
 class CommandObjectProcessGDBRemoteSpeedTest : public CommandObjectParsed {
 public:
   CommandObjectProcessGDBRemoteSpeedTest(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "process plugin packet speed-test",
                             "Tests packet speeds of various sizes to determine "
                             "the performance characteristics of the GDB remote "

lldb/unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

 namespace {
 
 struct MockDelegate : public GDBRemoteClientBase::ContinueDelegate {
   std::string output;
   std::string misc_data;
   unsigned stop_reply_called = 0;
   std::vector<std::string> structured_data_packets;
 
-  void HandleAsyncStdout(llvm::StringRef out) override { output += out; }
-  void HandleAsyncMisc(llvm::StringRef data) override { misc_data += data; }
-  void HandleStopReply() override { ++stop_reply_called; }
+  void HandleAsyncStdout(llvm::StringRef out, bool &handled) override {
+    output += out;
+    handled = true;
+  }
+  void HandleAsyncMisc(llvm::StringRef data, bool &handled) override {
+    misc_data += data;
+    handled = true;
+  }
+  void HandleStopReply(bool &handled) override {
+    ++stop_reply_called;
+    handled = true;
+  }
 
-  void HandleAsyncStructuredDataPacket(llvm::StringRef data) override {
+  void HandleAsyncStructuredDataPacket(llvm::StringRef data,
+                                       bool &handled) override {
     structured_data_packets.push_back(std::string(data));
+    handled = true;
   }
 };
 
 struct TestClient : public GDBRemoteClientBase {
   TestClient() : GDBRemoteClientBase("test.client", "test.client.listener") {
     m_send_acks = false;
   }
 };
protected:
   // We don't have a process to get the interrupt timeout from, so make one up.
   static std::chrono::seconds g_timeout;
   TestClient client;
   MockServer server;
   MockDelegate delegate;
   ListenerSP listener_sp = Listener::MakeListener("listener");
 
   StateType SendCPacket(StringExtractorGDBRemote &response) {
-    return client.SendContinuePacketAndWaitForResponse(delegate, LinuxSignals(),
-                                                       "c", g_timeout, 
-                                                       response);
+    return client.SendContinuePacketAndWaitForResponse(
+        {&delegate}, LinuxSignals(), "c", g_timeout, response);
   }
 
   void WaitForRunEvent() {
     EventSP event_sp;
     listener_sp->GetEventForBroadcasterWithType(
         &client, TestClient::eBroadcastBitRunPacketSent, event_sp, llvm::None);
   }
 };