From aa6395a002332689200f290514b3aa4439890113 Mon Sep 17 00:00:00 2001 From: Nav Date: Fri, 2 Jul 2021 01:34:17 +0100 Subject: [PATCH] Corrected issue with byte order expectations for target registers. This was the cause for that issue with GDB using the incorrect memory addresses for local variables, after the PC and SP had been changed. Also renamed SP start address parameter to make the byte order of the register clear. --- .../AVR8/Avr8TargetDescriptionFile.php | 8 +++---- src/DebugServers/GdbRsp/GdbRspDebugServer.cpp | 6 ++++-- .../EDBG/AVR/EdbgAvr8Interface.cpp | 21 ++++++++++++++----- src/Targets/Microchip/AVR/AVR8/Avr8.cpp | 10 ++++----- .../Microchip/AVR/AVR8/TargetParameters.hpp | 2 +- 5 files changed, 30 insertions(+), 17 deletions(-) diff --git a/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php b/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php index dbe7ec98..52b7373a 100644 --- a/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php +++ b/build/scripts/TargetDescriptionFiles/AVR8/Avr8TargetDescriptionFile.php @@ -44,7 +44,7 @@ class Avr8TargetDescriptionFile extends TargetDescriptionFile public ?int $ocdDataRegister = null; public ?int $statusRegisterStartAddress = null; public ?int $statusRegisterSize = null; - public ?int $stackPointerRegisterStartAddress = null; + public ?int $stackPointerRegisterLowAddress = null; public ?int $stackPointerRegisterSize = null; public ?int $spmcRegisterStartAddress = null; public ?int $osccalAddress = null; @@ -223,13 +223,13 @@ class Avr8TargetDescriptionFile extends TargetDescriptionFile if (isset($cpuRegisterGroup->registersMappedByName['sp'])) { $stackPointerRegister = $cpuRegisterGroup->registersMappedByName['sp']; $this->stackPointerRegisterSize = $stackPointerRegister->size; - $this->stackPointerRegisterStartAddress = $stackPointerRegister->offset; + $this->stackPointerRegisterLowAddress = $stackPointerRegister->offset; } else { if (isset($cpuRegisterGroup->registersMappedByName['spl'])) { $stackPointerLowRegister = $cpuRegisterGroup->registersMappedByName['spl']; $this->stackPointerRegisterSize = $stackPointerLowRegister->size; - $this->stackPointerRegisterStartAddress = $stackPointerLowRegister->offset; + $this->stackPointerRegisterLowAddress = $stackPointerLowRegister->offset; if (isset($cpuRegisterGroup->registersMappedByName['sph'])) { $stackPointerHighRegister = $cpuRegisterGroup->registersMappedByName['sph']; @@ -451,7 +451,7 @@ class Avr8TargetDescriptionFile extends TargetDescriptionFile $failures[] = 'Unknown AVR8 family'; } - if (is_null($this->stackPointerRegisterStartAddress)) { + if (is_null($this->stackPointerRegisterLowAddress)) { $failures[] = 'Missing stack pointer register start address.'; } diff --git a/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp b/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp index d0ffd017..3334fb33 100644 --- a/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp +++ b/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp @@ -312,10 +312,12 @@ void GdbRspDebugServer::handleGdbPacket(CommandPackets::ReadGeneralRegisters& pa ); /* - * Finally, implode the register values, convert to hexadecimal form and send to the GDB client. + * Finally, reverse the register values (as they're all currently in MSB, but GDB expects them in LSB), implode + * the register values, convert to hexadecimal form and send to the GDB client. */ auto registers = std::vector(); - for (const auto& reg : registerSet) { + for (auto& reg : registerSet) { + std::reverse(reg.value.begin(), reg.value.end()); registers.insert(registers.end(), reg.value.begin(), reg.value.end()); } diff --git a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp index e12c49d2..2234f579 100644 --- a/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp +++ b/src/DebugToolDrivers/Protocols/CMSIS-DAP/VendorSpecific/EDBG/AVR/EdbgAvr8Interface.cpp @@ -508,7 +508,7 @@ void EdbgAvr8Interface::configure(const TargetConfig& targetConfig) { void EdbgAvr8Interface::setTargetParameters(const Avr8Bit::TargetParameters& config) { this->targetParameters = config; - if (!config.stackPointerRegisterStartAddress.has_value()) { + if (!config.stackPointerRegisterLowAddress.has_value()) { throw Exception("Failed to find stack pointer register start address"); } @@ -816,11 +816,19 @@ std::uint32_t EdbgAvr8Interface::getProgramCounter() { } TargetRegister EdbgAvr8Interface::getStackPointerRegister() { - return TargetRegister(TargetRegisterDescriptor(TargetRegisterType::STACK_POINTER), this->readMemory( + auto stackPointerValue = this->readMemory( Avr8MemoryType::SRAM, - this->targetParameters.stackPointerRegisterStartAddress.value(), + this->targetParameters.stackPointerRegisterLowAddress.value(), this->targetParameters.stackPointerRegisterSize.value() - )); + ); + + /* + * The SP low byte comes before the high byte, so the SP is stored in LSB form. We use std::reverse() here to + * convert it to MSB. + */ + std::reverse(stackPointerValue.begin(), stackPointerValue.end()); + + return TargetRegister(TargetRegisterDescriptor(TargetRegisterType::STACK_POINTER), stackPointerValue); } TargetRegister EdbgAvr8Interface::getStatusRegister() { @@ -843,9 +851,12 @@ void EdbgAvr8Interface::setStackPointerRegister(const TargetRegister& stackPoint registerValue.insert(registerValue.begin(), maximumStackPointerRegisterSize - registerValue.size(), 0x00); } + // Convert SP to LSB + std::reverse(registerValue.begin(), registerValue.end()); + this->writeMemory( Avr8MemoryType::SRAM, - this->targetParameters.stackPointerRegisterStartAddress.value(), + this->targetParameters.stackPointerRegisterLowAddress.value(), registerValue ); } diff --git a/src/Targets/Microchip/AVR/AVR8/Avr8.cpp b/src/Targets/Microchip/AVR/AVR8/Avr8.cpp index 93d7a1f9..ad1c2aff 100644 --- a/src/Targets/Microchip/AVR/AVR8/Avr8.cpp +++ b/src/Targets/Microchip/AVR/AVR8/Avr8.cpp @@ -106,7 +106,7 @@ void Avr8::loadTargetParameters() { auto stackPointerRegister = this->targetDescriptionFile->getStackPointerRegister(); if (stackPointerRegister.has_value()) { - this->targetParameters->stackPointerRegisterStartAddress = cpuRegistersOffset + stackPointerRegister->offset; + this->targetParameters->stackPointerRegisterLowAddress = cpuRegistersOffset + stackPointerRegister->offset; this->targetParameters->stackPointerRegisterSize = stackPointerRegister->size; } else { @@ -115,7 +115,7 @@ void Avr8::loadTargetParameters() { auto stackPointerHighRegister = this->targetDescriptionFile->getStackPointerHighRegister(); if (stackPointerLowRegister.has_value()) { - this->targetParameters->stackPointerRegisterStartAddress = cpuRegistersOffset + this->targetParameters->stackPointerRegisterLowAddress = cpuRegistersOffset + stackPointerLowRegister->offset; this->targetParameters->stackPointerRegisterSize = stackPointerLowRegister->size; } @@ -760,10 +760,10 @@ TargetRegister Avr8::getProgramCounterRegister() { auto programCounter = this->getProgramCounter(); return TargetRegister(TargetRegisterDescriptor(TargetRegisterType::PROGRAM_COUNTER), { - static_cast(programCounter), - static_cast(programCounter >> 8), - static_cast(programCounter >> 16), static_cast(programCounter >> 24), + static_cast(programCounter >> 16), + static_cast(programCounter >> 8), + static_cast(programCounter), }); } diff --git a/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp b/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp index 21b910fd..0706afa4 100644 --- a/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp +++ b/src/Targets/Microchip/AVR/AVR8/TargetParameters.hpp @@ -29,7 +29,7 @@ namespace Bloom::Targets::Microchip::Avr::Avr8Bit std::optional ocdDataRegister; std::optional statusRegisterStartAddress; std::optional statusRegisterSize; - std::optional stackPointerRegisterStartAddress; + std::optional stackPointerRegisterLowAddress; std::optional stackPointerRegisterSize; std::optional spmcRegisterStartAddress; std::optional osccalAddress;