Enable breakpoint processing when stepping

Core/Core.cpp

@@ -360,6 +360,10 @@ void Core_RunLoopUntil(u64 globalticks) {
 				break;
 			}
 			break;
+		case CORE_REENTER_DISPATCH:
+			_dbg_assert_(false);
+			coreState = CORE_STEPPING_CPU;
+			break;
 		}
 	}
 }
@@ -423,6 +427,7 @@ static void Core_PerformCPUStep(MIPSDebugInterface *cpu, CPUStepType stepType) {
 					breakpointAddress = currentPc + 2 * cpu->getInstructionSize(0);
 				}
 			}
+			// Add a temporary breakpoint.
 			g_breakpoints.AddBreakPoint(breakpointAddress, true);
 			Core_Resume();
 		} else {
@@ -481,6 +486,9 @@ static bool Core_ProcessStepping(MIPSDebugInterface *cpu) {
 	case CORE_RUNNING_GE:
 		// All good
 		break;
+	case CORE_REENTER_DISPATCH:
+		_dbg_assert_(false);
+		return true;
 	default:
 		// Nothing to do.
 		return true;

Core/Debugger/Breakpoints.cpp

@@ -144,6 +144,10 @@ bool BreakpointManager::RangeContainsBreakPoint(u32 addr, u32 size)
 }
 
 int BreakpointManager::AddBreakPoint(u32 addr, bool temp) {
+	if (addr & 3) {
+		WARN_LOG(Log::Debugger, "Breakpoint added at %08x will not be effective - unaligned address.", addr);
+	}
+
 	size_t bp = FindBreakpoint(addr, true, temp);
 	if (bp == INVALID_BREAKPOINT) {
 		BreakPoint pt;
@@ -659,19 +663,24 @@ 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) {
+	if ((info.result & BREAK_ACTION_PAUSE) && g_breakpoints.CheckSkipFirst() != pc) {
 		Core_Break(BreakReason::RegBreakpoint, pc);
 	}
 
 	return info.result;
 }
 
+void BreakpointManager::ClearSkipFirst() {
+	breakSkipFirstAt_ = 0;
+	breakSkipFirstTicks_ = 0;
+}
+
 void BreakpointManager::SetSkipFirst(u32 pc) {
 	breakSkipFirstAt_ = pc;
 	breakSkipFirstTicks_ = CoreTiming::GetTicks(currentMIPS);
 }
 
-u32 BreakpointManager::CheckSkipFirst() {
+u32 BreakpointManager::CheckSkipFirst() const {
 	u32 pc = breakSkipFirstAt_;
 	if (breakSkipFirstTicks_ == CoreTiming::GetTicks(currentMIPS))
 		return pc;

Core/Debugger/Breakpoints.h

@@ -213,7 +213,8 @@ public:
 	BreakAction ExecRegBreakpoint(int reg, u32 pc);
 
 	void SetSkipFirst(u32 pc);
-	u32 CheckSkipFirst();
+	u32 CheckSkipFirst() const;
+	void ClearSkipFirst();
 
 	// Includes uncached addresses.
 	std::vector<MemCheck> GetMemCheckRanges(bool write);

Core/MIPS/MIPSTables.cpp

@@ -1086,28 +1086,6 @@ void MIPSDisAsm(MIPSOpcode op, u32 pc, char *out, size_t outSize, bool tabsToSpa
 	}
 }
 
-static inline void InterpretInstruction(MIPSState *mips, const MIPSInstruction *instr, MIPSOpcode op) {
-	if (instr && instr->interpret) {
-		instr->interpret(mips, op);
-	} else {
-		Core_ExecException(mips->pc, mips->pc, ExecExceptionType::ILLEGAL);
-	}
-}
-
-inline int GetInstructionCycleEstimate(const MIPSInstruction *instr) {
-	return instr ? instr->flags.cycles : 1;
-}
-
-void MIPSInterpret(MIPSState *mips, MIPSOpcode op) {
-	const MIPSInstruction *instr = MIPSGetInstruction(op);
-	InterpretInstruction(mips, instr, op);
-}
-
-// See the declaration comment in MIPSTables.h.
-void CDECL MIPSInterpretTrampoline(MIPSOpcode op) {
-	MIPSInterpret(currentMIPS, op);
-}
-
 // This is the fast runloop for the interpreter.
 // When making changes, always make sure it's in sync with the fallback loop, RunUntilDowncountZeroWithChecks, which is
 // far less efficient but supports breakpoints of all kinds etc.
@@ -1157,6 +1135,84 @@ static inline int GetGPRWriteTarget(const MIPSInstruction *instr, MIPSOpcode op)
 	return -1;
 }
 
+static bool CheckExecBreakpoints(const MIPSState *mips, u32 pc) {
+	if (g_breakpoints.HasBreakPoints() && g_breakpoints.IsAddressBreakPoint(pc) && g_breakpoints.CheckSkipFirst() != mips->pc) {
+		g_breakpoints.ExecBreakPoint(pc);
+		if (coreState == CORE_STEPPING_CPU) {  // after ExecBreakPoint.
+			if (g_breakpoints.IsTempBreakPoint(mips->pc))
+				g_breakpoints.RemoveBreakPoint(mips->pc);
+			return true;
+		}
+	}
+	return false;
+}
+
+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) {
+		// 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;
+		int sz = MIPSGetMemoryAccessSize(op);
+
+		if ((instr->flags & IN_MEM) != 0)
+			g_breakpoints.ExecMemCheck(addr, false, sz, mips->pc, "interpret");
+		if ((instr->flags & OUT_MEM) != 0)
+			g_breakpoints.ExecMemCheck(addr, true, sz, mips->pc, "interpret");
+
+		// If it tripped, bail without running.
+		return coreState == CORE_STEPPING_CPU;
+	}
+	return false;
+}
+
+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) {
+		int regTarget = GetGPRWriteTarget(instr, op);
+		if (regTarget >= 0 && (regBPMask & (1u << regTarget)) != 0) {
+			g_breakpoints.ExecRegBreakpoint(regTarget, mips->pc);
+			// If it tripped, bail without running.
+			if (coreState == CORE_STEPPING_CPU)
+				return true;
+		}
+	}
+	return false;
+}
+
+static inline void InterpretInstruction(MIPSState *mips, const MIPSInstruction *instr, MIPSOpcode op) {
+	if (instr && instr->interpret) {
+		instr->interpret(mips, op);
+	} else {
+		Core_ExecException(mips->pc, mips->pc, ExecExceptionType::ILLEGAL);
+	}
+}
+
+inline int GetInstructionCycleEstimate(const MIPSInstruction *instr) {
+	return instr ? instr->flags.cycles : 1;
+}
+
+void MIPSInterpret(MIPSState *mips, MIPSOpcode op) {
+	const MIPSInstruction *instr = MIPSGetInstruction(op);
+
+	// Check/trigger breakpoints. NOTE: We set coreState to CORE_STEPPING_CPU if a breakpoint was hit (this function
+	// is used to skip delay slots), but unlike when running free, we don't avoid executing the instruction (so you
+	// can actually step through).
+	if (g_breakpoints.HasBreakPoints()) {
+		CheckExecBreakpoints(mips, mips->pc);
+	}
+	if (g_breakpoints.HasMemChecks()) {
+		CheckMemBreakpoints(mips, instr, op);
+	}
+	if (g_breakpoints.GetRegBreakpointMask()) {
+		CheckRegBreakpoints(mips, instr, op, g_breakpoints.GetRegBreakpointMask());
+	}
+	InterpretInstruction(mips, instr, op);
+}
+
+// See the declaration comment in MIPSTables.h.
+void CDECL MIPSInterpretTrampoline(MIPSOpcode op) {
+	MIPSInterpret(currentMIPS, op);
+}
+
 // This is the slow, feature-rich runloop for the interpreter.
 // When making changes, always make sure it's in sync with the fast loop, RunUntilDowncountZeroFast, which is
 // much more efficient.
@@ -1180,43 +1236,20 @@ static void RunUntilDowncountZeroWithChecks(MIPSState *mips, u64 globalTicks) {
 			// backends and the IR interpreter, via JitBreakpoint()/IRRunBreakpoint()) rather
 			// than breaking unconditionally - it's what actually respects BREAK_ACTION_LOG vs
 			// BREAK_ACTION_PAUSE (a log-only breakpoint, added with log=true and no/false
-			// enabled, must not stop execution here). The old code called Core_Break()
-			// directly whenever IsAddressBreakPoint() was true - true for *any* non-ignored
-			// breakpoint, log-only included - so log-only address breakpoints always paused
-			// too, contradicting their own documented behavior.
-			if (hasBPs && g_breakpoints.IsAddressBreakPoint(mips->pc) && g_breakpoints.CheckSkipFirst() != mips->pc) {
-				g_breakpoints.ExecBreakPoint(mips->pc);
-				// If it tripped, bail without running - same convention as memchecks/reg
-				// breakpoints below.
-				if (coreState == CORE_STEPPING_CPU) {
-					if (g_breakpoints.IsTempBreakPoint(mips->pc))
-						g_breakpoints.RemoveBreakPoint(mips->pc);
-					break;
-				}
-			}
-			if (hasMCs && (instr->flags & (IN_MEM | OUT_MEM)) != 0 && g_breakpoints.CheckSkipFirst() != mips->pc && 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;
-				int sz = MIPSGetMemoryAccessSize(op);
-
-				if ((instr->flags & IN_MEM) != 0)
-					g_breakpoints.ExecMemCheck(addr, false, sz, mips->pc, "interpret");
-				if ((instr->flags & OUT_MEM) != 0)
-					g_breakpoints.ExecMemCheck(addr, true, sz, mips->pc, "interpret");
-
+			// enabled, must not stop execution here).
+			bool breakExec = false;  // We use this mechanism so we always check all breakpoint types - they can overlap.
+			if (hasBPs && CheckExecBreakpoints(mips, mips->pc)) {
 				// If it tripped, bail without running.
-				if (coreState == CORE_STEPPING_CPU)
-					break;
+				breakExec = true;
 			}
-			if (regBPMask != 0 && (instr->flags & (OUT_RT | OUT_RD | OUT_RA)) != 0 && g_breakpoints.CheckSkipFirst() != mips->pc) {
-				int regTarget = GetGPRWriteTarget(instr, op);
-				if (regTarget >= 0 && (regBPMask & (1u << regTarget)) != 0) {
-					g_breakpoints.ExecRegBreakpoint(regTarget, mips->pc);
-					// If it tripped, bail without running - same convention as memchecks above.
-					if (coreState == CORE_STEPPING_CPU)
-						break;
-				}
+			if (hasMCs && CheckMemBreakpoints(mips, instr, op)) {
+				breakExec = true;
+			}
+			if (regBPMask != 0 && CheckRegBreakpoints(mips, instr, op, regBPMask)) {
+				breakExec = true;
+			}
+			if (breakExec) {
+				break;
 			}
 
 			const bool wasInDelaySlot = mips->inDelaySlot;

UI/ImDebugger/ImDebugger.cpp

@@ -1034,7 +1034,7 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) {
 			ImGui::TableHeadersRow();
 
 			for (int i = 0; i < (int)bps.size(); i++) {
-				auto &bp = bps[i];
+				BreakPoint &bp = bps[i];
 				bool temp = bp.temporary;
 				if (temp) {
 					continue;
@@ -1048,6 +1048,7 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) {
 				if (ImGui::Selectable("", cfg.selectedBreakpoint == i, ImGuiSelectableFlags_SpanAllColumns | ImGuiSelectableFlags_AllowOverlap) && !bp.temporary) {
 					cfg.selectedBreakpoint = i;
 					cfg.selectedMemCheck = -1;
+					cfg.selectedRegBreakpoint = -1;
 				}
 				ImGui::SameLine();
 				ImGui::CheckboxFlags("##enabled", (int *)&bp.action, (int)BREAK_ACTION_PAUSE);
@@ -1087,6 +1088,7 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) {
 				if (ImGui::Selectable("##memcheck", cfg.selectedMemCheck == i, ImGuiSelectableFlags_SpanAllColumns | ImGuiSelectableFlags_AllowOverlap)) {
 					cfg.selectedBreakpoint = -1;
 					cfg.selectedMemCheck = i;
+					cfg.selectedRegBreakpoint = -1;
 				}
 				ImGui::SameLine();
 				ImGui::CheckboxFlags("##enabled", (int *)&mc.action, (int)BREAK_ACTION_PAUSE);
@@ -1107,6 +1109,37 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) {
 				ImGui::PopID();
 			}
 
+			// Finally, list register breakpoints.
+			for (auto &rbp : g_breakpoints.GetRegBreakpoints()) {
+				ImGui::TableNextRow();
+				ImGui::TableNextColumn();
+				ImGui::PushID(&rbp - &g_breakpoints.GetRegBreakpoints()[0] + 20000);
+				if (ImGui::Selectable("##regbp", cfg.selectedRegBreakpoint == &rbp - &g_breakpoints.GetRegBreakpoints()[0], ImGuiSelectableFlags_SpanAllColumns | ImGuiSelectableFlags_AllowOverlap)) {
+					cfg.selectedBreakpoint = -1;
+					cfg.selectedMemCheck = -1;
+					cfg.selectedRegBreakpoint = &rbp - &g_breakpoints.GetRegBreakpoints()[0];
+				}
+				ImGui::SameLine();
+				ImGui::CheckboxFlags("", (int *)&rbp.result, BREAK_ACTION_PAUSE);
+				ImGui::TableNextColumn();
+				ImGui::TextUnformatted("Reg");
+				ImGui::TableNextColumn();
+				ImGui::TextUnformatted(mipsDebug->GetRegName(0, rbp.reg).c_str());
+				ImGui::TableNextColumn();
+				ImGui::TextUnformatted("-");  // size/label
+				ImGui::TableNextColumn();
+				ImGui::TextUnformatted("-");  // opcode
+				ImGui::TableNextColumn();
+				if (rbp.hasCond) {
+					ImGui::TextUnformatted(rbp.cond.expressionString.c_str());
+				} else {
+					ImGui::TextUnformatted("-");  // condition
+				}
+				ImGui::TableNextColumn();
+				ImGui::Text("%d", rbp.numHits);
+				ImGui::PopID();
+			}
+
 			ImGui::EndTable();
 		}
 

UI/ImDebugger/ImDebugger.h

@@ -106,6 +106,7 @@ struct ImConfig {
 	int selectedFramebuffer = -1;
 	int selectedBreakpoint = -1;
 	int selectedMemCheck = -1;
+	int selectedRegBreakpoint = -1;
 	int selectedAtracCtx = 0;
 	int selectedMp3Ctx = 0;
 	int selectedAacCtx = 0;