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;