Debugger BreakpointWidget: Allow editing breakpoints

Source/Core/Core/PowerPC/BreakPoints.cpp

@@ -38,11 +38,10 @@ bool BreakPoints::IsTempBreakPoint(u32 address) const
 
 const TBreakPoint* BreakPoints::GetBreakpoint(u32 address) const
 {
-  auto bp = std::find_if(m_breakpoints.begin(), m_breakpoints.end(), [address](const auto& bp) {
-    return bp.is_enabled && bp.address == address;
-  });
+  auto bp = std::find_if(m_breakpoints.begin(), m_breakpoints.end(),
+                         [address](const auto& bp) { return bp.address == address; });
 
-  if (bp == m_breakpoints.end() || !EvaluateCondition(bp->condition))
+  if (bp == m_breakpoints.end())
     return nullptr;
 
   return &*bp;
@@ -120,9 +119,10 @@ void BreakPoints::Add(u32 address, bool temp)
 void BreakPoints::Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit,
                       std::optional<Expression> condition)
 {
-  // Only add new addresses
-  if (IsAddressBreakPoint(address))
-    return;
+  // Check for existing breakpoint, and overwrite with new info.
+  // This is assuming we usually want the new breakpoint over an old one.
+  auto iter = std::find_if(m_breakpoints.begin(), m_breakpoints.end(),
+                           [address](const auto& bp) { return bp.address == address; });
 
   TBreakPoint bp;  // breakpoint settings
   bp.is_enabled = true;
@@ -132,7 +132,15 @@ void BreakPoints::Add(u32 address, bool temp, bool break_on_hit, bool log_on_hit
   bp.address = address;
   bp.condition = std::move(condition);
 
-  m_breakpoints.emplace_back(std::move(bp));
+  if (iter != m_breakpoints.end())  // We found an existing breakpoint
+  {
+    bp.is_enabled = iter->is_enabled;
+    *iter = std::move(bp);
+  }
+  else
+  {
+    m_breakpoints.emplace_back(std::move(bp));
+  }
 
   JitInterface::InvalidateICache(address, 4, true);
 }
@@ -229,12 +237,25 @@ void MemChecks::AddFromStrings(const TMemChecksStr& mc_strings)
 
 void MemChecks::Add(const TMemCheck& memory_check)
 {
-  if (GetMemCheck(memory_check.start_address) != nullptr)
-    return;
-
   bool had_any = HasAny();
   Core::RunAsCPUThread([&] {
-    m_mem_checks.push_back(memory_check);
+    // Check for existing breakpoint, and overwrite with new info.
+    // This is assuming we usually want the new breakpoint over an old one.
+    const u32 address = memory_check.start_address;
+    auto old_mem_check =
+        std::find_if(m_mem_checks.begin(), m_mem_checks.end(),
+                     [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
+      *old_mem_check = std::move(memory_check);
+      old_mem_check->is_enabled = is_enabled;
+      old_mem_check->num_hits = 0;
+    }
+    else
+    {
+      m_mem_checks.push_back(memory_check);
+    }
     // If this is the first one, clear the JIT cache so it can switch to
     // watchpoint-compatible code.
     if (!had_any)

Source/Core/Core/PowerPC/PowerPC.cpp

@@ -612,7 +612,7 @@ void CheckBreakPoints()
 {
   const TBreakPoint* bp = PowerPC::breakpoints.GetBreakpoint(PC);
 
-  if (bp == nullptr)
+  if (!bp || !bp->is_enabled || !EvaluateCondition(bp->condition))
     return;
 
   if (bp->break_on_hit)

Source/Core/DolphinQt/CMakeLists.txt

@@ -186,6 +186,8 @@ add_executable(dolphin-emu
   Config/WiimoteControllersWidget.h
   ConvertDialog.cpp
   ConvertDialog.h
+  Debugger/BreakpointDialog.cpp
+  Debugger/BreakpointDialog.h
   Debugger/BreakpointWidget.cpp
   Debugger/BreakpointWidget.h
   Debugger/CodeDiffDialog.cpp
@@ -202,8 +204,6 @@ add_executable(dolphin-emu
   Debugger/MemoryWidget.h
   Debugger/NetworkWidget.cpp
   Debugger/NetworkWidget.h
-  Debugger/NewBreakpointDialog.cpp
-  Debugger/NewBreakpointDialog.h
   Debugger/PatchInstructionDialog.cpp
   Debugger/PatchInstructionDialog.h
   Debugger/RegisterColumn.cpp

Source/Core/DolphinQt/Debugger/BreakpointDialog.cpp

@@ -1,7 +1,7 @@
 // Copyright 2017 Dolphin Emulator Project
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-#include "DolphinQt/Debugger/NewBreakpointDialog.h"
+#include "DolphinQt/Debugger/BreakpointDialog.h"
 
 #include <QButtonGroup>
 #include <QCheckBox>
@@ -15,12 +15,14 @@
 #include <QRadioButton>
 #include <QVBoxLayout>
 
+#include "Core/PowerPC/BreakPoints.h"
 #include "Core/PowerPC/Expression.h"
+#include "Core/PowerPC/PowerPC.h"
 #include "DolphinQt/Debugger/BreakpointWidget.h"
 #include "DolphinQt/QtUtils/ModalMessageBox.h"
 
-NewBreakpointDialog::NewBreakpointDialog(BreakpointWidget* parent)
-    : QDialog(parent), m_parent(parent)
+BreakpointDialog::BreakpointDialog(BreakpointWidget* parent)
+    : QDialog(parent), m_parent(parent), m_open_mode(OpenMode::New)
 {
   setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint);
   setWindowTitle(tr("New Breakpoint"));
@@ -31,33 +33,91 @@ NewBreakpointDialog::NewBreakpointDialog(BreakpointWidget* parent)
   OnAddressTypeChanged();
 }
 
-void NewBreakpointDialog::CreateWidgets()
+BreakpointDialog::BreakpointDialog(BreakpointWidget* parent, const TBreakPoint* breakpoint)
+    : QDialog(parent), m_parent(parent), m_open_mode(OpenMode::EditBreakPoint)
+{
+  setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint);
+  setWindowTitle(tr("Edit Breakpoint"));
+  CreateWidgets();
+  ConnectWidgets();
+
+  m_instruction_address->setText(QString::number(breakpoint->address, 16));
+  if (breakpoint->condition)
+    m_instruction_condition->setText(QString::fromStdString(breakpoint->condition->GetText()));
+
+  m_do_break->setChecked(breakpoint->break_on_hit && !breakpoint->log_on_hit);
+  m_do_log->setChecked(!breakpoint->break_on_hit && breakpoint->log_on_hit);
+  m_do_log_and_break->setChecked(breakpoint->break_on_hit && breakpoint->log_on_hit);
+
+  OnBPTypeChanged();
+  OnAddressTypeChanged();
+}
+
+BreakpointDialog::BreakpointDialog(BreakpointWidget* parent, const TMemCheck* memcheck)
+    : QDialog(parent), m_parent(parent), m_open_mode(OpenMode::EditMemCheck)
+{
+  setWindowFlags(windowFlags() & ~Qt::WindowContextHelpButtonHint);
+  setWindowTitle(tr("Edit Breakpoint"));
+
+  CreateWidgets();
+  ConnectWidgets();
+
+  m_memory_address_from->setText(QString::number(memcheck->start_address, 16));
+  if (memcheck->is_ranged)
+  {
+    m_memory_use_address->setChecked(false);
+    m_memory_use_range->setChecked(true);
+    m_memory_address_to->setText(QString::number(memcheck->end_address, 16));
+  }
+  else
+  {
+    m_memory_address_to->setText(QString::number(memcheck->start_address + 1, 16));
+  }
+
+  m_memory_on_read->setChecked(memcheck->is_break_on_read && !memcheck->is_break_on_write);
+  m_memory_on_write->setChecked(!memcheck->is_break_on_read && memcheck->is_break_on_write);
+  m_memory_on_read_and_write->setChecked(memcheck->is_break_on_read && memcheck->is_break_on_write);
+
+  m_do_break->setChecked(memcheck->break_on_hit && !memcheck->log_on_hit);
+  m_do_log->setChecked(!memcheck->break_on_hit && memcheck->log_on_hit);
+  m_do_log_and_break->setChecked(memcheck->break_on_hit && memcheck->log_on_hit);
+
+  OnBPTypeChanged();
+  OnAddressTypeChanged();
+}
+
+void BreakpointDialog::CreateWidgets()
 {
   m_buttons = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
   auto* type_group = new QButtonGroup(this);
 
   // Instruction BP
-  auto* top_layout = new QHBoxLayout;
+  auto* instruction_widget = new QWidget;
+  auto* instruction_layout = new QGridLayout;
+
   m_instruction_bp = new QRadioButton(tr("Instruction Breakpoint"));
-  m_instruction_bp->setChecked(true);
   type_group->addButton(m_instruction_bp);
   m_instruction_box = new QGroupBox;
   m_instruction_address = new QLineEdit;
   m_instruction_condition = new QLineEdit;
   m_cond_help_btn = new QPushButton(tr("Help"));
 
-  top_layout->addWidget(m_instruction_bp);
-  top_layout->addStretch();
-  top_layout->addWidget(m_cond_help_btn);
+  auto* instruction_data_layout = new QGridLayout;
+  m_instruction_box->setLayout(instruction_data_layout);
+  instruction_data_layout->addWidget(new QLabel(tr("Address:")), 0, 0);
+  instruction_data_layout->addWidget(m_instruction_address, 0, 1);
+  instruction_data_layout->addWidget(new QLabel(tr("Condition:")), 1, 0);
+  instruction_data_layout->addWidget(m_instruction_condition, 1, 1);
 
-  auto* instruction_layout = new QGridLayout;
-  m_instruction_box->setLayout(instruction_layout);
-  instruction_layout->addWidget(new QLabel(tr("Address:")), 0, 0);
-  instruction_layout->addWidget(m_instruction_address, 0, 1);
-  instruction_layout->addWidget(new QLabel(tr("Condition:")), 1, 0);
-  instruction_layout->addWidget(m_instruction_condition, 1, 1);
+  instruction_layout->addWidget(m_instruction_bp, 0, 0, 1, 1);
+  instruction_layout->addWidget(m_cond_help_btn, 0, 1, 1, 1);
+  instruction_layout->addWidget(m_instruction_box, 1, 0, 1, 2);
+  instruction_widget->setLayout(instruction_layout);
 
   // Memory BP
+  auto* memory_widget = new QWidget;
+  auto* memory_layout = new QGridLayout;
+
   m_memory_bp = new QRadioButton(tr("Memory Breakpoint"));
   type_group->addButton(m_memory_bp);
   m_memory_box = new QGroupBox;
@@ -87,23 +147,27 @@ void NewBreakpointDialog::CreateWidgets()
   m_do_log_and_break = new QRadioButton(tr("Write to Log and Break"));
   m_do_log_and_break->setChecked(true);
 
-  auto* memory_layout = new QGridLayout;
-  m_memory_box->setLayout(memory_layout);
-  memory_layout->addWidget(m_memory_use_address, 0, 0);
-  memory_layout->addWidget(m_memory_use_range, 0, 3);
-  memory_layout->addWidget(m_memory_address_from_label, 1, 0);
-  memory_layout->addWidget(m_memory_address_from, 1, 1);
-  memory_layout->addWidget(m_memory_address_to_label, 1, 2);
-  memory_layout->addWidget(m_memory_address_to, 1, 3);
+  auto* memory_data_layout = new QGridLayout;
+  m_memory_box->setLayout(memory_data_layout);
+  memory_data_layout->addWidget(m_memory_use_address, 0, 0);
+  memory_data_layout->addWidget(m_memory_use_range, 0, 3);
+  memory_data_layout->addWidget(m_memory_address_from_label, 1, 0);
+  memory_data_layout->addWidget(m_memory_address_from, 1, 1);
+  memory_data_layout->addWidget(m_memory_address_to_label, 1, 2);
+  memory_data_layout->addWidget(m_memory_address_to, 1, 3);
   QGroupBox* condition_box = new QGroupBox(tr("Condition"));
   auto* condition_layout = new QHBoxLayout;
   condition_box->setLayout(condition_layout);
 
-  memory_layout->addWidget(condition_box, 2, 0, 1, -1);
+  memory_data_layout->addWidget(condition_box, 2, 0, 1, -1);
   condition_layout->addWidget(m_memory_on_read);
   condition_layout->addWidget(m_memory_on_write);
   condition_layout->addWidget(m_memory_on_read_and_write);
 
+  memory_layout->addWidget(m_memory_bp, 0, 0);
+  memory_layout->addWidget(m_memory_box, 1, 0);
+  memory_widget->setLayout(memory_layout);
+
   QGroupBox* action_box = new QGroupBox(tr("Action"));
   auto* action_layout = new QHBoxLayout;
   action_box->setLayout(action_layout);
@@ -112,42 +176,58 @@ void NewBreakpointDialog::CreateWidgets()
   action_layout->addWidget(m_do_log_and_break);
 
   auto* layout = new QVBoxLayout;
-
-  layout->addLayout(top_layout);
-  layout->addWidget(m_instruction_box);
-  layout->addWidget(m_memory_bp);
-  layout->addWidget(m_memory_box);
+  layout->addWidget(instruction_widget);
+  layout->addWidget(memory_widget);
   layout->addWidget(action_box);
   layout->addWidget(m_buttons);
 
-  setLayout(layout);
+  switch (m_open_mode)
+  {
+  case OpenMode::New:
+    m_instruction_bp->setChecked(true);
+    m_instruction_address->setFocus();
+    break;
+  case OpenMode::EditBreakPoint:
+    memory_widget->setVisible(false);
+    m_instruction_bp->setChecked(true);
+    m_instruction_address->setEnabled(false);
+    m_instruction_address->setFocus();
+    break;
+  case OpenMode::EditMemCheck:
+    instruction_widget->setVisible(false);
+    m_cond_help_btn->setVisible(false);
+    m_memory_bp->setChecked(true);
+    m_memory_address_from->setEnabled(false);
+    m_memory_address_to->setFocus();
+    break;
+  }
 
-  m_instruction_address->setFocus();
+  setLayout(layout);
 }
 
-void NewBreakpointDialog::ConnectWidgets()
+void BreakpointDialog::ConnectWidgets()
 {
-  connect(m_buttons, &QDialogButtonBox::accepted, this, &NewBreakpointDialog::accept);
-  connect(m_buttons, &QDialogButtonBox::rejected, this, &NewBreakpointDialog::reject);
+  connect(m_buttons, &QDialogButtonBox::accepted, this, &BreakpointDialog::accept);
+  connect(m_buttons, &QDialogButtonBox::rejected, this, &BreakpointDialog::reject);
 
-  connect(m_cond_help_btn, &QPushButton::clicked, this, &NewBreakpointDialog::ShowConditionHelp);
+  connect(m_cond_help_btn, &QPushButton::clicked, this, &BreakpointDialog::ShowConditionHelp);
 
-  connect(m_instruction_bp, &QRadioButton::toggled, this, &NewBreakpointDialog::OnBPTypeChanged);
-  connect(m_memory_bp, &QRadioButton::toggled, this, &NewBreakpointDialog::OnBPTypeChanged);
+  connect(m_instruction_bp, &QRadioButton::toggled, this, &BreakpointDialog::OnBPTypeChanged);
+  connect(m_memory_bp, &QRadioButton::toggled, this, &BreakpointDialog::OnBPTypeChanged);
 
   connect(m_memory_use_address, &QRadioButton::toggled, this,
-          &NewBreakpointDialog::OnAddressTypeChanged);
+          &BreakpointDialog::OnAddressTypeChanged);
   connect(m_memory_use_range, &QRadioButton::toggled, this,
-          &NewBreakpointDialog::OnAddressTypeChanged);
+          &BreakpointDialog::OnAddressTypeChanged);
 }
 
-void NewBreakpointDialog::OnBPTypeChanged()
+void BreakpointDialog::OnBPTypeChanged()
 {
   m_instruction_box->setEnabled(m_instruction_bp->isChecked());
   m_memory_box->setEnabled(m_memory_bp->isChecked());
 }
 
-void NewBreakpointDialog::OnAddressTypeChanged()
+void BreakpointDialog::OnAddressTypeChanged()
 {
   bool ranged = m_memory_use_range->isChecked();
 
@@ -157,7 +237,7 @@ void NewBreakpointDialog::OnAddressTypeChanged()
   m_memory_address_from_label->setText(ranged ? tr("From:") : tr("Address:"));
 }
 
-void NewBreakpointDialog::accept()
+void BreakpointDialog::accept()
 {
   auto invalid_input = [this](QString field) {
     ModalMessageBox::critical(this, tr("Error"),
@@ -227,7 +307,7 @@ void NewBreakpointDialog::accept()
   QDialog::accept();
 }
 
-void NewBreakpointDialog::ShowConditionHelp()
+void BreakpointDialog::ShowConditionHelp()
 {
   const auto message = QStringLiteral(
       "Set a code breakpoint for when an instruction is executed. Use with the code widget.\n"

Source/Core/DolphinQt/Debugger/BreakpointDialog.h

@@ -6,6 +6,8 @@
 #include <QDialog>
 
 #include "Common/CommonTypes.h"
+#include "Core/PowerPC/BreakPoints.h"
+#include "Core/PowerPC/PowerPC.h"
 
 class BreakpointWidget;
 class QCheckBox;
@@ -15,11 +17,20 @@ class QLabel;
 class QLineEdit;
 class QRadioButton;
 
-class NewBreakpointDialog : public QDialog
+class BreakpointDialog : public QDialog
 {
   Q_OBJECT
 public:
-  explicit NewBreakpointDialog(BreakpointWidget* parent);
+  enum class OpenMode
+  {
+    New,
+    EditBreakPoint,
+    EditMemCheck
+  };
+
+  explicit BreakpointDialog(BreakpointWidget* parent);
+  BreakpointDialog(BreakpointWidget* parent, const TBreakPoint* breakpoint);
+  BreakpointDialog(BreakpointWidget* parent, const TMemCheck* memcheck);
 
   void accept() override;
 
@@ -58,4 +69,6 @@ private:
 
   QDialogButtonBox* m_buttons;
   BreakpointWidget* m_parent;
+
+  OpenMode m_open_mode;
 };

Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp

@@ -19,8 +19,8 @@
 #include "Core/PowerPC/PPCSymbolDB.h"
 #include "Core/PowerPC/PowerPC.h"
 
+#include "DolphinQt/Debugger/BreakpointDialog.h"
 #include "DolphinQt/Debugger/MemoryWidget.h"
-#include "DolphinQt/Debugger/NewBreakpointDialog.h"
 #include "DolphinQt/Resources.h"
 #include "DolphinQt/Settings.h"
 
@@ -299,10 +299,27 @@ void BreakpointWidget::OnClear()
 
 void BreakpointWidget::OnNewBreakpoint()
 {
-  NewBreakpointDialog* dialog = new NewBreakpointDialog(this);
+  BreakpointDialog* dialog = new BreakpointDialog(this);
   dialog->exec();
 }
 
+void BreakpointWidget::OnEditBreakpoint(u32 address, bool is_instruction_bp)
+{
+  if (is_instruction_bp)
+  {
+    auto* dialog = new BreakpointDialog(this, PowerPC::breakpoints.GetBreakpoint(address));
+    dialog->exec();
+  }
+  else
+  {
+    auto* dialog = new BreakpointDialog(this, PowerPC::memchecks.GetMemCheck(address));
+    dialog->exec();
+  }
+
+  emit BreakpointsChanged();
+  Update();
+}
+
 void BreakpointWidget::OnLoad()
 {
   IniFile ini;
@@ -389,6 +406,9 @@ void BreakpointWidget::OnContextMenu()
       Update();
     });
   }
+  menu->addAction(tr("Edit..."), [this, bp_address, is_memory_breakpoint] {
+    OnEditBreakpoint(bp_address, !is_memory_breakpoint);
+  });
 
   menu->exec(QCursor::pos());
 }

Source/Core/DolphinQt/Debugger/BreakpointWidget.h

@@ -44,6 +44,7 @@ private:
   void OnDelete();
   void OnClear();
   void OnNewBreakpoint();
+  void OnEditBreakpoint(u32 address, bool is_instruction_bp);
   void OnLoad();
   void OnSave();
   void OnContextMenu();

Source/Core/DolphinQt/DolphinQt.vcxproj

@@ -126,6 +126,7 @@
     <ClCompile Include="Config\VerifyWidget.cpp" />
     <ClCompile Include="Config\WiimoteControllersWidget.cpp" />
     <ClCompile Include="ConvertDialog.cpp" />
+    <ClCompile Include="Debugger\BreakpointDialog.cpp" />
     <ClCompile Include="Debugger\BreakpointWidget.cpp" />
     <ClCompile Include="Debugger\CodeDiffDialog.cpp" />
     <ClCompile Include="Debugger\CodeViewWidget.cpp" />
@@ -134,7 +135,6 @@
     <ClCompile Include="Debugger\MemoryViewWidget.cpp" />
     <ClCompile Include="Debugger\MemoryWidget.cpp" />
     <ClCompile Include="Debugger\NetworkWidget.cpp" />
-    <ClCompile Include="Debugger\NewBreakpointDialog.cpp" />
     <ClCompile Include="Debugger\PatchInstructionDialog.cpp" />
     <ClCompile Include="Debugger\RegisterColumn.cpp" />
     <ClCompile Include="Debugger\RegisterWidget.cpp" />
@@ -319,6 +319,7 @@
     <QtMoc Include="Config\VerifyWidget.h" />
     <QtMoc Include="Config\WiimoteControllersWidget.h" />
     <QtMoc Include="ConvertDialog.h" />
+    <QtMoc Include="Debugger\BreakpointDialog.h" />
     <QtMoc Include="Debugger\BreakpointWidget.h" />
     <QtMoc Include="Debugger\CodeDiffDialog.h" />
     <QtMoc Include="Debugger\CodeViewWidget.h" />
@@ -327,7 +328,6 @@
     <QtMoc Include="Debugger\MemoryViewWidget.h" />
     <QtMoc Include="Debugger\MemoryWidget.h" />
     <QtMoc Include="Debugger\NetworkWidget.h" />
-    <QtMoc Include="Debugger\NewBreakpointDialog.h" />
     <QtMoc Include="Debugger\PatchInstructionDialog.h" />
     <QtMoc Include="Debugger\RegisterWidget.h" />
     <QtMoc Include="Debugger\ThreadWidget.h" />