From 0c510ca62ecad9a769b00189fae38bc6aba5000b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 16 Aug 2026 21:15:46 +0200 Subject: [PATCH] Add BreakpointManager::ChangeBreakPointAddress, use it from the ImDebugger ChangeBreakPointAddress() moves the breakpoint keeping its action, condition and log format, invalidates both ends, refuses to land on an existing breakpoint, and resets the hit count since it belonged to the old address. The edit form now works on a copy of the address and commits on deactivation rather than per keystroke, so typing one address doesn't churn through every prefix of it. The breakpoint edit form assigned straight to bp.addr and then invalidated the icache at "bp.addr - 4, 8" - which by then is the *new* address - need both. Also clear the selection after Delete in both edit forms - the reference into the vector is dangling from that point on. Harmless today, but only because nothing happens to touch it below. Covered by a new Breakpoints unit test (verified to fail without the duplicate check and the hit reset). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9 --- Core/Debugger/Breakpoints.cpp | 35 ++++++++++++++++++++++++ Core/Debugger/Breakpoints.h | 4 +++ UI/ImDebugger/ImDebugger.cpp | 38 +++++++++++++++----------- unittest/UnitTest.cpp | 51 +++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 16 deletions(-) diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index 12244c62fd..c9a5576eec 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/Core/Debugger/Breakpoints.cpp @@ -210,6 +210,41 @@ void BreakpointManager::ChangeBreakPoint(u32 addr, BreakAction action) { } } +// Relocates a breakpoint the user already set, rather than making them delete and re-add it. +// Returns false and changes nothing if there's no (non-temporary) breakpoint at oldAddr, or if +// newAddr already has one of its own. +// +// Refusing the duplicate matters: ExecBreakPoint() goes through FindBreakpoint(), which returns +// only one entry per address, so a second breakpoint at the same address is invisible - and if the +// one that gets found has a condition that evaluates false, the other silently never fires either. +bool BreakpointManager::ChangeBreakPointAddress(u32 oldAddr, u32 newAddr) { + if (oldAddr == newAddr) + return true; + + // Match non-temporary only - a temp breakpoint belongs to an in-flight step, not to the user. + size_t bp = FindBreakpoint(oldAddr, true, false); + if (bp == INVALID_BREAKPOINT) + return false; + // ...but collide against any breakpoint at all, temporary ones included. + if (FindBreakpoint(newAddr) != INVALID_BREAKPOINT) + return false; + + if (newAddr & 3) { + WARN_LOG(Log::Debugger, "Breakpoint moved to %08x will not be effective - unaligned address.", newAddr); + } + + breakPoints_[bp].addr = newAddr; + // The count belonged to the old address, so carrying it over would just be misleading. + breakPoints_[bp].numHits = 0; + + // Both ends need invalidating, not just the new one: under a JIT the old address still has a + // compiled-in check that now matches no breakpoint, and the new address has none at all. + currentMIPS->InvalidateICacheRangeDeferred(oldAddr - 4, 8); + currentMIPS->InvalidateICacheRangeDeferred(newAddr - 4, 8); + System_Notify(SystemNotification::DISASSEMBLY); + return true; +} + // This is not actually called, currently. void BreakpointManager::ClearAllBreakPoints() { if (!anyBreakPoints_) diff --git a/Core/Debugger/Breakpoints.h b/Core/Debugger/Breakpoints.h index 6a0c619f10..899bbf6078 100644 --- a/Core/Debugger/Breakpoints.h +++ b/Core/Debugger/Breakpoints.h @@ -163,6 +163,10 @@ public: void RemoveBreakPoint(u32 addr); void ChangeBreakPoint(u32 addr, bool enable); void ChangeBreakPoint(u32 addr, BreakAction action); + // Moves an existing breakpoint, keeping its action, condition and log format. Prefer this over + // assigning to BreakPoint::addr through GetBreakpointRefs() - there's cache invalidation and a + // duplicate check to get right, see the implementation. + bool ChangeBreakPointAddress(u32 oldAddr, u32 newAddr); void ClearAllBreakPoints(); // Makes a copy of the condition. diff --git a/UI/ImDebugger/ImDebugger.cpp b/UI/ImDebugger/ImDebugger.cpp index 8f57c56418..8690cf7e10 100644 --- a/UI/ImDebugger/ImDebugger.cpp +++ b/UI/ImDebugger/ImDebugger.cpp @@ -1149,23 +1149,27 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) { // Add edit form for breakpoints if (ImGui::BeginChild("bp_edit")) { auto &bp = bps[cfg.selectedBreakpoint]; - bool changed = false; ImGui::TextUnformatted("Edit breakpoint"); - if (ImGui::CheckboxFlags("Enabled", (int *)&bp.action, (int)BREAK_ACTION_PAUSE)) { - changed = true; - } - if (ImGui::CheckboxFlags("Log", (int *)&bp.action, (int)BREAK_ACTION_LOG)) { - changed = true; - } - if (ImGui::InputScalar("Address", ImGuiDataType_U32, &bp.addr, nullptr, nullptr, "%08x", ImGuiInputTextFlags_CharsHexadecimal)) { - changed = true; - } - if (changed) { - // This is a bit hacky, we should probably add a utility function in the breakpoint manager. - currentMIPS->InvalidateICacheRangeDeferred(bp.addr - 4, 8); + // No cache invalidation needed for these two: the JIT emits its call for any address + // that has a breakpoint at all, and the action bits are read live in + // ExecBreakPoint(). Only the set of addresses affects generated code. + ImGui::CheckboxFlags("Enabled", (int *)&bp.action, (int)BREAK_ACTION_PAUSE); + ImGui::CheckboxFlags("Log", (int *)&bp.action, (int)BREAK_ACTION_LOG); + + // Moving a breakpoint isn't just an assignment to bp.addr - the compiled code at + // both the old and the new address has to be invalidated, and landing on top of an + // existing breakpoint has to be refused. Edit a copy and let the manager do it. + // Committing on deactivation rather than per keystroke also avoids doing all that + // for each of the intermediate addresses you pass through while typing one. + u32 addr = bp.addr; + ImGui::InputScalar("Address", ImGuiDataType_U32, &addr, nullptr, nullptr, "%08x", ImGuiInputTextFlags_CharsHexadecimal); + if (ImGui::IsItemDeactivatedAfterEdit()) { + g_breakpoints.ChangeBreakPointAddress(bp.addr, addr); } if (ImGui::Button("Delete")) { g_breakpoints.RemoveBreakPoint(bp.addr); + // bp (and bps) are dangling from here on - don't touch them again this frame. + cfg.selectedBreakpoint = -1; } ImGui::EndChild(); } @@ -1212,12 +1216,14 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) { if (ImGui::InputScalar("End", ImGuiDataType_U32, &mc.end, NULL, NULL, "%08x", ImGuiInputTextFlags_CharsHexadecimal)) { changed = true; } - if (ImGui::Button("Delete")) { - g_breakpoints.RemoveMemCheck(mcs[cfg.selectedMemCheck].start, mcs[cfg.selectedMemCheck].end); - } if (changed) { g_breakpoints.NotifyChangedMemchecks(); } + if (ImGui::Button("Delete")) { + g_breakpoints.RemoveMemCheck(mc.start, mc.end); + // mc (and mcs) are dangling from here on - don't touch them again this frame. + cfg.selectedMemCheck = -1; + } ImGui::EndChild(); } } diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 0af5439927..fc6b7cef4a 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -86,6 +86,7 @@ #include "Common/Math/fast/fast_matrix.h" #include "Common/Serialize/Serializer.h" #include "Core/CmdLine.h" +#include "Core/Debugger/Breakpoints.h" #include "Core/Debugger/MemBlockInfo.h" #include "Core/FileSystems/ISOFileSystem.h" #include "Core/MemMap.h" @@ -545,6 +546,55 @@ bool TestMemBlockInfoSaveState() { return true; } +// Covers BreakpointManager::ChangeBreakPointAddress(), which the ImDebugger uses to relocate a +// breakpoint the user is editing. Only the pure bookkeeping is exercised here - there's no JIT in +// this build, so the cache invalidation it also does is a no-op. +bool TestBreakpoints() { + const u32 kAddrA = 0x08804000; + const u32 kAddrB = 0x08804100; + const u32 kAddrC = 0x08804200; + + g_breakpoints.AddBreakPoint(kAddrA); + g_breakpoints.ChangeBreakPoint(kAddrA, BreakAction(BREAK_ACTION_PAUSE | BREAK_ACTION_LOG)); + // Pretend it tripped a few times, so the reset below is actually testing something. + g_breakpoints.GetBreakpointRefs()[0].numHits = 7; + + // A plain move: gone from the old address, present at the new one, action carried over, and the + // hit count (which belonged to the old address) reset. + EXPECT_TRUE(g_breakpoints.ChangeBreakPointAddress(kAddrA, kAddrB)); + EXPECT_FALSE(g_breakpoints.IsAddressBreakPoint(kAddrA)); + EXPECT_TRUE(g_breakpoints.IsAddressBreakPoint(kAddrB)); + { + std::vector bps = g_breakpoints.GetBreakpoints(); + EXPECT_EQ_INT((int)bps.size(), 1); + EXPECT_EQ_INT((int)bps[0].action, (int)(BREAK_ACTION_PAUSE | BREAK_ACTION_LOG)); + EXPECT_EQ_INT((int)bps[0].numHits, 0); + } + + // Moving onto an address that already has a breakpoint must be refused rather than creating a + // duplicate - FindBreakpoint() only ever returns one entry per address, so the other would be + // silently dead. Neither breakpoint should move. + g_breakpoints.AddBreakPoint(kAddrC); + EXPECT_FALSE(g_breakpoints.ChangeBreakPointAddress(kAddrB, kAddrC)); + EXPECT_TRUE(g_breakpoints.IsAddressBreakPoint(kAddrB)); + EXPECT_TRUE(g_breakpoints.IsAddressBreakPoint(kAddrC)); + EXPECT_EQ_INT((int)g_breakpoints.GetBreakpoints().size(), 2); + + // Nothing to move. + EXPECT_FALSE(g_breakpoints.ChangeBreakPointAddress(kAddrA, 0x08804300)); + EXPECT_FALSE(g_breakpoints.IsAddressBreakPoint(0x08804300)); + + // Moving somewhere it already is succeeds and does nothing. + EXPECT_TRUE(g_breakpoints.ChangeBreakPointAddress(kAddrB, kAddrB)); + EXPECT_TRUE(g_breakpoints.IsAddressBreakPoint(kAddrB)); + EXPECT_EQ_INT((int)g_breakpoints.GetBreakpoints().size(), 2); + + g_breakpoints.RemoveBreakPoint(kAddrB); + g_breakpoints.RemoveBreakPoint(kAddrC); + EXPECT_EQ_INT((int)g_breakpoints.GetBreakpoints().size(), 0); + return true; +} + bool TestTinySet() { TinySet a; EXPECT_EQ_INT((int)a.size(), 0); @@ -1602,6 +1652,7 @@ TestItem availableTests[] = { TEST_ITEM(Parsers), TEST_ITEM(TruncateCpy), TEST_ITEM(MemBlockInfoSaveState), + TEST_ITEM(Breakpoints), TEST_ITEM(Utf8), TEST_ITEM(IRPassSimplify), TEST_ITEM(Jit),