Fix step over, other stepping issues

Core/Core.cpp

@@ -56,15 +56,15 @@
 static std::mutex g_stepMutex;
 struct StepCommand {
 	CPUStepType type;
-	int param;
+	int stepSize;
 	const char *reason;
 	u32 relatedAddr;
 	bool empty() const {
 		return type == CPUStepType::None;
 	}
 	void clear() {
 		type = CPUStepType::None;
-		param = 0;
+		stepSize = 0;
 		reason = "";
 		relatedAddr = 0;
 	}
@@ -243,11 +243,12 @@ bool Core_RequestSingleStep(CPUStepType type, int stepSize) {
 		ERROR_LOG(Log::CPU, "Can't submit two steps in one frame");
 		return false;
 	}
+	// Out-steps don't need a size.
+	_dbg_assert_(stepSize != 0 || type == CPUStepType::Out);
 	g_stepCommand = { type, stepSize };
 	return true;
 }
 
-// See comment in header.
 // Handles more advanced step types (used by the debugger).
 // stepSize is to support stepping through compound instructions like fused lui+ladd (li).
 // Yes, our disassembler does support those.
@@ -264,16 +265,18 @@ static void Core_PerformStep(MIPSDebugInterface *cpu, CPUStepType stepType, int
 		for (int i = 0; i < (int)(newAddress - currentPc) / 4; i++) {
 			currentMIPS->SingleStep();
 		}
-		return;
+		break;
 	}
 	case CPUStepType::Over:
 	{
 		u32 currentPc = cpu->GetPC();
 		u32 breakpointAddress = currentPc + stepSize;
 
 		g_breakpoints.SetSkipFirst(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.
+
 		if (info.isBranch) {
 			if (info.isConditional == false) {
 				if (info.isLinkedBranch) { // jal, jalr
@@ -290,10 +293,14 @@ static void Core_PerformStep(MIPSDebugInterface *cpu, CPUStepType stepType, int
 					breakpointAddress = currentPc + 2 * cpu->getInstructionSize(0);
 				}
 			}
+			g_breakpoints.AddBreakPoint(breakpointAddress, true);
+			Core_Resume();
+		} else {
+			// If not a branch, just do a simple single-step, no point in involving the breakpoint machinery.
+			for (int i = 0; i < (int)(breakpointAddress - currentPc) / 4; i++) {
+				currentMIPS->SingleStep();
+			}
 		}
-
-		g_breakpoints.AddBreakPoint(breakpointAddress, true);
-		Core_Resume();
 		break;
 	}
 	case CPUStepType::Out:
@@ -361,7 +368,7 @@ void Core_ProcessStepping(MIPSDebugInterface *cpu) {
 	Core_ResetException();
 
 	if (!g_stepCommand.empty()) {
-		Core_PerformStep(cpu, g_stepCommand.type, g_stepCommand.param);
+		Core_PerformStep(cpu, g_stepCommand.type, g_stepCommand.stepSize);
 		if (g_stepCommand.type == CPUStepType::Into) {
 			// We're already done. The other step types will resume the CPU.
 			System_Notify(SystemNotification::DISASSEMBLY_AFTERSTEP);
@@ -424,9 +431,10 @@ void Core_Break(const char *reason, u32 relatedAddress) {
 			case CPUStepType::Out:
 				// Allow overwriting the command.
 				break;
+			default:
+				ERROR_LOG(Log::CPU, "Core_Break called with a step-command already in progress: %s", g_stepCommand.reason);
+				return;
 			}
-			ERROR_LOG(Log::CPU, "Core_Break called with a step-command already in progress: %s", g_stepCommand.reason);
-			return;
 		}
 		mipsTracer.stop_tracing();
 		g_stepCommand.type = CPUStepType::None;

Core/Debugger/Breakpoints.cpp

@@ -33,12 +33,6 @@
 
 BreakpointManager g_breakpoints;
 
-std::atomic<bool> anyBreakPoints_(false);
-std::atomic<bool> anyMemChecks_(false);
-
-static std::mutex breakPointsMutex_;
-static std::mutex memCheckMutex_;
-
 void MemCheck::Log(u32 addr, bool write, int size, u32 pc, const char *reason) {
 	if (result & BREAK_ACTION_LOG) {
 		const char *type = write ? "Write" : "Read";
@@ -512,8 +506,7 @@ BreakAction BreakpointManager::ExecMemCheck(u32 address, bool write, int size, u
 	return BREAK_ACTION_IGNORE;
 }
 
-BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc)
-{
+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.
 	int size = MIPSAnalyst::OpMemoryAccessSize(pc);
@@ -550,13 +543,12 @@ BreakAction BreakpointManager::ExecOpMemCheck(u32 address, u32 pc)
 	return BREAK_ACTION_IGNORE;
 }
 
-void BreakpointManager::SetSkipFirst(u32 pc)
-{
+void BreakpointManager::SetSkipFirst(u32 pc) {
 	breakSkipFirstAt_ = pc;
 	breakSkipFirstTicks_ = CoreTiming::GetTicks();
 }
-u32 BreakpointManager::CheckSkipFirst()
-{
+
+u32 BreakpointManager::CheckSkipFirst() {
 	u32 pc = breakSkipFirstAt_;
 	if (breakSkipFirstTicks_ == CoreTiming::GetTicks())
 		return pc;
@@ -619,26 +611,16 @@ std::vector<MemCheck> BreakpointManager::GetMemCheckRanges(bool write) {
 	return memCheckRangesRead_;
 }
 
-std::vector<MemCheck> BreakpointManager::GetMemChecks()
-{
+std::vector<MemCheck> BreakpointManager::GetMemChecks() {
 	std::lock_guard<std::mutex> guard(memCheckMutex_);
 	return memChecks_;
 }
 
-std::vector<BreakPoint> BreakpointManager::GetBreakpoints()
-{
+std::vector<BreakPoint> BreakpointManager::GetBreakpoints() {
 	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	return breakPoints_;
 }
 
-bool BreakpointManager::HasBreakPoints() {
-	return anyBreakPoints_;
-}
-
-bool BreakpointManager::HasMemChecks() {
-	return anyMemChecks_;
-}
-
 void BreakpointManager::Update(u32 addr) {
 	if (MIPSComp::jit && addr != -1) {
 		bool resume = false;

Core/Debugger/Breakpoints.h

@@ -18,6 +18,8 @@
 #pragma once
 
 #include <vector>
+#include <atomic>
+#include <mutex>
 
 #include "Core/Debugger/DebugInterface.h"
 
@@ -174,8 +176,12 @@ class BreakpointManager {
 		return memChecks_;
 	}
 
-	bool HasBreakPoints();
-	bool HasMemChecks();
+	bool HasBreakPoints() const {
+		return anyBreakPoints_;
+	}
+	bool HasMemChecks() const {
+		return anyMemChecks_;
+	}
 
 	void Update(u32 addr = 0);
 
@@ -189,6 +195,12 @@ class BreakpointManager {
 	MemCheck *GetMemCheckLocked(u32 address, int size);
 	void UpdateCachedMemCheckRanges();
 
+	std::atomic<bool> anyBreakPoints_;
+	std::atomic<bool> anyMemChecks_;
+
+	std::mutex breakPointsMutex_;
+	std::mutex memCheckMutex_;
+
 	std::vector<BreakPoint> breakPoints_;
 	u32 breakSkipFirstAt_ = 0;
 	u64 breakSkipFirstTicks_ = 0;

UI/EmuScreen.cpp

@@ -963,6 +963,7 @@ bool EmuScreen::UnsyncKey(const KeyInput &key) {
 		case NKCODE_SHIFT_RIGHT: keyShiftRight_ = down; break;
 		case NKCODE_ALT_LEFT: keyAltLeft_ = down; break;
 		case NKCODE_ALT_RIGHT: keyAltRight_ = down; break;
+		default: break;
 		}
 	}
 

UI/ImDebugger/ImDebugger.cpp

@@ -224,15 +224,16 @@ static void DrawBreakpointsView(MIPSDebugInterface *mipsDebug, ImConfig &cfg) {
 
 			for (int i = 0; i < (int)bps.size(); i++) {
 				auto &bp = bps[i];
-				if (bp.temporary) {
-					// Probably won't happen a lot.. maybe a wrong Step Over
+				bool temp = bp.temporary;
+				if (temp) {
 					continue;
 				}
+
 				ImGui::TableNextRow();
 				ImGui::TableNextColumn();
 				ImGui::PushID(i);
 				// TODO: This clashes with the checkbox!
-				if (ImGui::Selectable("", cfg.selectedBreakpoint == i, ImGuiSelectableFlags_SpanAllColumns)) {
+				if (ImGui::Selectable("", cfg.selectedBreakpoint == i, ImGuiSelectableFlags_SpanAllColumns) && !bp.temporary) {
 					cfg.selectedBreakpoint = i;
 					cfg.selectedMemCheck = -1;
 				}
@@ -513,15 +514,7 @@ void ImDebugger::Frame(MIPSDebugInterface *mipsDebug, GPUDebugInterface *gpuDebu
 				if (ImGui::MenuItem("Run")) {
 					Core_Resume();
 				}
-				if (ImGui::MenuItem("Step Into", "F11")) {
-					Core_RequestSingleStep(CPUStepType::Into, 1);
-				}
-				if (ImGui::MenuItem("Step Over", "F10")) {
-					Core_RequestSingleStep(CPUStepType::Over, 1);
-				}
-				if (ImGui::MenuItem("Step Out", "Shift+F11")) {
-					Core_RequestSingleStep(CPUStepType::Out, 1);
-				}
+				// used to have the step commands here, but they belong in the disassembly window.
 			} else {
 				if (ImGui::MenuItem("Break")) {
 					Core_Break("Menu:Break");
@@ -626,10 +619,12 @@ void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, bool *open, CoreState c
 	if (ImGui::IsWindowFocused()) {
 		// Process stepping keyboard shortcuts.
 		if (ImGui::IsKeyPressed(ImGuiKey_F10)) {
-			Core_RequestSingleStep(CPUStepType::Over, 0);
+			u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC());
+			Core_RequestSingleStep(CPUStepType::Over, stepSize);
 		}
 		if (ImGui::IsKeyPressed(ImGuiKey_F11)) {
-			Core_RequestSingleStep(CPUStepType::Into, 0);
+			u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC());
+			Core_RequestSingleStep(CPUStepType::Into, stepSize);
 		}
 	}
 
@@ -654,7 +649,8 @@ void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, bool *open, CoreState c
 
 	ImGui::SameLine();
 	if (ImGui::SmallButton("Step Over")) {
-		Core_RequestSingleStep(CPUStepType::Over, 0);
+		u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC());
+		Core_RequestSingleStep(CPUStepType::Over, stepSize);
 	}
 
 	ImGui::SameLine();

UI/ImDebugger/ImDebugger.h

@@ -29,7 +29,9 @@ class GPUDebugInterface;
 class ImDisasmWindow {
 public:
 	void Draw(MIPSDebugInterface *mipsDebug, bool *open, CoreState coreState);
-
+	ImDisasmView &View() {
+		return disasmView_;
+	}
 private:
 	// We just keep the state directly in the window. Can refactor later.
 

UI/ImDebugger/ImDisasmView.h

@@ -54,7 +54,7 @@ class ImDisasmView {
 	}
 
 	void scrollStepping(u32 newPc);
-	u32 getInstructionSizeAt(u32 address);
+	u32 getInstructionSizeAt(u32 address);  // not const because it might have to analyze.
 
 	void gotoAddr(unsigned int addr) {
 		if (positionLocked_ != 0)