From 20c4649963321dbdaaa93d0afe6c0541d3af31de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 25 Nov 2024 19:02:17 +0100 Subject: [PATCH] Fix step over, other stepping issues --- Core/Core.cpp | 30 +++++++++++++++++++----------- Core/Debugger/Breakpoints.cpp | 30 ++++++------------------------ Core/Debugger/Breakpoints.h | 16 ++++++++++++++-- UI/EmuScreen.cpp | 1 + UI/ImDebugger/ImDebugger.cpp | 26 +++++++++++--------------- UI/ImDebugger/ImDebugger.h | 4 +++- UI/ImDebugger/ImDisasmView.h | 2 +- 7 files changed, 55 insertions(+), 54 deletions(-) diff --git a/Core/Core.cpp b/Core/Core.cpp index 10f974230880..3d5e272a163c 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -56,7 +56,7 @@ static std::mutex g_stepMutex; struct StepCommand { CPUStepType type; - int param; + int stepSize; const char *reason; u32 relatedAddr; bool empty() const { @@ -64,7 +64,7 @@ struct StepCommand { } 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,7 +265,7 @@ 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: { @@ -272,8 +273,10 @@ static void Core_PerformStep(MIPSDebugInterface *cpu, CPUStepType stepType, int 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; diff --git a/Core/Debugger/Breakpoints.cpp b/Core/Debugger/Breakpoints.cpp index eec7e08a5793..810643f6b94a 100644 --- a/Core/Debugger/Breakpoints.cpp +++ b/Core/Debugger/Breakpoints.cpp @@ -33,12 +33,6 @@ BreakpointManager g_breakpoints; -std::atomic anyBreakPoints_(false); -std::atomic 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 BreakpointManager::GetMemCheckRanges(bool write) { return memCheckRangesRead_; } -std::vector BreakpointManager::GetMemChecks() -{ +std::vector BreakpointManager::GetMemChecks() { std::lock_guard guard(memCheckMutex_); return memChecks_; } -std::vector BreakpointManager::GetBreakpoints() -{ +std::vector BreakpointManager::GetBreakpoints() { std::lock_guard 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; diff --git a/Core/Debugger/Breakpoints.h b/Core/Debugger/Breakpoints.h index 282abb76d95d..fb99be271a86 100644 --- a/Core/Debugger/Breakpoints.h +++ b/Core/Debugger/Breakpoints.h @@ -18,6 +18,8 @@ #pragma once #include +#include +#include #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 anyBreakPoints_; + std::atomic anyMemChecks_; + + std::mutex breakPointsMutex_; + std::mutex memCheckMutex_; + std::vector breakPoints_; u32 breakSkipFirstAt_ = 0; u64 breakSkipFirstTicks_ = 0; diff --git a/UI/EmuScreen.cpp b/UI/EmuScreen.cpp index 963cc53df3c6..19183362ad47 100644 --- a/UI/EmuScreen.cpp +++ b/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; } } diff --git a/UI/ImDebugger/ImDebugger.cpp b/UI/ImDebugger/ImDebugger.cpp index ef672c7da468..e0c8308f6db6 100644 --- a/UI/ImDebugger/ImDebugger.cpp +++ b/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(); diff --git a/UI/ImDebugger/ImDebugger.h b/UI/ImDebugger/ImDebugger.h index 933f272a3597..8e155f4cd6e4 100644 --- a/UI/ImDebugger/ImDebugger.h +++ b/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. diff --git a/UI/ImDebugger/ImDisasmView.h b/UI/ImDebugger/ImDisasmView.h index ceed40fba491..1ee22a558f01 100644 --- a/UI/ImDebugger/ImDisasmView.h +++ b/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)