[lldb] [gdb-remote] Support reading notification packets

[mgorny]

Summary

Add explicit support for reading (or skipping) notification packets
to GDBRemoteCommunication::ReadPacket(). Previously, the code would not
really distinguish between regular replies and notifications,
and would return the next packet to the caller without even a clear
indication whether it is a regular or notification packet.

Since the majority of existing call sites do not expect to read
notifications, add a new "allow_notification" parameter that defaults to
false. If it is false, then any notifications received from the server
are stashed onto a queue and the function waits for a regular reply
instead. If it is true, then the function either returns the first
notification queued previously or waits for any packet (either regular
or notification).

To make it possible to clearly distinguish between regular
and notification packets, add a new PacketResult::Notify. It is used
in lieu of PacketResult::Success when a notification packet is read.
While admittedly this is not the cleanest possible solution, it requires
minimal changes to the existing code. In the end, we expect only a few
call sites to actually expect and read notifications.

Split from the non-stop protocol support in D126614.

Sponsored by: The FreeBSD Foundation

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);
 
+  // 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::Notify;
+  }
+
   // Check for a packet from our cache first without trying any reading...
-  if (CheckForPacket(nullptr, 0, packet) != PacketType::Invalid)
-    return PacketResult::Success;
+  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);
+      while (packet_type == PacketType::Notify && !allow_notification) {
+        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;
     } 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) {

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

 
 namespace {
 
 class TestClient : public GDBRemoteCommunication {
 public:
   TestClient()
       : GDBRemoteCommunication("test.client", "test.client.listener") {}
 
-  PacketResult ReadPacket(StringExtractorGDBRemote &response) {
+  PacketResult ReadPacket(StringExtractorGDBRemote &response,
+                          bool allow_notify = false) {
     return GDBRemoteCommunication::ReadPacket(response, std::chrono::seconds(1),
-                                              /*sync_on_timeout*/ false);
+                                              /*sync_on_timeout*/ false,
+                                              allow_notify);
   }
 };
 
 class GDBRemoteCommunicationTest : public GDBRemoteTest {
 public:
   void SetUp() override {
     ASSERT_THAT_ERROR(GDBRemoteCommunication::ConnectLocally(client, server),
                       llvm::Succeeded());
for (const auto &Test : Tests) {
     SCOPED_TRACE(Test.Packet + " -> " + Test.Payload);
     StringExtractorGDBRemote response;
     ASSERT_TRUE(Write(Test.Packet));
     ASSERT_EQ(PacketResult::Success, client.ReadPacket(response));
     ASSERT_EQ(Test.Payload, response.GetStringRef());
     ASSERT_EQ(PacketResult::Success, server.GetAck());
   }
 }
+
+TEST_F(GDBRemoteCommunicationTest, ReadPacketNotify) {
+  StringExtractorGDBRemote response;
+  ASSERT_TRUE(Write("%Stdio:O48656c6c6f20776f726c640d0a#f1"));
+  ASSERT_TRUE(Write("%Stop:W29;process:114b63#97"));
+  ASSERT_EQ(PacketResult::Notify, client.ReadPacket(response, true));
+  ASSERT_EQ("Stdio:O48656c6c6f20776f726c640d0a", response.GetStringRef());
+  ASSERT_EQ(PacketResult::Notify, client.ReadPacket(response, true));
+  ASSERT_EQ("Stop:W29;process:114b63", response.GetStringRef());
+}
+
+TEST_F(GDBRemoteCommunicationTest, ReadPacketNotifyQueue) {
+  StringExtractorGDBRemote response;
+  ASSERT_TRUE(Write("%Stop:W29;process:114b63#97"));
+  ASSERT_TRUE(Write("$foobar#79"));
+  ASSERT_EQ(PacketResult::Success, client.ReadPacket(response));
+  ASSERT_EQ("foobar", response.GetStringRef());
+  ASSERT_EQ(PacketResult::Notify, client.ReadPacket(response, true));
+  ASSERT_EQ("Stop:W29;process:114b63", response.GetStringRef());
+}