From d385c86a98e7d37b3bfdcfae404d0d7255a78df3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 17 Aug 2026 15:36:55 +0200 Subject: [PATCH] Fix breakpoints being swallowed when you step onto them Set two breakpoints four bytes apart, both logging, run into the first, then press Next: the second one never logs, however many times you step. Reproduced on both the interpreter and the JIT. The skip-first mechanism was doing two different jobs with one marker. Every resume and every step recorded the address it started from, and any breakpoint check at that address was suppressed outright. That's right for the breakpoint you're parked on - you have to be able to get off it - but stepping *onto* an address is not the same as having reported the breakpoint there, and the next step suppressed it before it ever logged. Split into the two things that were being conflated: - resumedFrom_ is where the current run or step started. It only drops the pause, not the log or the hit count. It still covers the temporary breakpoint, which is what makes "run to here" work when you're already on that address. - reported_ is the breakpoint we already logged and counted. Reporting stops the CPU before the instruction runs, so the resume that follows arrives at the same pending execution and must not report it twice. Both are (address, tick count) pairs, which identify one pending execution of one instruction: ticks only move when the CPU retires an instruction, so the marker stops matching as soon as it runs, and a breakpoint in a loop still fires every iteration. reported_ can't be armed where the report happens, though. Under a JIT that's inside a compiled block whose cycles are already accounted for, so the tick count there isn't the settled one we see on the way back in - arming it there double-logged the breakpoint under -j. So the report just records the address, and NotifyResumingFrom() turns it into a real marker once the CPU has stopped. That also has to be idempotent: a step-over arms its temporary breakpoint and then calls Core_Resume(), which notifies a second time. MemCheck::Action() no longer pauses by itself - the caller decides, the same way ExecBreakPoint() already did, so all three breakpoint kinds share the handling. Verified on both backends: two adjacent breakpoints now log once each while stepping (was one log total), stepping off a breakpoint still doesn't re-log it (was two under -j), step-over still skips the call and logs a breakpoint at the address it lands on, and a breakpoint in a loop reports once per iteration. pspautotests 314/314. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9 --- Core/Core.cpp | 10 +- Core/Debugger/Breakpoints.cpp | 124 +++++++++++++----- Core/Debugger/Breakpoints.h | 84 +++++++++--- .../Debugger/WebSocket/SteppingSubscriber.cpp | 2 +- Core/MIPS/ARM/ArmJit.cpp | 4 +- Core/MIPS/ARM64/Arm64Jit.cpp | 4 +- Core/MIPS/IR/IRFrontend.cpp | 4 +- Core/MIPS/fake/FakeJit.cpp | 2 +- Core/MIPS/x86/Jit.cpp | 4 +- UI/ImDebugger/ImDisasmView.cpp | 2 +- unittest/UnitTest.cpp | 63 ++++++--- 11 files changed, 212 insertions(+), 91 deletions(-) diff --git a/Core/Core.cpp b/Core/Core.cpp index 05ca633c8f..0dc1e84425 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -417,7 +417,7 @@ static void Core_PerformCPUStep(MIPSDebugInterface *cpu, CPUStepType stepType) { { u32 currentPc = cpu->GetPC(); // If the current PC is on a breakpoint, the user still wants the step to happen. - g_breakpoints.SetSkipFirst(currentPc); + g_breakpoints.NotifyResumingFrom(currentPc); currentMIPS->SingleStep(); CoreTiming::Advance(currentMIPS); break; @@ -426,7 +426,7 @@ static void Core_PerformCPUStep(MIPSDebugInterface *cpu, CPUStepType stepType) { { u32 currentPc = cpu->GetPC(); - g_breakpoints.SetSkipFirst(currentPc); + g_breakpoints.NotifyResumingFrom(currentPc); MIPSAnalyst::MipsOpcodeInfo info = MIPSAnalyst::GetOpcodeInfo(cpu, cpu->GetPC()); // TODO: Doing a step over in a delay slot is a bit .. unclear. Maybe just do a single step. @@ -588,8 +588,8 @@ void Core_Break(BreakReason reason, u32 relatedAddress) { mipsTracer.stop_tracing(); // Whatever resume/step was in flight is over, so its "don't re-trigger the breakpoint we're - // sitting on" marker must not outlive it - see BreakpointManager::SetSkipFirst(). - g_breakpoints.ClearSkipFirst(); + // sitting on" marker must not outlive it - see BreakpointManager::NotifyResumingFrom(). + g_breakpoints.ClearResumeMarker(); // Execution stopped, so whatever step-over/step-out/run-until was in flight is over - either // it just completed, or something else (a breakpoint, a memcheck, the user hitting pause) @@ -613,7 +613,7 @@ void Core_Break(BreakReason reason, u32 relatedAddress) { void Core_Resume() { // If the current PC is on a breakpoint, the user doesn't want to do nothing. if (currentMIPS) { - g_breakpoints.SetSkipFirst(currentMIPS->pc); + g_breakpoints.NotifyResumingFrom(currentMIPS->pc); } // Handle resuming from GE. diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index d981bdbca1..e6af035830 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/Core/Debugger/Breakpoints.cpp @@ -62,9 +62,6 @@ BreakAction MemCheck::Apply(u32 addr, bool write, int size, u32 pc) { BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char *reason) { // Conditions have always already been checked if we get here. Log(addr, write, size, pc, reason); - if (action & BREAK_ACTION_PAUSE) { - Core_Break(BreakReason::MemoryBreakpoint, start); - } return action; } @@ -327,12 +324,11 @@ void BreakpointManager::ChangeBreakPointLogFormat(u32 addr, const std::string &f BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { if (!anyBreakPoints_) return BREAK_ACTION_NONE; - // Checked here rather than at each call site, so no path can forget it and log/count a hit for - // a breakpoint we're only just stepping off. See SetSkipFirst(). - if (ShouldSkipBreakpoint(addr)) - return BREAK_ACTION_NONE; BreakAction result = BREAK_ACTION_NONE; + // Checked here rather than at each call site, so no path can forget it and log/count a hit + // twice for the one execution of this instruction. + bool reported = AlreadyReportedAt(addr); size_t bp = FindBreakpoint(addr); if (bp != INVALID_BREAKPOINT) { @@ -343,8 +339,9 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { if (info.hasCond) condPassed = info.cond.Evaluate() != 0; - if (condPassed) { + if (condPassed && !reported) { ++info.numHits; + reported = true; if (action & BREAK_ACTION_LOG) { if (info.logFormat.empty()) { @@ -368,8 +365,22 @@ BreakAction BreakpointManager::ExecBreakPoint(u32 addr) { } } + // Nothing may pause us on the instruction we're resuming or stepping off, or we could never + // get off it. Deliberately separate from the reporting suppression above: this applies even to + // a breakpoint that was never reported (one you stepped onto), and it only drops the pause - + // the log and the hit count still happen. It covers the temporary breakpoint too, which is + // what makes "run to here" work when you're already parked on that address; a step can't get + // stuck on it, since the marker stops matching as soon as the instruction retires. + if (ShouldSuppressPauseAt(addr)) + result &= ~BREAK_ACTION_PAUSE; + if (result & BREAK_ACTION_PAUSE) { + // Only if something was actually reported for this instruction - stopping here for the + // temporary breakpoint alone must not suppress a breakpoint added while parked here. Core_Break(BreakReason::CpuBreakpoint, addr); + // After Core_Break(), which clears the previous stop's marker. + if (reported) + NoteStoppedOnReported(addr); System_Notify(SystemNotification::DISASSEMBLY); } @@ -525,8 +536,8 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u { if (!anyMemChecks_) return BREAK_ACTION_NONE; - // See SetSkipFirst() - same reason as in ExecBreakPoint(). - if (ShouldSkipBreakpoint(pc)) + // Same two-part suppression as ExecBreakPoint(), keyed on the instruction doing the access. + if (AlreadyReportedAt(pc)) return BREAK_ACTION_NONE; MemCheck *check = FindMemCheckInRange(address, size); if (check) { @@ -535,14 +546,25 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u return applyAction; MemCheck copy = *check; - return copy.Action(address, write, size, pc, reason); + BreakAction result = copy.Action(address, write, size, pc, reason); + if (ShouldSuppressPauseAt(pc)) + result &= ~BREAK_ACTION_PAUSE; + if (result & BREAK_ACTION_PAUSE) { + Core_Break(BreakReason::MemoryBreakpoint, copy.start); + NoteStoppedOnReported(pc); + } + return result; } return BREAK_ACTION_NONE; } BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc) { - if (ShouldSkipBreakpoint(pc)) + // Same two-part suppression as ExecBreakPoint(), keyed on the instruction doing the access. + if (AlreadyReportedAt(pc)) return BREAK_ACTION_NONE; + // pc moves to the delay slot below, but the markers have to stay keyed on the instruction the + // resume/step started from, which is the branch. + const u32 execPc = 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. int size = MIPSAnalyst::OpMemoryAccessSize(pc); @@ -570,7 +592,14 @@ BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc) { return applyAction; MemCheck copy = *check; - return copy.Action(address, write, size, pc, "CPU"); + BreakAction result = copy.Action(address, write, size, pc, "CPU"); + if (ShouldSuppressPauseAt(execPc)) + result &= ~BREAK_ACTION_PAUSE; + if (result & BREAK_ACTION_PAUSE) { + Core_Break(BreakReason::MemoryBreakpoint, copy.start); + NoteStoppedOnReported(execPc); + } + return result; } } return BREAK_ACTION_NONE; @@ -703,9 +732,9 @@ BreakAction BreakpointManager::ExecRegBreakpoint(int reg, u32 pc) { if (info.hasCond && !info.cond.Evaluate()) return BREAK_ACTION_NONE; - // Before the log and the hit count, not just the pause - stepping off a log+pause register - // breakpoint used to print it a second time and count it twice. See SetSkipFirst(). - if (ShouldSkipBreakpoint(pc)) + // Same two-part suppression as ExecBreakPoint(). Covering the log and the hit count, not just + // the pause - stepping off a log+pause register breakpoint used to print it a second time. + if (AlreadyReportedAt(pc)) return BREAK_ACTION_NONE; ++info.numHits; @@ -719,35 +748,58 @@ 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) { + + BreakAction result = info.result; + if (ShouldSuppressPauseAt(pc)) + result &= ~BREAK_ACTION_PAUSE; + if (result & BREAK_ACTION_PAUSE) { Core_Break(BreakReason::RegBreakpoint, pc); + NoteStoppedOnReported(pc); } - return info.result; + return result; } -void BreakpointManager::ClearSkipFirst() { - skipFirst_.valid = false; +void BreakpointManager::ClearResumeMarker() { + resumedFrom_.Clear(); + // Cleared on the way into stopping, and set again right after by whichever breakpoint reported, + // so it always describes why we are stopped *now*. + stoppedOnReported_.valid = false; } -void BreakpointManager::SetSkipFirst(u32 addr) { - // Nothing to suppress if no breakpoint machinery is armed, and not arming it needlessly keeps - // a stale marker from ever being able to swallow a breakpoint set later. - if (!anyBreakPoints_ && !anyMemChecks_ && regBreakpointMask_ == 0) { - skipFirst_.valid = false; - return; - } - skipFirst_.valid = true; - skipFirst_.addr = addr; - skipFirst_.ticks = CoreTiming::GetTicks(currentMIPS); +void BreakpointManager::ResetExecutionMarkers() { + resumedFrom_.Clear(); + reported_.Clear(); + stoppedOnReported_.valid = false; } -bool BreakpointManager::ShouldSkipBreakpoint(u32 addr) const { - if (!skipFirst_.valid) - return false; - if (skipFirst_.addr != addr && skipFirst_.addr != currentMIPS->pc) - return false; - return skipFirst_.ticks == CoreTiming::GetTicks(currentMIPS); +void BreakpointManager::NotifyResumingFrom(u32 addr) { + const u64 ticks = CoreTiming::GetTicks(currentMIPS); + resumedFrom_.Arm(addr, ticks); + // Only suppress reporting if we're resuming off the very breakpoint that reported. Stopping + // somewhere else and stepping onto an address with a breakpoint is a different thing: nothing + // has been reported there, so it still has to log and count when execution moves on. + // Deliberately does not consume stoppedOnReported_ - a single resume can come through here more + // than once (a step-over arms its temporary breakpoint and then calls Core_Resume(), which + // notifies again), and the second call must arrive at the same answer as the first. Core_Break() + // is what clears it, on the way into the next stop. + if (stoppedOnReported_.valid && stoppedOnReported_.addr == addr) + reported_.Arm(addr, ticks); + else + reported_.Clear(); +} + +bool BreakpointManager::ShouldSuppressPauseAt(u32 addr) const { + return resumedFrom_.Matches(addr, CoreTiming::GetTicks(currentMIPS)); +} + +bool BreakpointManager::AlreadyReportedAt(u32 addr) const { + return reported_.Matches(addr, CoreTiming::GetTicks(currentMIPS)); +} + +void BreakpointManager::NoteStoppedOnReported(u32 addr) { + stoppedOnReported_.valid = true; + stoppedOnReported_.addr = addr; } static MemCheck NotCached(MemCheck mc) { diff --git a/Core/Debugger/Breakpoints.h b/Core/Debugger/Breakpoints.h index 05289b1477..423926ab39 100644 --- a/Core/Debugger/Breakpoints.h +++ b/Core/Debugger/Breakpoints.h @@ -38,6 +38,16 @@ static inline BreakAction operator | (const BreakAction &lhs, const BreakAction return BreakAction((u32)lhs | (u32)rhs); } +static inline BreakAction operator ~ (const BreakAction &v) { + return BreakAction(~(u32)v); +} + +// For dropping an action that isn't wanted after all, e.g. result &= ~BREAK_ACTION_PAUSE. +static inline BreakAction &operator &= (BreakAction &lhs, const BreakAction &rhs) { + lhs = BreakAction((u32)lhs & (u32)rhs); + return lhs; +} + struct BreakPointCond { DebugInterface *debug = nullptr; PostfixExpression expression; @@ -125,6 +135,8 @@ struct MemCheck { // Called on the stored memcheck (affects numHits, etc.) BreakAction Apply(u32 addr, bool write, int size, u32 pc); // Called on a copy. + // Logs the hit and returns the action. Does not pause - that's the caller's call, since it's + // the one that knows whether we're just resuming off this instruction. BreakAction Action(u32 addr, bool write, int size, u32 pc, const char *reason); void Log(u32 addr, bool write, int size, u32 pc, const char *reason) const; @@ -251,20 +263,15 @@ public: // instruction that would write to reg - does not itself execute the instruction. BreakAction ExecRegBreakpoint(int reg, u32 pc); - // While the CPU sits on a breakpoint, resuming or stepping must not immediately re-trigger it, - // or you could never get off it. So a resume/step records the address it starts from, and - // breakpoint checks at that address are suppressed until the CPU retires an instruction. - // - // "Retires an instruction" is the tick count changing. CoreTiming::GetTicks() is continuous - // across CoreTiming::Advance() (it's globalTimer + slicelength - downcount, and Advance moves - // both by the same amount), so it only moves when the CPU actually consumes cycles. That makes - // the suppression cover exactly the one instruction it's meant to: come back around a loop to - // the same address and the breakpoint fires normally. - void SetSkipFirst(u32 addr); - void ClearSkipFirst(); - // addr is the instruction being checked, which under a JIT isn't necessarily currentMIPS->pc - // yet, so both are compared. - bool ShouldSkipBreakpoint(u32 addr) const; + // Sitting on a breakpoint and resuming or stepping needs two different things suppressed, and + // conflating them is a bug in both directions - see PendingExec and the two markers below. + // Call this when starting to run or step, with the address execution starts from. + void NotifyResumingFrom(u32 addr); + // The run or step that armed the above is over (we stopped for some reason). + void ClearResumeMarker(); + // Both markers are (address, tick count) pairs, so a reset or a savestate load can leave one + // that happens to match again. Call this when execution discontinuously jumps like that. + void ResetExecutionMarkers(); // Includes uncached addresses. std::vector GetMemCheckRanges(bool write); @@ -312,15 +319,52 @@ private: std::atomic anyMemChecks_; std::atomic regBreakpointMask_; - std::vector breakPoints_; - TempBreakPoint tempBreakPoint_; - // See SetSkipFirst(). Not keyed on address 0 meaning "none" - a breakpoint at 0 would then be - // permanently suppressed, which is how this used to be cleared. - struct { + // Identifies one pending execution of one instruction: the address, plus the tick count at the + // moment it is about to run. CoreTiming::GetTicks() is continuous across CoreTiming::Advance() + // (it's globalTimer + slicelength - downcount, and Advance moves both by the same amount), so + // it only changes when the CPU actually retires an instruction. That makes (addr, ticks) stop + // matching as soon as the instruction runs - come around a loop to the same address and it's a + // different pending execution, which is what makes a breakpoint in a loop keep firing. + // Has an explicit valid flag rather than treating address 0 as "none", or a breakpoint at 0 + // would be permanently suppressed. + struct PendingExec { bool valid = false; u32 addr = 0; u64 ticks = 0; - } skipFirst_; + + void Arm(u32 a, u64 t) { valid = true; addr = a; ticks = t; } + void Clear() { valid = false; } + bool Matches(u32 a, u64 t) const { return valid && addr == a && ticks == t; } + }; + + // Helpers over the markers below. addr is the instruction being checked - for a memcheck + // that's the instruction performing the access, not the address being accessed. + bool ShouldSuppressPauseAt(u32 addr) const; + bool AlreadyReportedAt(u32 addr) const; + void NoteStoppedOnReported(u32 addr); + + std::vector breakPoints_; + TempBreakPoint tempBreakPoint_; + // Where the current run or step started. A breakpoint there must not *pause*, or you could + // never get off a breakpoint you're stopped on. It must still log and count though: arriving + // at an address by stepping is not the same as having reported the breakpoint there, and + // treating it as such is what used to silently swallow a breakpoint you stepped onto. + PendingExec resumedFrom_; + // The breakpoint we already reported (logged and counted). Reporting stops the CPU before the + // instruction runs, so the resume or step that follows arrives at the same pending execution + // and must not report it a second time. + PendingExec reported_; + // How reported_ gets armed. It can't be armed where the report happens: under a JIT that's + // inside a compiled block, whose cycles are already accounted for, so the tick count there + // isn't the settled one we'll see on the way back in. So the report just records the address, + // and NotifyResumingFrom() turns it into a real (addr, ticks) marker once the CPU has stopped + // and the tick count means something again. The address is what distinguishes "stopped here + // because this breakpoint reported" from "a step happened to land here", which must still + // report when execution moves on. + struct { + bool valid = false; + u32 addr = 0; + } stoppedOnReported_; std::vector memChecks_; std::vector memCheckRangesRead_; diff --git a/Core/Debugger/WebSocket/SteppingSubscriber.cpp b/Core/Debugger/WebSocket/SteppingSubscriber.cpp index b25f52e21a..138cf0394c 100644 --- a/Core/Debugger/WebSocket/SteppingSubscriber.cpp +++ b/Core/Debugger/WebSocket/SteppingSubscriber.cpp @@ -318,7 +318,7 @@ void WebSocketSteppingState::PrepareResume() { currentMIPS->SingleStep(); } else { // If the current PC is on a breakpoint, the user doesn't want to do nothing. - g_breakpoints.SetSkipFirst(currentMIPS->pc); + g_breakpoints.NotifyResumingFrom(currentMIPS->pc); } } diff --git a/Core/MIPS/ARM/ArmJit.cpp b/Core/MIPS/ARM/ArmJit.cpp index 8e3f9b4678..1185059208 100644 --- a/Core/MIPS/ARM/ArmJit.cpp +++ b/Core/MIPS/ARM/ArmJit.cpp @@ -115,7 +115,7 @@ ArmJit::ArmJit(MIPSState *mipsState) : blocks(mipsState, this), gpr(mipsState, & // 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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } ArmJit::~ArmJit() { @@ -142,7 +142,7 @@ void ArmJit::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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } void ArmJit::UpdateFCR31() { diff --git a/Core/MIPS/ARM64/Arm64Jit.cpp b/Core/MIPS/ARM64/Arm64Jit.cpp index 5c92b99c92..b0de9786af 100644 --- a/Core/MIPS/ARM64/Arm64Jit.cpp +++ b/Core/MIPS/ARM64/Arm64Jit.cpp @@ -114,7 +114,7 @@ Arm64Jit::Arm64Jit(MIPSState *mipsState) : blocks(mipsState, this), gpr(mipsStat // 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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } Arm64Jit::~Arm64Jit() { @@ -142,7 +142,7 @@ void Arm64Jit::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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } void Arm64Jit::UpdateFCR31() { diff --git a/Core/MIPS/IR/IRFrontend.cpp b/Core/MIPS/IR/IRFrontend.cpp index b4a44a54e3..558a815967 100644 --- a/Core/MIPS/IR/IRFrontend.cpp +++ b/Core/MIPS/IR/IRFrontend.cpp @@ -40,7 +40,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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } void IRFrontend::DoState(PointerWrap &p) { @@ -58,7 +58,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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } void IRFrontend::FlushAll() { diff --git a/Core/MIPS/fake/FakeJit.cpp b/Core/MIPS/fake/FakeJit.cpp index af42f3d664..e8eb1964d9 100644 --- a/Core/MIPS/fake/FakeJit.cpp +++ b/Core/MIPS/fake/FakeJit.cpp @@ -64,7 +64,7 @@ void FakeJit::DoState(PointerWrap &p) { // The debugger sets this so that "go" on a breakpoint will actually... go. // But if they load a state, we can end up hitting it by mistake, since it's based on PC and ticks. - g_breakpoints.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } // This is here so the savestate matches between jit and non-jit. diff --git a/Core/MIPS/x86/Jit.cpp b/Core/MIPS/x86/Jit.cpp index 61bffec86b..699e4f9cae 100644 --- a/Core/MIPS/x86/Jit.cpp +++ b/Core/MIPS/x86/Jit.cpp @@ -126,7 +126,7 @@ Jit::Jit(MIPSState *mipsState) // 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.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } Jit::~Jit() { @@ -152,7 +152,7 @@ void Jit::DoState(PointerWrap &p) { // The debugger sets this so that "go" on a breakpoint will actually... go. // But if they load a state, we can end up hitting it by mistake, since it's based on PC and ticks. - g_breakpoints.ClearSkipFirst(); + g_breakpoints.ResetExecutionMarkers(); } void Jit::UpdateFCR31() { diff --git a/UI/ImDebugger/ImDisasmView.cpp b/UI/ImDebugger/ImDisasmView.cpp index e11d219d27..e87604b2eb 100644 --- a/UI/ImDebugger/ImDisasmView.cpp +++ b/UI/ImDebugger/ImDisasmView.cpp @@ -772,7 +772,7 @@ void ImDisasmView::PopupMenu(MIPSState *mips, ImControl &control) { } if (ImGui::MenuItem("Run to here")) { g_breakpoints.SetTempBreakPoint(curAddress_); - g_breakpoints.SetSkipFirst(curAddress_); + g_breakpoints.NotifyResumingFrom(curAddress_); if (Core_IsStepping()) { Core_Resume(); } diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 9cfa822545..63b00cf4e3 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -1068,32 +1068,57 @@ bool TestTempBreakpoints() { g_breakpoints.RemoveBreakPoint(kAddrA); EXPECT_FALSE(g_breakpoints.HasBreakPoints()); - // The "don't immediately re-trigger the breakpoint we're parked on" marker. Getting this wrong - // in the lenient direction silently swallows breakpoints, so pin the behaviour down. + // Resuming or stepping off a breakpoint needs two different things suppressed, and conflating + // them is a bug in both directions - either the breakpoint you're parked on gets logged twice, + // or a breakpoint you stepped onto is swallowed and never logged at all. No CPU is running + // here, so the tick count stays put and every check below is the same pending execution. { - g_breakpoints.ClearSkipFirst(); - // Nothing armed - nothing to suppress, so it must not arm and sit there waiting to swallow - // a breakpoint added later. - g_breakpoints.SetSkipFirst(kAddrA); - EXPECT_FALSE(g_breakpoints.ShouldSkipBreakpoint(kAddrA)); + auto hitsAt = [](u32 addr) -> int { + for (const BreakPoint &bp : g_breakpoints.GetBreakpoints()) { + if (bp.addr == addr) + return (int)bp.numHits; + } + return -1; + }; + g_breakpoints.ResetExecutionMarkers(); g_breakpoints.AddBreakPoint(kAddrA); - g_breakpoints.SetSkipFirst(kAddrA); - EXPECT_TRUE(g_breakpoints.ShouldSkipBreakpoint(kAddrA)); - // Only the address it was set for. - EXPECT_FALSE(g_breakpoints.ShouldSkipBreakpoint(kAddrB)); + g_breakpoints.AddBreakPoint(kAddrB); - // While suppressed, the breakpoint must not fire, log, or count a hit. - EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrA), (int)BREAK_ACTION_NONE); - EXPECT_EQ_INT((int)g_breakpoints.GetBreakpoints()[0].numHits, 0); - - g_breakpoints.ClearSkipFirst(); - EXPECT_FALSE(g_breakpoints.ShouldSkipBreakpoint(kAddrA)); - // ...and once it's cleared, it fires normally again. + // Running into a breakpoint reports it and pauses. EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE) != 0); - EXPECT_EQ_INT((int)g_breakpoints.GetBreakpoints()[0].numHits, 1); + EXPECT_EQ_INT(hitsAt(kAddrA), 1); + // Stepping off it must neither pause again (you'd never get anywhere) nor report again - + // the instruction hasn't run yet, so this is still the same execution of it. + g_breakpoints.NotifyResumingFrom(kAddrA); + EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrA), (int)BREAK_ACTION_NONE); + EXPECT_EQ_INT(hitsAt(kAddrA), 1); + + // Whereas a breakpoint we merely stepped *onto* was never reported, so it still has to log + // and count. Only its pause is dropped, so the step can complete. Suppressing this one + // entirely is what made the second of two adjacent breakpoints silently vanish. + g_breakpoints.NotifyResumingFrom(kAddrB); + EXPECT_EQ_INT((int)(g_breakpoints.ExecBreakPoint(kAddrB) & BREAK_ACTION_PAUSE), 0); + EXPECT_EQ_INT(hitsAt(kAddrB), 1); + + // Neither marker applies to an unrelated address, which keeps pausing and reporting. + EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrA) & BREAK_ACTION_PAUSE) != 0); + EXPECT_EQ_INT(hitsAt(kAddrA), 2); + + // Actually running into B (resuming from A, so nothing is suppressed at B) pauses there, + // and resuming off that reports nothing new - the suppression follows the breakpoint we + // stopped on, not just whatever address we happen to be sitting at. + g_breakpoints.NotifyResumingFrom(kAddrA); + EXPECT_TRUE((g_breakpoints.ExecBreakPoint(kAddrB) & BREAK_ACTION_PAUSE) != 0); + EXPECT_EQ_INT(hitsAt(kAddrB), 2); + g_breakpoints.NotifyResumingFrom(kAddrB); + EXPECT_EQ_INT((int)g_breakpoints.ExecBreakPoint(kAddrB), (int)BREAK_ACTION_NONE); + EXPECT_EQ_INT(hitsAt(kAddrB), 2); + + g_breakpoints.ResetExecutionMarkers(); g_breakpoints.RemoveBreakPoint(kAddrA); + g_breakpoints.RemoveBreakPoint(kAddrB); } g_symbolMap = nullptr;