diff --git a/src/DebugServer/Gdb/Connection.cpp b/src/DebugServer/Gdb/Connection.cpp index c2b049b7..5ab9cf23 100644 --- a/src/DebugServer/Gdb/Connection.cpp +++ b/src/DebugServer/Gdb/Connection.cpp @@ -7,7 +7,9 @@ #include #include "Exceptions/ClientDisconnected.hpp" +#include "Exceptions/DebugServerInterrupted.hpp" #include "Exceptions/ClientCommunicationError.hpp" + #include "src/Exceptions/Exception.hpp" #include "src/Logger/Logger.hpp" @@ -54,81 +56,85 @@ namespace Bloom::DebugServer::Gdb std::vector Connection::readRawPackets() { std::vector output; - const auto bytes = this->read(); + do { + const auto bytes = this->read(); - std::size_t bufferSize = bytes.size(); - for (std::size_t byteIndex = 0; byteIndex < bufferSize; byteIndex++) { - auto byte = bytes[byteIndex]; + std::size_t bufferSize = bytes.size(); + for (std::size_t byteIndex = 0; byteIndex < bufferSize; byteIndex++) { + auto byte = bytes[byteIndex]; - if (byte == 0x03) { - /* - * This is an interrupt packet - it doesn't carry any of the usual packet frame bytes, so we'll just - * add them here, in order to keep things consistent. - * - * Because we're effectively faking the packet frame, we can use any value for the checksum. - */ - output.push_back({'$', byte, '#', 'F', 'F'}); + if (byte == 0x03) { + /* + * This is an interrupt packet - it doesn't carry any of the usual packet frame bytes, so we'll + * just add them here, in order to keep things consistent. + * + * Because we're effectively faking the packet frame, we can use any value for the checksum. + */ + output.push_back({'$', byte, '#', 'F', 'F'}); + continue; + } - } else if (byte == '$') { - // Beginning of packet - RawPacket rawPacket; - rawPacket.push_back('$'); + if (byte == '$') { + // Beginning of packet + RawPacket rawPacket; + rawPacket.push_back('$'); - auto packetIndex = byteIndex; - bool validPacket = false; - bool isByteEscaped = false; + auto packetIndex = byteIndex; + bool validPacket = false; + bool isByteEscaped = false; - for (packetIndex++; packetIndex < bufferSize; packetIndex++) { - byte = bytes[packetIndex]; + for (packetIndex++; packetIndex < bufferSize; packetIndex++) { + byte = bytes[packetIndex]; - if (byte == '}' && !isByteEscaped) { - isByteEscaped = true; - continue; - } - - if (!isByteEscaped) { - if (byte == '$') { - // Unexpected end of packet - validPacket = false; - break; + if (byte == '}' && !isByteEscaped) { + isByteEscaped = true; + continue; } - if (byte == '#') { - // End of packet data - if ((bufferSize - 1) < (packetIndex + 2)) { - // There should be at least two more bytes in the buffer, for the checksum. + if (!isByteEscaped) { + if (byte == '$') { + // Unexpected end of packet break; } - rawPacket.push_back(byte); + if (byte == '#') { + // End of packet data + if ((bufferSize - 1) < (packetIndex + 2)) { + // There should be at least two more bytes in the buffer, for the checksum. + break; + } - // Add the checksum bytes and break the loop - rawPacket.push_back(bytes[++packetIndex]); - rawPacket.push_back(bytes[++packetIndex]); - validPacket = true; - break; + rawPacket.push_back(byte); + + // Add the checksum bytes and break the loop + rawPacket.push_back(bytes[++packetIndex]); + rawPacket.push_back(bytes[++packetIndex]); + validPacket = true; + break; + } + + } else { + // Escaped bytes are XOR'd with a 0x20 mask. + byte ^= 0x20; + isByteEscaped = false; } - } else { - // Escaped bytes are XOR'd with a 0x20 mask. - byte ^= 0x20; - isByteEscaped = false; + rawPacket.push_back(byte); } - rawPacket.push_back(byte); - } + if (validPacket) { + // Acknowledge receipt + this->write({'+'}); - if (validPacket) { - // Acknowledge receipt - this->write({'+'}); + Logger::debug("Read GDB packet: " + std::string(rawPacket.begin(), rawPacket.end())); - Logger::debug("Read GDB packet: " + std::string(rawPacket.begin(), rawPacket.end())); - - output.emplace_back(std::move(rawPacket)); - byteIndex = packetIndex; + output.emplace_back(std::move(rawPacket)); + byteIndex = packetIndex; + } } } - } + + } while (output.empty()); return output; } @@ -204,7 +210,7 @@ namespace Bloom::DebugServer::Gdb if (eventFileDescriptor.value() == this->interruptEventNotifier.getFileDescriptor()) { // Interrupted this->interruptEventNotifier.clear(); - return output; + throw DebugServerInterrupted(); } const auto bytesToRead = bytes.value_or(Connection::ABSOLUTE_MAXIMUM_PACKET_READ_SIZE); diff --git a/src/DebugServer/Gdb/Exceptions/DebugServerInterrupted.hpp b/src/DebugServer/Gdb/Exceptions/DebugServerInterrupted.hpp new file mode 100644 index 00000000..296e3f38 --- /dev/null +++ b/src/DebugServer/Gdb/Exceptions/DebugServerInterrupted.hpp @@ -0,0 +1,21 @@ +#pragma once + +#include "src/Exceptions/Exception.hpp" + +namespace Bloom::DebugServer::Gdb::Exceptions +{ + /** + * The GDB debug server spends most of its time in a blocking state, waiting for a new connection or for some data + * from the connected GDB client. The server implementation allows for interruptions to blocking IO calls. + * + * When an interrupt occurs, this exception is thrown and handled appropriately. + * + * For more on how the GDB server implementation allows for interruptions, see the "Servicing events" section in + * src/DebugServer/README.md. + */ + class DebugServerInterrupted: public Bloom::Exceptions::Exception + { + public: + explicit DebugServerInterrupted() = default; + }; +} diff --git a/src/DebugServer/Gdb/GdbRspDebugServer.cpp b/src/DebugServer/Gdb/GdbRspDebugServer.cpp index a6ca7153..31b60625 100644 --- a/src/DebugServer/Gdb/GdbRspDebugServer.cpp +++ b/src/DebugServer/Gdb/GdbRspDebugServer.cpp @@ -10,10 +10,10 @@ #include "Exceptions/ClientNotSupported.hpp" #include "Exceptions/ClientCommunicationError.hpp" #include "Exceptions/DebugSessionInitialisationFailure.hpp" +#include "Exceptions/DebugServerInterrupted.hpp" #include "src/Exceptions/Exception.hpp" #include "src/Exceptions/InvalidConfig.hpp" -#include "src/Exceptions/DebugServerInterrupted.hpp" // Command packets #include "CommandPackets/CommandPacket.hpp" @@ -147,15 +147,10 @@ namespace Bloom::DebugServer::Gdb auto connection = this->waitForConnection(); - if (!connection.has_value()) { - // Likely an interrupt - return control to DebugServerComponent::run() so it can process any events - return; - } - - Logger::info("Accepted GDP RSP connection from " + connection->getIpAddress()); + Logger::info("Accepted GDP RSP connection from " + connection.getIpAddress()); this->activeDebugSession.emplace( - std::move(connection.value()), + std::move(connection), this->getSupportedFeatures(), this->getGdbTargetDescriptor() ); @@ -185,13 +180,10 @@ namespace Bloom::DebugServer::Gdb auto commandPacket = this->waitForCommandPacket(); - if (commandPacket == nullptr) { - // Likely an interrupt - return; + if (commandPacket) { + commandPacket->handle(this->activeDebugSession.value(), this->targetControllerConsole); } - commandPacket->handle(this->activeDebugSession.value(), this->targetControllerConsole); - } catch (const ClientDisconnected&) { Logger::info("GDB RSP client disconnected"); this->activeDebugSession.reset(); @@ -215,13 +207,13 @@ namespace Bloom::DebugServer::Gdb return; } catch (const DebugServerInterrupted&) { - // Server was interrupted + // Server was interrupted by an event Logger::debug("GDB RSP interrupted"); return; } } - std::optional GdbRspDebugServer::waitForConnection() { + Connection GdbRspDebugServer::waitForConnection() { if (::listen(this->serverSocketFileDescriptor.value(), 3) != 0) { throw Exception("Failed to listen on server socket"); } @@ -233,10 +225,10 @@ namespace Bloom::DebugServer::Gdb || eventFileDescriptor.value() == this->interruptEventNotifier.getFileDescriptor() ) { this->interruptEventNotifier.clear(); - return std::nullopt; + throw DebugServerInterrupted(); } - return std::make_optional( + return Connection( this->serverSocketFileDescriptor.value(), this->interruptEventNotifier ); @@ -245,12 +237,11 @@ namespace Bloom::DebugServer::Gdb std::unique_ptr GdbRspDebugServer::waitForCommandPacket() { const auto rawPackets = this->activeDebugSession->connection.readRawPackets(); - if (rawPackets.empty()) { - // The wait was interrupted - return nullptr; + if (rawPackets.size() > 1) { + // We only process the last packet - any others will probably be duplicates from an impatient client. + Logger::warning("Multiple packets received from GDB - only the most recent will be processed"); } - // We only process the last packet - any others will probably be duplicates from an impatient client. return this->resolveCommandPacket(rawPackets.back()); } diff --git a/src/DebugServer/Gdb/GdbRspDebugServer.hpp b/src/DebugServer/Gdb/GdbRspDebugServer.hpp index b6638ab4..79444a1c 100644 --- a/src/DebugServer/Gdb/GdbRspDebugServer.hpp +++ b/src/DebugServer/Gdb/GdbRspDebugServer.hpp @@ -134,10 +134,8 @@ namespace Bloom::DebugServer::Gdb /** * Waits for a GDB client to connect on the listening socket. - * - * This function may return an std::nullopt, if the waiting was interrupted by some other event. */ - std::optional waitForConnection(); + Connection waitForConnection(); /** * Waits for a command packet from the connected GDB client. diff --git a/src/Exceptions/DebugServerInterrupted.hpp b/src/Exceptions/DebugServerInterrupted.hpp deleted file mode 100644 index 7f08057f..00000000 --- a/src/Exceptions/DebugServerInterrupted.hpp +++ /dev/null @@ -1,12 +0,0 @@ -#pragma once - -#include "Exception.hpp" - -namespace Bloom::Exceptions -{ - class DebugServerInterrupted: public Exception - { - public: - explicit DebugServerInterrupted() = default; - }; -}