From 6a458bf14808cc0d8a4bd0fe2e8a4f2d31a82f86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 9 Oct 2026 22:52:32 +0200 Subject: [PATCH] Debugger: Fix breakpoints swallowed or reported twice when resuming The skip-first marker suppressed everything at the address you resume or step from. That's needed for the pause, or you could never get off a breakpoint, but it also dropped the log and hit count. So a log breakpoint you'd stepped onto, or were parked on for some other reason, never logged when execution moved on. Now the skip only drops the pause, unless the breakpoint at that address is the one that stopped us, which has already logged and counted this execution. The check moves into the Exec* functions instead of being repeated in the interpreter, IR interpreter and three JITs, where the register breakpoint path only applied it to the pause and so logged twice when stepping off a log+pause one. The marker gets an explicit valid flag, since address 0 meant "none", and Core_Break() clears it. Based on the earlier cleanup-skip-first branch, reduced to one marker plus a flag. Checked over the WebSocket debugger on the interpreter, IR, x86 JIT and JIT-IR: log breakpoints on a parked and a stepped-onto address each log once, and stepping off a log+pause one doesn't report it again. Co-Authored-By: Claude Opus 5.5 (1M context) --- Core/Core.cpp | 1 + Core/Debugger/Breakpoints.cpp | 89 +++++++++++++++++++++++++--------- Core/Debugger/Breakpoints.h | 24 +++++++-- Core/MIPS/IR/IRFrontend.cpp | 4 +- Core/MIPS/IR/IRInterpreter.cpp | 10 ---- Core/MIPS/MIPSTables.cpp | 6 +-- unittest/UnitTest.cpp | 49 +++++++++++++++++++ 7 files changed, 142 insertions(+), 41 deletions(-) diff --git a/Core/Core.cpp b/Core/Core.cpp index d37365d54b..84fbeca516 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -687,6 +687,7 @@ void Core_Break(BreakReason reason, u32 relatedAddress, const BreakpointHit *hit // later at an address nobody is waiting for anymore. Same as gdb dropping its step-resume // breakpoint, or lldb discarding the thread plan, on any stop. g_breakpoints.ClearTempBreakPoint(); + g_breakpoints.ClearSkipFirst(); // Same reasoning for a cpu.runUntilTime deadline - it belonged to the run that just ended. CoreTiming::SetBreakDeadlineUs(0); diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index bcdc07ec3d..999203bc65 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/Core/Debugger/Breakpoints.cpp @@ -61,9 +61,10 @@ BreakAction MemCheck::Apply(u32 addr, bool write, int size, u32 pc) { return BREAK_ACTION_NONE; } -BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char *reason) { +BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char *reason, bool skipPause) { // Conditions have always already been checked if we get here. Log(addr, write, size, pc, reason); + const BreakAction action = skipPause ? BreakAction(this->action & ~BREAK_ACTION_PAUSE) : this->action; BreakpointHit hit; if (WebSocketDebuggerHasClients() || (action & BREAK_ACTION_PAUSE)) { @@ -358,11 +359,15 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { BreakAction result = BREAK_ACTION_NONE; BreakpointHit hit; + const bool skipPause = SkipPauseAt(addr); size_t bp = FindBreakpoint(addr); - if (bp != INVALID_BREAKPOINT) { + if (bp != INVALID_BREAKPOINT && !SkipReportAt(addr)) { BreakPoint &info = breakPoints_[bp]; - const BreakAction action = info.action; + BreakAction action = info.action; + if (skipPause) { + action = BreakAction(action & ~BREAK_ACTION_PAUSE); + } bool condPassed = true; if (info.hasCond) @@ -404,7 +409,7 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { } } - if (tempBreakPoint_.valid && tempBreakPoint_.addr == addr) { + if (tempBreakPoint_.valid && tempBreakPoint_.addr == addr && !skipPause) { // The condition, when set, narrows down which hit counts - to one thread for a step // ("threadid == ..."), or to a later frame for run-to-cursor ("flipcount > ..."). A hit that // fails it leaves the breakpoint armed, so the next one gets a chance. @@ -418,6 +423,9 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { // hit stays kind None when only the temporary breakpoint fired - there's no user // breakpoint to describe in that case, just a step completing. Core_Break(BreakReason::CpuBreakpoint, addr, hit.kind != BreakpointKind::None ? &hit : nullptr); + if (hit.kind != BreakpointKind::None) { + NoteStoppedOnReport(addr); + } System_Notify(SystemNotification::DISASSEMBLY); } @@ -577,7 +585,7 @@ MemCheck *BreakpointManager::FindMemCheckInRange(u32 address, int size) { BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u32 pc, const char *reason) { - if (!anyMemChecks_) + if (!anyMemChecks_ || SkipReportAt(pc)) return BREAK_ACTION_NONE; MemCheck *check = FindMemCheckInRange(address, size); if (check) { @@ -586,7 +594,11 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u return applyAction; MemCheck copy = *check; - return copy.Action(address, write, size, pc, reason); + const BreakAction result = copy.Action(address, write, size, pc, reason, SkipPauseAt(pc)); + if (result & BREAK_ACTION_PAUSE) { + NoteStoppedOnReport(pc); + } + return result; } return BREAK_ACTION_NONE; } @@ -594,6 +606,11 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc) { // Note: currently, we don't check "on changed" for HLE (ExecMemCheck.) // We'd need to more carefully specify memory changes in HLE for that. + // pc may be the delay slot, but the skip is keyed on where execution resumes, which is the branch. + const u32 execPc = currentMIPS->pc; + if (SkipReportAt(execPc)) { + return BREAK_ACTION_NONE; + } int size = MIPSAnalyst::OpMemoryAccessSize(pc); if (size == 0 && MIPSAnalyst::OpHasDelaySlot(pc)) { // This means that the delay slot is what tripped us. @@ -619,7 +636,11 @@ BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc) { return applyAction; MemCheck copy = *check; - return copy.Action(address, write, size, pc, "CPU"); + const BreakAction result = copy.Action(address, write, size, pc, "CPU", SkipPauseAt(execPc)); + if (result & BREAK_ACTION_PAUSE) { + NoteStoppedOnReport(execPc); + } + return result; } } return BREAK_ACTION_NONE; @@ -752,23 +773,28 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) { if (info.hasCond && !info.cond.Evaluate()) return BREAK_ACTION_NONE; + if (SkipReportAt(pc)) { + return BREAK_ACTION_NONE; + } + const BreakAction result = SkipPauseAt(pc) ? BreakAction(info.result & ~BREAK_ACTION_PAUSE) : info.result; + ++info.numHits; BreakpointHit hit; - if (WebSocketDebuggerHasClients() || (info.result & BREAK_ACTION_PAUSE)) { + if (WebSocketDebuggerHasClients() || (result & BREAK_ACTION_PAUSE)) { hit.kind = BreakpointKind::Register; hit.pc = pc; hit.address = pc; hit.reg = reg; hit.numHits = info.numHits; - hit.logged = (info.result & BREAK_ACTION_LOG) != 0; - hit.paused = (info.result & BREAK_ACTION_PAUSE) != 0; + hit.logged = (result & BREAK_ACTION_LOG) != 0; + hit.paused = (result & BREAK_ACTION_PAUSE) != 0; if (info.hasCond) hit.condition = info.cond.expressionString; WebSocketNotifyBreakpointHit(hit); } - if (info.result & BREAK_ACTION_LOG) { + if (result & BREAK_ACTION_LOG) { if (info.logFormat.empty()) { NOTICE_LOG(Log::JIT, "BKP reg write r%d, PC=%08x (%s)", reg, pc, g_symbolMap->GetDescription(pc).c_str()); } else { @@ -777,28 +803,47 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) { NOTICE_LOG(Log::JIT, "BKP reg write r%d, PC=%08x: %s", reg, pc, formatted.c_str()); } } - if ((info.result & BREAK_ACTION_PAUSE) && g_breakpoints.CheckSkipFirst() != pc) { + if (result & BREAK_ACTION_PAUSE) { Core_Break(BreakReason::RegBreakpoint, pc, &hit); + NoteStoppedOnReport(pc); } - return info.result; + return result; } void BreakpointManager::ClearSkipFirst() { - breakSkipFirstAt_ = 0; - breakSkipFirstTicks_ = 0; + skipFirstValid_ = false; + stoppedOnReport_ = false; } void BreakpointManager::SetSkipFirst(u32 pc) { - breakSkipFirstAt_ = pc; - breakSkipFirstTicks_ = CoreTiming::GetTicks(currentMIPS); + const u64 ticks = CoreTiming::GetTicks(currentMIPS); + // A step-over arms its temporary breakpoint and then resumes, which comes through here again. + if (skipFirstValid_ && skipFirstAt_ == pc && skipFirstTicks_ == ticks) { + return; + } + skipFirstValid_ = true; + skipFirstAt_ = pc; + skipFirstTicks_ = ticks; + // Consumed here, since a plain step stops again without going through Core_Break(). + skipFirstReport_ = stoppedOnReport_ && stoppedOnReportAt_ == pc; + if (skipFirstReport_) { + stoppedOnReport_ = false; + } } -u32 BreakpointManager::CheckSkipFirst() const { - u32 pc = breakSkipFirstAt_; - if (breakSkipFirstTicks_ == CoreTiming::GetTicks(currentMIPS)) - return pc; - return 0; +bool BreakpointManager::SkipPauseAt(u32 pc) const { + return skipFirstValid_ && skipFirstAt_ == pc && skipFirstTicks_ == CoreTiming::GetTicks(currentMIPS); +} + +bool BreakpointManager::SkipReportAt(u32 pc) const { + return skipFirstReport_ && SkipPauseAt(pc); +} + +// Call after Core_Break(), which clears it. +void BreakpointManager::NoteStoppedOnReport(u32 pc) { + stoppedOnReport_ = true; + stoppedOnReportAt_ = pc; } static MemCheck NotCached(MemCheck mc) { diff --git a/Core/Debugger/Breakpoints.h b/Core/Debugger/Breakpoints.h index 9713d1b1e1..ad28b42682 100644 --- a/Core/Debugger/Breakpoints.h +++ b/Core/Debugger/Breakpoints.h @@ -125,7 +125,7 @@ struct MemCheck { // Called on the stored memcheck (affects numHits, etc.) BreakAction Apply(u32 addr, bool write, int size, u32 pc); // Called on a copy. - BreakAction Action(u32 addr, bool write, int size, u32 pc, const char *reason); + BreakAction Action(u32 addr, bool write, int size, u32 pc, const char *reason, bool skipPause); void Log(u32 addr, bool write, int size, u32 pc, const char *reason) const; @@ -251,8 +251,10 @@ public: // instruction that would write to reg - does not itself execute the instruction. BreakAction ExecRegBreakpoint(int reg, u32 pc); + // Call when starting to run or step from pc. Breakpoints at pc then don't pause until the + // instruction there has run, and if one of them is what stopped us, it doesn't report again. void SetSkipFirst(u32 pc); - u32 CheckSkipFirst() const; + // Execution stopped, or jumped (reset, savestate load). void ClearSkipFirst(); // Includes uncached addresses. @@ -301,10 +303,24 @@ private: std::atomic anyMemChecks_; std::atomic regBreakpointMask_; + bool SkipPauseAt(u32 pc) const; + bool SkipReportAt(u32 pc) const; + void NoteStoppedOnReport(u32 pc); + std::vector breakPoints_; TempBreakPoint tempBreakPoint_; - u32 breakSkipFirstAt_ = 0; - u64 breakSkipFirstTicks_ = 0; + // GetTicks() only moves when an instruction retires, so (pc, ticks) stops matching as soon as + // the instruction at pc has run, and a loop back to pc is a new execution. + bool skipFirstValid_ = false; + u32 skipFirstAt_ = 0; + u64 skipFirstTicks_ = 0; + // Set when the breakpoint we're resuming from is the one that stopped us, and so already + // logged and counted this execution. + bool skipFirstReport_ = false; + // The instruction whose breakpoint stopped us. Only the address: inside a JIT block the tick + // count isn't settled yet, so SetSkipFirst() pairs it with the ticks once we've stopped. + bool stoppedOnReport_ = false; + u32 stoppedOnReportAt_ = 0; std::vector memChecks_; std::vector memCheckRangesRead_; diff --git a/Core/MIPS/IR/IRFrontend.cpp b/Core/MIPS/IR/IRFrontend.cpp index f90ad5835e..9373319494 100644 --- a/Core/MIPS/IR/IRFrontend.cpp +++ b/Core/MIPS/IR/IRFrontend.cpp @@ -41,7 +41,7 @@ IRFrontend::IRFrontend(bool startDefaultPrefix) { // The debugger sets this so that "go" on a breakpoint will actually... go. // But if they reset, we can end up hitting it by mistake, since it's based on PC and ticks. - g_breakpoints.SetSkipFirst(0); + g_breakpoints.ClearSkipFirst(); } void IRFrontend::DoState(PointerWrap &p) { @@ -61,7 +61,7 @@ void IRFrontend::DoState(PointerWrap &p) { // The debugger sets this so that "go" on a breakpoint will actually... go. // But if they reset, we can end up hitting it by mistake, since it's based on PC and ticks. - g_breakpoints.SetSkipFirst(0); + g_breakpoints.ClearSkipFirst(); } bool IRFrontend::UpdateFCR31(u32 fcr31) { diff --git a/Core/MIPS/IR/IRInterpreter.cpp b/Core/MIPS/IR/IRInterpreter.cpp index 5ff0593c11..a5686aa448 100644 --- a/Core/MIPS/IR/IRInterpreter.cpp +++ b/Core/MIPS/IR/IRInterpreter.cpp @@ -53,11 +53,6 @@ alignas(16) static const uint32_t lowBytesMask[4] = { }; u32 IRRunBreakpoint(u32 pc) { - // Should we skip this breakpoint? - uint32_t skipFirst = g_breakpoints.CheckSkipFirst(); - if (skipFirst == pc || skipFirst == currentMIPS->pc) - return 0; - // Did we already hit one? if (coreState != CORE_RUNNING_CPU && coreState != CORE_NEXTFRAME) return 1; @@ -67,11 +62,6 @@ u32 IRRunBreakpoint(u32 pc) { } u32 IRRunMemCheck(u32 pc, u32 addr) { - // Should we skip this breakpoint? - uint32_t skipFirst = g_breakpoints.CheckSkipFirst(); - if (skipFirst == pc || skipFirst == currentMIPS->pc) - return 0; - // Did we already hit one? if (coreState != CORE_RUNNING_CPU && coreState != CORE_NEXTFRAME) return 1; diff --git a/Core/MIPS/MIPSTables.cpp b/Core/MIPS/MIPSTables.cpp index 7688e6e54b..d9a5221f42 100644 --- a/Core/MIPS/MIPSTables.cpp +++ b/Core/MIPS/MIPSTables.cpp @@ -1297,7 +1297,7 @@ static inline int GetGPRWriteTarget(const MIPSInstruction *instr, MIPSOpcode op) } static bool CheckExecBreakpoints(const MIPSState *mips, u32 pc) { - if (g_breakpoints.HasBreakPoints() && g_breakpoints.NeedsBreakCheckAt(pc) && g_breakpoints.CheckSkipFirst() != mips->pc) { + if (g_breakpoints.HasBreakPoints() && g_breakpoints.NeedsBreakCheckAt(pc)) { g_breakpoints.ExecBreakPoint(pc); // No temp-breakpoint cleanup needed here (the JIT path never had any either) - Core_Break() // drops it for us on the way into stepping, whichever breakpoint it was that stopped us. @@ -1309,7 +1309,7 @@ static bool CheckExecBreakpoints(const MIPSState *mips, u32 pc) { // instr must not be null - the callers skip these checks for undecodable opcodes. static bool CheckMemBreakpoints(const MIPSState *mips, const MIPSInstruction *instr, const MIPSOpcode op) { - if ((instr->flags & (IN_MEM | OUT_MEM)) != 0 && g_breakpoints.CheckSkipFirst() != mips->pc && instr->interpret != &Int_Syscall) { + if ((instr->flags & (IN_MEM | OUT_MEM)) != 0 && instr->interpret != &Int_Syscall) { // This is common for all IN_MEM/OUT_MEM funcs. int offset = (instr->flags & IS_VFPU) != 0 ? SignExtend16ToS32(op & 0xFFFC) : SignExtend16ToS32(op); u32 addr = (mips->r[_RS(op)] + offset) & 0xFFFFFFFC; @@ -1327,7 +1327,7 @@ static bool CheckMemBreakpoints(const MIPSState *mips, const MIPSInstruction *in } static bool CheckRegBreakpoints(const MIPSState *mips, const MIPSInstruction *instr, const MIPSOpcode op, u32 regBPMask) { - if ((instr->flags & (OUT_RT | OUT_RD | OUT_RA)) != 0 && g_breakpoints.CheckSkipFirst() != mips->pc) { + if ((instr->flags & (OUT_RT | OUT_RD | OUT_RA)) != 0) { int regTarget = GetGPRWriteTarget(instr, op); if (regTarget >= 0 && (regBPMask & (1u << regTarget)) != 0) { g_breakpoints.ExecRegBreakpoint(regTarget, mips->pc); diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index a230e5aeaf..90ec581f63 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -903,6 +903,55 @@ bool TestTempBreakpoints() { g_breakpoints.RemoveBreakPoint(kAddrA); EXPECT_FALSE(g_breakpoints.HasBreakPoints()); + // Resuming from a breakpoint must not pause there again, and must not report it again if it's + // the one that stopped us. A breakpoint we merely stepped onto still has to log and count. + // Nothing retires an instruction here unless we move downcount, so the ticks stay put. + { + auto hitsAt = [](u32 addr) -> int { + for (const BreakPoint &bp : g_breakpoints.GetBreakpoints()) { + if (bp.addr == addr) + return (int)bp.numHits; + } + return -1; + }; + + g_breakpoints.ClearSkipFirst(); + g_breakpoints.AddBreakPoint(kAddrA); + g_breakpoints.AddBreakPoint(kAddrB); + + EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE) != 0); + EXPECT_EQ_INT(hitsAt(kAddrA), 1); + + // Resuming off it. A step-over sets the skip twice for one resume, and that must not lose it. + g_breakpoints.SetSkipFirst(kAddrA); + g_breakpoints.SetSkipFirst(kAddrA); + EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrA), (int)BREAK_ACTION_NONE); + EXPECT_EQ_INT(hitsAt(kAddrA), 1); + + // Stepped onto B, never reported there: it logs and counts, only the pause is dropped. + g_breakpoints.SetSkipFirst(kAddrB); + EXPECT_EQ_INT((int)(g_breakpoints.ExecBreakPoint(kAddrB) & BREAK_ACTION_PAUSE), 0); + EXPECT_EQ_INT(hitsAt(kAddrB), 1); + + // An unrelated address still pauses and reports. + EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE) != 0); + EXPECT_EQ_INT(hitsAt(kAddrA), 2); + + // Step off A, which runs it, and land on A again (a one-instruction loop). That's a new + // execution, which nothing has reported yet. + g_breakpoints.SetSkipFirst(kAddrA); + EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrA), (int)BREAK_ACTION_NONE); + currentMIPS->downcount -= 1; + g_breakpoints.SetSkipFirst(kAddrA); + EXPECT_EQ_INT((int)(g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE), 0); + EXPECT_EQ_INT(hitsAt(kAddrA), 3); + currentMIPS->downcount += 1; + + g_breakpoints.ClearSkipFirst(); + g_breakpoints.RemoveBreakPoint(kAddrA); + g_breakpoints.RemoveBreakPoint(kAddrB); + } + g_symbolMap = nullptr; return true; }