From 8235c38df79bd48ae3d84fe35471befb6f0ce5eb Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:15:23 +0200 Subject: [PATCH] Debugger: Small other cleanup Change misleading names. Fix function usage: Intepreter and Step Out will not check breakpoints in their own wrong way anymore (e.g. breaking on log-only breakpoints). --- Source/Core/Core/Core.cpp | 6 ++-- Source/Core/Core/FifoPlayer/FifoPlayer.cpp | 4 +-- Source/Core/Core/HW/CPU.cpp | 7 ++-- Source/Core/Core/HW/CPU.h | 12 +++---- .../CachedInterpreter/CachedInterpreter.cpp | 3 +- Source/Core/Core/PowerPC/GDBStub.cpp | 2 +- .../Core/PowerPC/Interpreter/Interpreter.cpp | 32 +------------------ Source/Core/Core/PowerPC/Jit64/Jit.cpp | 2 +- Source/Core/Core/PowerPC/JitArm64/Jit.cpp | 2 +- Source/Core/Core/PowerPC/PowerPC.cpp | 31 +++++++++++------- Source/Core/Core/PowerPC/PowerPC.h | 7 ++-- Source/Core/DolphinQt/Debugger/CodeWidget.cpp | 9 +++--- 12 files changed, 48 insertions(+), 69 deletions(-) diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 8399d55cba..dc362b242d 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -704,13 +704,13 @@ void SetState(Core::System& system, State state, bool report_state_change) case State::Paused: // NOTE: GetState() will return State::Paused immediately, even before anything has // stopped (including the CPU). - system.GetCPU().EnableStepping(true); // Break + system.GetCPU().SetStepping(true); // Break Wiimote::Pause(); ResetRumble(); break; case State::Running: { - system.GetCPU().EnableStepping(false); + system.GetCPU().SetStepping(false); Wiimote::Resume(); break; } @@ -813,7 +813,7 @@ static bool PauseAndLock(Core::System& system, bool do_lock, bool unpause_on_unl // The CPU is responsible for managing the Audio and FIFO state so we use its // mechanism to unpause them. If we unpaused the systems above when releasing // the locks then they could call CPU::Break which would require detecting it - // and re-pausing with CPU::EnableStepping. + // and re-pausing with CPU::SetStepping. was_unpaused = system.GetCPU().PauseAndLock(false, unpause_on_unlock, true); } diff --git a/Source/Core/Core/FifoPlayer/FifoPlayer.cpp b/Source/Core/Core/FifoPlayer/FifoPlayer.cpp index 83eae87b9e..dcf50b4684 100644 --- a/Source/Core/Core/FifoPlayer/FifoPlayer.cpp +++ b/Source/Core/Core/FifoPlayer/FifoPlayer.cpp @@ -228,7 +228,7 @@ public: IsPlayingBackFifologWithBrokenEFBCopies = m_parent->m_File->HasBrokenEFBCopies(); // Without this call, we deadlock in initialization in dual core, as the FIFO is disabled and // thus ClearEfb()'s call to WaitForGPUInactive() never returns - m_parent->m_system.GetCPU().EnableStepping(false); + m_parent->m_system.GetCPU().SetStepping(false); m_parent->m_CurrentFrame = m_parent->m_FrameRangeStart; m_parent->LoadMemory(); @@ -243,7 +243,7 @@ public: void SingleStep() override { // NOTE: AdvanceFrame() will get stuck forever in Dual Core because the FIFO - // is disabled by CPU::EnableStepping(true) so the frame never gets displayed. + // is disabled by CPU::SetStepping(true) so the frame never gets displayed. PanicAlertFmtT("Cannot SingleStep the FIFO. Use Frame Advance instead."); } diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 15237dab30..583693c21a 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -88,6 +88,7 @@ void CPUManager::Run() // Adjust PC for JIT when debugging // SingleStep so that the "continue", "step over" and "step out" debugger functions // work when the PC is at a breakpoint at the beginning of the block + // Don't use PowerPCManager::CheckBreakPoints, otherwise you get double logging // If watchpoints are enabled, any instruction could be a breakpoint. if (power_pc.GetMode() != PowerPC::CoreMode::Interpreter) { @@ -174,7 +175,7 @@ void CPUManager::Run() // Requires holding m_state_change_lock void CPUManager::RunAdjacentSystems(bool running) { - // NOTE: We're assuming these will not try to call Break or EnableStepping. + // NOTE: We're assuming these will not try to call Break or SetStepping. m_system.GetFifo().EmulatorState(running); // Core is responsible for shutting down the sound stream. if (m_state != State::PowerDown) @@ -247,7 +248,7 @@ bool CPUManager::SetStateLocked(State s) return true; } -void CPUManager::EnableStepping(bool stepping) +void CPUManager::SetStepping(bool stepping) { std::lock_guard stepping_lock(m_stepping_lock); std::unique_lock state_lock(m_state_change_lock); @@ -290,7 +291,7 @@ void CPUManager::Break() void CPUManager::Continue() { - EnableStepping(false); + SetStepping(false); Core::CallOnStateChangedCallbacks(Core::State::Running); } diff --git a/Source/Core/Core/HW/CPU.h b/Source/Core/Core/HW/CPU.h index e328f645e8..57e8a31a29 100644 --- a/Source/Core/Core/HW/CPU.h +++ b/Source/Core/Core/HW/CPU.h @@ -62,10 +62,10 @@ public: void StepOpcode(Common::Event* event = nullptr); // Enable or Disable Stepping. [Will deadlock if called from a system thread] - void EnableStepping(bool stepping); + void SetStepping(bool stepping); - // Breakpoint activation for system threads. Similar to EnableStepping(true). - // NOTE: Unlike EnableStepping, this does NOT synchronize with the CPU Thread + // Breakpoint activation for system threads. Similar to SetStepping(true). + // NOTE: Unlike SetStepping, this does NOT synchronize with the CPU Thread // which enables it to avoid deadlocks but also makes it less safe so it // should not be used by the Host. void Break(); @@ -91,7 +91,7 @@ public: // Return value for do_lock == true is whether the state was State::Running or not. // Return value for do_lock == false is whether the state was changed *to* State::Running or not. // Cannot be used by System threads as it will deadlock. It is threadsafe otherwise. - // "control_adjacent" causes PauseAndLock to behave like EnableStepping by modifying the + // "control_adjacent" causes PauseAndLock to behave like SetStepping by modifying the // state of the Audio and FIFO subsystems as well. bool PauseAndLock(bool do_lock, bool unpause_on_unlock = true, bool control_adjacent = false); @@ -110,9 +110,9 @@ private: // Read access is unsynchronized. State m_state = State::PowerDown; - // Synchronizes EnableStepping and PauseAndLock so only one instance can be + // Synchronizes SetStepping and PauseAndLock so only one instance can be // active at a time. Simplifies code by eliminating several edge cases where - // the EnableStepping(true)/PauseAndLock(true) case must release the state lock + // the SetStepping(true)/PauseAndLock(true) case must release the state lock // and wait for the CPU Thread which would otherwise require additional flags. // NOTE: When using the stepping lock, it must always be acquired first. If // the lock is acquired after the state lock then that is guaranteed to diff --git a/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp b/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp index d2296641c2..f8b5c6f53b 100644 --- a/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp +++ b/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp @@ -249,8 +249,7 @@ bool CachedInterpreter::CheckProgramException(CachedInterpreter& cached_interpre bool CachedInterpreter::CheckBreakpoint(CachedInterpreter& cached_interpreter, u32 data) { - cached_interpreter.m_system.GetPowerPC().CheckBreakPoints(); - if (cached_interpreter.m_system.GetCPU().GetState() != CPU::State::Running) + if (cached_interpreter.m_system.GetPowerPC().CheckAndHandleBreakPoints()) { cached_interpreter.m_ppc_state.downcount -= data; return true; diff --git a/Source/Core/Core/PowerPC/GDBStub.cpp b/Source/Core/Core/PowerPC/GDBStub.cpp index 0f6617a5b4..c293977aeb 100644 --- a/Source/Core/Core/PowerPC/GDBStub.cpp +++ b/Source/Core/Core/PowerPC/GDBStub.cpp @@ -865,7 +865,7 @@ static void WriteMemory(const Core::CPUThreadGuard& guard) static void Step() { auto& system = Core::System::GetInstance(); - system.GetCPU().EnableStepping(true); + system.GetCPU().SetStepping(true); Core::CallOnStateChangedCallbacks(Core::State::Paused); } diff --git a/Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp b/Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp index a7c2fbed52..89153f801e 100644 --- a/Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp +++ b/Source/Core/Core/PowerPC/Interpreter/Interpreter.cpp @@ -261,38 +261,8 @@ void Interpreter::Run() s_pc_vec.erase(s_pc_vec.begin()); #endif - // 2: check for breakpoint - if (power_pc.GetBreakPoints().IsAddressBreakPoint(m_ppc_state.pc)) - { -#ifdef SHOW_HISTORY - NOTICE_LOG_FMT(POWERPC, "----------------------------"); - NOTICE_LOG_FMT(POWERPC, "Blocks:"); - for (const u32 entry : s_pc_block_vec) - NOTICE_LOG_FMT(POWERPC, "PC: {:#010x}", entry); - NOTICE_LOG_FMT(POWERPC, "----------------------------"); - NOTICE_LOG_FMT(POWERPC, "Steps:"); - for (size_t j = 0; j < s_pc_vec.size(); j++) - { - // Write space - if (j > 0) - { - if (s_pc_vec[j] != s_pc_vec[(j - 1) + 4] - NOTICE_LOG_FMT(POWERPC, ""); - } - - NOTICE_LOG_FMT(POWERPC, "PC: {:#010x}", s_pc_vec[j]); - } -#endif - INFO_LOG_FMT(POWERPC, "Hit Breakpoint - {:08x}", m_ppc_state.pc); - cpu.Break(); - if (GDBStub::IsActive()) - GDBStub::TakeControl(); - if (power_pc.GetBreakPoints().IsTempBreakPoint(m_ppc_state.pc)) - power_pc.GetBreakPoints().Remove(m_ppc_state.pc); - - Host_UpdateDisasmDialog(); + if (power_pc.CheckAndHandleBreakPoints()) return; - } cycles += SingleStepInner(); } m_ppc_state.downcount -= cycles; diff --git a/Source/Core/Core/PowerPC/Jit64/Jit.cpp b/Source/Core/Core/PowerPC/Jit64/Jit.cpp index 3c27e16fad..98d6dbb281 100644 --- a/Source/Core/Core/PowerPC/Jit64/Jit.cpp +++ b/Source/Core/Core/PowerPC/Jit64/Jit.cpp @@ -1043,7 +1043,7 @@ bool Jit64::DoJit(u32 em_address, JitBlock* b, u32 nextPC) MOV(32, PPCSTATE(pc), Imm32(op.address)); ABI_PushRegistersAndAdjustStack({}, 0); - ABI_CallFunctionP(PowerPC::CheckBreakPointsFromJIT, &power_pc); + ABI_CallFunctionP(PowerPC::CheckAndHandleBreakPointsFromJIT, &power_pc); ABI_PopRegistersAndAdjustStack({}, 0); MOV(64, R(RSCRATCH), ImmPtr(cpu.GetStatePtr())); CMP(32, MatR(RSCRATCH), Imm32(Common::ToUnderlying(CPU::State::Running))); diff --git a/Source/Core/Core/PowerPC/JitArm64/Jit.cpp b/Source/Core/Core/PowerPC/JitArm64/Jit.cpp index e5e6f76dda..c005484377 100644 --- a/Source/Core/Core/PowerPC/JitArm64/Jit.cpp +++ b/Source/Core/Core/PowerPC/JitArm64/Jit.cpp @@ -1249,7 +1249,7 @@ bool JitArm64::DoJit(u32 em_address, JitBlock* b, u32 nextPC) MOVI2R(DISPATCHER_PC, op.address); STP(IndexType::Signed, DISPATCHER_PC, DISPATCHER_PC, PPC_REG, PPCSTATE_OFF(pc)); - ABI_CallFunction(&PowerPC::CheckBreakPointsFromJIT, &m_system.GetPowerPC()); + ABI_CallFunction(&PowerPC::CheckAndHandleBreakPointsFromJIT, &m_system.GetPowerPC()); LDR(IndexType::Unsigned, ARM64Reg::W0, ARM64Reg::X0, MOVPage2R(ARM64Reg::X0, cpu.GetStatePtr())); diff --git a/Source/Core/Core/PowerPC/PowerPC.cpp b/Source/Core/Core/PowerPC/PowerPC.cpp index 5b32f5c1f5..e3f66d8895 100644 --- a/Source/Core/Core/PowerPC/PowerPC.cpp +++ b/Source/Core/Core/PowerPC/PowerPC.cpp @@ -629,19 +629,13 @@ void PowerPCManager::CheckExternalExceptions() m_system.GetJitInterface().UpdateMembase(); } -void PowerPCManager::CheckBreakPoints() +bool PowerPCManager::CheckBreakPoints() { const TBreakPoint* bp = m_breakpoints.GetBreakpoint(m_ppc_state.pc); if (!bp || !bp->is_enabled || !EvaluateCondition(m_system, bp->condition)) - return; + return false; - if (bp->break_on_hit) - { - m_system.GetCPU().Break(); - if (GDBStub::IsActive()) - GDBStub::TakeControl(); - } if (bp->log_on_hit) { NOTICE_LOG_FMT(MEMMAP, @@ -652,8 +646,21 @@ void PowerPCManager::CheckBreakPoints() m_ppc_state.gpr[8], m_ppc_state.gpr[9], m_ppc_state.gpr[10], m_ppc_state.gpr[11], m_ppc_state.gpr[12], LR(m_ppc_state)); } - if (m_breakpoints.IsTempBreakPoint(m_ppc_state.pc)) - m_breakpoints.Remove(m_ppc_state.pc); + if (bp->break_on_hit) + return true; + return false; +} + +bool PowerPCManager::CheckAndHandleBreakPoints() +{ + if (CheckBreakPoints()) + { + m_system.GetCPU().Break(); + if (GDBStub::IsActive()) + GDBStub::TakeControl(); + return true; + } + return false; } void PowerPCState::SetSR(u32 index, u32 value) @@ -722,8 +729,8 @@ void CheckExternalExceptionsFromJIT(PowerPCManager& power_pc) power_pc.CheckExternalExceptions(); } -void CheckBreakPointsFromJIT(PowerPCManager& power_pc) +void CheckAndHandleBreakPointsFromJIT(PowerPCManager& power_pc) { - power_pc.CheckBreakPoints(); + power_pc.CheckAndHandleBreakPoints(); } } // namespace PowerPC diff --git a/Source/Core/Core/PowerPC/PowerPC.h b/Source/Core/Core/PowerPC/PowerPC.h index b38c4f5555..662507697e 100644 --- a/Source/Core/Core/PowerPC/PowerPC.h +++ b/Source/Core/Core/PowerPC/PowerPC.h @@ -281,7 +281,10 @@ public: void SingleStep(); void CheckExceptions(); void CheckExternalExceptions(); - void CheckBreakPoints(); + // Evaluate the breakpoints in order to log. Returns whether it would break. + bool CheckBreakPoints(); + // Evaluate the breakpoints in order to log and/or break. Returns whether it breaks. + bool CheckAndHandleBreakPoints(); void RunLoop(); u64 ReadFullTimeBaseValue() const; @@ -330,7 +333,7 @@ void UpdatePerformanceMonitor(u32 cycles, u32 num_load_stores, u32 num_fp_inst, void CheckExceptionsFromJIT(PowerPCManager& power_pc); void CheckExternalExceptionsFromJIT(PowerPCManager& power_pc); -void CheckBreakPointsFromJIT(PowerPCManager& power_pc); +void CheckAndHandleBreakPointsFromJIT(PowerPCManager& power_pc); // Easy register access macros. #define HID0(ppc_state) ((UReg_HID0&)(ppc_state).spr[SPR_HID0]) diff --git a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp index e5dd96014b..361f142c7c 100644 --- a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp @@ -484,7 +484,7 @@ void CodeWidget::StepOver() auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); breakpoints.ClearAllTemporary(); breakpoints.Add(m_system.GetPPCState().pc + 4, true); - cpu.EnableStepping(false); + cpu.SetStepping(false); Core::DisplayMessage(tr("Step over in progress...").toStdString(), 2000); } else @@ -547,8 +547,7 @@ void CodeWidget::StepOut() do { power_pc.SingleStep(); - } while (ppc_state.pc != next_pc && clock::now() < timeout && - !breakpoints.IsAddressBreakPoint(ppc_state.pc)); + } while (ppc_state.pc != next_pc && clock::now() < timeout && !power_pc.CheckBreakPoints()); } else { @@ -556,14 +555,14 @@ void CodeWidget::StepOut() } inst = PowerPC::MMU::HostRead_Instruction(guard, ppc_state.pc); - } while (clock::now() < timeout && !breakpoints.IsAddressBreakPoint(ppc_state.pc)); + } while (clock::now() < timeout && !power_pc.CheckBreakPoints()); power_pc.SetMode(old_mode); } emit Host::GetInstance()->UpdateDisasmDialog(); - if (breakpoints.IsAddressBreakPoint(ppc_state.pc)) + if (power_pc.CheckBreakPoints()) Core::DisplayMessage(tr("Breakpoint encountered! Step out aborted.").toStdString(), 2000); else if (clock::now() >= timeout) Core::DisplayMessage(tr("Step out timed out!").toStdString(), 2000);