diff --git a/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp b/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp index f17f365d..e54da159 100644 --- a/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp +++ b/src/DebugServer/Gdb/AvrGdb/AvrGdbRsp.cpp @@ -42,7 +42,7 @@ namespace DebugServer::Gdb::AvrGdb {} std::string AvrGdbRsp::getName() const { - return "AVR GDB Remote Serial Protocol Debug Server"; + return "AVR GDB Remote Serial Protocol Server"; } std::unique_ptr AvrGdbRsp::rawPacketToCommandPacket( diff --git a/src/DebugServer/Gdb/Connection.cpp b/src/DebugServer/Gdb/Connection.cpp index 0c316413..7e45d673 100644 --- a/src/DebugServer/Gdb/Connection.cpp +++ b/src/DebugServer/Gdb/Connection.cpp @@ -11,6 +11,7 @@ #include "Exceptions/ClientCommunicationError.hpp" #include "src/Exceptions/Exception.hpp" +#include "src/Exceptions/InternalFatalErrorException.hpp" #include "src/Logger/Logger.hpp" #include "src/Services/StringService.hpp" @@ -207,6 +208,10 @@ namespace DebugServer::Gdb bool interruptible, std::optional timeout ) { + if (bytes.has_value() && *bytes > Connection::ABSOLUTE_MAXIMUM_PACKET_READ_SIZE) { + throw InternalFatalErrorException{"Attempted to read too many bytes from GDB socket"}; + } + auto output = std::vector{}; if (this->readInterruptEnabled != interruptible) { diff --git a/src/DebugServer/Gdb/DebugSession.hpp b/src/DebugServer/Gdb/DebugSession.hpp index 52c288fc..a5cf18d6 100644 --- a/src/DebugServer/Gdb/DebugSession.hpp +++ b/src/DebugServer/Gdb/DebugSession.hpp @@ -36,7 +36,7 @@ namespace DebugServer::Gdb * The current server configuration. * * TODO: I think this should be moved out of the DebugSession struct and passed into CommandPacket::handle() - * function. Review after v1.1.0. + * function. Review after v2.0.0. */ const GdbDebugServerConfig& serverConfig; diff --git a/src/DebugServer/Gdb/GdbRspDebugServer.hpp b/src/DebugServer/Gdb/GdbRspDebugServer.hpp index e0376247..4e0e4957 100644 --- a/src/DebugServer/Gdb/GdbRspDebugServer.hpp +++ b/src/DebugServer/Gdb/GdbRspDebugServer.hpp @@ -117,10 +117,6 @@ namespace DebugServer::Gdb GdbRspDebugServer& operator = (const GdbRspDebugServer& other) = delete; GdbRspDebugServer& operator = (GdbRspDebugServer&& other) = delete; - [[nodiscard]] std::string getName() const override { - return "GDB Remote Serial Protocol DebugServer"; - } - /** * Prepares the GDB server for listing on the selected address and port. */ @@ -291,6 +287,7 @@ namespace DebugServer::Gdb return; } catch (const ::Exceptions::FatalErrorException& exception) { + Logger::error("Fatal error occurred - closing connection"); this->endDebugSession(); throw exception; } diff --git a/src/DebugToolDrivers/Protocols/RiscVDebugSpec/DebugTranslator.cpp b/src/DebugToolDrivers/Protocols/RiscVDebugSpec/DebugTranslator.cpp index 41e28c77..812b35d5 100644 --- a/src/DebugToolDrivers/Protocols/RiscVDebugSpec/DebugTranslator.cpp +++ b/src/DebugToolDrivers/Protocols/RiscVDebugSpec/DebugTranslator.cpp @@ -28,8 +28,7 @@ #include "src/Exceptions/Exception.hpp" #include "src/Exceptions/InvalidConfig.hpp" #include "src/Exceptions/InternalFatalErrorException.hpp" -#include "src/TargetController/Exceptions/DeviceFailure.hpp" -#include "src/TargetController/Exceptions/DeviceInitializationFailure.hpp" +#include "src/TargetController/Exceptions/TargetFailure.hpp" #include "src/TargetController/Exceptions/TargetOperationFailure.hpp" #include "src/Logger/Logger.hpp" @@ -80,18 +79,13 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec void DebugTranslator::activate() { this->debugModuleDescriptor.hartIndices = this->discoverHartIndices(); if (this->debugModuleDescriptor.hartIndices.empty()) { - throw Exceptions::TargetOperationFailure{"Failed to discover any RISC-V harts"}; + throw Exceptions::TargetFailure{"Failed to discover any RISC-V harts"}; } - Logger::debug("Discovered RISC-V harts: " + std::to_string(this->debugModuleDescriptor.hartIndices.size())); - - /* - * We only support debugging a single RISC-V hart, for now. - * - * If we discover more than one, we select the first one and ensure that this is explicitly communicated to the - * user. - */ if (this->debugModuleDescriptor.hartIndices.size() > 1) { + Logger::debug( + "Discovered RISC-V harts: " + std::to_string(this->debugModuleDescriptor.hartIndices.size()) + ); Logger::warning("Bloom only supports debugging a single RISC-V hart - selecting first available"); } @@ -156,7 +150,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec } if (this->debugModuleDescriptor.memoryAccessStrategies.empty()) { - throw Exceptions::TargetOperationFailure{"Target doesn't support any known memory access strategies"}; + throw Exceptions::TargetFailure{"Target doesn't support any known memory access strategies"}; } this->memoryAccessStrategy = this->determineMemoryAccessStrategy(); @@ -203,7 +197,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec this->writeDebugModuleControlRegister(controlRegister); if (!statusRegister.allHalted) { - throw Exceptions::Exception{"Target took too long to halt selected harts"}; + throw Exceptions::TargetOperationFailure{"Target took too long to halt selected harts"}; } } @@ -233,7 +227,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec if (!statusRegister.allResumeAcknowledge) { Logger::debug("Failed to resume target execution - stopping target"); this->stop(); - throw Exceptions::Exception{"Target took too long to acknowledge resume request"}; + throw Exceptions::TargetOperationFailure{"Target took too long to acknowledge resume request"}; } } @@ -292,7 +286,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec }); if (!statusRegister.allHaveReset) { - throw Exceptions::Exception{"Target took too long to reset"}; + throw Exceptions::TargetOperationFailure{"Target took too long to reset"}; } this->initDebugControlStatusRegister(); @@ -307,7 +301,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec const auto triggerDescriptorOpt = this->getAvailableTrigger(); if (!triggerDescriptorOpt.has_value()) { - throw Exceptions::Exception{"Insufficient resources - no available trigger"}; + throw Exceptions::TargetOperationFailure{"Insufficient resources - no available trigger"}; } const auto& triggerDescriptor = triggerDescriptorOpt->get(); @@ -343,13 +337,13 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec return; } - throw Exceptions::Exception{"Unsupported trigger"}; + throw Exceptions::TargetOperationFailure{"Unsupported trigger"}; } void DebugTranslator::clearTriggerBreakpoint(TargetMemoryAddress address) { const auto triggerIndexIt = this->triggerIndicesByBreakpointAddress.find(address); if (triggerIndexIt == this->triggerIndicesByBreakpointAddress.end()) { - throw Exceptions::Exception{"Unknown hardware breakpoint"}; + throw Exceptions::TargetOperationFailure{"Unknown hardware breakpoint"}; } const auto& triggerDescriptor = this->debugModuleDescriptor.triggerDescriptorsByIndex.at(triggerIndexIt->second); @@ -566,7 +560,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec } if (writeSelectError != AbstractCommandError::NONE) { - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Failed to write to TRIGGER_SELECT register - abstract command error: 0x" + Services::StringService::toHex(static_cast(writeSelectError)) }; @@ -649,7 +643,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec } if (!controlRegister.debugModuleActive) { - throw Exceptions::Exception{"Took too long to enable debug module"}; + throw Exceptions::TargetOperationFailure{"Took too long to enable debug module"}; } } @@ -671,7 +665,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec } if (controlRegister.debugModuleActive) { - throw Exceptions::Exception{"Took too long to disable debug module"}; + throw Exceptions::TargetOperationFailure{"Took too long to disable debug module"}; } } @@ -720,7 +714,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec const auto result = this->tryReadCpuRegister(number, flags); if (!result.hasValue()) { - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Failed to read CPU register (number: 0x" + Services::StringService::toHex(number) + ") - abstract command error: 0x" + Services::StringService::toHex(static_cast(result.error())) @@ -770,7 +764,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec ) { const auto commandError = this->tryWriteCpuRegister(number, value, flags); if (commandError != AbstractCommandError::NONE) { - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Failed to write to CPU register (number: 0x" + Services::StringService::toHex(number) + ") - abstract command error: 0x" + Services::StringService::toHex(static_cast(commandError)) @@ -825,7 +819,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec } if (abstractStatusRegister.busy) { - throw Exceptions::Exception{"Abstract command took too long to execute"}; + throw Exceptions::TargetOperationFailure{"Abstract command took too long to execute"}; } if (abstractStatusRegister.commandError != AbstractCommandError::NONE) { @@ -840,7 +834,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec ) { const auto commandError = this->tryExecuteAbstractCommand(abstractCommandRegister); if (commandError != AbstractCommandError::NONE) { - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Failed to execute abstract command - error: 0x" + Services::StringService::toHex(static_cast(commandError)) }; @@ -969,7 +963,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec }); if (programOpcodes.size() > this->debugModuleDescriptor.programBufferSize) { - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Cannot read memory via RISC-V debug module program buffer - insufficient program buffer size" }; } @@ -1043,7 +1037,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec if (abstractStatusRegister.commandError != AbstractCommandError::NONE) { this->clearAbstractCommandError(); - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Program buffer execution failed - abstract command error: 0x" + Services::StringService::toHex(abstractStatusRegister.commandError) }; @@ -1091,7 +1085,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec }); if (programOpcodes.size() > this->debugModuleDescriptor.programBufferSize) { - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Cannot write to memory via RISC-V debug module program buffer - insufficient program buffer size" }; } @@ -1144,7 +1138,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec if (abstractStatusRegister.commandError != AbstractCommandError::NONE) { this->clearAbstractCommandError(); - throw Exceptions::Exception{ + throw Exceptions::TargetOperationFailure{ "Program buffer execution failed - abstract command error: 0x" + Services::StringService::toHex(abstractStatusRegister.commandError) }; @@ -1203,7 +1197,7 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec return; } - throw Exceptions::Exception{"Unsupported trigger"}; + throw Exceptions::TargetOperationFailure{"Unsupported trigger"}; } DebugTranslator::PreservedCpuRegister::PreservedCpuRegister( @@ -1238,9 +1232,9 @@ namespace DebugToolDrivers::Protocols::RiscVDebugSpec * will be left in an undefined state. More specifically, the state of the program running on the target * may be corrupted. We cannot recover from this. * - * DeviceFailure exceptions are considered to be fatal. A clean shutdown will follow. + * TargetFailure exceptions are considered to be fatal. A clean shutdown will follow. */ - throw Exceptions::DeviceFailure{ + throw Exceptions::TargetFailure{ "Failed to restore CPU register (number: 0x" + Services::StringService::toHex(this->registerNumber) + ") - error: " + exception.getMessage() + " - the target is now in an undefined state and may require a reset" diff --git a/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.cpp b/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.cpp index 9212038e..6525c920 100644 --- a/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.cpp +++ b/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.cpp @@ -142,21 +142,24 @@ namespace DebugToolDrivers::Wch::Protocols::WchLink Targets::TargetMemoryAddress startAddress, Targets::TargetMemoryBufferSpan buffer ) { - constexpr auto packetSize = std::uint8_t{64}; + constexpr auto packetSize = WchLinkInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE; const auto bufferSize = static_cast(buffer.size()); - const auto packetsRequired = static_cast( + const auto packetsRequired = static_cast( std::ceil(static_cast(bufferSize) / static_cast(packetSize)) ); - for (auto i = std::uint32_t{0}; i < packetsRequired; ++i) { - const auto segmentSize = static_cast( - std::min( - static_cast(bufferSize - (i * packetSize)), - packetSize - ) + for (auto i = std::size_t{0}; i < packetsRequired; ++i) { + const auto segmentSize = std::min( + static_cast(bufferSize - (i * packetSize)), + packetSize ); + assert(segmentSize <= 0xFF); + const auto response = this->sendCommandAndWaitForResponse( - Commands::PreparePartialFlashBlockWrite{startAddress + (packetSize * i), segmentSize} + Commands::PreparePartialFlashBlockWrite{ + static_cast(startAddress + (packetSize * i)), + static_cast(segmentSize) + } ); if (response.payload.size() != 1) { diff --git a/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.hpp b/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.hpp index 51a452ce..e6f6bfea 100644 --- a/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.hpp +++ b/src/DebugToolDrivers/WCH/Protocols/WchLink/WchLinkInterface.hpp @@ -30,6 +30,8 @@ namespace DebugToolDrivers::Wch::Protocols::WchLink : public ::DebugToolDrivers::Protocols::RiscVDebugSpec::DebugTransportModuleInterface { public: + static constexpr Targets::TargetMemorySize MAX_PARTIAL_BLOCK_WRITE_SIZE = 64; + WchLinkInterface(Usb::UsbInterface& usbInterface, Usb::UsbDevice& usbDevice); DeviceInfo getDeviceInfo(); diff --git a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp index 9446ca6b..dd225afb 100644 --- a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp +++ b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.cpp @@ -210,7 +210,7 @@ namespace DebugToolDrivers::Wch * See WchLinkDebugInterface::writeFlashMemory() below, for more. */ const auto bufferSize = static_cast(buffer.size()); - const auto alignmentSize = bufferSize > WchLinkDebugInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE + const auto alignmentSize = bufferSize > WchLinkInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE ? this->programmingBlockSize : 1; @@ -286,7 +286,7 @@ namespace DebugToolDrivers::Wch * Writes any number of bytes to flash, but limited to a maximum of 64 bytes per write. Larger writes * must be split into multiple writes. * - Full block write - * Writes an entire block to flash. Where the block size is target-specific (resides in the target's + * Writes an entire block to flash, where the block size is target-specific (resides in the target's * TDF). Requires alignment to the block size. Requires reattaching to the target at the end of the * programming session. * @@ -294,7 +294,7 @@ namespace DebugToolDrivers::Wch * target. But the partial block write is faster and more suitable for writing buffers that are smaller than * 64 bytes, such as when we're inserting software breakpoints. */ - if (buffer.size() <= WchLinkDebugInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE) { + if (buffer.size() <= WchLinkInterface::MAX_PARTIAL_BLOCK_WRITE_SIZE) { return this->wchLinkInterface.writeFlashPartialBlock(startAddress, buffer); } diff --git a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp index f3f86332..548a4427 100644 --- a/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp +++ b/src/DebugToolDrivers/WCH/WchLinkDebugInterface.hpp @@ -77,8 +77,6 @@ namespace DebugToolDrivers::Wch ) override; private: - static constexpr Targets::TargetMemorySize MAX_PARTIAL_BLOCK_WRITE_SIZE = 64; - const WchLinkToolConfig& toolConfig; const Targets::RiscV::RiscVTargetConfig& targetConfig; const Targets::RiscV::TargetDescriptionFile& targetDescriptionFile; diff --git a/src/TargetController/Exceptions/TargetFailure.hpp b/src/TargetController/Exceptions/TargetFailure.hpp new file mode 100644 index 00000000..7789c996 --- /dev/null +++ b/src/TargetController/Exceptions/TargetFailure.hpp @@ -0,0 +1,14 @@ +#pragma once + +#include "src/Exceptions/FatalErrorException.hpp" + +namespace Exceptions +{ + class TargetFailure: public FatalErrorException + { + public: + explicit TargetFailure(const std::string& message) + : FatalErrorException(message) + {} + }; +} diff --git a/src/Targets/RiscV/Wch/WchRiscV.cpp b/src/Targets/RiscV/Wch/WchRiscV.cpp index 6383d6f0..2f464952 100644 --- a/src/Targets/RiscV/Wch/WchRiscV.cpp +++ b/src/Targets/RiscV/Wch/WchRiscV.cpp @@ -118,7 +118,7 @@ namespace Targets::RiscV::Wch * * @TODO: Currently, this just assumes that the alias segment always maps to the program memory segment, but I * believe it may map to the boot program memory segment in some cases. This needs to be revisited - * before v1.1.0. + * before v2.0.0. */ if (memorySegmentDescriptor == this->mappedProgramMemorySegmentDescriptor) { const auto newAddress = startAddress - this->mappedProgramMemorySegmentDescriptor.addressRange.startAddress diff --git a/src/Targets/TargetPinDescriptor.hpp b/src/Targets/TargetPinDescriptor.hpp index 37fbe854..bbd7db94 100644 --- a/src/Targets/TargetPinDescriptor.hpp +++ b/src/Targets/TargetPinDescriptor.hpp @@ -10,7 +10,7 @@ namespace Targets { public: std::string position; - std::uint16_t numericPosition; // TODO: Consider removing this bodge. Review after v1.1.0 + std::uint16_t numericPosition; // TODO: Consider removing this bodge. Review after v2.0.0 std::optional padKey; TargetPinDescriptor(