[lldb/gdb-remote] Ignore spurious ACK packets

Although I cannot find any mention of this in the specification, both
gdb and lldb agree on sending an initial + packet after establishing the
connection.

OTOH, gdbserver and lldb-server behavior is subtly different. While
lldb-server *expects* the initial ack, and drops the connection if it is
not received, gdbserver will just ignore a spurious ack at _any_ point
in the connection.

This patch changes lldb's behavior to match that of gdb. An ACK packet
is ignored at any point in the connection (except when expecting an ACK
packet, of course). This is inline with the "be strict in what you
generate, and lenient in what you accept" philosophy, and also enables
us to remove some special cases from the server code. I've extended the
same handling to NAK (-) packets, mainly because I don't see a reason to
treat them differently here.

(The background here is that we had a stub which was sending spurious
+ packets. This bug has since been fixed, but I think this change makes
sense nonetheless.)

Differential Revision: https://reviews.llvm.org/D114520

1 files changed, 13 insertions, 2 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 5c8dd03..25ae088 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -188,7 +188,7 @@ GDBRemoteCommunication::SendRawPacketNoLock(llvm::StringRef packet,
 
 GDBRemoteCommunication::PacketResult GDBRemoteCommunication::GetAck() {
   StringExtractorGDBRemote packet;
-  PacketResult result = ReadPacket(packet, GetPacketTimeout(), false);
+  PacketResult result = WaitForPacketNoLock(packet, GetPacketTimeout(), false);
   if (result == PacketResult::Success) {
     if (packet.GetResponseType() ==
         StringExtractorGDBRemote::ResponseType::eAck)
@@ -220,7 +220,18 @@ GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
                                    Timeout<std::micro> timeout,
                                    bool sync_on_timeout) {
-  return WaitForPacketNoLock(response, timeout, sync_on_timeout);
+  using ResponseType = StringExtractorGDBRemote::ResponseType;
+
+  Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PACKETS));
+  for (;;) {
+    PacketResult result =
+        WaitForPacketNoLock(response, timeout, sync_on_timeout);
+    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