This revision was automatically updated to reflect the committed changes.
Closed by commit rG165545c7a431: [lldb/gdb-remote] Ignore spurious ACK packets
(authored by labath).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114520/new/
https://reviews.llvm.org/D114520
Files:
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
Index: lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
===================================================================
--- lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
+++ lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
@@ -65,8 +65,7 @@
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;
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
===================================================================
--- lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -45,7 +45,9 @@
};
} // 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 @@
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);
Index: lldb/tools/lldb-server/lldb-platform.cpp
===================================================================
--- lldb/tools/lldb-server/lldb-platform.cpp
+++ lldb/tools/lldb-server/lldb-platform.cpp
@@ -364,23 +364,17 @@
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);
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
@@ -104,7 +104,6 @@
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;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -1088,18 +1088,6 @@
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;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
@@ -44,10 +44,6 @@
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;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
@@ -46,7 +46,7 @@
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 @@
return SendPacketNoLock("OK");
}
-bool GDBRemoteCommunicationServer::HandshakeWithClient() {
- return GetAck() == PacketResult::Success;
-}
-
GDBRemoteCommunication::PacketResult
GDBRemoteCommunicationServer::SendJSONResponse(const json::Value &value) {
std::string json_string;
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -188,7 +188,7 @@
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::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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
