[lldb] [gdb-remote] Be more explicit about notification reading

[mgorny]

Summary

The current GDBRemoteCommunication::ReadPacket() support
for notifications is very limited. Notably, it can return notifications
when the caller does not expect them and it does not provide an explicit
API to distinguish them from standard packets. This is not a major
problem right now, as we force serialization when running in non-stop
mode and can predict when we expect notifications to arrive.

Firstly, add an explicit 'allow_notification' parameter to ReadPacket()
and WaitForPacketNoLock(). When it is false (the default),
notifications are pushed onto a queue rather than returned
to the caller. Effectively, the existing callers can now safely expect
the reading functions to return only standard responses,
and the non-stop logic can explicitly permit notifications.

Secondly, add an explicit PacketResult value for notifications. This
value can only be returned when allow_notification is true, so it does
not require changes for most of the callers. For the few that support
notifications, it makes it possible to distinguish standard packets
from notification packets.

Update the non-stop client code to take advantage of the new API,
and therefore be more strict when dealing with responses in non-stop
protocol mode.

Sponsored by: The FreeBSD Foundation

[mgorny]

I'm going to merge this into D126614.

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

StateType GDBRemoteClientBase::SendContinuePacketAndWaitForResponse(
   // The main ReadPacket loop wakes up at computed_timeout intervals, just to
   // check that the connection hasn't dropped.  When we wake up we also check
   // whether there is an interrupt request that has reached its endpoint.
   // If we want a shorter interrupt timeout that kWakeupInterval, we need to
   // choose the shorter interval for the wake up as well.
   std::chrono::seconds computed_timeout =
       std::min(interrupt_timeout, kWakeupInterval);
   for (;;) {
-    PacketResult read_result = ReadPacket(response, computed_timeout, false);
+    PacketResult read_result =
+        ReadPacket(response, computed_timeout, false, m_non_stop_protocol);
     // Reset the computed_timeout to the default value in case we are going
     // round again.
     computed_timeout = std::min(interrupt_timeout, kWakeupInterval);
     switch (read_result) {
     case PacketResult::ErrorReplyTimeout: {
       std::lock_guard<std::mutex> lock(m_mutex);
       if (m_async_count == 0) {
         continue;
case PacketResult::ErrorReplyTimeout: {
         auto new_wait = m_interrupt_endpoint - cur_time;
         computed_timeout = std::min(
             kWakeupInterval,
             std::chrono::duration_cast<std::chrono::seconds>(new_wait));
         continue;
       }
       break;
     }
+    case PacketResult::Notify:
     case PacketResult::Success:
       break;
     default:
       LLDB_LOGF(log, "GDBRemoteClientBase::%s () ReadPacket(...) => false",
                 __FUNCTION__);
       return eStateInvalid;
     }
     if (response.Empty())
       return eStateInvalid;
 
     LLDB_LOGF(log, "GDBRemoteClientBase::%s () got packet: %s", __FUNCTION__,
               response.GetStringRef().data());
 
     // We do not currently support full asynchronous communication.  Instead,
     // when in non-stop mode, we:
     // 1) get the "OK" response to the continue packet
     // 2) wait for the asynchronous %Stop notification
     // 3) drain the notification queue
     // 4) issue a "vCtrlC" command to ensure that all threads stop
     // 5) repeat 1-3 for the response to this packet
 
-    if (response.IsOKResponse())
+    if (read_result == PacketResult::Success && m_non_stop_protocol &&
+        response.IsOKResponse())
       continue;
-    // TODO: get a proper mechanism for async notifications
-    if (response.GetStringRef().startswith("Stop:")) {
+    if (read_result == PacketResult::Notify &&
+        response.GetStringRef().startswith("Stop:")) {
       response.Reset(response.GetStringRef().substr(5));
 
       if (!DrainNotificationQueue("vStopped"))
         return eStateInvalid;
 
       switch (response.GetStringRef()[0]) {
       case 'W':
       case 'X':
if (read_result == PacketResult::Notify &&
         }
         if (!stop_response.IsOKResponse()) {
           LLDB_LOGF(log, "GDBRemoteClientBase::%s () vCtrlC failed",
                     __FUNCTION__);
           return eStateInvalid;
         }
 
         // vCtrlC may not do anything, so timeout if we don't get notification
-        if (ReadPacket(stop_response, milliseconds(500), false) ==
-            PacketResult::Success) {
+        if (ReadPacket(stop_response, milliseconds(500), false, true) ==
+            PacketResult::Notify) {
           if (!stop_response.GetStringRef().startswith("Stop:")) {
             LLDB_LOGF(log,
                       "GDBRemoteClientBase::%s () unexpected response "
                       "after vCtrlC",
                       __FUNCTION__);
             return eStateInvalid;
           }
 

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

 //===-- GDBRemoteCommunication.h --------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATION_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_GDB_REMOTE_GDBREMOTECOMMUNICATION_H
 
 #include "GDBRemoteCommunicationHistory.h"
 
 #include <condition_variable>
+#include <deque>
 #include <future>
 #include <mutex>
 #include <queue>
 #include <string>
 #include <vector>
 
 #include "lldb/Core/Communication.h"
 #include "lldb/Host/Config.h"

 public:
   enum {
     eBroadcastBitRunPacketSent = kLoUserBroadcastBit,
   };
 
   enum class PacketType { Invalid = 0, Standard, Notify };
 
   enum class PacketResult {
-    Success = 0,        // Success
-    ErrorSendFailed,    // Status sending the packet
-    ErrorSendAck,       // Didn't get an ack back after sending a packet
-    ErrorReplyFailed,   // Status getting the reply
-    ErrorReplyTimeout,  // Timed out waiting for reply
-    ErrorReplyInvalid,  // Got a reply but it wasn't valid for the packet that
+    Success = 0,         // Success
+    ErrorSendFailed,     // Status sending the packet
+    ErrorSendAck,        // Didn't get an ack back after sending a packet
+    ErrorReplyFailed,    // Status getting the reply
+    ErrorReplyTimeout,   // Timed out waiting for reply
+    ErrorReplyInvalid,   // Got a reply but it wasn't valid for the packet that
                          // was sent
-    ErrorReplyAck,      // Sending reply ack failed
-    ErrorDisconnected,  // We were disconnected
-    ErrorNoSequenceLock // We couldn't get the sequence lock for a multi-packet
+    ErrorReplyAck,       // Sending reply ack failed
+    ErrorDisconnected,   // We were disconnected
+    ErrorNoSequenceLock, // We couldn't get the sequence lock for a multi-packet
                          // request
+    Notify,              // Successfully gotten a notification packet
   };
 
   // Class to change the timeout for a given scope and restore it to the
   // original value when the
   // created ScopedTimeout object got out of scope
   class ScopedTimeout {
   public:
     ScopedTimeout(GDBRemoteCommunication &gdb_comm,
protected:
   std::chrono::seconds m_packet_timeout;
   uint32_t m_echo_number;
   LazyBool m_supports_qEcho;
   GDBRemoteCommunicationHistory m_history;
   bool m_send_acks;
   bool m_is_platform; // Set to true if this class represents a platform,
                       // false if this class represents a debug session for
                       // a single process
+  std::deque<std::string> m_notification_packet_queue;
 
   CompressionType m_compression_type;
 
   PacketResult SendPacketNoLock(llvm::StringRef payload);
   PacketResult SendNotificationPacketNoLock(llvm::StringRef notify_type,
                                             std::deque<std::string>& queue,
                                             llvm::StringRef payload);
   PacketResult SendRawPacketNoLock(llvm::StringRef payload,
                                    bool skip_ack = false);
 
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
-                          Timeout<std::micro> timeout, bool sync_on_timeout);
+                          Timeout<std::micro> timeout, bool sync_on_timeout,
+                          bool allow_notification = false);
 
   PacketResult ReadPacketWithOutputSupport(
       StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
       bool sync_on_timeout,
       llvm::function_ref<void(llvm::StringRef)> output_callback);
 
   PacketResult WaitForPacketNoLock(StringExtractorGDBRemote &response,
                                    Timeout<std::micro> timeout,
-                                   bool sync_on_timeout);
+                                   bool sync_on_timeout,
+                                   bool allow_notification = false);
 
   bool CompressionIsEnabled() {
     return m_compression_type != CompressionType::None;
   }
 
   // If compression is enabled, decompress the packet in m_bytes and update
   // m_bytes with the uncompressed version.
   // Returns 'true' packet was decompressed and m_bytes is the now-decompressed

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

while (result == PacketResult::Success && response.IsNormalResponse() &&
     std::string output;
     if (response.GetHexByteString(output))
       output_callback(output);
     result = ReadPacket(response, timeout, sync_on_timeout);
   }
   return result;
 }
 
-GDBRemoteCommunication::PacketResult
-GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
-                                   Timeout<std::micro> timeout,
-                                   bool sync_on_timeout) {
+GDBRemoteCommunication::PacketResult GDBRemoteCommunication::ReadPacket(
+    StringExtractorGDBRemote &response, Timeout<std::micro> timeout,
+    bool sync_on_timeout, bool allow_notification) {
   using ResponseType = StringExtractorGDBRemote::ResponseType;
 
   Log *log = GetLog(GDBRLog::Packets);
   for (;;) {
-    PacketResult result =
-        WaitForPacketNoLock(response, timeout, sync_on_timeout);
+    PacketResult result = WaitForPacketNoLock(
+        response, timeout, sync_on_timeout, allow_notification);
     if (result != PacketResult::Success ||
         (response.GetResponseType() != ResponseType::eAck &&
          response.GetResponseType() != ResponseType::eNack))
       return result;
     LLDB_LOG(log, "discarding spurious `{0}` packet", response.GetStringRef());
   }
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet,
                                             Timeout<std::micro> timeout,
-                                            bool sync_on_timeout) {
+                                            bool sync_on_timeout,
+                                            bool allow_notification) {
   uint8_t buffer[8192];
   Status error;
 
   Log *log = GetLog(GDBRLog::Packets);
 
-  // Check for a packet from our cache first without trying any reading...
-  if (CheckForPacket(nullptr, 0, packet) != PacketType::Invalid)
+  // If the caller expects notifications, check the notification queue first.
+  if (allow_notification && !m_notification_packet_queue.empty()) {
+    packet.Reset(m_notification_packet_queue.front());
+    m_notification_packet_queue.pop_front();
     return PacketResult::Success;
+  }
+
+  // Check for a packet from our cache first without trying any reading...
+  PacketType packet_type = CheckForPacket(nullptr, 0, packet);
+  if (packet_type == PacketType::Notify && !allow_notification) {
+    // If the caller does not support notifications, queue it for later.
+    m_notification_packet_queue.push_back(packet.GetStringRef().str());
+    packet_type = CheckForPacket(nullptr, 0, packet);
+  }
+  if (packet_type != PacketType::Invalid)
+    return packet_type == PacketType::Standard ? PacketResult::Success
+                                               : PacketResult::Notify;
 
   bool timed_out = false;
   bool disconnected = false;
   while (IsConnected() && !timed_out) {
     lldb::ConnectionStatus status = eConnectionStatusNoConnection;
     size_t bytes_read = Read(buffer, sizeof(buffer), timeout, status, &error);
 
     LLDB_LOGV(log,
               "Read(buffer, sizeof(buffer), timeout = {0}, "
               "status = {1}, error = {2}) => bytes_read = {3}",
               timeout, Communication::ConnectionStatusAsString(status), error,
               bytes_read);
 
     if (bytes_read > 0) {
-      if (CheckForPacket(buffer, bytes_read, packet) != PacketType::Invalid)
-        return PacketResult::Success;
+      packet_type = CheckForPacket(buffer, bytes_read, packet);
+      if (packet_type == PacketType::Notify && !allow_notification)
+        m_notification_packet_queue.push_back(packet.GetStringRef().str());
+      else if (packet_type != PacketType::Invalid)
+        return packet_type == PacketType::Standard ? PacketResult::Success
+                                                   : PacketResult::Notify;
     } else {
       switch (status) {
       case eConnectionStatusTimedOut:
       case eConnectionStatusInterrupted:
         if (sync_on_timeout) {
           /// Sync the remote GDB server and make sure we get a response that
           /// corresponds to what we send.
           ///
case PacketResult::ErrorReplyAck:
     Stream << "ErrorReplyAck";
     break;
   case PacketResult::ErrorDisconnected:
     Stream << "ErrorDisconnected";
     break;
   case PacketResult::ErrorNoSequenceLock:
     Stream << "ErrorNoSequenceLock";
     break;
+  case PacketResult::Notify:
+    Stream << "Notify";
+    break;
   }
 }
 
 std::string GDBRemoteCommunication::ExpandRLE(std::string packet) {
   // Reserve enough byte for the most common case (no RLE used).
   std::string decoded;
   decoded.reserve(packet.size());
   for (std::string::const_iterator c = packet.begin(); c != packet.end(); ++c) {