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; }