From bd3cf67cbc354dc93edf14a788e602c69c054d38 Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Sat, 15 Jun 2024 11:36:38 +0200 Subject: [PATCH] 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);