More bits of refactoring in the GDB RSP server implementation

src/DebugServer/Gdb/Connection.cpp

@@ -7,7 +7,9 @@
 #include <algorithm>
 
 #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<RawPacket> Connection::readRawPackets() {
         std::vector<RawPacket> output;
 
-        const auto bytes = this->read();
+        do {
+            const auto bytes = this->read();
 
-        std::size_t bufferSize = bytes.size();
+            std::size_t bufferSize = bytes.size();
-        for (std::size_t byteIndex = 0; byteIndex < bufferSize; byteIndex++) {
+            for (std::size_t byteIndex = 0; byteIndex < bufferSize; byteIndex++) {
-            auto byte = bytes[byteIndex];
+                auto byte = bytes[byteIndex];
 
-            if (byte == 0x03) {
+                if (byte == 0x03) {
-                /*
+                    /*
-                 * This is an interrupt packet - it doesn't carry any of the usual packet frame bytes, so we'll just
+                     * This is an interrupt packet - it doesn't carry any of the usual packet frame bytes, so we'll
-                 * add them here, in order to keep things consistent.
+                     * 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.
+                     * Because we're effectively faking the packet frame, we can use any value for the checksum.
-                 */
+                     */
-                output.push_back({'$', byte, '#', 'F', 'F'});
+                    output.push_back({'$', byte, '#', 'F', 'F'});
+                    continue;
+                }
 
-            } else if (byte == '$') {
+                if (byte == '$') {
-                // Beginning of packet
+                    // Beginning of packet
-                RawPacket rawPacket;
+                    RawPacket rawPacket;
-                rawPacket.push_back('$');
+                    rawPacket.push_back('$');
 
-                auto packetIndex = byteIndex;
+                    auto packetIndex = byteIndex;
-                bool validPacket = false;
+                    bool validPacket = false;
-                bool isByteEscaped = false;
+                    bool isByteEscaped = false;
 
-                for (packetIndex++; packetIndex < bufferSize; packetIndex++) {
+                    for (packetIndex++; packetIndex < bufferSize; packetIndex++) {
-                    byte = bytes[packetIndex];
+                        byte = bytes[packetIndex];
 
-                    if (byte == '}' && !isByteEscaped) {
+                        if (byte == '}' && !isByteEscaped) {
-                        isByteEscaped = true;
+                            isByteEscaped = true;
-                        continue;
+                            continue;
-                    }
-
-                    if (!isByteEscaped) {
-                        if (byte == '$') {
-                            // Unexpected end of packet
-                            validPacket = false;
-                            break;
                         }
 
-                        if (byte == '#') {
+                        if (!isByteEscaped) {
-                            // End of packet data
+                            if (byte == '$') {
-                            if ((bufferSize - 1) < (packetIndex + 2)) {
+                                // Unexpected end of packet
-                                // There should be at least two more bytes in the buffer, for the checksum.
                                 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(byte);
-                            rawPacket.push_back(bytes[++packetIndex]);
+
-                            rawPacket.push_back(bytes[++packetIndex]);
+                                // Add the checksum bytes and break the loop
-                            validPacket = true;
+                                rawPacket.push_back(bytes[++packetIndex]);
-                            break;
+                                rawPacket.push_back(bytes[++packetIndex]);
+                                validPacket = true;
+                                break;
+                            }
+
+                        } else {
+                            // Escaped bytes are XOR'd with a 0x20 mask.
+                            byte ^= 0x20;
+                            isByteEscaped = false;
                         }
 
-                    } else {
+                        rawPacket.push_back(byte);
-                        // Escaped bytes are XOR'd with a 0x20 mask.
-                        byte ^= 0x20;
-                        isByteEscaped = false;
                     }
 
-                    rawPacket.push_back(byte);
+                    if (validPacket) {
-                }
+                        // Acknowledge receipt
+                        this->write({'+'});
 
-                if (validPacket) {
+                        Logger::debug("Read GDB packet: " + std::string(rawPacket.begin(), rawPacket.end()));
-                    // Acknowledge receipt
-                    this->write({'+'});
 
-                    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);

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;
+    };
+}

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()) {
+                Logger::info("Accepted GDP RSP connection from " + connection.getIpAddress());
-                    // Likely an interrupt - return control to DebugServerComponent::run() so it can process any events
-                    return;
-                }
-
-                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) {
+            if (commandPacket) {
-                // Likely an interrupt
+                commandPacket->handle(this->activeDebugSession.value(), this->targetControllerConsole);
-                return;
             }
 
-            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<Connection> 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<Connection>(
+        return Connection(
             this->serverSocketFileDescriptor.value(),
             this->interruptEventNotifier
         );
@@ -245,12 +237,11 @@ namespace Bloom::DebugServer::Gdb
     std::unique_ptr<CommandPacket> GdbRspDebugServer::waitForCommandPacket() {
         const auto rawPackets = this->activeDebugSession->connection.readRawPackets();
 
-        if (rawPackets.empty()) {
+        if (rawPackets.size() > 1) {
-            // The wait was interrupted
+            // We only process the last packet - any others will probably be duplicates from an impatient client.
-            return nullptr;
+            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());
     }
 

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<Connection> waitForConnection();
+        Connection waitForConnection();
 
         /**
          * Waits for a command packet from the connected GDB client.

src/Exceptions/DebugServerInterrupted.hpp

@@ -1,12 +0,0 @@
-#pragma once
-
-#include "Exception.hpp"
-
-namespace Bloom::Exceptions
-{
-    class DebugServerInterrupted: public Exception
-    {
-    public:
-        explicit DebugServerInterrupted() = default;
-    };
-}