[lldb/gdb-remote] Ignore spurious ACK packets

[labath]

Summary

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.)

[mgorny]

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

56

I don't get how this is relevant to the patch in question.

[labath]

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

56

It's not directly related, but in the process of adding comments to the test cases I realized that this test case doesn't make sense (there's no need to escape a ]), so I changed into what the author probably meant to test (escaping a }).

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

if (bytes_written == packet_length) {
                 packet_data);
     }
   }
   return PacketResult::ErrorSendFailed;
 }
 
 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)
       return PacketResult::Success;
     else
       return PacketResult::ErrorSendAck;
   }
   return result;
GDBRemoteCommunication::ReadPacketWithOutputSupport(
   }
   return result;
 }
 
 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
 GDBRemoteCommunication::WaitForPacketNoLock(StringExtractorGDBRemote &packet,
                                             Timeout<std::micro> timeout,
                                             bool sync_on_timeout) {
   uint8_t buffer[8192];
   Status error;

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

public:
   void
   RegisterPacketHandler(StringExtractorGDBRemote::ServerPacketType packet_type,
                         PacketHandler handler);
 
   PacketResult GetPacketAndSendResponse(Timeout<std::micro> timeout,
                                         Status &error, bool &interrupt,
                                         bool &quit);
 
-  // After connecting, do a little handshake with the client to make sure
-  // we are at least communicating
-  bool HandshakeWithClient();
-
 protected:
   std::map<StringExtractorGDBRemote::ServerPacketType, PacketHandler>
       m_packet_handlers;
   bool m_exit_now; // use in asynchronous handling to indicate process should
                    // exit.
 
   bool m_send_error_strings = false; // If the client enables this then
                                      // we will send error strings as well.

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

void GDBRemoteCommunicationServer::RegisterPacketHandler(
   m_packet_handlers[packet_type] = std::move(handler);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::GetPacketAndSendResponse(
     Timeout<std::micro> timeout, Status &error, bool &interrupt, bool &quit) {
   StringExtractorGDBRemote packet;
 
-  PacketResult packet_result = WaitForPacketNoLock(packet, timeout, false);
+  PacketResult packet_result = ReadPacket(packet, timeout, false);
   if (packet_result == PacketResult::Success) {
     const StringExtractorGDBRemote::ServerPacketType packet_type =
         packet.GetServerPacketType();
     switch (packet_type) {
     case StringExtractorGDBRemote::eServerPacketType_nack:
     case StringExtractorGDBRemote::eServerPacketType_ack:
       break;
 
GDBRemoteCommunicationServer::SendIllFormedResponse(
   return SendErrorResponse(0x03);
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::SendOKResponse() {
   return SendPacketNoLock("OK");
 }
 
-bool GDBRemoteCommunicationServer::HandshakeWithClient() {
-  return GetAck() == PacketResult::Success;
-}
-
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServer::SendJSONResponse(const json::Value &value) {
   std::string json_string;
   raw_string_ostream os(json_string);
   os << value;
   os.flush();
   StreamGDBRemote escaped_response;
   escaped_response.PutEscapedBytes(json_string.c_str(), json_string.size());

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

protected:
   Communication m_stdio_communication;
   MainLoop::ReadHandleUP m_stdio_handle_up;
 
   lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
   llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map;
   std::mutex m_saved_registers_mutex;
   std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map;
   uint32_t m_next_saved_registers_id = 1;
-  bool m_handshake_completed = false;
   bool m_thread_suffix_supported = false;
   bool m_list_threads_in_stop_reply = false;
 
   NativeProcessProtocol::Extension m_extensions_supported = {};
 
   PacketResult SendONotification(const char *buffer, uint32_t len);
 
   PacketResult SendWResponse(NativeProcessProtocol *process);

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

void GDBRemoteCommunicationServerLLGS::NewSubprocess(
   assert(child_pid != LLDB_INVALID_PROCESS_ID);
   assert(m_debugged_processes.find(child_pid) == m_debugged_processes.end());
   m_debugged_processes[child_pid] = std::move(child_process);
 }
 
 void GDBRemoteCommunicationServerLLGS::DataAvailableCallback() {
   Log *log(GetLogIfAnyCategoriesSet(GDBR_LOG_COMM));
 
-  if (!m_handshake_completed) {
-    if (!HandshakeWithClient()) {
-      LLDB_LOGF(log,
-                "GDBRemoteCommunicationServerLLGS::%s handshake with "
-                "client failed, exiting",
-                __FUNCTION__);
-      m_mainloop.RequestTermination();
-      return;
-    }
-    m_handshake_completed = true;
-  }
-
   bool interrupt = false;
   bool done = false;
   Status error;
   while (true) {
     const PacketResult result = GetPacketAndSendResponse(
         std::chrono::microseconds(0), error, interrupt, done);
     if (result == PacketResult::ErrorReplyTimeout)
       break; // No more packets in the queue

lldb/tools/lldb-server/lldb-platform.cpp

if (platform.IsConnected()) {
                                                 "", // hostname
                                                 pid, port, socket_name);
         if (error.Success())
           platform.SetPendingGdbServer(pid, *port, socket_name);
         else
           fprintf(stderr, "failed to start gdbserver: %s\n", error.AsCString());
       }
 
-      // After we connected, we need to get an initial ack from...
-      if (platform.HandshakeWithClient()) {
-        bool interrupt = false;
-        bool done = false;
-        while (!interrupt && !done) {
-          if (platform.GetPacketAndSendResponse(llvm::None, error, interrupt,
-                                                done) !=
-              GDBRemoteCommunication::PacketResult::Success)
-            break;
-        }
 
-        if (error.Fail()) {
-          WithColor::error() << error.AsCString() << '\n';
-        }
-      } else {
-        WithColor::error() << "handshake with client failed\n";
-      }
-    }
+      if (error.Fail())
   } while (g_server);
 
   fprintf(stderr, "lldb-server exiting...\n");
 
   return 0;
 }

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

protected:
   bool Write(llvm::StringRef packet) {
     ConnectionStatus status;
     return server.Write(packet.data(), packet.size(), status, nullptr) ==
            packet.size();
   }
 };
 } // end anonymous namespace
 
-TEST_F(GDBRemoteCommunicationTest, ReadPacket_checksum) {
+// Test that we can decode packets correctly. In particular, verify that
+// checksum calculation works.
+TEST_F(GDBRemoteCommunicationTest, ReadPacket) {
   struct TestCase {
     llvm::StringLiteral Packet;
     llvm::StringLiteral Payload;
   };
   static constexpr TestCase Tests[] = {
       {{"$#00"}, {""}},
       {{"$foobar#79"}, {"foobar"}},
-      {{"$}}#fa"}, {"]"}},
-      {{"$x*%#c7"}, {"xxxxxxxxx"}},
+      {{"$}]#da"}, {"}"}},          // Escaped }
+      {{"$x*%#c7"}, {"xxxxxxxxx"}}, // RLE
+      {{"+$#00"}, {""}},            // Spurious ACK
+      {{"-$#00"}, {""}},            // Spurious NAK
   };
   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());
   }
 }

lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h

public:
   }
 
   PacketResult SendPacket(llvm::StringRef payload) {
     return GDBRemoteCommunicationServer::SendPacketNoLock(payload);
   }
 
   PacketResult GetPacket(StringExtractorGDBRemote &response) {
     const bool sync_on_timeout = false;
-    return WaitForPacketNoLock(response, std::chrono::seconds(1),
-                               sync_on_timeout);
+    return ReadPacket(response, std::chrono::seconds(1), sync_on_timeout);
   }
 
   using GDBRemoteCommunicationServer::SendErrorResponse;
   using GDBRemoteCommunicationServer::SendOKResponse;
   using GDBRemoteCommunicationServer::SendUnimplementedResponse;
 };
 
 class MockServerWithMockConnection : public MockServer {