Refactored GDB register handling in GDB debug server.

Accounted for size discrepancies in register descriptors (target register descriptors and GDB register descriptors).

src/DebugServers/GdbRsp/AvrGdbRsp/AvrGdbRsp.cpp

@@ -8,7 +8,13 @@ using namespace Bloom::Exceptions;
 using Bloom::Targets::TargetRegisterDescriptor;
 using Bloom::Targets::TargetRegisterType;
 
-void AvrGdbRsp::loadRegisterNumberToDescriptorMapping() {
+void AvrGdbRsp::init() {
+    this->loadRegisterMappings();
+
+    GdbRspDebugServer::init();
+}
+
+void AvrGdbRsp::loadRegisterMappings() {
     auto& registerDescriptorsByType = this->targetDescriptor.registerDescriptorsByType;
     if (!registerDescriptorsByType.contains(TargetRegisterType::STATUS_REGISTER)) {
         throw Exception("Missing status register descriptor");
@@ -23,40 +29,112 @@ void AvrGdbRsp::loadRegisterNumberToDescriptorMapping() {
     }
 
     if (!registerDescriptorsByType.contains(TargetRegisterType::GENERAL_PURPOSE_REGISTER)
-        || registerDescriptorsByType.at(TargetRegisterType::GENERAL_PURPOSE_REGISTER).size() != 32) {
+        || registerDescriptorsByType.at(TargetRegisterType::GENERAL_PURPOSE_REGISTER).size() != 32
+    ) {
         throw Exception("Unexpected general purpose register count");
     }
 
-    auto& gpRegisterDescriptors = registerDescriptorsByType.at(TargetRegisterType::GENERAL_PURPOSE_REGISTER);
+    /*
+     * Worth noting that gpRegisterDescriptors will always be sorted in the correct order, from register 0 to 31.
+     *
+     * Hmm, but the sorting is based on the start address (see TargetRegisterDescriptor::<() for more). So effectively,
+     * we're assuming that the registers will be laid out in the correct order, in memory. I think this assumption is
+     * fair.
+     */
+    const auto& gpRegisterDescriptors = registerDescriptorsByType.at(TargetRegisterType::GENERAL_PURPOSE_REGISTER);
 
-    std::size_t descriptorIndex = 0;
+    // General purpose CPU registers
-    for (auto& descriptor : gpRegisterDescriptors) {
+    GdbRegisterNumberType regNumber = 0;
-        this->registerNumberToDescriptorMapping.insert(std::pair(
+    for (const auto& descriptor : gpRegisterDescriptors) {
-            static_cast<GdbRegisterNumber>(descriptorIndex),
+        this->registerDescriptorsByGdbNumber.insert(std::pair(
+            regNumber,
+            RegisterDescriptor(
+                regNumber,
+                1,
+                "General Purpose Register " + std::to_string(regNumber)
+            )
+        ));
+
+        this->targetRegisterDescriptorsByGdbNumber.insert(std::pair(
+            regNumber,
             descriptor
         ));
 
-        descriptorIndex++;
+        regNumber++;
     }
 
-    this->registerNumberToDescriptorMapping.insert(std::pair(
+    const auto statusDescriptor = RegisterDescriptor(
-        static_cast<GdbRegisterNumber>(32),
+        32,
+        1,
+        "Status Register"
+    );
+
+    this->registerDescriptorsByGdbNumber.insert(std::pair(statusDescriptor.number, statusDescriptor));
+    this->targetRegisterDescriptorsByGdbNumber.insert(std::pair(
+        statusDescriptor.number,
         *(registerDescriptorsByType.at(TargetRegisterType::STATUS_REGISTER).begin())
     ));
 
-    this->registerNumberToDescriptorMapping.insert(std::pair(
+    const auto stackPointerDescriptor = RegisterDescriptor(
-        static_cast<GdbRegisterNumber>(33),
+        33,
+        2,
+        "Stack Pointer Register"
+    );
+
+    this->registerDescriptorsByGdbNumber.insert(std::pair(stackPointerDescriptor.number, stackPointerDescriptor));
+    this->targetRegisterDescriptorsByGdbNumber.insert(std::pair(
+        stackPointerDescriptor.number,
         *(registerDescriptorsByType.at(TargetRegisterType::STACK_POINTER).begin())
     ));
 
-    this->registerNumberToDescriptorMapping.insert(std::pair(
+    const auto programCounterDescriptor = RegisterDescriptor(
-        static_cast<GdbRegisterNumber>(34),
+        34,
+        4,
+        "Program Counter"
+    );
+
+    this->registerDescriptorsByGdbNumber.insert(std::pair(
+        programCounterDescriptor.number,
+        programCounterDescriptor
+    ));
+    this->targetRegisterDescriptorsByGdbNumber.insert(std::pair(
+        programCounterDescriptor.number,
         *(registerDescriptorsByType.at(TargetRegisterType::PROGRAM_COUNTER).begin())
     ));
+
+    if (registerDescriptorsByType.at(TargetRegisterType::STATUS_REGISTER).size() > statusDescriptor.size) {
+        throw Exception("AVR8 status target register size exceeds the GDB register size.");
+    }
+
+    if (registerDescriptorsByType.at(TargetRegisterType::STACK_POINTER).size() > stackPointerDescriptor.size) {
+        throw Exception("AVR8 stack pointer target register size exceeds the GDB register size.");
+    }
+
+    if (registerDescriptorsByType.at(TargetRegisterType::PROGRAM_COUNTER).size() > programCounterDescriptor.size) {
+        throw Exception("AVR8 program counter size exceeds the GDB register size.");
+    }
 }
 
-void AvrGdbRsp::init() {
+std::optional<GdbRegisterNumberType> AvrGdbRsp::getRegisterNumberFromTargetRegisterDescriptor(
-    this->loadRegisterNumberToDescriptorMapping();
+    const Targets::TargetRegisterDescriptor& registerDescriptor
-
+) {
-    GdbRspDebugServer::init();
+    return this->targetRegisterDescriptorsByGdbNumber.valueAt(registerDescriptor);
+}
+
+const RegisterDescriptor& AvrGdbRsp::getRegisterDescriptorFromNumber(GdbRegisterNumberType number) {
+    if (this->registerDescriptorsByGdbNumber.contains(number)) {
+        return this->registerDescriptorsByGdbNumber.at(number);
+    }
+
+    throw Exception("Unknown register from GDB - register number (" + std::to_string(number)
+        + ") not mapped to any GDB register descriptor.");
+}
+
+const TargetRegisterDescriptor& AvrGdbRsp::getTargetRegisterDescriptorFromNumber(GdbRegisterNumberType number) {
+    if (this->targetRegisterDescriptorsByGdbNumber.contains(number)) {
+        return this->targetRegisterDescriptorsByGdbNumber.at(number);
+    }
+
+    throw Exception("Unknown register from GDB - register number (" + std::to_string(number)
+        + ") not mapped to any target register descriptor.");
 }

src/DebugServers/GdbRsp/AvrGdbRsp/AvrGdbRsp.hpp

@@ -33,23 +33,9 @@ namespace Bloom::DebugServers::Gdb
         }
 
     protected:
-        /**
+        void init() override;
-         * For AVR targets, avr-gdb defines 35 registers in total:
-         *
-         * Register number 0 through 31 are general purpose registers
-         * Register number 32 is the status register (SREG)
-         * Register number 33 is the stack pointer register
-         * Register number 34 is the program counter register
-         *
-         * Only general purpose registers have register IDs. The others do not require an ID.
-         *
-         * @return
-         */
-        const BiMap<GdbRegisterNumber,Targets::TargetRegisterDescriptor>& getRegisterNumberToDescriptorMapping() override {
-            return this->registerNumberToDescriptorMapping;
-        };
 
-        void loadRegisterNumberToDescriptorMapping();
+        void loadRegisterMappings();
 
         /**
          * avr-gdb uses the most significant 15 bits in memory addresses to indicate the type of memory being
@@ -76,15 +62,47 @@ namespace Bloom::DebugServers::Gdb
             return address & this->gdbInternalMemoryMask ? (address & ~(this->gdbInternalMemoryMask)) : address;
         };
 
-        void init() override;
+        const BiMap<GdbRegisterNumberType, RegisterDescriptor>& getRegisterNumberToDescriptorMapping() override {
+            return this->registerDescriptorsByGdbNumber;
+        };
+
+        std::optional<GdbRegisterNumberType> getRegisterNumberFromTargetRegisterDescriptor(
+            const Targets::TargetRegisterDescriptor& registerDescriptor
+        ) override;
+
+        const RegisterDescriptor& getRegisterDescriptorFromNumber(GdbRegisterNumberType number) override;
+
+        const Targets::TargetRegisterDescriptor& getTargetRegisterDescriptorFromNumber(
+            GdbRegisterNumberType number
+        ) override;
 
     private:
+        /*
+         * For AVR targets, avr-gdb defines 35 registers in total:
+         *
+         * Register number 0 through 31 are general purpose registers
+         * Register number 32 is the status register (SREG)
+         * Register number 33 is the stack pointer register
+         * Register number 34 is the program counter register
+         *
+         * In this class, we maintain two bidirectional mappings:
+         *
+         *  - registerDescriptorsByGdbNumber
+         *      A mapping of GDB register numbers to GDB register descriptors.
+         *
+         *  - targetRegisterDescriptorsByGdbNumber
+         *      A mapping of GDB register numbers to target register descriptors.
+         *
+         * The functions above provide an interface for the retrieval of GDB register descriptors and target register
+         * descriptors, by their respective GDB register number.
+         */
+        BiMap<GdbRegisterNumberType, RegisterDescriptor> registerDescriptorsByGdbNumber = {};
+        BiMap<GdbRegisterNumberType, Targets::TargetRegisterDescriptor> targetRegisterDescriptorsByGdbNumber = {};
+
         /**
          * The mask used by the AVR GDB client to encode the memory type into memory addresses.
          * See AvrGdbRsp::getMemoryTypeFromGdbAddress() for more.
          */
-        unsigned int gdbInternalMemoryMask = 0xFE0000u;
+        unsigned int gdbInternalMemoryMask = 0xFE0000U;
-
-        BiMap<GdbRegisterNumber, Targets::TargetRegisterDescriptor> registerNumberToDescriptorMapping = {};
     };
 }

src/DebugServers/GdbRsp/GdbRspDebugServer.cpp

@@ -73,40 +73,20 @@ void GdbRspDebugServer::handleGdbPacket(CommandPackets::ReadRegisters& packet) {
 
     try {
         auto descriptors = TargetRegisterDescriptors();
-        auto registerNumberToDescriptorMapping = this->getRegisterNumberToDescriptorMapping();
 
         if (packet.registerNumber.has_value()) {
             Logger::debug("Reading register number: " + std::to_string(packet.registerNumber.value()));
-            descriptors.insert(this->getRegisterDescriptorFromNumber(packet.registerNumber.value()));
+            descriptors.insert(this->getTargetRegisterDescriptorFromNumber(packet.registerNumber.value()));
 
         } else {
-            // Read all descriptors
+            // Read all target registers mapped to a GDB register
-            for (auto& descriptor : registerNumberToDescriptorMapping.getMap()) {
+            for (const auto& descriptor : this->getRegisterNumberToDescriptorMapping().getMap()) {
-                descriptors.insert(descriptor.second);
+                descriptors.insert(this->getTargetRegisterDescriptorFromNumber(descriptor.second.number));
             }
         }
 
         auto registerSet = this->targetControllerConsole.readRegisters(descriptors);
 
-        /*
-         * Remove any registers that are not mapped to GDB register numbers (as we won't know where to place
-         * them in our response to GDB). All registers that are expected from the GDB client should be mapped
-         * to register numbers.
-         *
-         * Registers that are not mapped to a GDB register number are presumed to be unknown to GDB, so GDB shouldn't
-         * complain about not receiving them.
-         */
-        registerSet.erase(
-            std::remove_if(
-                registerSet.begin(),
-                registerSet.end(),
-                [&registerNumberToDescriptorMapping] (const TargetRegister& reg) {
-                    return !registerNumberToDescriptorMapping.contains(reg.descriptor);
-                }
-            ),
-            registerSet.end()
-        );
-
         /*
          * Sort each register by their respective GDB register number - this will leave us with a collection of
          * registers in the order expected by the GDB client.
@@ -114,19 +94,28 @@ void GdbRspDebugServer::handleGdbPacket(CommandPackets::ReadRegisters& packet) {
         std::sort(
             registerSet.begin(),
             registerSet.end(),
-            [this, &registerNumberToDescriptorMapping] (const TargetRegister& registerA, const TargetRegister& registerB) {
+            [this] (const TargetRegister& registerA, const TargetRegister& registerB) {
-                return registerNumberToDescriptorMapping.valueAt(registerA.descriptor) <
+                return this->getRegisterNumberFromTargetRegisterDescriptor(registerA.descriptor) <
-                    registerNumberToDescriptorMapping.valueAt(registerB.descriptor);
+                    this->getRegisterNumberFromTargetRegisterDescriptor(registerB.descriptor);
             }
         );
 
         /*
-         * Finally, reverse the register values (as they're all currently in MSB, but GDB expects them in LSB), implode
+         * Finally, reverse the register values (as they're all currently in MSB, but GDB expects them in LSB), ensure
-         * the register values, convert to hexadecimal form and send to the GDB client.
+         * that each register value size matches the size in the associated GDB register descriptor, implode the
+         * values, convert to hexadecimal form and send to the GDB client.
          */
         auto registers = std::vector<unsigned char>();
         for (auto& reg : registerSet) {
             std::reverse(reg.value.begin(), reg.value.end());
+
+            const auto gdbRegisterNumber = this->getRegisterNumberFromTargetRegisterDescriptor(reg.descriptor).value();
+            const auto& gdbRegisterDescriptor = this->getRegisterDescriptorFromNumber(gdbRegisterNumber);
+
+            if (reg.value.size() < gdbRegisterDescriptor.size) {
+                reg.value.insert(reg.value.end(), (gdbRegisterDescriptor.size - reg.value.size()), 0x00);
+            }
+
             registers.insert(registers.end(), reg.value.begin(), reg.value.end());
         }
 
@@ -145,7 +134,7 @@ void GdbRspDebugServer::handleGdbPacket(CommandPackets::WriteRegister& packet) {
     Logger::debug("Handling WriteRegister packet");
 
     try {
-        auto registerDescriptor = this->getRegisterDescriptorFromNumber(packet.registerNumber);
+        auto registerDescriptor = this->getTargetRegisterDescriptorFromNumber(packet.registerNumber);
         this->targetControllerConsole.writeRegisters({
             TargetRegister(registerDescriptor, packet.registerValue)
         });

src/DebugServers/GdbRsp/GdbRspDebugServer.hpp

@@ -218,35 +218,38 @@ namespace Bloom::DebugServers::Gdb
         virtual std::uint32_t removeMemoryTypeIndicatorFromGdbAddress(std::uint32_t address) = 0;
 
         /**
-         * Like with the method of encoding memory type information onto memory addresses, GDB clients also expect
+         * Should return the mapping of GDB register numbers to GDB register descriptors.
-         * a pre-defined set of registers. The defined set being dependant on the target. This is hardcoded in the the
-         * GDB client source. The order of the registers is also pre-defined in the GDB client.
-         *
-         * For an example, see the implementation of this method in the AvrGdbRsp class.
-         *
-         * @return
          */
-        virtual const BiMap<
+        virtual const BiMap<GdbRegisterNumberType, RegisterDescriptor>& getRegisterNumberToDescriptorMapping() = 0;
-            GdbRegisterNumber,
-            Targets::TargetRegisterDescriptor
-        >& getRegisterNumberToDescriptorMapping() = 0;
 
         /**
-         * Obtains the appropriate register descriptor from a register number.
+         * Should retrieve the GDB register number, given a target register descriptor. Or std::nullopt if the target
+         * register descriptor isn't mapped to any GDB register.
+         *
+         * @param registerDescriptor
+         * @return
+         */
+        virtual std::optional<GdbRegisterNumberType> getRegisterNumberFromTargetRegisterDescriptor(
+            const Targets::TargetRegisterDescriptor& registerDescriptor
+        ) = 0;
+
+        /**
+         * Should retrieve the GDB register descriptor for a given GDB register number.
          *
          * @param number
          * @return
          */
-        virtual Targets::TargetRegisterDescriptor getRegisterDescriptorFromNumber(GdbRegisterNumber number) {
+        virtual const RegisterDescriptor& getRegisterDescriptorFromNumber(GdbRegisterNumberType number) = 0;
-            auto mapping = this->getRegisterNumberToDescriptorMapping();
 
-            if (!mapping.contains(number)) {
+        /**
-                throw Exceptions::Exception("Unknown register from GDB - register number ("
+         * Should retrieve the mapped target register descriptor for a given GDB register number.
-                                                + std::to_string(number) + ") not mapped to any register descriptor.");
+         *
-            }
+         * @param number
-
+         * @return
-            return mapping.valueAt(number).value();
+         */
-        }
+        virtual const Targets::TargetRegisterDescriptor& getTargetRegisterDescriptorFromNumber(
+            GdbRegisterNumberType number
+        ) = 0;
 
         void onTargetControllerStateReported(const Events::TargetControllerStateReported& event);