Author: Pavel Labath Date: 2021-11-25T12:34:08+01:00 New Revision: 165545c7a431aa3682fd9468014771c1c5228226
URL: https://github.com/llvm/llvm-project/commit/165545c7a431aa3682fd9468014771c1c5228226 DIFF: https://github.com/llvm/llvm-project/commit/165545c7a431aa3682fd9468014771c1c5228226.diff LOG: [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 Added: Modified: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h lldb/tools/lldb-server/lldb-platform.cpp lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h Removed: ################################################################################ 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<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 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<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(); @@ -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<StringExtractorGDBRemote::ServerPacketType, PacketHandler> 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<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; 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; _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
