Merge pull request #11274 from TryTwo/PR_Conditional_BP_Add_Memory

Debugger: Add conditional breakpoints to memory BPs

Source/Core/Core/Debugger/PPCDebugInterface.cpp

@@ -335,7 +335,7 @@ void PPCDebugInterface::ToggleMemCheck(u32 address, bool read, bool write, bool
     MemCheck.log_on_hit = log;
     MemCheck.break_on_hit = true;
 
-    PowerPC::memchecks.Add(MemCheck);
+    PowerPC::memchecks.Add(std::move(MemCheck));
   }
   else
   {

Source/Core/Core/PowerPC/BreakPoints.cpp

@@ -83,7 +83,6 @@ void BreakPoints::AddFromStrings(const TBreakPointsStr& bp_strings)
 
     if (iss.peek() == '$')
       iss.ignore();
-
     iss >> std::hex >> bp.address;
     iss >> flags;
     bp.is_enabled = flags.find('n') != flags.npos;
@@ -203,11 +202,21 @@ MemChecks::TMemChecksStr MemChecks::GetStrings() const
   {
     std::ostringstream ss;
     ss.imbue(std::locale::classic());
+    ss << fmt::format("${:08x} {:08x} ", mc.start_address, mc.end_address);
+    if (mc.is_enabled)
+      ss << 'n';
+    if (mc.is_break_on_read)
+      ss << 'r';
+    if (mc.is_break_on_write)
+      ss << 'w';
+    if (mc.log_on_hit)
+      ss << 'l';
+    if (mc.break_on_hit)
+      ss << 'b';
+    if (mc.condition)
+      ss << "c " << mc.condition->GetText();
 
-    ss << std::hex << mc.start_address << " " << mc.end_address << " " << (mc.is_enabled ? "n" : "")
-       << (mc.is_break_on_read ? "r" : "") << (mc.is_break_on_write ? "w" : "")
-       << (mc.log_on_hit ? "l" : "") << (mc.break_on_hit ? "b" : "");
-    mc_strings.push_back(ss.str());
+    mc_strings.emplace_back(ss.str());
   }
 
   return mc_strings;
@@ -221,6 +230,9 @@ void MemChecks::AddFromStrings(const TMemChecksStr& mc_strings)
     std::istringstream iss(mc_string);
     iss.imbue(std::locale::classic());
 
+    if (iss.peek() == '$')
+      iss.ignore();
+
     std::string flags;
     iss >> std::hex >> mc.start_address >> mc.end_address >> flags;
 
@@ -230,12 +242,19 @@ void MemChecks::AddFromStrings(const TMemChecksStr& mc_strings)
     mc.is_break_on_write = flags.find('w') != flags.npos;
     mc.log_on_hit = flags.find('l') != flags.npos;
     mc.break_on_hit = flags.find('b') != flags.npos;
+    if (flags.find('c') != std::string::npos)
+    {
+      iss >> std::ws;
+      std::string condition;
+      std::getline(iss, condition);
+      mc.condition = Expression::TryParse(condition);
+    }
 
-    Add(mc);
+    Add(std::move(mc));
   }
 }
 
-void MemChecks::Add(const TMemCheck& memory_check)
+void MemChecks::Add(TMemCheck memory_check)
 {
   bool had_any = HasAny();
   Core::RunAsCPUThread([&] {
@@ -254,7 +273,7 @@ void MemChecks::Add(const TMemCheck& memory_check)
     }
     else
     {
-      m_mem_checks.push_back(memory_check);
+      m_mem_checks.emplace_back(std::move(memory_check));
     }
     // If this is the first one, clear the JIT cache so it can switch to
     // watchpoint-compatible code.
@@ -338,7 +357,8 @@ bool TMemCheck::Action(Common::DebugInterface* debug_interface, u64 value, u32 a
   if (!is_enabled)
     return false;
 
-  if ((write && is_break_on_write) || (!write && is_break_on_read))
+  if (((write && is_break_on_write) || (!write && is_break_on_read)) &&
+      EvaluateCondition(this->condition))
   {
     if (log_on_hit)
     {

Source/Core/Core/PowerPC/BreakPoints.h

@@ -42,6 +42,8 @@ struct TMemCheck
 
   u32 num_hits = 0;
 
+  std::optional<Expression> condition;
+
   // returns whether to break
   bool Action(Common::DebugInterface* debug_interface, u64 value, u32 addr, bool write, size_t size,
               u32 pc);
@@ -93,7 +95,7 @@ public:
   TMemChecksStr GetStrings() const;
   void AddFromStrings(const TMemChecksStr& mc_strings);
 
-  void Add(const TMemCheck& memory_check);
+  void Add(TMemCheck memory_check);
 
   bool ToggleBreakPoint(u32 address);
 

Source/Core/Core/PowerPC/GDBStub.cpp

@@ -871,7 +871,7 @@ static bool AddBreakpoint(BreakpointType type, u32 addr, u32 len)
     new_memcheck.break_on_hit = true;
     new_memcheck.log_on_hit = false;
     new_memcheck.is_enabled = true;
-    PowerPC::memchecks.Add(new_memcheck);
+    PowerPC::memchecks.Add(std::move(new_memcheck));
     INFO_LOG_FMT(GDB_STUB, "gdb: added {} memcheck: {:08x} bytes at {:08x}", static_cast<int>(type),
                  len, addr);
   }

Source/Core/DolphinQt/Debugger/BreakpointDialog.cpp

@@ -43,7 +43,7 @@ BreakpointDialog::BreakpointDialog(BreakpointWidget* parent, const TBreakPoint*
 
   m_instruction_address->setText(QString::number(breakpoint->address, 16));
   if (breakpoint->condition)
-    m_instruction_condition->setText(QString::fromStdString(breakpoint->condition->GetText()));
+    m_conditional->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);
@@ -73,6 +73,8 @@ BreakpointDialog::BreakpointDialog(BreakpointWidget* parent, const TMemCheck* me
   {
     m_memory_address_to->setText(QString::number(memcheck->start_address + 1, 16));
   }
+  if (memcheck->condition)
+    m_conditional->setText(QString::fromStdString(memcheck->condition->GetText()));
 
   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);
@@ -88,7 +90,8 @@ BreakpointDialog::BreakpointDialog(BreakpointWidget* parent, const TMemCheck* me
 
 void BreakpointDialog::CreateWidgets()
 {
-  m_buttons = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel);
+  m_buttons = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel |
+                                   QDialogButtonBox::Help);
   auto* type_group = new QButtonGroup(this);
 
   // Instruction BP
@@ -99,18 +102,13 @@ void BreakpointDialog::CreateWidgets()
   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"));
 
-  auto* instruction_data_layout = new QGridLayout;
+  auto* instruction_data_layout = new QHBoxLayout;
   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);
+  instruction_data_layout->addWidget(new QLabel(tr("Address:")));
+  instruction_data_layout->addWidget(m_instruction_address);
 
   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);
 
@@ -155,6 +153,7 @@ void BreakpointDialog::CreateWidgets()
   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);
@@ -169,12 +168,23 @@ void BreakpointDialog::CreateWidgets()
   memory_widget->setLayout(memory_layout);
 
   QGroupBox* action_box = new QGroupBox(tr("Action"));
+
+  QHBoxLayout* conditional_layout = new QHBoxLayout;
+  m_conditional = new QLineEdit();
+  conditional_layout->addWidget(new QLabel(tr("Condition:")));
+  conditional_layout->addWidget(m_conditional);
+
   auto* action_layout = new QHBoxLayout;
-  action_box->setLayout(action_layout);
   action_layout->addWidget(m_do_log);
   action_layout->addWidget(m_do_break);
   action_layout->addWidget(m_do_log_and_break);
 
+  auto* action_vlayout = new QVBoxLayout;
+  action_vlayout->addLayout(conditional_layout);
+  action_vlayout->addLayout(action_layout);
+
+  action_box->setLayout(action_vlayout);
+
   auto* layout = new QVBoxLayout;
   layout->addWidget(instruction_widget);
   layout->addWidget(memory_widget);
@@ -195,7 +205,6 @@ void BreakpointDialog::CreateWidgets()
     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();
@@ -209,8 +218,7 @@ void BreakpointDialog::ConnectWidgets()
 {
   connect(m_buttons, &QDialogButtonBox::accepted, this, &BreakpointDialog::accept);
   connect(m_buttons, &QDialogButtonBox::rejected, this, &BreakpointDialog::reject);
-
-  connect(m_cond_help_btn, &QPushButton::clicked, this, &BreakpointDialog::ShowConditionHelp);
+  connect(m_buttons, &QDialogButtonBox::helpRequested, this, &BreakpointDialog::ShowConditionHelp);
 
   connect(m_instruction_bp, &QRadioButton::toggled, this, &BreakpointDialog::OnBPTypeChanged);
   connect(m_memory_bp, &QRadioButton::toggled, this, &BreakpointDialog::OnBPTypeChanged);
@@ -257,6 +265,14 @@ void BreakpointDialog::accept()
 
   bool good;
 
+  const QString condition = m_conditional->text().trimmed();
+
+  if (!condition.isEmpty() && !Expression::TryParse(condition.toUtf8().constData()))
+  {
+    invalid_input(tr("Condition"));
+    return;
+  }
+
   if (instruction)
   {
     u32 address = m_instruction_address->text().toUInt(&good, 16);
@@ -267,14 +283,6 @@ void BreakpointDialog::accept()
       return;
     }
 
-    const QString condition = m_instruction_condition->text().trimmed();
-
-    if (!condition.isEmpty() && !Expression::TryParse(condition.toUtf8().constData()))
-    {
-      invalid_input(tr("Condition"));
-      return;
-    }
-
     m_parent->AddBP(address, false, do_break, do_log, condition);
   }
   else
@@ -296,11 +304,11 @@ void BreakpointDialog::accept()
         return;
       }
 
-      m_parent->AddRangedMBP(from, to, on_read, on_write, do_log, do_break);
+      m_parent->AddRangedMBP(from, to, on_read, on_write, do_log, do_break, condition);
     }
     else
     {
-      m_parent->AddAddressMBP(from, on_read, on_write, do_log, do_break);
+      m_parent->AddAddressMBP(from, on_read, on_write, do_log, do_break, condition);
     }
   }
 
@@ -310,8 +318,6 @@ void BreakpointDialog::accept()
 void BreakpointDialog::ShowConditionHelp()
 {
   const auto message = QStringLiteral(
-      "Set a code breakpoint for when an instruction is executed. Use with the code widget.\n"
-      "\n"
       "Conditions:\n"
       "Sets an expression that is evaluated when a breakpoint is hit. If the expression is false "
       "or 0, the breakpoint is ignored until hit again. Statements should be separated by a comma. "

Source/Core/DolphinQt/Debugger/BreakpointDialog.h

@@ -16,6 +16,7 @@ class QGroupBox;
 class QLabel;
 class QLineEdit;
 class QRadioButton;
+class QPushButton;
 
 class BreakpointDialog : public QDialog
 {
@@ -46,8 +47,6 @@ private:
   QRadioButton* m_instruction_bp;
   QGroupBox* m_instruction_box;
   QLineEdit* m_instruction_address;
-  QLineEdit* m_instruction_condition;
-  QPushButton* m_cond_help_btn;
 
   // Memory BPs
   QRadioButton* m_memory_bp;
@@ -67,6 +66,9 @@ private:
   QRadioButton* m_do_break;
   QRadioButton* m_do_log_and_break;
 
+  QLineEdit* m_conditional;
+  QPushButton* m_cond_help_btn;
+
   QDialogButtonBox* m_buttons;
   BreakpointWidget* m_parent;
 

Source/Core/DolphinQt/Debugger/BreakpointWidget.cpp

@@ -255,6 +255,13 @@ void BreakpointWidget::Update()
 
     m_table->setItem(i, 4, create_item(flags));
 
+    QString condition;
+
+    if (mbp.condition)
+      condition = QString::fromStdString(mbp.condition->GetText());
+
+    m_table->setItem(i, 5, create_item(condition));
+
     i++;
   }
 }
@@ -430,7 +437,7 @@ void BreakpointWidget::AddBP(u32 addr, bool temp, bool break_on_hit, bool log_on
 }
 
 void BreakpointWidget::AddAddressMBP(u32 addr, bool on_read, bool on_write, bool do_log,
-                                     bool do_break)
+                                     bool do_break, const QString& condition)
 {
   TMemCheck check;
 
@@ -441,10 +448,11 @@ void BreakpointWidget::AddAddressMBP(u32 addr, bool on_read, bool on_write, bool
   check.is_break_on_write = on_write;
   check.log_on_hit = do_log;
   check.break_on_hit = do_break;
-
+  check.condition =
+      !condition.isEmpty() ? Expression::TryParse(condition.toUtf8().constData()) : std::nullopt;
   {
     const QSignalBlocker blocker(Settings::Instance());
-    PowerPC::memchecks.Add(check);
+    PowerPC::memchecks.Add(std::move(check));
   }
 
   emit BreakpointsChanged();
@@ -452,7 +460,7 @@ void BreakpointWidget::AddAddressMBP(u32 addr, bool on_read, bool on_write, bool
 }
 
 void BreakpointWidget::AddRangedMBP(u32 from, u32 to, bool on_read, bool on_write, bool do_log,
-                                    bool do_break)
+                                    bool do_break, const QString& condition)
 {
   TMemCheck check;
 
@@ -463,10 +471,11 @@ void BreakpointWidget::AddRangedMBP(u32 from, u32 to, bool on_read, bool on_writ
   check.is_break_on_write = on_write;
   check.log_on_hit = do_log;
   check.break_on_hit = do_break;
-
+  check.condition =
+      !condition.isEmpty() ? Expression::TryParse(condition.toUtf8().constData()) : std::nullopt;
   {
     const QSignalBlocker blocker(Settings::Instance());
-    PowerPC::memchecks.Add(check);
+    PowerPC::memchecks.Add(std::move(check));
   }
 
   emit BreakpointsChanged();

Source/Core/DolphinQt/Debugger/BreakpointWidget.h

@@ -23,9 +23,9 @@ public:
   void AddBP(u32 addr);
   void AddBP(u32 addr, bool temp, 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);
+                     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,
-                    bool do_break = true);
+                    bool do_break = true, const QString& condition = {});
   void UpdateButtonsEnabled();
   void Update();
 

Source/Core/DolphinQt/Debugger/MemoryViewWidget.cpp

@@ -797,7 +797,7 @@ void MemoryViewWidget::ToggleBreakpoint(u32 addr, bool row)
       check.log_on_hit = m_do_log;
       check.break_on_hit = true;
 
-      PowerPC::memchecks.Add(check);
+      PowerPC::memchecks.Add(std::move(check));
     }
     else if (check_ptr != nullptr)
     {