Moved GDB packet parsing out of CommandPacketFactory and deleted that class

CMakeLists.txt

@@ -129,7 +129,6 @@ add_executable(Bloom
     src/DebugServers/GdbRsp/GdbDebugServerConfig.cpp
     src/DebugServers/GdbRsp/Connection.cpp
     src/DebugServers/GdbRsp/DebugSession.cpp
-    src/DebugServers/GdbRsp/CommandPackets/CommandPacketFactory.cpp
     src/DebugServers/GdbRsp/CommandPackets/CommandPacket.cpp
     src/DebugServers/GdbRsp/CommandPackets/SupportedFeaturesQuery.cpp
     src/DebugServers/GdbRsp/CommandPackets/ReadRegisters.cpp

src/DebugServers/GdbRsp/CommandPackets/CommandPacketFactory.cpp

@@ -1,141 +0,0 @@
-#include "CommandPacketFactory.hpp"
-
-#include <vector>
-#include <memory>
-#include <map>
-
-namespace Bloom::DebugServers::Gdb
-{
-    using CommandPackets::CommandPacket;
-
-    std::vector<std::vector<unsigned char>> CommandPacketFactory::extractRawPackets(
-        std::vector<unsigned char> buffer
-    ) {
-        std::vector<std::vector<unsigned char>> output;
-
-        std::size_t bufferIndex;
-        std::size_t bufferSize = buffer.size();
-        unsigned char byte;
-        for (bufferIndex = 0; bufferIndex < bufferSize; bufferIndex++) {
-            byte = buffer[bufferIndex];
-
-            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'});
-
-            } else if (byte == '$') {
-                // Beginning of packet
-                std::vector<unsigned char> rawPacket;
-                rawPacket.push_back('$');
-
-                auto packetIndex = bufferIndex;
-                bool validPacket = false;
-                bool isByteEscaped = false;
-
-                for (packetIndex++; packetIndex < bufferSize; packetIndex++) {
-                    byte = buffer[packetIndex];
-
-                    if (byte == '}' && !isByteEscaped) {
-                        isByteEscaped = true;
-                        continue;
-                    }
-
-                    if (byte == '$' && !isByteEscaped) {
-                        // Unexpected end of packet
-                        validPacket = false;
-                        break;
-                    }
-
-                    if (byte == '#' && !isByteEscaped) {
-                        // End of packet data
-                        if ((bufferSize - 1) < (packetIndex + 2)) {
-                            // There should be at least two more bytes in the buffer, for the checksum.
-                            break;
-                        }
-
-                        rawPacket.push_back(byte);
-
-                        // Add the checksum bytes and break the loop
-                        rawPacket.push_back(buffer[++packetIndex]);
-                        rawPacket.push_back(buffer[++packetIndex]);
-                        validPacket = true;
-                        break;
-                    }
-
-                    if (isByteEscaped) {
-                        // Escaped bytes are XOR'd with a 0x20 mask.
-                        byte ^= 0x20;
-                        isByteEscaped = false;
-                    }
-
-                    rawPacket.push_back(byte);
-                }
-
-                if (validPacket) {
-                    output.push_back(rawPacket);
-                    bufferIndex = packetIndex;
-                }
-            }
-        }
-
-        return output;
-    }
-
-    std::unique_ptr<CommandPacket> CommandPacketFactory::create(std::vector<unsigned char> rawPacket) {
-        if (rawPacket.size() == 5 && rawPacket[1] == 0x03) {
-            // This is an interrupt request - create a fake packet for it
-            return std::make_unique<CommandPackets::InterruptExecution>(rawPacket);
-        }
-
-        auto rawPacketString = std::string(rawPacket.begin(), rawPacket.end());
-
-        if (rawPacketString.size() >= 2) {
-            /*
-             * First byte of the raw packet will be 0x24 ('$'), so find() should return 1, not 0, when
-             * looking for a command identifier string.
-             */
-            if (rawPacketString.find("qSupported") == 1) {
-                return std::make_unique<CommandPackets::SupportedFeaturesQuery>(rawPacket);
-            }
-
-            if (rawPacketString[1] == 'g' || rawPacketString[1] == 'p') {
-                return std::make_unique<CommandPackets::ReadRegisters>(rawPacket);
-            }
-
-            if (rawPacketString[1] == 'P') {
-                return std::make_unique<CommandPackets::WriteRegister>(rawPacket);
-            }
-
-            if (rawPacketString[1] == 'c') {
-                return std::make_unique<CommandPackets::ContinueExecution>(rawPacket);
-            }
-
-            if (rawPacketString[1] == 's') {
-                return std::make_unique<CommandPackets::StepExecution>(rawPacket);
-            }
-
-//            if (rawPacketString[1] == 'm') {
-//                return std::make_unique<CommandPackets::ReadMemory>(rawPacket);
-//            }
-//
-//            if (rawPacketString[1] == 'M') {
-//                return std::make_unique<CommandPackets::WriteMemory>(rawPacket);
-//            }
-
-            if (rawPacketString[1] == 'Z') {
-                return std::make_unique<CommandPackets::SetBreakpoint>(rawPacket);
-            }
-
-            if (rawPacketString[1] == 'z') {
-                return std::make_unique<CommandPackets::RemoveBreakpoint>(rawPacket);
-            }
-        }
-
-        return std::make_unique<CommandPacket>(rawPacket);
-    }
-}

src/DebugServers/GdbRsp/CommandPackets/CommandPacketFactory.hpp

@@ -1,47 +0,0 @@
-#pragma once
-
-#include <vector>
-#include <memory>
-
-// Command packets
-#include "CommandPacket.hpp"
-#include "InterruptExecution.hpp"
-#include "SupportedFeaturesQuery.hpp"
-#include "ReadRegisters.hpp"
-#include "WriteRegister.hpp"
-#include "StepExecution.hpp"
-#include "ContinueExecution.hpp"
-#include "SetBreakpoint.hpp"
-#include "RemoveBreakpoint.hpp"
-
-namespace Bloom::DebugServers::Gdb
-{
-    /**
-     * The CommandPacketFactory class provides a means for extracting raw packet data from a raw buffer, and
-     * constructing the appropriate CommandPacket objects.
-     */
-    class CommandPacketFactory
-    {
-    public:
-        /**
-         * Extracts raw GDB RSP packets from buffer.
-         *
-         * @param buffer
-         *  The buffer from which to extract the raw GDB RSP packets.
-         *
-         * @return
-         *  A vector of raw packets.
-         */
-        static std::vector<std::vector<unsigned char>> extractRawPackets(std::vector<unsigned char> buffer);
-
-        /**
-         * Constructs the appropriate CommandPacket object from a single raw GDB RSP packet.
-         *
-         * @param rawPacket
-         *  The raw GDB RSP packet from which to construct the CommandPacket object.
-         *
-         * @return
-         */
-        static std::unique_ptr<CommandPackets::CommandPacket> create(std::vector<unsigned char> rawPacket);
-    };
-}

src/DebugServers/GdbRsp/CommandPackets/InterruptExecution.hpp

@@ -10,7 +10,7 @@ namespace Bloom::DebugServers::Gdb::CommandPackets
      *
      * Technically, interrupts are not sent by the client in the form of a typical GDP RSP packet. Instead, they're
      * just sent as a single byte from the client. We fake the packet on our end, to save us the headache of dealing
-     * with this inconsistency. We do this in CommandPacketFactory::extractRawPackets().
+     * with this inconsistency. We do this in Connection::readRawPackets().
      */
     class InterruptExecution: public CommandPacket
     {

src/DebugServers/GdbRsp/Connection.cpp

@@ -5,8 +5,6 @@
 #include <cerrno>
 #include <fcntl.h>
 
-#include "CommandPackets/CommandPacketFactory.hpp"
-
 #include "Exceptions/ClientDisconnected.hpp"
 #include "Exceptions/ClientCommunicationError.hpp"
 #include "src/Exceptions/Exception.hpp"
@@ -16,11 +14,11 @@
 
 namespace Bloom::DebugServers::Gdb
 {
-    using namespace CommandPackets;
-    using namespace ResponsePackets;
     using namespace Exceptions;
     using namespace Bloom::Exceptions;
 
+    using ResponsePackets::ResponsePacket;
+
     void Connection::accept(int serverSocketFileDescriptor) {
         int socketAddressLength = sizeof(this->socketAddress);
 
@@ -63,25 +61,80 @@ namespace Bloom::DebugServers::Gdb
         }
     }
 
-    std::vector<std::unique_ptr<CommandPacket>> Connection::readPackets() {
+    std::vector<RawPacketType> Connection::readRawPackets() {
-        auto buffer = this->read();
+        std::vector<RawPacketType> output;
-        Logger::debug("GDB client data received (" + std::to_string(buffer.size()) + " bytes): "
-            + std::string(buffer.begin(), buffer.end()));
 
-        auto rawPackets = CommandPacketFactory::extractRawPackets(buffer);
+        const auto bytes = this->read();
-        std::vector<std::unique_ptr<CommandPacket>> output;
 
-        for (const auto& rawPacket : rawPackets) {
+        std::size_t bufferSize = bytes.size();
-            try {
+        for (std::size_t byteIndex = 0; byteIndex < bufferSize; byteIndex++) {
-                output.push_back(CommandPacketFactory::create(rawPacket));
+            auto byte = bytes[byteIndex];
-                this->write({'+'});
 
-            } catch (const ClientDisconnected& exception) {
+            if (byte == 0x03) {
-                throw exception;
+                /*
+                 * 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'});
 
-            } catch (const Exception& exception) {
+            } else if (byte == '$') {
-                Logger::error("Failed to parse GDB packet - " + exception.getMessage());
+                // Beginning of packet
-                this->write({'-'});
+                RawPacketType rawPacket;
+                rawPacket.push_back('$');
+
+                auto packetIndex = byteIndex;
+                bool validPacket = false;
+                bool isByteEscaped = false;
+
+                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 == '#') {
+                            // End of packet data
+                            if ((bufferSize - 1) < (packetIndex + 2)) {
+                                // There should be at least two more bytes in the buffer, for the checksum.
+                                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;
+                    }
+
+                    rawPacket.push_back(byte);
+                }
+
+                if (validPacket) {
+                    // Acknowledge receipt
+                    this->write({'+'});
+
+                    output.push_back(rawPacket);
+                    byteIndex = packetIndex;
+                }
             }
         }
 

src/DebugServers/GdbRsp/Connection.hpp

@@ -10,15 +10,14 @@
 #include <arpa/inet.h>
 
 #include "src/Helpers/EventNotifier.hpp"
-#include "src/DebugServers/GdbRsp/CommandPackets/CommandPacket.hpp"
+
+#include "src/DebugServers/GdbRsp/Packet.hpp"
 #include "src/DebugServers/GdbRsp/ResponsePackets/ResponsePacket.hpp"
 
 namespace Bloom::DebugServers::Gdb
 {
     /**
      * The Connection class represents an active connection between the GDB RSP server and client.
-     *
-     * All interfacing with the GDB client should take place here.
      */
     class Connection
     {
@@ -57,11 +56,11 @@ namespace Bloom::DebugServers::Gdb
         };
 
         /**
-         * Waits for incoming data from the client and returns any received command packets.
+         * Waits for incoming data from the client and returns the raw GDB packets.
          *
          * @return
          */
-        std::vector<std::unique_ptr<CommandPackets::CommandPacket>> readPackets();
+        std::vector<RawPacketType> readRawPackets();
 
         /**
          * Sends a response packet to the client.

src/DebugServers/GdbRsp/GdbRspDebugServer.cpp

@@ -14,6 +14,18 @@
 #include "src/Exceptions/InvalidConfig.hpp"
 #include "src/Exceptions/DebugServerInterrupted.hpp"
 
+// Command packets
+#include "CommandPackets/CommandPacket.hpp"
+#include "CommandPackets/SupportedFeaturesQuery.hpp"
+#include "CommandPackets/InterruptExecution.hpp"
+#include "CommandPackets/ContinueExecution.hpp"
+#include "CommandPackets/StepExecution.hpp"
+#include "CommandPackets/ReadRegisters.hpp"
+#include "CommandPackets/WriteRegister.hpp"
+#include "CommandPackets/SetBreakpoint.hpp"
+#include "CommandPackets/RemoveBreakpoint.hpp"
+
+// Response packets
 #include "ResponsePackets/TargetStopped.hpp"
 
 namespace Bloom::DebugServers::Gdb
@@ -21,6 +33,8 @@ namespace Bloom::DebugServers::Gdb
     using namespace Exceptions;
     using namespace Bloom::Exceptions;
 
+    using CommandPackets::CommandPacket;
+
     GdbRspDebugServer::GdbRspDebugServer(
         const DebugServerConfig& debugServerConfig,
         EventListener& eventListener
@@ -146,13 +160,14 @@ namespace Bloom::DebugServers::Gdb
                 }
             }
 
-//            auto packets = this->activeDebugSession->connection.readPackets();
+            auto commandPacket = this->waitForCommandPacket();
-//
+
-//            // Only process the last packet - any others will likely be duplicates from an impatient client
+            if (commandPacket == nullptr) {
-//            if (!packets.empty()) {
+                // Likely an interrupt
-//                // Double-dispatch to appropriate handler
+                return;
-//                packets.back()->dispatchToHandler(*this);
+            }
-//            }
+
+            commandPacket->handle(this->activeDebugSession.value(), this->targetControllerConsole);
 
         } catch (const ClientDisconnected&) {
             Logger::info("GDB RSP client disconnected");
@@ -214,6 +229,63 @@ namespace Bloom::DebugServers::Gdb
         return std::nullopt;
     }
 
+    std::unique_ptr<CommandPacket> GdbRspDebugServer::waitForCommandPacket() {
+        const auto rawPackets = this->activeDebugSession->connection.readRawPackets();
+
+        if (rawPackets.empty()) {
+            // The wait was interrupted
+            return nullptr;
+        }
+
+        // We only process the last packet - any others will probably be duplicates from an impatient client.
+        return this->resolveCommandPacket(rawPackets.back());
+    }
+
+    std::unique_ptr<CommandPacket> GdbRspDebugServer::resolveCommandPacket(const RawPacketType& rawPacket) {
+        if (rawPacket.size() == 5 && rawPacket[1] == 0x03) {
+            // This is an interrupt request - create a fake packet for it
+            return std::make_unique<CommandPackets::InterruptExecution>(rawPacket);
+        }
+
+        const auto rawPacketString = std::string(rawPacket.begin(), rawPacket.end());
+
+        if (rawPacketString.size() >= 2) {
+            /*
+             * First byte of the raw packet will be 0x24 ('$'), so find() should return 1, not 0, when
+             * looking for a command identifier string.
+             */
+            if (rawPacketString.find("qSupported") == 1) {
+                return std::make_unique<CommandPackets::SupportedFeaturesQuery>(rawPacket);
+            }
+
+            if (rawPacketString[1] == 'g' || rawPacketString[1] == 'p') {
+                return std::make_unique<CommandPackets::ReadRegisters>(rawPacket);
+            }
+
+            if (rawPacketString[1] == 'P') {
+                return std::make_unique<CommandPackets::WriteRegister>(rawPacket);
+            }
+
+            if (rawPacketString[1] == 'c') {
+                return std::make_unique<CommandPackets::ContinueExecution>(rawPacket);
+            }
+
+            if (rawPacketString[1] == 's') {
+                return std::make_unique<CommandPackets::StepExecution>(rawPacket);
+            }
+
+            if (rawPacketString[1] == 'Z') {
+                return std::make_unique<CommandPackets::SetBreakpoint>(rawPacket);
+            }
+
+            if (rawPacketString[1] == 'z') {
+                return std::make_unique<CommandPackets::RemoveBreakpoint>(rawPacket);
+            }
+        }
+
+        return std::make_unique<CommandPacket>(rawPacket);
+    }
+
     void GdbRspDebugServer::terminateActiveDebugSession() {
         if (this->activeDebugSession.has_value()) {
             this->activeDebugSession->terminate();

src/DebugServers/GdbRsp/GdbRspDebugServer.hpp

@@ -19,7 +19,7 @@
 #include "Signal.hpp"
 #include "RegisterDescriptor.hpp"
 #include "Feature.hpp"
-#include "CommandPackets/CommandPacketFactory.hpp"
+#include "CommandPackets/CommandPacket.hpp"
 
 #include "src/EventManager/Events/TargetControllerStateReported.hpp"
 #include "src/EventManager/Events/TargetExecutionStopped.hpp"
@@ -103,6 +103,10 @@ namespace Bloom::DebugServers::Gdb
          */
         int enableReuseAddressSocketOption = 1;
 
+        /**
+         * When a connection with a GDB client is established, a new instance of the DebugSession class is created and
+         * held here. A value of std::nullopt means there is no active debug session present.
+         */
         std::optional<DebugSession> activeDebugSession;
 
         /**
@@ -112,10 +116,51 @@ namespace Bloom::DebugServers::Gdb
          */
         std::optional<Connection> waitForConnection();
 
+        /**
+         * Waits for a command packet from the connected GDB client.
+         *
+         * @return
+         */
+        std::unique_ptr<CommandPackets::CommandPacket> waitForCommandPacket();
+
+        /**
+         * Should construct a derived instance of the CommandPackets::CommandPacket class, from a raw packet.
+         *
+         * Derived implementations of this GDB server class can override this function to include support for more
+         * specific GDB commands. For example, the AVR GDB implementation overrides this function to support AVR
+         * specific implementations of the read and write memory GDB commands.
+         *
+         * This function should never return a nullptr. If the command is unknown, it should simply return an
+         * instance of the CommandPackets::CommandPacket base class. The handler (CommandPacket::handle()) for the base
+         * class will handle unknown packets by responding with an empty response packet (as is expected by the GDB
+         * client).
+         *
+         * @param rawPacket
+         * @return
+         */
+        virtual std::unique_ptr<CommandPackets::CommandPacket> resolveCommandPacket(const RawPacketType& rawPacket);
+
+        /**
+         * Terminates any active debug session (if any) by closing the connection to the GDB client.
+         */
         void terminateActiveDebugSession();
 
+        /**
+         * Should return the GDB target descriptor for the connected target.
+         *
+         * NOTE: This function returns a target descriptor specific to GDB and the target. It's not the same data
+         * struct as Targets::TargetDescriptor. But the GDB target descriptor does hold an instance to
+         * Targets::TargetDescriptor. See the Gdb::TargetDescriptor::targetDescriptor class member.
+         *
+         * @return
+         */
         virtual const TargetDescriptor& getGdbTargetDescriptor() = 0;
 
+        /**
+         * Responds to any unexpected TargetController state changes.
+         *
+         * @param event
+         */
         void onTargetControllerStateReported(const Events::TargetControllerStateReported& event);
 
         /**

src/DebugServers/GdbRsp/Packet.hpp

@@ -9,6 +9,8 @@
 
 namespace Bloom::DebugServers::Gdb
 {
+    using RawPacketType = std::vector<unsigned char>;
+
     /**
      * The Packet class implements the data structure for GDB RSP packets.
      *
@@ -17,7 +19,7 @@ namespace Bloom::DebugServers::Gdb
     class Packet
     {
     public:
-        explicit Packet(const std::vector<unsigned char>& rawPacket) {
+        explicit Packet(const RawPacketType& rawPacket) {
             this->init(rawPacket);
         }
 
@@ -119,7 +121,7 @@ namespace Bloom::DebugServers::Gdb
     protected:
         std::vector<unsigned char> data;
 
-        void init(const std::vector<unsigned char>& rawPacket) {
+        void init(const RawPacketType& rawPacket) {
             this->data.insert(
                 this->data.begin(),
                 rawPacket.begin() + 1,