From 18fb3b56ceb87c6884babe59f072132af92a8e95 Mon Sep 17 00:00:00 2001 From: Nav Date: Sat, 17 Sep 2022 20:12:26 +0100 Subject: [PATCH] Grouped the buffers from GDB's flash write packets so that we only flush once we have the full buffer. This fixes an issue with GDB programming, where it was sending misaligned buffers and program memory wasn't being properly updated. --- .../Gdb/AvrGdb/CommandPackets/FlashDone.cpp | 25 +++++++++- .../Gdb/AvrGdb/CommandPackets/FlashErase.cpp | 20 ++++++++ .../Gdb/AvrGdb/CommandPackets/FlashWrite.cpp | 47 ++++++++++++++++--- src/DebugServer/Gdb/DebugSession.hpp | 17 +++++++ src/DebugServer/Gdb/ProgrammingSession.hpp | 29 ++++++++++++ 5 files changed, 130 insertions(+), 8 deletions(-) create mode 100644 src/DebugServer/Gdb/ProgrammingSession.hpp diff --git a/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashDone.cpp b/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashDone.cpp index d3f0eb6b..87284913 100644 --- a/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashDone.cpp +++ b/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashDone.cpp @@ -23,12 +23,35 @@ namespace Bloom::DebugServer::Gdb::AvrGdb::CommandPackets Logger::debug("Handling FlashDone packet"); try { + if (debugSession.programmingSession.has_value()) { + const auto& programmingSession = debugSession.programmingSession.value(); + + targetControllerConsole.enableProgrammingMode(); + + targetControllerConsole.writeMemory( + Targets::TargetMemoryType::FLASH, + programmingSession.startAddress, + std::move(programmingSession.buffer) + ); + + debugSession.programmingSession.reset(); + } + targetControllerConsole.disableProgrammingMode(); debugSession.connection.writePacket(OkResponsePacket()); } catch (const Exception& exception) { - Logger::error("Failed to disabling programming mode - " + exception.getMessage()); + Logger::error("Failed to handle FlashDone packet - " + exception.getMessage()); + debugSession.programmingSession.reset(); + + try { + targetControllerConsole.disableProgrammingMode(); + + } catch (const Exception& exception) { + Logger::error("Failed to disable programming mode - " + exception.getMessage()); + } + debugSession.connection.writePacket(ErrorResponsePacket()); } } diff --git a/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashErase.cpp b/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashErase.cpp index 8da7b47d..bf3e5852 100644 --- a/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashErase.cpp +++ b/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashErase.cpp @@ -52,6 +52,17 @@ namespace Bloom::DebugServer::Gdb::AvrGdb::CommandPackets Logger::debug("Handling FlashErase packet"); try { + const auto flashPageSize = debugSession.gdbTargetDescriptor.targetDescriptor.memoryDescriptorsByType.at( + Targets::TargetMemoryType::FLASH + ).pageSize.value(); + + if ((this->bytes % flashPageSize) != 0) { + throw Exception( + "Invalid erase size (" + std::to_string(this->bytes) + " bytes) provided by GDB - must be a " + "multiple of the target's flash page size (" + std::to_string(flashPageSize) + " bytes)" + ); + } + targetControllerConsole.enableProgrammingMode(); targetControllerConsole.writeMemory( @@ -64,6 +75,15 @@ namespace Bloom::DebugServer::Gdb::AvrGdb::CommandPackets } catch (const Exception& exception) { Logger::error("Failed to erase flash memory - " + exception.getMessage()); + debugSession.programmingSession.reset(); + + try { + targetControllerConsole.disableProgrammingMode(); + + } catch (const Exception& exception) { + Logger::error("Failed to disable programming mode - " + exception.getMessage()); + } + debugSession.connection.writePacket(ErrorResponsePacket()); } } diff --git a/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashWrite.cpp b/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashWrite.cpp index 87d62a10..f4483b89 100644 --- a/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashWrite.cpp +++ b/src/DebugServer/Gdb/AvrGdb/CommandPackets/FlashWrite.cpp @@ -52,18 +52,51 @@ namespace Bloom::DebugServer::Gdb::AvrGdb::CommandPackets Logger::debug("Handling FlashWrite packet"); try { - targetControllerConsole.enableProgrammingMode(); + if (this->buffer.empty()) { + throw Exception("Received empty buffer from GDB"); + } - targetControllerConsole.writeMemory( - Targets::TargetMemoryType::FLASH, - this->startAddress, - this->buffer - ); + if (!debugSession.programmingSession.has_value()) { + debugSession.programmingSession = ProgrammingSession(this->startAddress, this->buffer); + + } else { + auto& programmingSession = debugSession.programmingSession.value(); + const auto currentEndAddress = programmingSession.startAddress + programmingSession.buffer.size() - 1; + const auto expectedStartAddress = (currentEndAddress + 1); + + if (this->startAddress < expectedStartAddress) { + throw Exception("Invalid start address from GDB - the buffer would overlap a previous buffer"); + } + + if (this->startAddress > expectedStartAddress) { + // There is a gap in the buffer sent by GDB. Fill it with 0xFF + programmingSession.buffer.insert( + programmingSession.buffer.end(), + this->startAddress - expectedStartAddress, + 0xFF + ); + } + + programmingSession.buffer.insert( + programmingSession.buffer.end(), + this->buffer.begin(), + this->buffer.end() + ); + } debugSession.connection.writePacket(OkResponsePacket()); } catch (const Exception& exception) { - Logger::error("Failed to write to flash memory - " + exception.getMessage()); + Logger::error("Failed to handle FlashWrite packet - " + exception.getMessage()); + debugSession.programmingSession.reset(); + + try { + targetControllerConsole.disableProgrammingMode(); + + } catch (const Exception& exception) { + Logger::error("Failed to disable programming mode - " + exception.getMessage()); + } + debugSession.connection.writePacket(ErrorResponsePacket()); } } diff --git a/src/DebugServer/Gdb/DebugSession.hpp b/src/DebugServer/Gdb/DebugSession.hpp index 69b3f087..3e9b332f 100644 --- a/src/DebugServer/Gdb/DebugSession.hpp +++ b/src/DebugServer/Gdb/DebugSession.hpp @@ -1,10 +1,12 @@ #pragma once #include +#include #include "TargetDescriptor.hpp" #include "Connection.hpp" #include "Feature.hpp" +#include "ProgrammingSession.hpp" namespace Bloom::DebugServer::Gdb { @@ -33,6 +35,21 @@ namespace Bloom::DebugServer::Gdb */ bool waitingForBreak = false; + /** + * When the user attempts to program the target via GDB's 'load' command, GDB will send a number of + * FlashWrite (vFlashWrite) packets to Bloom. We group the data in these packets and flush it all at once, upon + * receiving a FlashDone (vFlashDone) packet. + * + * The grouped data is held in a ProgrammingSession object, against the active debug session. Once the data has + * been flushed, the ProgrammingSession object is destroyed. + * + * See the ProgrammingSession struct and GDB RSP documentation for more. + * + * This member holds the current (if any) ProgrammingSession object. It should only be populated during + * programming. + */ + std::optional programmingSession = std::nullopt; + DebugSession( Connection&& connection, const std::set>>& supportedFeatures, diff --git a/src/DebugServer/Gdb/ProgrammingSession.hpp b/src/DebugServer/Gdb/ProgrammingSession.hpp new file mode 100644 index 00000000..747f1666 --- /dev/null +++ b/src/DebugServer/Gdb/ProgrammingSession.hpp @@ -0,0 +1,29 @@ +#pragma once + +#include "src/Targets/TargetMemory.hpp" + +namespace Bloom::DebugServer::Gdb +{ + /** + * A programming session is created upon receiving the first FlashWrite (vFlashWrite) packet from GDB. + * + * The programming session holds the start address and a single buffer, which contains the sum of the numerous + * buffers received by GDB (via multiple FlashWrite packets). Upon receiving a FlashDone (vFlashDone) packet, we + * write the whole buffer to the target's program memory and then destroy the programming session. + * + * See FlashWrite::handle() and FlashDone::handle() for more. + */ + struct ProgrammingSession + { + Targets::TargetMemoryAddress startAddress = 0x00; + Targets::TargetMemoryBuffer buffer; + + ProgrammingSession( + Targets::TargetMemoryAddress startAddress, + const Targets::TargetMemoryBuffer& initialBuffer + ) + : startAddress(startAddress) + , buffer(initialBuffer) + {}; + }; +}