From 9aeeea37620cf7c554d1462188ad06c2f6d32617 Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:02:15 +0200 Subject: [PATCH 1/6] Debugger: Small Breakpoint cleanup Reuse more code, change misleading names, remove useless documentation, add useful documentation --- Source/Core/Core/Debugger/DebugInterface.h | 4 +-- .../Core/Core/Debugger/PPCDebugInterface.cpp | 10 ++---- Source/Core/Core/Debugger/PPCDebugInterface.h | 4 +-- Source/Core/Core/PowerPC/BreakPoints.cpp | 33 ++++++++++++------- Source/Core/Core/PowerPC/BreakPoints.h | 15 ++++----- Source/Core/Core/PowerPC/GDBStub.cpp | 3 +- .../DolphinQt/Debugger/BreakpointWidget.cpp | 4 +-- .../DolphinQt/Debugger/CodeViewWidget.cpp | 8 ++--- 8 files changed, 41 insertions(+), 40 deletions(-) diff --git a/Source/Core/Core/Debugger/DebugInterface.h b/Source/Core/Core/Debugger/DebugInterface.h index 9802c31f3a..67ff54fcdb 100644 --- a/Source/Core/Core/Debugger/DebugInterface.h +++ b/Source/Core/Core/Debugger/DebugInterface.h @@ -73,8 +73,8 @@ public: } virtual bool IsAlive() const { return true; } virtual bool IsBreakpoint(u32 /*address*/) const { return false; } - virtual void SetBreakpoint(u32 /*address*/) {} - virtual void ClearBreakpoint(u32 /*address*/) {} + virtual void AddBreakpoint(u32 /*address*/) {} + virtual void RemoveBreakpoint(u32 /*address*/) {} virtual void ClearAllBreakpoints() {} virtual void ToggleBreakpoint(u32 /*address*/) {} virtual void ClearAllMemChecks() {} diff --git a/Source/Core/Core/Debugger/PPCDebugInterface.cpp b/Source/Core/Core/Debugger/PPCDebugInterface.cpp index 9389fe76d8..3819f62493 100644 --- a/Source/Core/Core/Debugger/PPCDebugInterface.cpp +++ b/Source/Core/Core/Debugger/PPCDebugInterface.cpp @@ -357,12 +357,12 @@ bool PPCDebugInterface::IsBreakpoint(u32 address) const return m_system.GetPowerPC().GetBreakPoints().IsAddressBreakPoint(address); } -void PPCDebugInterface::SetBreakpoint(u32 address) +void PPCDebugInterface::AddBreakpoint(u32 address) { m_system.GetPowerPC().GetBreakPoints().Add(address); } -void PPCDebugInterface::ClearBreakpoint(u32 address) +void PPCDebugInterface::RemoveBreakpoint(u32 address) { m_system.GetPowerPC().GetBreakPoints().Remove(address); } @@ -374,11 +374,7 @@ void PPCDebugInterface::ClearAllBreakpoints() void PPCDebugInterface::ToggleBreakpoint(u32 address) { - auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); - if (breakpoints.IsAddressBreakPoint(address)) - breakpoints.Remove(address); - else - breakpoints.Add(address); + m_system.GetPowerPC().GetBreakPoints().ToggleBreakPoint(address); } void PPCDebugInterface::ClearAllMemChecks() diff --git a/Source/Core/Core/Debugger/PPCDebugInterface.h b/Source/Core/Core/Debugger/PPCDebugInterface.h index 86207bc1da..5f8f8b04de 100644 --- a/Source/Core/Core/Debugger/PPCDebugInterface.h +++ b/Source/Core/Core/Debugger/PPCDebugInterface.h @@ -80,8 +80,8 @@ public: u32 address) const override; bool IsAlive() const override; bool IsBreakpoint(u32 address) const override; - void SetBreakpoint(u32 address) override; - void ClearBreakpoint(u32 address) override; + void AddBreakpoint(u32 address) override; + void RemoveBreakpoint(u32 address) override; void ClearAllBreakpoints() override; void ToggleBreakpoint(u32 address) override; void ClearAllMemChecks() override; diff --git a/Source/Core/Core/PowerPC/BreakPoints.cpp b/Source/Core/Core/PowerPC/BreakPoints.cpp index 1e8689dd82..24b15750e0 100644 --- a/Source/Core/Core/PowerPC/BreakPoints.cpp +++ b/Source/Core/Core/PowerPC/BreakPoints.cpp @@ -27,14 +27,13 @@ BreakPoints::~BreakPoints() = default; bool BreakPoints::IsAddressBreakPoint(u32 address) const { - return std::any_of(m_breakpoints.begin(), m_breakpoints.end(), - [address](const auto& bp) { return bp.address == address; }); + return GetBreakpoint(address) != nullptr; } bool BreakPoints::IsBreakPointEnable(u32 address) const { - return std::any_of(m_breakpoints.begin(), m_breakpoints.end(), - [address](const auto& bp) { return bp.is_enabled && bp.address == address; }); + const TBreakPoint* bp = GetBreakpoint(address); + return bp != nullptr && bp->is_enabled; } bool BreakPoints::IsTempBreakPoint(u32 address) const @@ -153,6 +152,16 @@ void BreakPoints::Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit } bool BreakPoints::ToggleBreakPoint(u32 address) +{ + if (!Remove(address)) + { + Add(address); + return true; + } + return false; +} + +bool BreakPoints::ToggleEnable(u32 address) { auto iter = std::find_if(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp) { return bp.address == address; }); @@ -164,16 +173,18 @@ bool BreakPoints::ToggleBreakPoint(u32 address) return true; } -void BreakPoints::Remove(u32 address) +bool BreakPoints::Remove(u32 address) { const auto iter = std::find_if(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp) { return bp.address == address; }); if (iter == m_breakpoints.cend()) - return; + return false; m_breakpoints.erase(iter); m_system.GetJitInterface().InvalidateICache(address, 4, true); + + return true; } void BreakPoints::Clear() @@ -281,9 +292,8 @@ void MemChecks::Add(TMemCheck memory_check) [address](const auto& check) { return check.start_address == address; }); if (old_mem_check != m_mem_checks.end()) { - const bool is_enabled = old_mem_check->is_enabled; // Preserve enabled status + memory_check.is_enabled = old_mem_check->is_enabled; // Preserve enabled status *old_mem_check = std::move(memory_check); - old_mem_check->is_enabled = is_enabled; old_mem_check->num_hits = 0; } else @@ -297,7 +307,7 @@ void MemChecks::Add(TMemCheck memory_check) m_system.GetMMU().DBATUpdated(); } -bool MemChecks::ToggleBreakPoint(u32 address) +bool MemChecks::ToggleEnable(u32 address) { auto iter = std::find_if(m_mem_checks.begin(), m_mem_checks.end(), [address](const auto& bp) { return bp.start_address == address; }); @@ -309,20 +319,21 @@ bool MemChecks::ToggleBreakPoint(u32 address) return true; } -void MemChecks::Remove(u32 address) +bool MemChecks::Remove(u32 address) { const auto iter = std::find_if(m_mem_checks.cbegin(), m_mem_checks.cend(), [address](const auto& check) { return check.start_address == address; }); if (iter == m_mem_checks.cend()) - return; + return false; const Core::CPUThreadGuard guard(m_system); m_mem_checks.erase(iter); if (!HasAny()) m_system.GetJitInterface().ClearCache(guard); m_system.GetMMU().DBATUpdated(); + return true; } void MemChecks::Clear() diff --git a/Source/Core/Core/PowerPC/BreakPoints.h b/Source/Core/Core/PowerPC/BreakPoints.h index ce509e5d97..6c60b8a24e 100644 --- a/Source/Core/Core/PowerPC/BreakPoints.h +++ b/Source/Core/Core/PowerPC/BreakPoints.h @@ -66,23 +66,22 @@ public: TBreakPointsStr GetStrings() const; void AddFromStrings(const TBreakPointsStr& bp_strings); - // is address breakpoint bool IsAddressBreakPoint(u32 address) const; bool IsBreakPointEnable(u32 adresss) const; bool IsTempBreakPoint(u32 address) const; const TBreakPoint* GetBreakpoint(u32 address) const; - // Add BreakPoint + // Add BreakPoint. If one already exists on the same address, replace it. void Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit, std::optional condition); void Add(u32 address, bool temp = false); void Add(TBreakPoint bp); - // Modify Breakpoint bool ToggleBreakPoint(u32 address); + bool ToggleEnable(u32 address); - // Remove Breakpoint - void Remove(u32 address); + // Remove Breakpoint. Returns whether it was removed. + bool Remove(u32 address); void Clear(); void ClearAllTemporary(); @@ -111,12 +110,12 @@ public: void Add(TMemCheck memory_check); - bool ToggleBreakPoint(u32 address); + bool ToggleEnable(u32 address); - // memory breakpoint TMemCheck* GetMemCheck(u32 address, size_t size = 1); bool OverlapsMemcheck(u32 address, u32 length) const; - void Remove(u32 address); + // Remove Breakpoint. Returns whether it was removed. + bool Remove(u32 address); void Clear(); bool HasAny() const { return !m_mem_checks.empty(); } diff --git a/Source/Core/Core/PowerPC/GDBStub.cpp b/Source/Core/Core/PowerPC/GDBStub.cpp index df87a6c75a..0f6617a5b4 100644 --- a/Source/Core/Core/PowerPC/GDBStub.cpp +++ b/Source/Core/Core/PowerPC/GDBStub.cpp @@ -164,9 +164,8 @@ static void RemoveBreakpoint(BreakpointType type, u32 addr, u32 len) if (type == BreakpointType::ExecuteHard || type == BreakpointType::ExecuteSoft) { auto& breakpoints = Core::System::GetInstance().GetPowerPC().GetBreakPoints(); - while (breakpoints.IsAddressBreakPoint(addr)) + if (breakpoints.Remove(addr)) { - breakpoints.Remove(addr); INFO_LOG_FMT(GDB_STUB, "gdb: removed a breakpoint: {:08x} bytes at {:08x}", len, addr); } } diff --git a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp index 130e68e860..3bed35c576 100644 --- a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp @@ -215,9 +215,9 @@ void BreakpointWidget::OnClicked(QTableWidgetItem* item) if (item->column() == ENABLED_COLUMN) { if (item->data(IS_MEMCHECK_ROLE).toBool()) - m_system.GetPowerPC().GetMemChecks().ToggleBreakPoint(address); + m_system.GetPowerPC().GetMemChecks().ToggleEnable(address); else - m_system.GetPowerPC().GetBreakPoints().ToggleBreakPoint(address); + m_system.GetPowerPC().GetBreakPoints().ToggleEnable(address); emit BreakpointsChanged(); Update(); diff --git a/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp index 51f2814d9b..e2df3420a6 100644 --- a/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp @@ -869,7 +869,7 @@ void CodeViewWidget::OnRunToHere() { const u32 addr = GetContextAddress(); - m_system.GetPowerPC().GetDebugInterface().SetBreakpoint(addr); + m_system.GetPowerPC().GetDebugInterface().AddBreakpoint(addr); m_system.GetPowerPC().GetDebugInterface().RunToBreakpoint(); Update(); } @@ -1137,11 +1137,7 @@ void CodeViewWidget::showEvent(QShowEvent* event) void CodeViewWidget::ToggleBreakpoint() { - auto& power_pc = m_system.GetPowerPC(); - if (power_pc.GetDebugInterface().IsBreakpoint(GetContextAddress())) - power_pc.GetBreakPoints().Remove(GetContextAddress()); - else - power_pc.GetBreakPoints().Add(GetContextAddress()); + m_system.GetPowerPC().GetBreakPoints().ToggleBreakPoint(GetContextAddress()); emit BreakpointsChanged(); Update(); From 8235c38df79bd48ae3d84fe35471befb6f0ce5eb Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:15:23 +0200 Subject: [PATCH 2/6] 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); From 037de1ce929b9c785f11e9f595ededd1497bfe41 Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:23:50 +0200 Subject: [PATCH 3/6] Debugger: fix Run to Here Now it actually does what it says on the name, instead of creating a breapoint and doing nothing else (not even updating the widget). Also, it now can't be selected if emulation isn't running. Closes https://bugs.dolphin-emu.org/issues/13532 --- Source/Core/Core/Debugger/DebugInterface.h | 2 +- Source/Core/Core/Debugger/PPCDebugInterface.cpp | 6 +++++- Source/Core/Core/Debugger/PPCDebugInterface.h | 2 +- Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp | 10 ++++------ 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/Source/Core/Core/Debugger/DebugInterface.h b/Source/Core/Core/Debugger/DebugInterface.h index 67ff54fcdb..cc07d06d3b 100644 --- a/Source/Core/Core/Debugger/DebugInterface.h +++ b/Source/Core/Core/Debugger/DebugInterface.h @@ -99,7 +99,7 @@ public: virtual u32 GetPC() const { return 0; } virtual void SetPC(u32 /*address*/) {} virtual void Step() {} - virtual void RunToBreakpoint() {} + virtual void RunTo(u32 /*address*/) {} virtual u32 GetColor(const CPUThreadGuard* /*guard*/, u32 /*address*/) const { return 0xFFFFFFFF; diff --git a/Source/Core/Core/Debugger/PPCDebugInterface.cpp b/Source/Core/Core/Debugger/PPCDebugInterface.cpp index 3819f62493..72cfd05b2c 100644 --- a/Source/Core/Core/Debugger/PPCDebugInterface.cpp +++ b/Source/Core/Core/Debugger/PPCDebugInterface.cpp @@ -20,6 +20,7 @@ #include "Core/Config/MainSettings.h" #include "Core/Core.h" #include "Core/Debugger/OSThread.h" +#include "Core/HW/CPU.h" #include "Core/HW/DSP.h" #include "Core/PatchEngine.h" #include "Core/PowerPC/MMU.h" @@ -502,8 +503,11 @@ void PPCDebugInterface::SetPC(u32 address) m_system.GetPPCState().pc = address; } -void PPCDebugInterface::RunToBreakpoint() +void PPCDebugInterface::RunTo(u32 address) { + auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); + breakpoints.Add(address, true); + m_system.GetCPU().SetStepping(false); } std::shared_ptr PPCDebugInterface::NetworkLogger() diff --git a/Source/Core/Core/Debugger/PPCDebugInterface.h b/Source/Core/Core/Debugger/PPCDebugInterface.h index 5f8f8b04de..797f2d5254 100644 --- a/Source/Core/Core/Debugger/PPCDebugInterface.h +++ b/Source/Core/Core/Debugger/PPCDebugInterface.h @@ -100,7 +100,7 @@ public: u32 GetPC() const override; void SetPC(u32 address) override; void Step() override {} - void RunToBreakpoint() override; + void RunTo(u32 address) override; u32 GetColor(const Core::CPUThreadGuard* guard, u32 address) const override; std::string_view GetDescription(u32 address) const override; diff --git a/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp index e2df3420a6..36e7ade321 100644 --- a/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp @@ -594,7 +594,7 @@ void CodeViewWidget::OnContextMenu() menu->addAction(tr("Set symbol &end address"), this, &CodeViewWidget::OnSetSymbolEndAddress); menu->addSeparator(); - menu->addAction(tr("Run &To Here"), this, &CodeViewWidget::OnRunToHere); + auto* run_to_action = menu->addAction(tr("Run &To Here"), this, &CodeViewWidget::OnRunToHere); auto* function_action = menu->addAction(tr("&Add function"), this, &CodeViewWidget::OnAddFunction); auto* ppc_action = menu->addAction(tr("PPC vs Host"), this, &CodeViewWidget::OnPPCComparison); @@ -645,8 +645,8 @@ void CodeViewWidget::OnContextMenu() follow_branch_action->setEnabled(follow_branch_enabled); for (auto* action : - {copy_address_action, copy_line_action, copy_hex_action, function_action, ppc_action, - insert_blr_action, insert_nop_action, replace_action, assemble_action}) + {copy_address_action, copy_line_action, copy_hex_action, function_action, run_to_action, + ppc_action, insert_blr_action, insert_nop_action, replace_action, assemble_action}) { action->setEnabled(running); } @@ -869,9 +869,7 @@ void CodeViewWidget::OnRunToHere() { const u32 addr = GetContextAddress(); - m_system.GetPowerPC().GetDebugInterface().AddBreakpoint(addr); - m_system.GetPowerPC().GetDebugInterface().RunToBreakpoint(); - Update(); + m_system.GetPowerPC().GetDebugInterface().RunTo(addr); } void CodeViewWidget::OnPPCComparison() From bd3cf67cbc354dc93edf14a788e602c69c054d38 Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:36:38 +0200 Subject: [PATCH 4/6] Debugger: Rework temporary breakpoints Before: 1. In theory there could be multiple, but in practice they were (manually) cleared before creating one 2. (Some of) the conditions to clear one were either to reach it, to create a new one (due to the point above), or to step. This created weird behavior: let's say you Step Over a `bl` (thus creating a temporary breakpoint on `pc+4`), and you reached a regular breakpoint inside the `bl`. The temporary one would still be there: if you resumed, the emulation would still stop there, as a sort of Step Out. But, if before resuming, you made a Step, then it wouldn't do that. 3. The breakpoint widget had no idea concept of them, and will treat them as regular breakpoints. Also, they'll be shown only when the widget is updated in some other way, leading to more confusion. 4. Because only one breakpoint could exist per address, the creation of a temporary breakpoint on a top of a regular one would delete it and inherit its properties (e.g. being log-only). This could happen, for instance, if you Stepped Over a `bl` specifically, and pc+4 had a regular breakpoint. Now there can only be one temporary breakpoint, which is automatically cleared whenever emulation is paused. So, removing some manual clearing from 1., and removing the weird behavior of 2. As it is stored in a separate variable, it won't be seen at all depending on the function used (fixing 3., and removing some checks in other places), and it won't replace a regular breakpoint, instead simply having priority (fixing 4.). --- Source/Core/Core/Debugger/CodeTrace.cpp | 1 - .../Core/Core/Debugger/PPCDebugInterface.cpp | 2 +- Source/Core/Core/HW/CPU.cpp | 2 + Source/Core/Core/PowerPC/BreakPoints.cpp | 80 ++++++++++--------- Source/Core/Core/PowerPC/BreakPoints.h | 19 +++-- Source/Core/Core/PowerPC/PowerPC.cpp | 3 - .../DolphinQt/Debugger/BranchWatchDialog.cpp | 8 +- .../DolphinQt/Debugger/BreakpointDialog.cpp | 2 +- .../DolphinQt/Debugger/BreakpointWidget.cpp | 13 ++- .../DolphinQt/Debugger/BreakpointWidget.h | 2 +- .../DolphinQt/Debugger/CodeViewWidget.cpp | 5 +- Source/Core/DolphinQt/Debugger/CodeWidget.cpp | 7 +- 12 files changed, 74 insertions(+), 70 deletions(-) diff --git a/Source/Core/Core/Debugger/CodeTrace.cpp b/Source/Core/Core/Debugger/CodeTrace.cpp index a6411acc14..9036213324 100644 --- a/Source/Core/Core/Debugger/CodeTrace.cpp +++ b/Source/Core/Core/Debugger/CodeTrace.cpp @@ -189,7 +189,6 @@ AutoStepResults CodeTrace::AutoStepping(const Core::CPUThreadGuard& guard, bool stop_condition = HitType::ACTIVE; auto& power_pc = guard.GetSystem().GetPowerPC(); - power_pc.GetBreakPoints().ClearAllTemporary(); using clock = std::chrono::steady_clock; clock::time_point timeout = clock::now() + std::chrono::seconds(4); diff --git a/Source/Core/Core/Debugger/PPCDebugInterface.cpp b/Source/Core/Core/Debugger/PPCDebugInterface.cpp index 72cfd05b2c..786f95bfa7 100644 --- a/Source/Core/Core/Debugger/PPCDebugInterface.cpp +++ b/Source/Core/Core/Debugger/PPCDebugInterface.cpp @@ -506,7 +506,7 @@ void PPCDebugInterface::SetPC(u32 address) void PPCDebugInterface::RunTo(u32 address) { auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); - breakpoints.Add(address, true); + breakpoints.SetTemporary(address); m_system.GetCPU().SetStepping(false); } diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 583693c21a..7a161b0bf7 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -244,6 +244,8 @@ bool CPUManager::SetStateLocked(State s) { if (m_state == State::PowerDown) return false; + if (s == State::Stepping) + m_system.GetPowerPC().GetBreakPoints().ClearTemporary(); m_state = s; return true; } diff --git a/Source/Core/Core/PowerPC/BreakPoints.cpp b/Source/Core/Core/PowerPC/BreakPoints.cpp index 24b15750e0..565dc961e5 100644 --- a/Source/Core/Core/PowerPC/BreakPoints.cpp +++ b/Source/Core/Core/PowerPC/BreakPoints.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -36,14 +37,17 @@ bool BreakPoints::IsBreakPointEnable(u32 address) const return bp != nullptr && bp->is_enabled; } -bool BreakPoints::IsTempBreakPoint(u32 address) const +const TBreakPoint* BreakPoints::GetBreakpoint(u32 address) const { - return std::any_of(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp) { - return bp.address == address && bp.is_temporary; - }); + // Give priority to the temporary breakpoint (it could be in the same address of a regular + // breakpoint that doesn't break) + if (m_temp_breakpoint && m_temp_breakpoint->address == address) + return &*m_temp_breakpoint; + + return GetRegularBreakpoint(address); } -const TBreakPoint* BreakPoints::GetBreakpoint(u32 address) const +const TBreakPoint* BreakPoints::GetRegularBreakpoint(u32 address) const { auto bp = std::find_if(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp_) { return bp_.address == address; }); @@ -59,21 +63,18 @@ BreakPoints::TBreakPointsStr BreakPoints::GetStrings() const TBreakPointsStr bp_strings; for (const TBreakPoint& bp : m_breakpoints) { - if (!bp.is_temporary) - { - std::ostringstream ss; - ss.imbue(std::locale::classic()); - ss << fmt::format("${:08x} ", bp.address); - if (bp.is_enabled) - ss << "n"; - if (bp.log_on_hit) - ss << "l"; - if (bp.break_on_hit) - ss << "b"; - if (bp.condition) - ss << "c " << bp.condition->GetText(); - bp_strings.emplace_back(ss.str()); - } + std::ostringstream ss; + ss.imbue(std::locale::classic()); + ss << fmt::format("${:08x} ", bp.address); + if (bp.is_enabled) + ss << "n"; + if (bp.log_on_hit) + ss << "l"; + if (bp.break_on_hit) + ss << "b"; + if (bp.condition) + ss << "c " << bp.condition->GetText(); + bp_strings.emplace_back(ss.str()); } return bp_strings; @@ -102,7 +103,6 @@ void BreakPoints::AddFromStrings(const TBreakPointsStr& bp_strings) std::getline(iss, condition); bp.condition = Expression::TryParse(condition); } - bp.is_temporary = false; Add(std::move(bp)); } } @@ -117,12 +117,12 @@ void BreakPoints::Add(TBreakPoint bp) m_breakpoints.emplace_back(std::move(bp)); } -void BreakPoints::Add(u32 address, bool temp) +void BreakPoints::Add(u32 address) { - BreakPoints::Add(address, temp, true, false, std::nullopt); + BreakPoints::Add(address, true, false, std::nullopt); } -void BreakPoints::Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit, +void BreakPoints::Add(u32 address, bool break_on_hit, bool log_on_hit, std::optional condition) { // Check for existing breakpoint, and overwrite with new info. @@ -132,7 +132,6 @@ void BreakPoints::Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit TBreakPoint bp; // breakpoint settings bp.is_enabled = true; - bp.is_temporary = temp; bp.break_on_hit = break_on_hit; bp.log_on_hit = log_on_hit; bp.address = address; @@ -151,6 +150,20 @@ void BreakPoints::Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit m_system.GetJitInterface().InvalidateICache(address, 4, true); } +void BreakPoints::SetTemporary(u32 address) +{ + TBreakPoint bp; // breakpoint settings + bp.is_enabled = true; + bp.break_on_hit = true; + bp.log_on_hit = false; + bp.address = address; + bp.condition = std::nullopt; + + m_temp_breakpoint.emplace(std::move(bp)); + + m_system.GetJitInterface().InvalidateICache(address, 4, true); +} + bool BreakPoints::ToggleBreakPoint(u32 address) { if (!Remove(address)) @@ -195,22 +208,15 @@ void BreakPoints::Clear() } m_breakpoints.clear(); + ClearTemporary(); } -void BreakPoints::ClearAllTemporary() +void BreakPoints::ClearTemporary() { - auto bp = m_breakpoints.begin(); - while (bp != m_breakpoints.end()) + if (m_temp_breakpoint) { - if (bp->is_temporary) - { - m_system.GetJitInterface().InvalidateICache(bp->address, 4, true); - bp = m_breakpoints.erase(bp); - } - else - { - ++bp; - } + m_system.GetJitInterface().InvalidateICache(m_temp_breakpoint->address, 4, true); + m_temp_breakpoint.reset(); } } diff --git a/Source/Core/Core/PowerPC/BreakPoints.h b/Source/Core/Core/PowerPC/BreakPoints.h index 6c60b8a24e..9b07fb53c0 100644 --- a/Source/Core/Core/PowerPC/BreakPoints.h +++ b/Source/Core/Core/PowerPC/BreakPoints.h @@ -20,7 +20,6 @@ struct TBreakPoint { u32 address = 0; bool is_enabled = false; - bool is_temporary = false; bool log_on_hit = false; bool break_on_hit = false; std::optional condition; @@ -68,14 +67,21 @@ public: bool IsAddressBreakPoint(u32 address) const; bool IsBreakPointEnable(u32 adresss) const; - bool IsTempBreakPoint(u32 address) const; + // Get the breakpoint in this address (for most purposes) const TBreakPoint* GetBreakpoint(u32 address) const; + // Get the breakpoint in this address (ignore temporary breakpoint, e.g. for editing purposes) + const TBreakPoint* GetRegularBreakpoint(u32 address) const; // Add BreakPoint. If one already exists on the same address, replace it. - void Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit, - std::optional condition); - void Add(u32 address, bool temp = false); + void Add(u32 address, bool break_on_hit, bool log_on_hit, std::optional condition); + void Add(u32 address); void Add(TBreakPoint bp); + // Add temporary breakpoint (e.g., Step Over, Run to Here) + // It can be on the same address of a regular breakpoint (it will have priority in this case) + // It's cleared whenever the emulation is paused for any reason + // (CPUManager::SetStateLocked(State::Paused)) + // TODO: Should it somehow force to resume emulation when called? + void SetTemporary(u32 address); bool ToggleBreakPoint(u32 address); bool ToggleEnable(u32 address); @@ -83,10 +89,11 @@ public: // Remove Breakpoint. Returns whether it was removed. bool Remove(u32 address); void Clear(); - void ClearAllTemporary(); + void ClearTemporary(); private: TBreakPoints m_breakpoints; + std::optional m_temp_breakpoint; Core::System& m_system; }; diff --git a/Source/Core/Core/PowerPC/PowerPC.cpp b/Source/Core/Core/PowerPC/PowerPC.cpp index e3f66d8895..e97db77fc7 100644 --- a/Source/Core/Core/PowerPC/PowerPC.cpp +++ b/Source/Core/Core/PowerPC/PowerPC.cpp @@ -270,9 +270,6 @@ void PowerPCManager::Init(CPUCore cpu_core) auto& memory = m_system.GetMemory(); m_ppc_state.iCache.Init(memory); m_ppc_state.dCache.Init(memory); - - if (Config::Get(Config::MAIN_ENABLE_DEBUGGING)) - m_breakpoints.ClearAllTemporary(); } void PowerPCManager::Reset() diff --git a/Source/Core/DolphinQt/Debugger/BranchWatchDialog.cpp b/Source/Core/DolphinQt/Debugger/BranchWatchDialog.cpp index f16afbf51f..4b6233678c 100644 --- a/Source/Core/DolphinQt/Debugger/BranchWatchDialog.cpp +++ b/Source/Core/DolphinQt/Debugger/BranchWatchDialog.cpp @@ -1021,7 +1021,7 @@ void BranchWatchDialog::SetBreakpoints(bool break_on_hit, bool log_on_hit) const for (const QModelIndex& index : m_index_list_temp) { const u32 address = m_table_proxy->data(index, UserRole::ClickRole).value(); - breakpoints.Add(address, false, break_on_hit, log_on_hit, {}); + breakpoints.Add(address, break_on_hit, log_on_hit, {}); } emit m_code_widget->BreakpointsChanged(); m_code_widget->Update(); @@ -1111,11 +1111,9 @@ QMenu* BranchWatchDialog::GetTableContextMenu(const QModelIndex& index) for (auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); const QModelIndex& idx : m_index_list_temp) { - if (const TBreakPoint* bp = - breakpoints.GetBreakpoint(m_table_proxy->data(idx, UserRole::ClickRole).value())) + if (const TBreakPoint* bp = breakpoints.GetRegularBreakpoint( + m_table_proxy->data(idx, UserRole::ClickRole).value())) { - if (bp->is_temporary) - continue; if (bp->break_on_hit && bp->log_on_hit) { bp_both_count += 1; diff --git a/Source/Core/DolphinQt/Debugger/BreakpointDialog.cpp b/Source/Core/DolphinQt/Debugger/BreakpointDialog.cpp index 1899b42168..ba7fe1feb5 100644 --- a/Source/Core/DolphinQt/Debugger/BreakpointDialog.cpp +++ b/Source/Core/DolphinQt/Debugger/BreakpointDialog.cpp @@ -289,7 +289,7 @@ void BreakpointDialog::accept() return; } - m_parent->AddBP(address, false, do_break, do_log, condition); + m_parent->AddBP(address, do_break, do_log, condition); } else { diff --git a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp index 3bed35c576..0bf6c5b29c 100644 --- a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp @@ -443,8 +443,8 @@ void BreakpointWidget::OnEditBreakpoint(u32 address, bool is_instruction_bp) { if (is_instruction_bp) { - auto* dialog = - new BreakpointDialog(this, m_system.GetPowerPC().GetBreakPoints().GetBreakpoint(address)); + auto* dialog = new BreakpointDialog( + this, m_system.GetPowerPC().GetBreakPoints().GetRegularBreakpoint(address)); dialog->setAttribute(Qt::WA_DeleteOnClose, true); SetQWidgetWindowDecorations(dialog); dialog->exec(); @@ -602,14 +602,13 @@ void BreakpointWidget::OnItemChanged(QTableWidgetItem* item) void BreakpointWidget::AddBP(u32 addr) { - AddBP(addr, false, true, true, {}); + AddBP(addr, true, true, {}); } -void BreakpointWidget::AddBP(u32 addr, bool temp, bool break_on_hit, bool log_on_hit, - const QString& condition) +void BreakpointWidget::AddBP(u32 addr, bool break_on_hit, bool log_on_hit, const QString& condition) { m_system.GetPowerPC().GetBreakPoints().Add( - addr, temp, break_on_hit, log_on_hit, + addr, break_on_hit, log_on_hit, !condition.isEmpty() ? Expression::TryParse(condition.toUtf8().constData()) : std::nullopt); emit BreakpointsChanged(); @@ -619,7 +618,7 @@ void BreakpointWidget::AddBP(u32 addr, bool temp, bool break_on_hit, bool log_on void BreakpointWidget::EditBreakpoint(u32 address, int edit, std::optional string) { TBreakPoint bp; - const TBreakPoint* old_bp = m_system.GetPowerPC().GetBreakPoints().GetBreakpoint(address); + const TBreakPoint* old_bp = m_system.GetPowerPC().GetBreakPoints().GetRegularBreakpoint(address); bp.is_enabled = edit == ENABLED_COLUMN ? !old_bp->is_enabled : old_bp->is_enabled; bp.log_on_hit = edit == LOG_COLUMN ? !old_bp->log_on_hit : old_bp->log_on_hit; bp.break_on_hit = edit == BREAK_COLUMN ? !old_bp->break_on_hit : old_bp->break_on_hit; diff --git a/Source/Core/DolphinQt/Debugger/BreakpointWidget.h b/Source/Core/DolphinQt/Debugger/BreakpointWidget.h index 3e204b41c1..1689270d44 100644 --- a/Source/Core/DolphinQt/Debugger/BreakpointWidget.h +++ b/Source/Core/DolphinQt/Debugger/BreakpointWidget.h @@ -34,7 +34,7 @@ public: ~BreakpointWidget(); void AddBP(u32 addr); - void AddBP(u32 addr, bool temp, bool break_on_hit, bool log_on_hit, const QString& condition); + void AddBP(u32 addr, bool break_on_hit, bool log_on_hit, const QString& condition); void AddAddressMBP(u32 addr, bool on_read = true, bool on_write = true, bool do_log = true, bool do_break = true, const QString& condition = {}); void AddRangedMBP(u32 from, u32 to, bool do_read = true, bool do_write = true, bool do_log = true, diff --git a/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp index 36e7ade321..6e7e12dcd2 100644 --- a/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp @@ -382,10 +382,11 @@ void CodeViewWidget::Update(const Core::CPUThreadGuard* guard) if (ins == "blr") ins_item->setForeground(dark_theme ? QColor(0xa0FFa0) : Qt::darkGreen); - if (debug_interface.IsBreakpoint(addr)) + const TBreakPoint* bp = power_pc.GetBreakPoints().GetRegularBreakpoint(addr); + if (bp != nullptr) { auto icon = Resources::GetThemeIcon("debugger_breakpoint").pixmap(QSize(rowh - 2, rowh - 2)); - if (!power_pc.GetBreakPoints().IsBreakPointEnable(addr)) + if (!bp->is_enabled) { QPixmap disabled_icon(icon.size()); disabled_icon.fill(Qt::transparent); diff --git a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp index 361f142c7c..36d3d92284 100644 --- a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp @@ -455,7 +455,6 @@ void CodeWidget::Step() auto& power_pc = m_system.GetPowerPC(); PowerPC::CoreMode old_mode = power_pc.GetMode(); power_pc.SetMode(PowerPC::CoreMode::Interpreter); - power_pc.GetBreakPoints().ClearAllTemporary(); cpu.StepOpcode(&sync_event); sync_event.WaitFor(std::chrono::milliseconds(20)); power_pc.SetMode(old_mode); @@ -482,8 +481,7 @@ void CodeWidget::StepOver() if (inst.LK) { auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); - breakpoints.ClearAllTemporary(); - breakpoints.Add(m_system.GetPPCState().pc + 4, true); + breakpoints.SetTemporary(m_system.GetPPCState().pc + 4); cpu.SetStepping(false); Core::DisplayMessage(tr("Step over in progress...").toStdString(), 2000); } @@ -519,12 +517,9 @@ void CodeWidget::StepOut() auto& power_pc = m_system.GetPowerPC(); auto& ppc_state = power_pc.GetPPCState(); - auto& breakpoints = power_pc.GetBreakPoints(); { Core::CPUThreadGuard guard(m_system); - breakpoints.ClearAllTemporary(); - PowerPC::CoreMode old_mode = power_pc.GetMode(); power_pc.SetMode(PowerPC::CoreMode::Interpreter); From 719af828e57fa032536953028c5906bfbed7f121 Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:46:28 +0200 Subject: [PATCH 5/6] BreakpointWidget: Can create new breakpoints when emulation isn't running It works perfectly fine, so why not? Also, consistency with CodeViewWidget. --- Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp index 0bf6c5b29c..5b983ceb78 100644 --- a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp @@ -173,7 +173,6 @@ void BreakpointWidget::CreateWidgets() m_load = m_toolbar->addAction(tr("Load"), this, &BreakpointWidget::OnLoad); m_save = m_toolbar->addAction(tr("Save"), this, &BreakpointWidget::OnSave); - m_new->setEnabled(false); m_load->setEnabled(false); m_save->setEnabled(false); @@ -252,7 +251,6 @@ void BreakpointWidget::UpdateButtonsEnabled() return; const bool is_initialised = Core::GetState(m_system) != Core::State::Uninitialized; - m_new->setEnabled(is_initialised); m_load->setEnabled(is_initialised); m_save->setEnabled(is_initialised); } From 5b13903e6a7e6ffcc707005692c3418d14e98137 Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 18:20:04 +0200 Subject: [PATCH 6/6] Intepreter: Step before checking for breakpoints This way, by pressing Continue on top of a breakpoint, the emulation will actually continue (like on Cached Interpreter and JIT), instead of doing nothing. --- Source/Core/Core/HW/CPU.cpp | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/Source/Core/Core/HW/CPU.cpp b/Source/Core/Core/HW/CPU.cpp index 7a161b0bf7..1eae912476 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -85,23 +85,20 @@ void CPUManager::Run() m_state_cpu_thread_active = true; state_lock.unlock(); - // Adjust PC for JIT when debugging + // Adjust PC 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) + if (power_pc.GetBreakPoints().IsAddressBreakPoint(power_pc.GetPPCState().pc) || + power_pc.GetMemChecks().HasAny()) { - if (power_pc.GetBreakPoints().IsAddressBreakPoint(power_pc.GetPPCState().pc) || - power_pc.GetMemChecks().HasAny()) - { - m_state = State::Stepping; - PowerPC::CoreMode old_mode = power_pc.GetMode(); - power_pc.SetMode(PowerPC::CoreMode::Interpreter); - power_pc.SingleStep(); - power_pc.SetMode(old_mode); - m_state = State::Running; - } + m_state = State::Stepping; + PowerPC::CoreMode old_mode = power_pc.GetMode(); + power_pc.SetMode(PowerPC::CoreMode::Interpreter); + power_pc.SingleStep(); + power_pc.SetMode(old_mode); + m_state = State::Running; } // Enter a fast runloop