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),