diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 5c8dd03daf1c6..25ae08838bf86 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 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 diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp index 11cac9fa3a4d1..49d88b72b01bf 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp @@ -46,7 +46,7 @@ GDBRemoteCommunicationServer::GetPacketAndSendResponse( Timeout 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(); @@ -150,10 +150,6 @@ GDBRemoteCommunicationServer::SendOKResponse() { return SendPacketNoLock("OK"); } -bool GDBRemoteCommunicationServer::HandshakeWithClient() { - return GetAck() == PacketResult::Success; -} - GDBRemoteCommunication::PacketResult GDBRemoteCommunicationServer::SendJSONResponse(const json::Value &value) { std::string json_string; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h index 68448eae2b9f8..5de344061ec91 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h @@ -44,10 +44,6 @@ class GDBRemoteCommunicationServer : public GDBRemoteCommunication { 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 m_packet_handlers; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp index 5360db3d84623..30f14a52dfb52 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp @@ -1088,18 +1088,6 @@ void GDBRemoteCommunicationServerLLGS::NewSubprocess( 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; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h index 6c75771f64271..17ee4130dc346 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h @@ -104,7 +104,6 @@ class GDBRemoteCommunicationServerLLGS std::mutex m_saved_registers_mutex; std::unordered_map 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; diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp index d4b54362bb468..9e07f4c8debdc 100644 --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -364,23 +364,17 @@ int main_platform(int argc, char *argv[]) { 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"; + 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'; } } while (g_server); diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp index 3150477c4e9c2..4289a8393808f 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp @@ -45,7 +45,9 @@ class GDBRemoteCommunicationTest : public GDBRemoteTest { }; } // 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; @@ -53,8 +55,10 @@ TEST_F(GDBRemoteCommunicationTest, ReadPacket_checksum) { 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); diff --git a/lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h b/lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h index 27ce6b9b26f20..5ecda1a01e503 100644 --- a/lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h +++ b/lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h @@ -65,8 +65,7 @@ class MockServer : public GDBRemoteCommunicationServer { 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;