Debugger: correctness fixes and cleanup

* Remove m_current_choice, it's not correct to rely on thread name entry. In extreme corner cases a newly thread can be created, old destroyed with the same entry name. (reoccuring LV2 SPU/PPU ID)
* Remove m_no_thread_selected, can be easily replaced with std::weak_ptr expired() function and is more accurate this way.
* In HandleBreakpointRequest: only remove breakpoint on valid PPU thread and not any thread! also fix potential nullptr deref if thread has recently been destroyed.
This commit is contained in:
Eladash 2020-12-16 16:44:41 +02:00 committed by Ivan
parent ef884642e4
commit 2c06043617
5 changed files with 23 additions and 21 deletions

View File

@ -86,18 +86,21 @@ void breakpoint_list::AddBreakpoint(u32 pc)
*/ */
void breakpoint_list::HandleBreakpointRequest(u32 loc) void breakpoint_list::HandleBreakpointRequest(u32 loc)
{ {
const auto cpu = this->cpu.lock();
if (!cpu || cpu->id_type() != 1 || !vm::check_addr(loc, vm::page_allocated | vm::page_executable))
{
// TODO: SPU breakpoints
return;
}
if (m_breakpoint_handler->HasBreakpoint(loc)) if (m_breakpoint_handler->HasBreakpoint(loc))
{ {
RemoveBreakpoint(loc); RemoveBreakpoint(loc);
} }
else else
{ {
const auto cpu = this->cpu.lock(); AddBreakpoint(loc);
if (cpu->id_type() == 1 && vm::check_addr(loc, vm::page_allocated | vm::page_executable))
{
AddBreakpoint(loc);
}
} }
} }

View File

@ -226,7 +226,7 @@ void debugger_frame::keyPressEvent(QKeyEvent* event)
const auto cpu = this->cpu.lock(); const auto cpu = this->cpu.lock();
int i = m_debugger_list->currentRow(); int i = m_debugger_list->currentRow();
if (!isActiveWindow() || !cpu || m_no_thread_selected) if (!isActiveWindow() || !cpu)
{ {
return; return;
} }
@ -356,8 +356,6 @@ void debugger_frame::UpdateUI()
{ {
UpdateUnitList(); UpdateUnitList();
if (m_no_thread_selected) return;
const auto cpu = this->cpu.lock(); const auto cpu = this->cpu.lock();
if (!cpu) if (!cpu)
@ -449,18 +447,22 @@ void debugger_frame::UpdateUnitList()
void debugger_frame::OnSelectUnit() void debugger_frame::OnSelectUnit()
{ {
if (m_choice_units->count() < 1 || m_current_choice == m_choice_units->currentText()) return; if (m_choice_units->count() < 1) return;
m_current_choice = m_choice_units->currentText(); const auto weak = m_choice_units->currentData().value<std::weak_ptr<cpu_thread>>();
m_no_thread_selected = m_current_choice == NoThreadString;
m_debugger_list->m_no_thread_selected = m_no_thread_selected; if (!weak.owner_before(cpu) && !cpu.owner_before(weak))
{
// They match, nothing to do.
return;
}
m_disasm.reset(); m_disasm.reset();
cpu.reset(); cpu.reset();
if (!m_no_thread_selected) if (!weak.expired())
{ {
if (const auto cpu0 = m_choice_units->currentData().value<std::weak_ptr<cpu_thread>>().lock()) if (const auto cpu0 = weak.lock())
{ {
if (cpu0->id_type() == 1) if (cpu0->id_type() == 1)
{ {
@ -481,7 +483,7 @@ void debugger_frame::OnSelectUnit()
} }
} }
EnableButtons(!m_no_thread_selected); EnableButtons(true);
m_debugger_list->UpdateCPUData(this->cpu, m_disasm); m_debugger_list->UpdateCPUData(this->cpu, m_disasm);
m_breakpoint_list->UpdateCPUData(this->cpu, m_disasm); m_breakpoint_list->UpdateCPUData(this->cpu, m_disasm);
@ -698,7 +700,7 @@ void debugger_frame::EnableUpdateTimer(bool enable)
void debugger_frame::EnableButtons(bool enable) void debugger_frame::EnableButtons(bool enable)
{ {
if (m_no_thread_selected) enable = false; if (cpu.expired()) enable = false;
m_go_to_addr->setEnabled(enable); m_go_to_addr->setEnabled(enable);
m_go_to_pc->setEnabled(enable); m_go_to_pc->setEnabled(enable);

View File

@ -36,7 +36,6 @@ class debugger_frame : public custom_dock_widget
QPushButton* m_btn_step_over; QPushButton* m_btn_step_over;
QPushButton* m_btn_run; QPushButton* m_btn_run;
QComboBox* m_choice_units; QComboBox* m_choice_units;
QString m_current_choice;
QTimer* m_update; QTimer* m_update;
QSplitter* m_splitter; QSplitter* m_splitter;
@ -45,7 +44,6 @@ class debugger_frame : public custom_dock_widget
u32 m_last_pc = -1; u32 m_last_pc = -1;
std::vector<char> m_last_query_state; std::vector<char> m_last_query_state;
u32 m_last_step_over_breakpoint = -1; u32 m_last_step_over_breakpoint = -1;
bool m_no_thread_selected = true;
std::shared_ptr<CPUDisAsm> m_disasm; std::shared_ptr<CPUDisAsm> m_disasm;
std::weak_ptr<cpu_thread> cpu; std::weak_ptr<cpu_thread> cpu;

View File

@ -149,7 +149,7 @@ void debugger_list::keyPressEvent(QKeyEvent* event)
void debugger_list::mouseDoubleClickEvent(QMouseEvent* event) void debugger_list::mouseDoubleClickEvent(QMouseEvent* event)
{ {
if (event->button() == Qt::LeftButton && !Emu.IsStopped() && !m_no_thread_selected) if (event->button() == Qt::LeftButton)
{ {
int i = currentRow(); int i = currentRow();
if (i < 0) return; if (i < 0) return;

View File

@ -16,7 +16,6 @@ class debugger_list : public QListWidget
public: public:
u32 m_pc = 0; u32 m_pc = 0;
u32 m_item_count = 30; u32 m_item_count = 30;
bool m_no_thread_selected;
QColor m_color_bp; QColor m_color_bp;
QColor m_color_pc; QColor m_color_pc;
QColor m_text_color_bp; QColor m_text_color_bp;