[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
GitOrigin-RevId: 165545c7a431aa3682fd9468014771c1c5228226
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 5c8dd03..25ae088 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/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
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
index 11cac9f..49d88b7 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
+++ b/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;
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
index 68448ea..5de3440 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
+++ b/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;
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
index 5360db3..30f14a5 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ b/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;
diff --git a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
index 6c75771..17ee413 100644
--- a/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
+++ b/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;
diff --git a/tools/lldb-server/lldb-platform.cpp b/tools/lldb-server/lldb-platform.cpp
index d4b5436..9e07f4c 100644
--- a/tools/lldb-server/lldb-platform.cpp
+++ b/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);
diff --git a/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp b/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
index 3150477..4289a83 100644
--- a/unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
+++ b/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);
diff --git a/unittests/Process/gdb-remote/GDBRemoteTestUtils.h b/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
index 27ce6b9..5ecda1a 100644
--- a/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
+++ b/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;
