Merge pull request #12977 from mitaclaw/branch-watch-tool-fixes-4

Branch Watch Tool: Refactors, Fixes, and Features

Source/Core/Core/Boot/Boot_BS2Emu.cpp

@@ -163,7 +163,7 @@ bool CBoot::RunApploader(Core::System& system, const Core::CPUThreadGuard& guard
 
   const bool resume_branch_watch = branch_watch.GetRecordingActive();
   if (system.IsBranchWatchIgnoreApploader())
-    branch_watch.Pause();
+    branch_watch.SetRecordingActive(guard, false);
 
   // Call iAppLoaderEntry.
   DEBUG_LOG_FMT(BOOT, "Call iAppLoaderEntry");
@@ -226,7 +226,7 @@ bool CBoot::RunApploader(Core::System& system, const Core::CPUThreadGuard& guard
   // return
   ppc_state.pc = ppc_state.gpr[3];
 
-  branch_watch.SetRecordingActive(resume_branch_watch);
+  branch_watch.SetRecordingActive(guard, resume_branch_watch);
 
   return true;
 }

Source/Core/Core/Debugger/BranchWatch.cpp

@@ -38,14 +38,14 @@ union USnapshotMetadata
   using Inspection = BranchWatch::SelectionInspection;
   using StorageType = unsigned long long;
 
-  static_assert(Inspection::EndOfEnumeration == Inspection{(1u << 3) + 1});
+  static_assert(Inspection::EndOfEnumeration == Inspection{(1u << 5) + 1});
 
   StorageType hex;
 
   BitField<0, 1, bool, StorageType> is_virtual;
   BitField<1, 1, bool, StorageType> condition;
   BitField<2, 1, bool, StorageType> is_selected;
-  BitField<3, 4, Inspection, StorageType> inspection;
+  BitField<3, 6, Inspection, StorageType> inspection;
 
   USnapshotMetadata() : hex(0) {}
   explicit USnapshotMetadata(bool is_virtual_, bool condition_, bool is_selected_,
@@ -69,18 +69,22 @@ void BranchWatch::Save(const CPUThreadGuard& guard, std::FILE* file) const
   if (file == nullptr)
     return;
 
+  const bool is_reduction_phase = GetRecordingPhase() == Phase::Reduction;
+
   const auto routine = [&](const Collection& collection, bool is_virtual, bool condition) {
     for (const Collection::value_type& kv : collection)
     {
-      const auto iter = std::find_if(
+      const auto iter = std::ranges::find_if(m_selection, [&](const Selection::value_type& value) {
-          m_selection.begin(), m_selection.end(),
+        return value.collection_ptr == &kv;
-          [&](const Selection::value_type& value) { return value.collection_ptr == &kv; });
+      });
+      const bool selected = iter != m_selection.end();
+      if (is_reduction_phase && !selected)
+        continue;  // Unselected hits are irrelevant to the reduction phase.
+      const auto inspection = selected ? iter->inspection : SelectionInspection{};
       fmt::println(file, "{:08x} {:08x} {:08x} {} {} {:x}", kv.first.origin_addr,
                    kv.first.destin_addr, kv.first.original_inst.hex, kv.second.total_hits,
                    kv.second.hits_snapshot,
-                   iter == m_selection.end() ?
+                   USnapshotMetadata(is_virtual, condition, selected, inspection).hex);
-                       USnapshotMetadata(is_virtual, condition, false, {}).hex :
-                       USnapshotMetadata(is_virtual, condition, true, iter->inspection).hex);
     }
   };
   routine(m_collection_vt, true, true);

Source/Core/Core/Debugger/BranchWatch.h

@@ -63,6 +63,8 @@ enum class BranchWatchSelectionInspection : u8
   SetDestinBLR = 1u << 1,
   SetOriginSymbolBLR = 1u << 2,
   SetDestinSymbolBLR = 1u << 3,
+  InvertBranchOption = 1u << 4,  // Used for both conditions and decrement checks.
+  MakeUnconditional = 1u << 5,
   EndOfEnumeration,
 };
 
@@ -117,9 +119,7 @@ public:
   using SelectionInspection = BranchWatchSelectionInspection;
 
   bool GetRecordingActive() const { return m_recording_active; }
-  void SetRecordingActive(bool active) { m_recording_active = active; }
+  void SetRecordingActive(const CPUThreadGuard& guard, bool active) { m_recording_active = active; }
-  void Start() { SetRecordingActive(true); }
-  void Pause() { SetRecordingActive(false); }
   void Clear(const CPUThreadGuard& guard);
 
   void Save(const CPUThreadGuard& guard, std::FILE* file) const;

Source/Core/Core/PowerPC/Gekko.h

@@ -294,6 +294,14 @@ union UGeckoInstruction
   };
 };
 
+// Precondition: inst is a bx, bcx, bclrx, or bcctrx instruction.
+constexpr bool BranchIsConditional(UGeckoInstruction inst)
+{
+  if (inst.OPCD == 18)  // bx
+    return false;
+  return (inst.BO & 0b10100) != 0b10100;  // 1z1zz - Branch always
+}
+
 // Used in implementations of rlwimi, rlwinm, and rlwnm
 inline u32 MakeRotationMask(u32 mb, u32 me)
 {

Source/Core/DolphinQt/Debugger/BranchWatchDialog.h

@@ -66,7 +66,7 @@ protected:
   void showEvent(QShowEvent* event) override;
 
 private:
-  void OnStartPause(bool checked);
+  void OnStartPause(bool checked) const;
   void OnClearBranchWatch();
   void OnSave();
   void OnSaveAs();
@@ -76,44 +76,59 @@ private:
   void OnCodePathNotTaken();
   void OnBranchWasOverwritten();
   void OnBranchNotOverwritten();
-  void OnWipeRecentHits();
+  void OnWipeRecentHits() const;
-  void OnWipeInspection();
+  void OnWipeInspection() const;
-  void OnTimeout();
+  void OnTimeout() const;
-  void OnEmulationStateChanged(Core::State new_state);
+  void OnEmulationStateChanged(Core::State new_state) const;
   void OnThemeChanged();
   void OnHelp();
   void OnToggleAutoSave(bool checked);
-  void OnHideShowControls(bool checked);
+  void OnHideShowControls(bool checked) const;
-  void OnToggleIgnoreApploader(bool checked);
+  void OnToggleIgnoreApploader(bool checked) const;
 
-  void OnTableClicked(const QModelIndex& index);
+  void OnTableClicked(const QModelIndex& index) const;
-  void OnTableContextMenu(const QPoint& pos);
+  void OnTableContextMenu(const QPoint& pos) const;
-  void OnTableHeaderContextMenu(const QPoint& pos);
+  void OnTableHeaderContextMenu(const QPoint& pos) const;
-  void OnTableDelete();
+  void OnTableDelete() const;
-  void OnTableDeleteKeypress();
+  void OnTableDeleteKeypress() const;
-  void OnTableSetBLR();
+  void OnTableSetBLR() const;
-  void OnTableSetNOP();
+  void OnTableSetNOP() const;
-  void OnTableCopyAddress();
+  void OnTableInvertCondition() const;
-  void OnTableSetBreakpointBreak();
+  void OnTableInvertDecrementCheck() const;
-  void OnTableSetBreakpointLog();
+  void OnTableMakeUnconditional() const;
-  void OnTableSetBreakpointBoth();
+  void OnTableCopyAddress() const;
+  void OnTableSetBreakpointBreak() const;
+  void OnTableSetBreakpointLog() const;
+  void OnTableSetBreakpointBoth() const;
 
-  void SaveSettings();
+  void ConnectSlots();
+  void DisconnectSlots();
+  void Show();
+  void Hide();
+  void LoadQSettings();
+  void SaveQSettings() const;
 
 public:
   // TODO: Step doesn't cause EmulationStateChanged to be emitted, so it has to call this manually.
-  void Update();
+  void Update() const;
 
 private:
-  void UpdateStatus();
+  void UpdateStatus() const;
   void UpdateIcons();
   void Save(const Core::CPUThreadGuard& guard, const std::string& filepath);
   void Load(const Core::CPUThreadGuard& guard, const std::string& filepath);
   void AutoSave(const Core::CPUThreadGuard& guard);
   void SetStubPatches(u32 value) const;
+  void SetEditPatches(u32 (*transform)(u32)) const;
   void SetBreakpoints(bool break_on_hit, bool log_on_hit) const;
 
-  [[nodiscard]] QMenu* GetTableContextMenu(const QModelIndex& index);
+  void SetBreakpointMenuActionsIcons() const;
+  QMenu* GetTableContextMenu(const QModelIndex& index) const;
+  QMenu* GetTableContextMenu_Instruction(bool core_initialized) const;
+  QMenu* GetTableContextMenu_Condition(bool core_initialized) const;
+  QMenu* GetTableContextMenu_Origin(bool core_initialized) const;
+  QMenu* GetTableContextMenu_Destin(bool core_initialized) const;
+  QMenu* GetTableContextMenu_Symbol(bool core_initialized) const;
 
   Core::System& m_system;
   Core::BranchWatch& m_branch_watch;
@@ -122,6 +137,9 @@ private:
   QPushButton *m_btn_start_pause, *m_btn_clear_watch, *m_btn_path_was_taken, *m_btn_path_not_taken,
       *m_btn_was_overwritten, *m_btn_not_overwritten, *m_btn_wipe_recent_hits;
   QAction* m_act_autosave;
+  QAction* m_act_invert_condition;
+  QAction* m_act_invert_decrement_check;
+  QAction* m_act_make_unconditional;
   QAction* m_act_insert_nop;
   QAction* m_act_insert_blr;
   QAction* m_act_copy_address;
@@ -129,10 +147,14 @@ private:
   QAction* m_act_break_on_hit;
   QAction* m_act_log_on_hit;
   QAction* m_act_both_on_hit;
-  QMenu* m_mnu_table_context = nullptr;
+  QMenu *m_mnu_table_context_instruction, *m_mnu_table_context_condition,
+      *m_mnu_table_context_origin, *m_mnu_table_context_destin_or_symbol,
+      *m_mnu_table_context_other;
   QMenu* m_mnu_column_visibility;
 
   QToolBar* m_control_toolbar;
+  QAction *m_act_branch_type_filters, *m_act_origin_destin_filters, *m_act_condition_filters,
+      *m_act_misc_controls;
   QTableView* m_table_view;
   BranchWatchProxyModel* m_table_proxy;
   BranchWatchTableModel* m_table_model;
@@ -141,6 +163,6 @@ private:
 
   QIcon m_icn_full, m_icn_partial;
 
-  QModelIndexList m_index_list_temp;
+  mutable QModelIndexList m_index_list_temp;
   std::optional<std::string> m_autosave_filepath;
 };

Source/Core/DolphinQt/Debugger/BranchWatchTableModel.cpp

@@ -145,6 +145,16 @@ void BranchWatchTableModel::OnWipeInspection()
                    roles);
 }
 
+void BranchWatchTableModel::OnDebugFontChanged(const QFont& font)
+{
+  setFont(font);
+}
+
+void BranchWatchTableModel::OnPPCSymbolsChanged()
+{
+  UpdateSymbols();
+}
+
 void BranchWatchTableModel::Save(const Core::CPUThreadGuard& guard, std::FILE* file) const
 {
   m_branch_watch.Save(guard, file);
@@ -185,6 +195,12 @@ void BranchWatchTableModel::SetInspected(const QModelIndex& index)
   const int row = index.row();
   switch (index.column())
   {
+  case Column::Instruction:
+    SetInspected(index, Core::BranchWatchSelectionInspection::InvertBranchOption);
+    return;
+  case Column::Condition:
+    SetInspected(index, Core::BranchWatchSelectionInspection::MakeUnconditional);
+    return;
   case Column::Origin:
     SetOriginInspected(m_branch_watch.GetSelection()[row].collection_ptr->first.origin_addr);
     return;
@@ -200,35 +216,35 @@ void BranchWatchTableModel::SetInspected(const QModelIndex& index)
   }
 }
 
+void BranchWatchTableModel::SetInspected(const QModelIndex& index,
+                                         Core::BranchWatchSelectionInspection inspection)
+{
+  static const QList<int> roles = {Qt::FontRole, Qt::ForegroundRole};
+  m_branch_watch.SetSelectedInspected(index.row(), inspection);
+  emit dataChanged(index, index, roles);
+}
+
 void BranchWatchTableModel::SetOriginInspected(u32 origin_addr)
 {
-  using Inspection = Core::BranchWatchSelectionInspection;
-  static const QList<int> roles = {Qt::FontRole, Qt::ForegroundRole};
-
   const Core::BranchWatch::Selection& selection = m_branch_watch.GetSelection();
   for (std::size_t i = 0; i < selection.size(); ++i)
   {
     if (selection[i].collection_ptr->first.origin_addr != origin_addr)
       continue;
-    m_branch_watch.SetSelectedInspected(i, Inspection::SetOriginNOP);
+    SetInspected(createIndex(static_cast<int>(i), Column::Origin),
-    const QModelIndex index = createIndex(static_cast<int>(i), Column::Origin);
+                 Core::BranchWatchSelectionInspection::SetOriginNOP);
-    emit dataChanged(index, index, roles);
   }
 }
 
 void BranchWatchTableModel::SetDestinInspected(u32 destin_addr, bool nested)
 {
-  using Inspection = Core::BranchWatchSelectionInspection;
-  static const QList<int> roles = {Qt::FontRole, Qt::ForegroundRole};
-
   const Core::BranchWatch::Selection& selection = m_branch_watch.GetSelection();
   for (std::size_t i = 0; i < selection.size(); ++i)
   {
     if (selection[i].collection_ptr->first.destin_addr != destin_addr)
       continue;
-    m_branch_watch.SetSelectedInspected(i, Inspection::SetDestinBLR);
+    SetInspected(createIndex(static_cast<int>(i), Column::Destination),
-    const QModelIndex index = createIndex(static_cast<int>(i), Column::Destination);
+                 Core::BranchWatchSelectionInspection::SetDestinBLR);
-    emit dataChanged(index, index, roles);
   }
 
   if (nested)
@@ -238,23 +254,18 @@ void BranchWatchTableModel::SetDestinInspected(u32 destin_addr, bool nested)
 
 void BranchWatchTableModel::SetSymbolInspected(u32 symbol_addr, bool nested)
 {
-  using Inspection = Core::BranchWatchSelectionInspection;
-  static const QList<int> roles = {Qt::FontRole, Qt::ForegroundRole};
-
   for (qsizetype i = 0; i < m_symbol_list.size(); ++i)
   {
     const SymbolListValueType& value = m_symbol_list[i];
     if (value.origin_addr.isValid() && value.origin_addr.value<u32>() == symbol_addr)
     {
-      m_branch_watch.SetSelectedInspected(i, Inspection::SetOriginSymbolBLR);
+      SetInspected(createIndex(static_cast<int>(i), Column::OriginSymbol),
-      const QModelIndex index = createIndex(i, Column::OriginSymbol);
+                   Core::BranchWatchSelectionInspection::SetOriginSymbolBLR);
-      emit dataChanged(index, index, roles);
     }
     if (value.destin_addr.isValid() && value.destin_addr.value<u32>() == symbol_addr)
     {
-      m_branch_watch.SetSelectedInspected(i, Inspection::SetDestinSymbolBLR);
+      SetInspected(createIndex(static_cast<int>(i), Column::DestinSymbol),
-      const QModelIndex index = createIndex(i, Column::DestinSymbol);
+                   Core::BranchWatchSelectionInspection::SetDestinSymbolBLR);
-      emit dataChanged(index, index, roles);
     }
   }
 
@@ -263,6 +274,13 @@ void BranchWatchTableModel::SetSymbolInspected(u32 symbol_addr, bool nested)
   SetDestinInspected(symbol_addr, true);
 }
 
+const Core::BranchWatchSelectionValueType&
+BranchWatchTableModel::GetBranchWatchSelection(const QModelIndex& index) const
+{
+  ASSERT(index.isValid());
+  return m_branch_watch.GetSelection()[index.row()];
+}
+
 void BranchWatchTableModel::PrefetchSymbols()
 {
   if (m_branch_watch.GetRecordingPhase() != Core::BranchWatch::Phase::Reduction)
@@ -296,25 +314,14 @@ static QString GetInstructionMnemonic(u32 hex)
   return QString::fromLatin1(disas.data(), split);
 }
 
-static bool BranchIsUnconditional(UGeckoInstruction inst)
-{
-  if (inst.OPCD == 18)  // bx
-    return true;
-  // If BranchWatch is doing its job, the input will be only bcx, bclrx, and bcctrx instructions.
-  DEBUG_ASSERT(inst.OPCD == 16 || (inst.OPCD == 19 && (inst.SUBOP10 == 16 || inst.SUBOP10 == 528)));
-  if ((inst.BO & 0b10100) == 0b10100)  // 1z1zz - Branch always
-    return true;
-  return false;
-}
-
 static QString GetConditionString(const Core::BranchWatch::Selection::value_type& value,
                                   const Core::BranchWatch::Collection::value_type* kv)
 {
   if (value.condition == false)
     return BranchWatchTableModel::tr("false");
-  if (BranchIsUnconditional(kv->first.original_inst))
+  if (BranchIsConditional(kv->first.original_inst))
-    return QStringLiteral("");
     return BranchWatchTableModel::tr("true");
+  return QStringLiteral("");
 }
 
 QVariant BranchWatchTableModel::DisplayRoleData(const QModelIndex& index) const
@@ -351,21 +358,25 @@ QVariant BranchWatchTableModel::DisplayRoleData(const QModelIndex& index) const
 QVariant BranchWatchTableModel::FontRoleData(const QModelIndex& index) const
 {
   m_font.setBold([&]() -> bool {
+    using Inspection = Core::BranchWatchSelectionInspection;
+    const auto get_bit_test = [this, &index](Inspection inspection_mask) -> bool {
+      const Inspection inspection = m_branch_watch.GetSelection()[index.row()].inspection;
+      return (inspection & inspection_mask) != Inspection{};
+    };
     switch (index.column())
     {
-      using Inspection = Core::BranchWatchSelectionInspection;
+    case Column::Instruction:
+      return get_bit_test(Inspection::InvertBranchOption);
+    case Column::Condition:
+      return get_bit_test(Inspection::MakeUnconditional);
     case Column::Origin:
-      return (m_branch_watch.GetSelection()[index.row()].inspection & Inspection::SetOriginNOP) !=
+      return get_bit_test(Inspection::SetOriginNOP);
-             Inspection{};
     case Column::Destination:
-      return (m_branch_watch.GetSelection()[index.row()].inspection & Inspection::SetDestinBLR) !=
+      return get_bit_test(Inspection::SetDestinBLR);
-             Inspection{};
     case Column::OriginSymbol:
-      return (m_branch_watch.GetSelection()[index.row()].inspection &
+      return get_bit_test(Inspection::SetOriginSymbolBLR);
-              Inspection::SetOriginSymbolBLR) != Inspection{};
     case Column::DestinSymbol:
-      return (m_branch_watch.GetSelection()[index.row()].inspection &
+      return get_bit_test(Inspection::SetDestinSymbolBLR);
-              Inspection::SetDestinSymbolBLR) != Inspection{};
     }
     // Importantly, this code path avoids subscripting the selection to get an inspection value.
     return false;
@@ -396,31 +407,25 @@ QVariant BranchWatchTableModel::TextAlignmentRoleData(const QModelIndex& index)
 
 QVariant BranchWatchTableModel::ForegroundRoleData(const QModelIndex& index) const
 {
+  using Inspection = Core::BranchWatchSelectionInspection;
+  const auto get_brush_v = [this, &index](Inspection inspection_mask) -> QVariant {
+    const Inspection inspection = m_branch_watch.GetSelection()[index.row()].inspection;
+    return (inspection & inspection_mask) != Inspection{} ? QBrush(Qt::red) : QVariant();
+  };
   switch (index.column())
   {
-    using Inspection = Core::BranchWatchSelectionInspection;
+  case Column::Instruction:
+    return get_brush_v(Inspection::InvertBranchOption);
+  case Column::Condition:
+    return get_brush_v(Inspection::MakeUnconditional);
   case Column::Origin:
-  {
+    return get_brush_v(Inspection::SetOriginNOP);
-    const Inspection inspection = m_branch_watch.GetSelection()[index.row()].inspection;
-    return (inspection & Inspection::SetOriginNOP) != Inspection{} ? QBrush(Qt::red) : QVariant();
-  }
   case Column::Destination:
-  {
+    return get_brush_v(Inspection::SetDestinBLR);
-    const Inspection inspection = m_branch_watch.GetSelection()[index.row()].inspection;
-    return (inspection & Inspection::SetDestinBLR) != Inspection{} ? QBrush(Qt::red) : QVariant();
-  }
   case Column::OriginSymbol:
-  {
+    return get_brush_v(Inspection::SetOriginSymbolBLR);
-    const Inspection inspection = m_branch_watch.GetSelection()[index.row()].inspection;
-    return (inspection & Inspection::SetOriginSymbolBLR) != Inspection{} ? QBrush(Qt::red) :
-                                                                           QVariant();
-  }
   case Column::DestinSymbol:
-  {
+    return get_brush_v(Inspection::SetDestinSymbolBLR);
-    const Inspection inspection = m_branch_watch.GetSelection()[index.row()].inspection;
-    return (inspection & Inspection::SetDestinSymbolBLR) != Inspection{} ? QBrush(Qt::red) :
-                                                                           QVariant();
-  }
   }
   // Importantly, this code path avoids subscripting the selection to get an inspection value.
   return QVariant();
@@ -455,9 +460,9 @@ static int GetConditionInteger(const Core::BranchWatch::Selection::value_type& v
 {
   if (value.condition == false)
     return 0;
-  if (BranchIsUnconditional(kv->first.original_inst))
+  if (BranchIsConditional(kv->first.original_inst))
-    return 2;
     return 1;
+  return 2;
 }
 
 QVariant BranchWatchTableModel::SortRoleData(const QModelIndex& index) const

Source/Core/DolphinQt/Debugger/BranchWatchTableModel.h

@@ -15,6 +15,8 @@
 namespace Core
 {
 class BranchWatch;
+enum class BranchWatchSelectionInspection : u8;
+struct BranchWatchSelectionValueType;
 class CPUThreadGuard;
 class System;
 }  // namespace Core
@@ -97,6 +99,8 @@ public:
   void OnBranchNotOverwritten(const Core::CPUThreadGuard& guard);
   void OnWipeRecentHits();
   void OnWipeInspection();
+  void OnDebugFontChanged(const QFont& font);
+  void OnPPCSymbolsChanged();
 
   void Save(const Core::CPUThreadGuard& guard, std::FILE* file) const;
   void Load(const Core::CPUThreadGuard& guard, std::FILE* file);
@@ -104,9 +108,12 @@ public:
   void UpdateHits();
   void SetInspected(const QModelIndex& index);
 
+  const Core::BranchWatchSelectionValueType&
+  GetBranchWatchSelection(const QModelIndex& index) const;
   const SymbolList& GetSymbolList() const { return m_symbol_list; }
 
 private:
+  void SetInspected(const QModelIndex& index, Core::BranchWatchSelectionInspection inspection);
   void SetOriginInspected(u32 origin_addr);
   void SetDestinInspected(u32 destin_addr, bool nested);
   void SetSymbolInspected(u32 symbol_addr, bool nested);

Source/Core/DolphinQt/Debugger/CodeWidget.cpp

@@ -180,7 +180,7 @@ void CodeWidget::ConnectWidgets()
   });
   connect(m_search_callstack, &QLineEdit::textChanged, this, &CodeWidget::UpdateCallstack);
 
-  connect(m_branch_watch, &QPushButton::pressed, this, &CodeWidget::OnBranchWatchDialog);
+  connect(m_branch_watch, &QPushButton::clicked, this, &CodeWidget::OnBranchWatchDialog);
 
   connect(m_symbols_list, &QListWidget::itemPressed, this, &CodeWidget::OnSelectSymbol);
   connect(m_callstack_list, &QListWidget::itemPressed, this, &CodeWidget::OnSelectCallstack);