From f77bf1ec6b606ba837296906437c27bca3f3ffee Mon Sep 17 00:00:00 2001 From: chaoticgd <43898262+chaoticgd@users.noreply.github.com> Date: Wed, 16 Oct 2024 01:18:01 +0100 Subject: [PATCH] Debugger: Be smarter about deciding when functions should be hashed --- 3rdparty/ccc/src/ccc/symbol_database.cpp | 2 +- .../Debugger/SymbolTree/SymbolTreeNode.cpp | 149 +++++++++++++----- pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.h | 4 +- .../Debugger/SymbolTree/SymbolTreeWidgets.cpp | 48 ++++-- .../Debugger/SymbolTree/SymbolTreeWidgets.h | 4 +- pcsx2/DebugTools/SymbolGuardian.cpp | 9 +- 6 files changed, 157 insertions(+), 59 deletions(-) diff --git a/3rdparty/ccc/src/ccc/symbol_database.cpp b/3rdparty/ccc/src/ccc/symbol_database.cpp index 82451bbb77..87b1e7d015 100644 --- a/3rdparty/ccc/src/ccc/symbol_database.cpp +++ b/3rdparty/ccc/src/ccc/symbol_database.cpp @@ -753,7 +753,7 @@ void SourceFile::check_functions_match(const SymbolDatabase& database) u32 modified = 0; for(FunctionHandle function_handle : functions()) { const ccc::Function* function = database.functions.symbol_from_handle(function_handle); - if(!function || function->original_hash() == 0) { + if(!function || function->current_hash() == 0 || function->original_hash() == 0) { continue; } diff --git a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.cpp b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.cpp index 284c78095c..9834492865 100644 --- a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.cpp +++ b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.cpp @@ -41,7 +41,7 @@ bool SymbolTreeNode::readFromVM(DebugInterface& cpu, const ccc::SymbolDatabase& data_changed |= updateDisplayString(cpu, database); data_changed |= updateLiveness(cpu); - data_changed |= updateHash(cpu); + data_changed |= updateMatchesMemory(cpu, database); return data_changed; } @@ -479,7 +479,7 @@ bool SymbolTreeNode::updateLiveness(DebugInterface& cpu) return true; } -bool SymbolTreeNode::updateHash(DebugInterface& cpu) +bool SymbolTreeNode::updateMatchesMemory(DebugInterface& cpu, const ccc::SymbolDatabase& database) { bool matching = true; @@ -487,66 +487,54 @@ bool SymbolTreeNode::updateHash(DebugInterface& cpu) { case ccc::SymbolDescriptor::FUNCTION: { - cpu.GetSymbolGuardian().Read([&](const ccc::SymbolDatabase& database) -> void { - const ccc::Function* function = database.functions.symbol_from_handle(symbol.handle()); - if (!function || function->original_hash() == 0) - return; + const ccc::Function* function = database.functions.symbol_from_handle(symbol.handle()); + if (!function || function->current_hash() == 0 || function->original_hash() == 0) + return false; + + matching = function->current_hash() == function->original_hash(); - matching = function->current_hash() == function->original_hash(); - }); break; } case ccc::SymbolDescriptor::GLOBAL_VARIABLE: { - cpu.GetSymbolGuardian().Read([&](const ccc::SymbolDatabase& database) -> void { - const ccc::GlobalVariable* global_variable = database.global_variables.symbol_from_handle(symbol.handle()); - if (!global_variable) - return; + const ccc::GlobalVariable* global_variable = database.global_variables.symbol_from_handle(symbol.handle()); + if (!global_variable) + return false; - const ccc::SourceFile* source_file = database.source_files.symbol_from_handle(global_variable->source_file()); - if (!source_file) - return; + const ccc::SourceFile* source_file = database.source_files.symbol_from_handle(global_variable->source_file()); + if (!source_file) + return false; + + matching = source_file->functions_match(); - matching = source_file->functions_match(); - }); break; } case ccc::SymbolDescriptor::LOCAL_VARIABLE: { - cpu.GetSymbolGuardian().Read([&](const ccc::SymbolDatabase& database) -> void { - const ccc::LocalVariable* local_variable = database.local_variables.symbol_from_handle(symbol.handle()); - if (!local_variable) - return; + const ccc::LocalVariable* local_variable = database.local_variables.symbol_from_handle(symbol.handle()); + if (!local_variable) + return false; - const ccc::Function* function = database.functions.symbol_from_handle(local_variable->function()); - if (!function) - return; + const ccc::Function* function = database.functions.symbol_from_handle(local_variable->function()); + if (!function || function->current_hash() == 0 || function->original_hash() == 0) + return false; - const ccc::SourceFile* source_file = database.source_files.symbol_from_handle(function->source_file()); - if (!source_file) - return; + matching = function->current_hash() == function->original_hash(); - matching = source_file->functions_match(); - }); break; } case ccc::SymbolDescriptor::PARAMETER_VARIABLE: { - cpu.GetSymbolGuardian().Read([&](const ccc::SymbolDatabase& database) -> void { - const ccc::ParameterVariable* parameter_variable = database.parameter_variables.symbol_from_handle(symbol.handle()); - if (!parameter_variable) - return; + const ccc::ParameterVariable* parameter_variable = database.parameter_variables.symbol_from_handle(symbol.handle()); + if (!parameter_variable) + return false; - const ccc::Function* function = database.functions.symbol_from_handle(parameter_variable->function()); - if (!function) - return; + const ccc::Function* function = database.functions.symbol_from_handle(parameter_variable->function()); + if (!function || function->current_hash() == 0 || function->original_hash() == 0) + return false; - const ccc::SourceFile* source_file = database.source_files.symbol_from_handle(function->source_file()); - if (!source_file) - return; + matching = function->current_hash() == function->original_hash(); - matching = source_file->functions_match(); - }); break; } default: @@ -567,6 +555,85 @@ bool SymbolTreeNode::matchesMemory() const return m_matches_memory; } +void SymbolTreeNode::updateSymbolHashes(std::span nodes, DebugInterface& cpu, ccc::SymbolDatabase& database) +{ + std::set functions; + std::set source_files; + + // Determine which functions we need to hash again, and in the case of + // global variables, which source files are associated with those functions + // so that we can check if they still match. + for (const SymbolTreeNode* node : nodes) + { + switch (node->symbol.descriptor()) + { + case ccc::SymbolDescriptor::FUNCTION: + { + functions.emplace(node->symbol.handle()); + break; + } + case ccc::SymbolDescriptor::GLOBAL_VARIABLE: + { + const ccc::GlobalVariable* global_variable = database.global_variables.symbol_from_handle(node->symbol.handle()); + if (!global_variable) + continue; + + ccc::SourceFile* source_file = database.source_files.symbol_from_handle(global_variable->source_file()); + if (!source_file) + continue; + + for (ccc::FunctionHandle function : source_file->functions()) + functions.emplace(function); + + source_files.emplace(source_file); + + break; + } + case ccc::SymbolDescriptor::LOCAL_VARIABLE: + { + const ccc::LocalVariable* local_variable = database.local_variables.symbol_from_handle(node->symbol.handle()); + if (!local_variable) + continue; + + functions.emplace(local_variable->function()); + + break; + } + case ccc::SymbolDescriptor::PARAMETER_VARIABLE: + { + const ccc::ParameterVariable* parameter_variable = database.parameter_variables.symbol_from_handle(node->symbol.handle()); + if (!parameter_variable) + continue; + + functions.emplace(parameter_variable->function()); + + break; + } + default: + { + } + } + } + + // Update the hashes for the enumerated functions. + for (ccc::FunctionHandle function_handle : functions) + { + ccc::Function* function = database.functions.symbol_from_handle(function_handle); + if (!function || function->original_hash() == 0) + continue; + + std::optional hash = SymbolGuardian::HashFunction(*function, cpu); + if (!hash.has_value()) + continue; + + function->set_current_hash(*hash); + } + + // Check that the enumerated source files still have matching functions. + for (ccc::SourceFile* source_file : source_files) + source_file->check_functions_match(database); +} + bool SymbolTreeNode::anySymbolsValid(const ccc::SymbolDatabase& database) const { if (symbol.lookup_symbol(database)) diff --git a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.h b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.h index ff4040ddda..3437023bc9 100644 --- a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.h +++ b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeNode.h @@ -62,9 +62,11 @@ public: bool updateLiveness(DebugInterface& cpu); - bool updateHash(DebugInterface& cpu); + bool updateMatchesMemory(DebugInterface& cpu, const ccc::SymbolDatabase& database); bool matchesMemory() const; + static void updateSymbolHashes(std::span nodes, DebugInterface& cpu, ccc::SymbolDatabase& database); + bool anySymbolsValid(const ccc::SymbolDatabase& database) const; const SymbolTreeNode* parent() const; diff --git a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.cpp b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.cpp index a5907ee468..f70a944987 100644 --- a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.cpp +++ b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.cpp @@ -24,17 +24,29 @@ SymbolTreeWidget::SymbolTreeWidget(u32 flags, s32 symbol_address_alignment, Debu setupMenu(); - connect(m_ui.refreshButton, &QPushButton::clicked, this, &SymbolTreeWidget::reset); + connect(m_ui.refreshButton, &QPushButton::clicked, this, [&]() { + m_cpu.GetSymbolGuardian().ReadWrite([&](ccc::SymbolDatabase& database) { + m_cpu.GetSymbolGuardian().UpdateFunctionHashes(database, m_cpu); + }); + + reset(); + }); + connect(m_ui.filterBox, &QLineEdit::textEdited, this, &SymbolTreeWidget::reset); connect(m_ui.newButton, &QPushButton::clicked, this, &SymbolTreeWidget::onNewButtonPressed); connect(m_ui.deleteButton, &QPushButton::clicked, this, &SymbolTreeWidget::onDeleteButtonPressed); - connect(m_ui.treeView->verticalScrollBar(), &QScrollBar::valueChanged, this, &SymbolTreeWidget::updateVisibleNodes); + connect(m_ui.treeView->verticalScrollBar(), &QScrollBar::valueChanged, this, [&]() { + updateVisibleNodes(false); + }); m_ui.treeView->setContextMenuPolicy(Qt::CustomContextMenu); connect(m_ui.treeView, &QTreeView::customContextMenuRequested, this, &SymbolTreeWidget::openMenu); - connect(m_ui.treeView, &QTreeView::expanded, this, &SymbolTreeWidget::updateVisibleNodes); + + connect(m_ui.treeView, &QTreeView::expanded, this, [&]() { + updateVisibleNodes(true); + }); } SymbolTreeWidget::~SymbolTreeWidget() = default; @@ -43,15 +55,15 @@ void SymbolTreeWidget::resizeEvent(QResizeEvent* event) { QWidget::resizeEvent(event); - updateVisibleNodes(); + updateVisibleNodes(false); } void SymbolTreeWidget::updateModel() { if (!m_model || m_model->needsReset()) reset(); - - updateVisibleNodes(); + else + updateVisibleNodes(true); } void SymbolTreeWidget::reset() @@ -61,10 +73,6 @@ void SymbolTreeWidget::reset() m_ui.treeView->setColumnHidden(SymbolTreeModel::SIZE, !m_show_size_column || !m_show_size_column->isChecked()); - m_cpu.GetSymbolGuardian().ReadWrite([&](ccc::SymbolDatabase& database) { - m_cpu.GetSymbolGuardian().UpdateFunctionHashes(database, m_cpu); - }); - SymbolFilters filters; std::unique_ptr root; m_cpu.GetSymbolGuardian().Read([&](const ccc::SymbolDatabase& database) -> void { @@ -82,25 +90,39 @@ void SymbolTreeWidget::reset() m_model->reset(std::move(root)); // Read the initial values for visible nodes. - updateVisibleNodes(); + updateVisibleNodes(true); if (!filters.string.isEmpty()) expandGroups(QModelIndex()); } } -void SymbolTreeWidget::updateVisibleNodes() +void SymbolTreeWidget::updateVisibleNodes(bool update_hashes) { if (!m_model) return; + // Enumerate visible symbol nodes. + std::vector nodes; QModelIndex index = m_ui.treeView->indexAt(m_ui.treeView->rect().topLeft()); while (m_ui.treeView->visualRect(index).intersects(m_ui.treeView->viewport()->rect())) { - m_model->setData(index, QVariant(), SymbolTreeModel::UPDATE_FROM_MEMORY_ROLE); + nodes.emplace_back(m_model->nodeFromIndex(index)); index = m_ui.treeView->indexBelow(index); } + // Hash functions for symbols with visible nodes. + if (update_hashes) + { + m_cpu.GetSymbolGuardian().ReadWrite([&](ccc::SymbolDatabase& database) { + SymbolTreeNode::updateSymbolHashes(nodes, m_cpu, database); + }); + } + + // Update the values of visible nodes from memory. + for (const SymbolTreeNode* node : nodes) + m_model->setData(m_model->indexFromNode(*node), QVariant(), SymbolTreeModel::UPDATE_FROM_MEMORY_ROLE); + m_ui.treeView->update(); } diff --git a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.h b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.h index ca5c3c01fa..992b619eca 100644 --- a/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.h +++ b/pcsx2-qt/Debugger/SymbolTree/SymbolTreeWidgets.h @@ -21,7 +21,7 @@ public: void updateModel(); void reset(); - void updateVisibleNodes(); + void updateVisibleNodes(bool update_hashes); void expandGroups(QModelIndex index); signals: @@ -41,7 +41,7 @@ protected: const ccc::SourceFile* source_file = nullptr; }; - explicit SymbolTreeWidget(u32 flags, s32 symbol_address_alignment, DebugInterface& cpu, QWidget* parent = nullptr); + SymbolTreeWidget(u32 flags, s32 symbol_address_alignment, DebugInterface& cpu, QWidget* parent = nullptr); void resizeEvent(QResizeEvent* event) override; diff --git a/pcsx2/DebugTools/SymbolGuardian.cpp b/pcsx2/DebugTools/SymbolGuardian.cpp index 78d97ec8a6..4dd549cefb 100644 --- a/pcsx2/DebugTools/SymbolGuardian.cpp +++ b/pcsx2/DebugTools/SymbolGuardian.cpp @@ -192,7 +192,14 @@ std::optional SymbolGuardian::HashFunction(const ccc::Functio ccc::FunctionHash hash; for (u32 i = 0; i < function.size() / 4; i++) - hash.update(reader.read32(function.address().value + i * 4)); + { + bool valid; + u32 value = reader.read32(function.address().value + i * 4, valid); + if (!valid) + return std::nullopt; + + hash.update(value); + } return hash; }