From 7a54632966732def377528d8e3940752c28061d5 Mon Sep 17 00:00:00 2001 From: Nav Date: Sat, 19 Oct 2024 23:10:34 +0100 Subject: [PATCH] Implemented disabling of GDB packet acknowledgement, and disabled it by default. The new `packetAcknowledgement` debug server config param can be used to keep it enabled. --- src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp | 9 ++++- .../Gdb/CommandPackets/CommandPacket.cpp | 11 ++++++ src/DebugServer/Gdb/Connection.cpp | 38 ++++++++++++------- src/DebugServer/Gdb/Connection.hpp | 2 + src/DebugServer/Gdb/Feature.hpp | 2 + src/DebugServer/Gdb/GdbDebugServerConfig.cpp | 12 ++++++ src/DebugServer/Gdb/GdbDebugServerConfig.hpp | 13 +++++++ 7 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp b/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp index 541657da..cbacf144 100644 --- a/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp +++ b/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp @@ -155,11 +155,18 @@ namespace DebugServer::Gdb::AvrGdb } std::set>> AvrGdbRsp::getSupportedFeatures() { - return { + auto output = std::set>>{ + {Feature::HARDWARE_BREAKPOINTS, std::nullopt}, {Feature::SOFTWARE_BREAKPOINTS, std::nullopt}, {Feature::MEMORY_MAP_READ, std::nullopt}, {Feature::VCONT_ACTIONS_QUERY, std::nullopt}, }; + + if (!this->debugServerConfig.packetAcknowledgement) { + output.emplace(Feature::NO_ACK_MODE, std::nullopt); + } + + return output; } void AvrGdbRsp::handleTargetStoppedGdbResponse(Targets::TargetMemoryAddress programAddress) { diff --git a/src/DebugServer/Gdb/CommandPackets/CommandPacket.cpp b/src/DebugServer/Gdb/CommandPackets/CommandPacket.cpp index d608c8a7..7e0ecb65 100644 --- a/src/DebugServer/Gdb/CommandPackets/CommandPacket.cpp +++ b/src/DebugServer/Gdb/CommandPackets/CommandPacket.cpp @@ -61,6 +61,17 @@ namespace DebugServer::Gdb::CommandPackets return; } + if (!debugSession.serverConfig.packetAcknowledgement && packetString.find("QStartNoAckMode") == 0) { + Logger::info("Handling QStartNoAckMode"); + /* + * We respond to the command before actually disabling packet acknowledgement, because GDB will send one + * final acknowledgement byte to acknowledge the response. + */ + debugSession.connection.writePacket(OkResponsePacket{}); + debugSession.connection.packetAcknowledgement = false; + return; + } + Logger::debug("Unknown GDB RSP packet: " + packetString + " - returning empty response"); // GDB expects an empty response for all unsupported commands diff --git a/src/DebugServer/Gdb/Connection.cpp b/src/DebugServer/Gdb/Connection.cpp index 331db8e8..8dff1041 100644 --- a/src/DebugServer/Gdb/Connection.cpp +++ b/src/DebugServer/Gdb/Connection.cpp @@ -135,8 +135,10 @@ namespace DebugServer::Gdb ) ); - // Acknowledge receipt - this->write({'+'}); + if (this->packetAcknowledgement) { + // Acknowledge receipt + this->write({'+'}); + } output.emplace_back(std::move(rawPacket)); byteIndex = packetIndex; @@ -149,22 +151,32 @@ namespace DebugServer::Gdb } void Connection::writePacket(const ResponsePacket& packet) { - // Write the packet repeatedly until the GDB client acknowledges it. - int attempts = 0; const auto rawPacket = packet.toRawPacket(); Logger::debug("Writing GDB packet: " + std::string{rawPacket.begin(), rawPacket.end()}); - do { - if (attempts > 10) { - throw ClientCommunicationError{ - "Failed to write GDB response packet - client failed to acknowledge receipt - retry limit reached" - }; - } + this->write(rawPacket); - this->write(rawPacket); - attempts++; - } while (this->readSingleByte(false).value_or(0) != '+'); + if (this->packetAcknowledgement) { + auto attempts = std::size_t{0}; + auto ackByte = this->readSingleByte(false); + while (ackByte != '+') { + if (attempts > 10) { + throw ClientCommunicationError{ + "Failed to write GDB response packet - client failed to acknowledge receipt - retry limit reached" + }; + } + + if (ackByte == '-') { + // GDB has requested retransmission + Logger::debug("Sending packet again, upon GDB's request"); + this->write(rawPacket); + } + + ackByte = this->readSingleByte(false); + ++attempts; + } + } } void Connection::accept(int serverSocketFileDescriptor) { diff --git a/src/DebugServer/Gdb/Connection.hpp b/src/DebugServer/Gdb/Connection.hpp index a384a7ac..7d1959ee 100644 --- a/src/DebugServer/Gdb/Connection.hpp +++ b/src/DebugServer/Gdb/Connection.hpp @@ -31,6 +31,8 @@ namespace DebugServer::Gdb */ static constexpr auto ABSOLUTE_MAXIMUM_PACKET_READ_SIZE = 2097000; // 2MiB + bool packetAcknowledgement = true; + Connection(int serverSocketFileDescriptor, EventFdNotifier& interruptEventNotifier); Connection() = delete; diff --git a/src/DebugServer/Gdb/Feature.hpp b/src/DebugServer/Gdb/Feature.hpp index 06aba8e3..a158158d 100644 --- a/src/DebugServer/Gdb/Feature.hpp +++ b/src/DebugServer/Gdb/Feature.hpp @@ -11,6 +11,7 @@ namespace DebugServer::Gdb PACKET_SIZE, MEMORY_MAP_READ, VCONT_ACTIONS_QUERY, + NO_ACK_MODE, }; static inline BiMap getGdbFeatureToNameMapping() { @@ -20,6 +21,7 @@ namespace DebugServer::Gdb {Feature::PACKET_SIZE, "PacketSize"}, {Feature::MEMORY_MAP_READ, "qXfer:memory-map:read"}, {Feature::VCONT_ACTIONS_QUERY, "vContSupported"}, + {Feature::NO_ACK_MODE, "QStartNoAckMode"}, }; } } diff --git a/src/DebugServer/Gdb/GdbDebugServerConfig.cpp b/src/DebugServer/Gdb/GdbDebugServerConfig.cpp index 0578ced2..2c7bc446 100644 --- a/src/DebugServer/Gdb/GdbDebugServerConfig.cpp +++ b/src/DebugServer/Gdb/GdbDebugServerConfig.cpp @@ -42,5 +42,17 @@ namespace DebugServer::Gdb ); } } + + if (debugServerConfig.debugServerNode["packetAcknowledgement"]) { + if (YamlUtilities::isCastable(debugServerConfig.debugServerNode["packetAcknowledgement"])) { + this->packetAcknowledgement = debugServerConfig.debugServerNode["packetAcknowledgement"].as(); + + } else { + Logger::error( + "Invalid GDB debug server config parameter ('packetAcknowledgement') provided - value must be" + " castable to a boolean. The parameter will be ignored." + ); + } + } } } diff --git a/src/DebugServer/Gdb/GdbDebugServerConfig.hpp b/src/DebugServer/Gdb/GdbDebugServerConfig.hpp index 86b34e3d..5e4e0a54 100644 --- a/src/DebugServer/Gdb/GdbDebugServerConfig.hpp +++ b/src/DebugServer/Gdb/GdbDebugServerConfig.hpp @@ -35,6 +35,19 @@ namespace DebugServer::Gdb */ bool rangeStepping = true; + /** + * Determines whether Bloom will seek to disable packet acknowledgement with GDB, at the start of the debug + * session. + * + * If this is set to false, Bloom will communicate its ability to disable package acknowledgment to GDB. + * GDB may then send the appropriate packet to disable packet acknowledgment. However, this isn't + * guaranteed - GDB may be configured to keep packet acknowledgment enabled (via the + * `set remote noack-packet off` command). + * + * This parameter is optional. If not specified, the default value set here will be used. + */ + bool packetAcknowledgement = false; + explicit GdbDebugServerConfig(const DebugServerConfig& debugServerConfig); }; }