diff --git a/Source/Core/Core/Core.cpp b/Source/Core/Core/Core.cpp index 1a43f3189f..bf850a253d 100644 --- a/Source/Core/Core/Core.cpp +++ b/Source/Core/Core/Core.cpp @@ -709,7 +709,7 @@ void SetState(Core::System& system, State state, bool report_state_change, #endif // USE_RETRO_ACHIEVEMENTS // 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(); #ifdef USE_RETRO_ACHIEVEMENTS @@ -718,7 +718,7 @@ void SetState(Core::System& system, State state, bool report_state_change, break; case State::Running: { - system.GetCPU().EnableStepping(false); + system.GetCPU().SetStepping(false); Wiimote::Resume(); break; } @@ -821,7 +821,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/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/DebugInterface.h b/Source/Core/Core/Debugger/DebugInterface.h index 9802c31f3a..cc07d06d3b 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() {} @@ -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 9389fe76d8..786f95bfa7 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" @@ -357,12 +358,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 +375,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() @@ -506,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.SetTemporary(address); + 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 86207bc1da..797f2d5254 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; @@ -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/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..1eae912476 100644 --- a/Source/Core/Core/HW/CPU.cpp +++ b/Source/Core/Core/HW/CPU.cpp @@ -85,22 +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 @@ -174,7 +172,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) @@ -243,11 +241,13 @@ 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; } -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 +290,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/BreakPoints.cpp b/Source/Core/Core/PowerPC/BreakPoints.cpp index 1e8689dd82..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 @@ -27,24 +28,26 @@ 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; }); -} - -bool BreakPoints::IsTempBreakPoint(u32 address) const -{ - return std::any_of(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp) { - return bp.address == address && bp.is_temporary; - }); + const TBreakPoint* bp = GetBreakpoint(address); + return bp != nullptr && bp->is_enabled; } const TBreakPoint* BreakPoints::GetBreakpoint(u32 address) const +{ + // 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::GetRegularBreakpoint(u32 address) const { auto bp = std::find_if(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp_) { return bp_.address == address; }); @@ -60,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; @@ -103,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)); } } @@ -118,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. @@ -133,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; @@ -152,7 +150,31 @@ 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)) + { + 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 +186,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() @@ -184,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(); } } @@ -281,9 +298,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 +313,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 +325,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..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; @@ -66,28 +65,35 @@ 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; + // 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 - void Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit, - std::optional condition); - void Add(u32 address, bool temp = false); + // Add BreakPoint. If one already exists on the same address, replace it. + 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); - // 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(); + void ClearTemporary(); private: TBreakPoints m_breakpoints; + std::optional m_temp_breakpoint; Core::System& m_system; }; @@ -111,12 +117,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/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 df87a6c75a..c293977aeb 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); } } @@ -866,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..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() @@ -629,19 +626,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 +643,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 +726,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/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 a678889aad..200d414395 100644 --- a/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp @@ -174,7 +174,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); @@ -216,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(); @@ -253,7 +252,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); } @@ -444,8 +442,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(); @@ -603,14 +601,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(); @@ -620,7 +617,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 51f2814d9b..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); @@ -594,7 +595,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 +646,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 +870,7 @@ void CodeViewWidget::OnRunToHere() { const u32 addr = GetContextAddress(); - m_system.GetPowerPC().GetDebugInterface().SetBreakpoint(addr); - m_system.GetPowerPC().GetDebugInterface().RunToBreakpoint(); - Update(); + m_system.GetPowerPC().GetDebugInterface().RunTo(addr); } void CodeViewWidget::OnPPCComparison() @@ -1137,11 +1136,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(); diff --git a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp index e5dd96014b..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,9 +481,8 @@ void CodeWidget::StepOver() if (inst.LK) { auto& breakpoints = m_system.GetPowerPC().GetBreakPoints(); - breakpoints.ClearAllTemporary(); - breakpoints.Add(m_system.GetPPCState().pc + 4, true); - cpu.EnableStepping(false); + breakpoints.SetTemporary(m_system.GetPPCState().pc + 4); + cpu.SetStepping(false); Core::DisplayMessage(tr("Step over in progress...").toStdString(), 2000); } else @@ -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); @@ -547,8 +542,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 +550,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);