From a3ed513b84cf4bb3ac10d9cdb9b188e45d56e438 Mon Sep 17 00:00:00 2001
From: Nav <nav@navnav.me>
Date: Sun, 17 Nov 2024 18:18:44 +0000
Subject: [PATCH] Fixed bug in EDBG driver that was resulting in program memory
 corruption when flashing the target with software breakpoints installed

---
 .../Protocols/EDBG/AVR/EdbgAvr8Interface.cpp  | 36 +++++++++++++++----
 .../Protocols/EDBG/AVR/EdbgAvr8Interface.hpp  | 12 +++++--
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp
index 9f115757..b530e1a1 100644
--- a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp
+++ b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.cpp
@@ -430,13 +430,7 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
     }
 
     void EdbgAvr8Interface::clearAllBreakpoints() {
-        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
-            ClearAllSoftwareBreakpoints{}
-        );
-
-        if (responseFrame.id == Avr8ResponseId::FAILED) {
-            throw Avr8CommandFailure{"AVR8 Clear all software breakpoints command failed", responseFrame};
-        }
+        this->clearAllSoftwareBreakpoints();
 
         // Clear all hardware breakpoints
         while (!this->hardwareBreakpointNumbersByAddress.empty()) {
@@ -889,6 +883,24 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
             return;
         }
 
+        /*
+         * When clearing individual software breakpoints via EDBG tools, the breakpoints are not actually removed
+         * until this next program flow command.
+         *
+         * This wouldn't be a problem, if the tool handled the stale breakpoint removals properly, when programming
+         * the target (overwriting the program memory at which the software breakpoints reside). But the tool
+         * doesn't handle this properly - it seems to completely ignore the fact that the program memory has been
+         * updated, and so it will corrupt the new program by overwriting the program memory where the old
+         * software breakpoints used to reside. The memory is overwritten with the old instruction - the one that
+         * was captured at the time the software breakpoint was inserted. So we end up with a corrupted program.
+         *
+         * To avoid this issue, we send the 'clear all software breakpoints' command to the tool, just before
+         * entering programming mode. That command will clear all breakpoints immediately, preventing program
+         * memory corruption at the next flow control command.
+         */
+        Logger::debug("Clearing all software breakpoints in preparation for programming mode");
+        this->clearAllSoftwareBreakpoints();
+
         const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
             EnterProgrammingMode{}
         );
@@ -1222,6 +1234,16 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         this->targetAttached = false;
     }
 
+    void EdbgAvr8Interface::clearAllSoftwareBreakpoints() {
+        const auto responseFrame = this->edbgInterface->sendAvrCommandFrameAndWaitForResponseFrame(
+            ClearAllSoftwareBreakpoints{}
+        );
+
+        if (responseFrame.id == Avr8ResponseId::FAILED) {
+            throw Avr8CommandFailure{"AVR8 Clear all software breakpoints command failed", responseFrame};
+        }
+    }
+
     std::unique_ptr<AvrEvent> EdbgAvr8Interface::getAvrEvent() {
         auto event = this->edbgInterface->requestAvrEvent();
 
diff --git a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp
index b95faae3..ff874e8b 100644
--- a/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp
+++ b/src/DebugToolDrivers/Microchip/Protocols/EDBG/AVR/EdbgAvr8Interface.hpp
@@ -202,10 +202,10 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
         void clearHardwareBreakpoint(Targets::TargetMemoryAddress address) override;
 
         /**
-         * Issues the "Software Breakpoint Clear All" command to the debug tool, clearing all software breakpoints
-         * that were set *in the current debug session*.
+         * Clears all software and hardware breakpoints on the target.
          *
-         * If the debug session ended before any of the set breakpoints were cleared, this will *not* clear them.
+         * This function will not clear any untracked hardware breakpoints (breakpoints that were installed in a
+         * previous session).
          */
         void clearAllBreakpoints() override;
 
@@ -457,6 +457,12 @@ namespace DebugToolDrivers::Microchip::Protocols::Edbg::Avr
          */
         void detach();
 
+        /**
+         * Issues the "Software Breakpoint Clear All" command to the debug tool, clearing all software breakpoints
+         * immediately.
+         */
+        void clearAllSoftwareBreakpoints();
+
         /**
          * Fetches any queued events belonging to the AVR8 Generic protocol (such as target break events).
          *