Debugger: Pointer comparison bugfix

rpcs3/Emu/CPU/CPUDisAsm.h

@@ -21,7 +21,8 @@ protected:
 	cpu_disasm_mode m_mode{};
 	const u8* m_offset{};
 	const u32 m_start_pc;
-	const std::add_pointer_t<const cpu_thread> m_cpu{};
+	std::add_pointer_t<const cpu_thread> m_cpu{};
+	std::shared_ptr<cpu_thread> m_cpu_handle;
 	u32 m_op = 0;
 
 	void format_by_mode()
@@ -75,6 +76,25 @@ public:
 		return std::exchange(m_offset, ptr);
 	}
 
+	cpu_thread* get_cpu() const
+	{
+		return const_cast<cpu_thread*>(m_cpu);
+	}
+
+	void set_cpu_handle(std::shared_ptr<cpu_thread> cpu)
+	{
+		m_cpu_handle = std::move(cpu);
+
+		if (!m_cpu)
+		{
+			m_cpu = m_cpu_handle.get();
+		}
+		else
+		{
+			AUDIT(m_cpu == m_cpu_handle.get());
+		}
+	}
+
 protected:
 	CPUDisAsm(cpu_disasm_mode mode, const u8* offset, u32 start_pc = 0, const cpu_thread* cpu = nullptr)
 		: m_mode(mode)

rpcs3/Emu/CPU/CPUThread.cpp

@@ -1219,7 +1219,7 @@ cpu_thread* cpu_thread::get_next_cpu()
 	return nullptr;
 }
 
-std::shared_ptr<CPUDisAsm> make_disasm(const cpu_thread* cpu);
+std::shared_ptr<CPUDisAsm> make_disasm(const cpu_thread* cpu, std::shared_ptr<cpu_thread> handle);
 
 void cpu_thread::dump_all(std::string& ret) const
 {
@@ -1235,7 +1235,7 @@ void cpu_thread::dump_all(std::string& ret) const
 	if (u32 cur_pc = get_pc(); cur_pc != umax)
 	{
 		// Dump a snippet of currently executed code (may be unreliable with non-static-interpreter decoders)
-		auto disasm = make_disasm(this);
+		auto disasm = make_disasm(this, nullptr);
 
 		const auto rsx = try_get<rsx::thread>();
 

rpcs3/rpcs3qt/breakpoint_list.cpp

@@ -34,10 +34,9 @@ breakpoint_list::breakpoint_list(QWidget* parent, breakpoint_handler* handler) :
 /**
 * It's unfortunate I need a method like this to sync these.  Should ponder a cleaner way to do this.
 */
-void breakpoint_list::UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm)
+void breakpoint_list::UpdateCPUData(std::shared_ptr<CPUDisAsm> disasm)
 {
-	m_cpu = cpu;
+	m_disasm = std::move(disasm);
-	m_disasm = disasm;
 }
 
 void breakpoint_list::ClearBreakpoints()
@@ -102,18 +101,20 @@ bool breakpoint_list::AddBreakpoint(u32 pc)
 */
 void breakpoint_list::HandleBreakpointRequest(u32 loc, bool only_add)
 {
-	if (!m_cpu || m_cpu->state & cpu_flag::exit)
+	const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
+	if (!cpu || cpu->state & cpu_flag::exit)
 	{
 		return;
 	}
 
-	if (!is_using_interpreter(m_cpu->get_class()))
+	if (!is_using_interpreter(cpu->get_class()))
 	{
 		QMessageBox::warning(this, tr("Interpreters-Only Feature!"), tr("Cannot set breakpoints on non-interpreter decoders."));
 		return;
 	}
 
-	switch (m_cpu->get_class())
+	switch (cpu->get_class())
 	{
 	case thread_class::spu:
 	{
@@ -123,7 +124,7 @@ void breakpoint_list::HandleBreakpointRequest(u32 loc, bool only_add)
 			return;
 		}
 
-		const auto spu = static_cast<spu_thread*>(m_cpu);
+		const auto spu = static_cast<spu_thread*>(cpu);
 		auto& list = spu->local_breakpoints;
 		const u32 pos_at = loc / 4;
 		const u32 pos_bit = 1u << (pos_at % 8);

rpcs3/rpcs3qt/breakpoint_list.h

@@ -14,7 +14,7 @@ class breakpoint_list : public QListWidget
 
 public:
 	breakpoint_list(QWidget* parent, breakpoint_handler* handler);
-	void UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm);
+	void UpdateCPUData(std::shared_ptr<CPUDisAsm> disasm);
 	void ClearBreakpoints();
 	bool AddBreakpoint(u32 pc);
 	void RemoveBreakpoint(u32 addr);
@@ -33,6 +33,5 @@ private:
 	breakpoint_handler* m_ppu_breakpoint_handler;
 	QMenu* m_context_menu = nullptr;
 	QAction* m_delete_action;
-	cpu_thread* m_cpu = nullptr;
+	std::shared_ptr<CPUDisAsm> m_disasm = nullptr;
-	CPUDisAsm* m_disasm = nullptr;
 };

rpcs3/rpcs3qt/debugger_frame.cpp

@@ -53,15 +53,30 @@ extern bool is_using_interpreter(thread_class t_class)
 	}
 }
 
-extern std::shared_ptr<CPUDisAsm> make_disasm(const cpu_thread* cpu)
+extern std::shared_ptr<CPUDisAsm> make_disasm(const cpu_thread* cpu, std::shared_ptr<cpu_thread> handle)
 {
+	if (!handle)
+	{
+		switch (cpu->get_class())
+		{
+		case thread_class::ppu: handle = idm::get<named_thread<ppu_thread>>(cpu->id); break;
+		case thread_class::spu: handle = idm::get<named_thread<spu_thread>>(cpu->id); break;
+		default: break;
+		}
+	}
+
+	std::shared_ptr<CPUDisAsm> result;
+
 	switch (cpu->get_class())
 	{
-	case thread_class::ppu: return std::make_shared<PPUDisAsm>(cpu_disasm_mode::interpreter, vm::g_sudo_addr);
+	case thread_class::ppu: result = std::make_shared<PPUDisAsm>(cpu_disasm_mode::interpreter, vm::g_sudo_addr); break;
-	case thread_class::spu: return std::make_shared<SPUDisAsm>(cpu_disasm_mode::interpreter, static_cast<const spu_thread*>(cpu)->ls);
+	case thread_class::spu: result = std::make_shared<SPUDisAsm>(cpu_disasm_mode::interpreter, static_cast<const spu_thread*>(cpu)->ls); break;
-	case thread_class::rsx: return std::make_shared<RSXDisAsm>(cpu_disasm_mode::interpreter, vm::g_sudo_addr, 0, cpu);
+	case thread_class::rsx: result = std::make_shared<RSXDisAsm>(cpu_disasm_mode::interpreter, vm::g_sudo_addr, 0, cpu); break;
-	default: return nullptr;
+	default: return result;
 	}
+
+	result->set_cpu_handle(std::move(handle));
+	return result;
 }
 
 debugger_frame::debugger_frame(std::shared_ptr<gui_settings> gui_settings, QWidget *parent)
@@ -1088,12 +1103,7 @@ void debugger_frame::OnSelectUnit()
 
 			if (selected == m_cpu.get())
 			{
-				m_disasm = make_disasm(selected);
+				m_disasm = make_disasm(selected, m_cpu);
-			}
-			else
-			{
-				m_cpu.reset();
-				selected = nullptr;
 			}
 
 			break;
@@ -1104,12 +1114,7 @@ void debugger_frame::OnSelectUnit()
 
 			if (selected == m_cpu.get())
 			{
-				m_disasm = make_disasm(selected);
+				m_disasm = make_disasm(selected, m_cpu);
-			}
-			else
-			{
-				m_cpu.reset();
-				selected = nullptr;
 			}
 
 			break;
@@ -1120,7 +1125,7 @@ void debugger_frame::OnSelectUnit()
 
 			if (get_cpu())
 			{
-				m_disasm = make_disasm(m_rsx);
+				m_disasm = make_disasm(m_rsx, nullptr);
 			}
 
 			break;
@@ -1129,10 +1134,17 @@ void debugger_frame::OnSelectUnit()
 		}
 	}
 
+	if (!m_disasm)
+	{
+		m_cpu.reset();
+		m_rsx = nullptr;
+	}
+
 	EnableButtons(true);
 
-	m_debugger_list->UpdateCPUData(get_cpu(), m_disasm.get());
+	m_debugger_list->UpdateCPUData(m_disasm);
-	m_breakpoint_list->UpdateCPUData(get_cpu(), m_disasm.get());
+	m_breakpoint_list->UpdateCPUData(m_disasm);
+
 	ShowPC(true);
 	DoUpdate();
 	UpdateUI();
@@ -1272,8 +1284,8 @@ void debugger_frame::OnSelectSPUDisassembler()
 
 		EnableButtons(true);
 
-		m_debugger_list->UpdateCPUData(nullptr, m_disasm.get());
+		m_debugger_list->UpdateCPUData(m_disasm);
-		m_breakpoint_list->UpdateCPUData(nullptr, m_disasm.get());
+		m_breakpoint_list->UpdateCPUData(m_disasm);
 		ShowPC(true);
 		DoUpdate();
 		UpdateUI();

rpcs3/rpcs3qt/debugger_list.cpp

@@ -46,7 +46,9 @@ debugger_list::debugger_list(QWidget* parent, std::shared_ptr<gui_settings> gui_
 
 		u32 pc = m_start_addr;
 
-		for (; m_cpu && m_cpu->get_class() == thread_class::rsx && row; row--)
+		const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
+		for (; cpu && cpu->get_class() == thread_class::rsx && row; row--)
 		{
 			// If scrolling forwards (downwards), we can skip entire commands
 			pc += std::max<u32>(m_disasm->disasm(pc), 4);
@@ -56,20 +58,21 @@ debugger_list::debugger_list(QWidget* parent, std::shared_ptr<gui_settings> gui_
 	});
 }
 
-void debugger_list::UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm)
+void debugger_list::UpdateCPUData(std::shared_ptr<CPUDisAsm> disasm)
 {
-	if (m_cpu != cpu)
+	if ((!m_disasm) != (!disasm) || (m_disasm && disasm->get_cpu() != m_disasm->get_cpu()))
 	{
-		m_cpu = cpu;
 		m_selected_instruction = -1;
 		m_showing_selected_instruction = false;
 	}
 
-	m_disasm = disasm;
+	m_disasm = std::move(disasm);
 }
 
 u32 debugger_list::GetStartAddress(u32 address)
 {
+	const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
 	const u32 steps = m_item_count / 3;
 	const u32 inst_count_jump_on_step = std::min<u32>(steps, 4);
 
@@ -78,9 +81,9 @@ u32 debugger_list::GetStartAddress(u32 address)
 
 	u32 result = address & address_mask;
 
-	if (m_cpu && m_cpu->get_class() == thread_class::rsx)
+	if (cpu && cpu->get_class() == thread_class::rsx)
 	{
-		if (auto [count, res] = static_cast<rsx::thread*>(m_cpu)->try_get_pc_of_x_cmds_backwards(steps, address); count == steps)
+		if (auto [count, res] = static_cast<rsx::thread*>(cpu)->try_get_pc_of_x_cmds_backwards(steps, address); count == steps)
 		{
 			result = res;
 		}
@@ -92,9 +95,9 @@ u32 debugger_list::GetStartAddress(u32 address)
 
 	u32 upper_bound = (m_start_addr + (steps * 4)) & address_mask;
 
-	if (m_cpu && m_cpu->get_class() == thread_class::rsx)
+	if (cpu && cpu->get_class() == thread_class::rsx)
 	{
-		if (auto [count, res] = static_cast<rsx::thread*>(m_cpu)->try_get_pc_of_x_cmds_backwards(0 - steps, m_start_addr); count == steps)
+		if (auto [count, res] = static_cast<rsx::thread*>(cpu)->try_get_pc_of_x_cmds_backwards(0 - steps, m_start_addr); count == steps)
 		{
 			upper_bound = res;
 		}
@@ -131,21 +134,25 @@ u32 debugger_list::GetStartAddress(u32 address)
 
 bool debugger_list::IsSpu() const
 {
-	return (m_cpu && m_cpu->get_class() == thread_class::spu) || (m_disasm && !m_cpu);
+	const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
+	return (cpu && cpu->get_class() == thread_class::spu) || (m_disasm && !cpu);
 }
 
 void debugger_list::ShowAddress(u32 addr, bool select_addr, bool direct)
 {
+	const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
 	const decltype(spu_thread::local_breakpoints)* spu_bps_list{};
 
-	if (m_cpu && m_cpu->get_class() == thread_class::spu)
+	if (cpu && cpu->get_class() == thread_class::spu)
 	{
-		spu_bps_list = &static_cast<spu_thread*>(m_cpu)->local_breakpoints;
+		spu_bps_list = &static_cast<spu_thread*>(cpu)->local_breakpoints;
 	}
 
 	auto IsBreakpoint = [&](u32 pc)
 	{
-		switch (m_cpu ? m_cpu->get_class() : thread_class::general)
+		switch (cpu ? cpu->get_class() : thread_class::general)
 		{
 		case thread_class::ppu:
 		{
@@ -195,7 +202,7 @@ void debugger_list::ShowAddress(u32 addr, bool select_addr, bool direct)
 		}
 	}
 
-	if (!m_disasm || (m_cpu && m_cpu->state.all_of(cpu_flag::exit + cpu_flag::wait)))
+	if (!m_disasm || (cpu && cpu->state.all_of(cpu_flag::exit + cpu_flag::wait)))
 	{
 		for (uint i = 0; i < m_item_count; ++i)
 		{
@@ -209,7 +216,7 @@ void debugger_list::ShowAddress(u32 addr, bool select_addr, bool direct)
 	{
 		const bool is_spu = IsSpu();
 		const u32 address_limits = (is_spu ? 0x3fffc : ~3);
-		const u32 current_pc = (m_cpu ? m_cpu->get_pc() : 0);
+		const u32 current_pc = (cpu ? cpu->get_pc() : 0);
 		m_start_addr &= address_limits;
 		pc = m_start_addr;
 
@@ -243,14 +250,14 @@ void debugger_list::ShowAddress(u32 addr, bool select_addr, bool direct)
 				list_item->setBackground(default_background);
 			}
 
-			if (m_cpu && m_cpu->get_class() == thread_class::ppu && !vm::check_addr(pc, 0))
+			if (cpu && cpu->get_class() == thread_class::ppu && !vm::check_addr(pc, 0))
 			{
 				list_item->setText((IsBreakpoint(pc) ? ">> " : "   ") + qstr(fmt::format("[%08x]  ?? ?? ?? ??:", pc)));
 				count = 4;
 				continue;
 			}
 
-			if (m_cpu && m_cpu->get_class() == thread_class::ppu && !vm::check_addr(pc, vm::page_executable))
+			if (cpu && cpu->get_class() == thread_class::ppu && !vm::check_addr(pc, vm::page_executable))
 			{
 				const u32 data = *vm::get_super_ptr<atomic_be_t<u32>>(pc);
 				list_item->setText((IsBreakpoint(pc) ? ">> " : "   ") + qstr(fmt::format("[%08x]  %02x %02x %02x %02x:", pc,
@@ -292,16 +299,18 @@ void debugger_list::EnableThreadFollowing(bool enable)
 
 void debugger_list::scroll(s32 steps)
 {
-	for (; m_cpu && m_cpu->get_class() == thread_class::rsx && steps > 0; steps--)
+	const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
+	for (; cpu && cpu->get_class() == thread_class::rsx && steps > 0; steps--)
 	{
 		// If scrolling forwards (downwards), we can skip entire commands
 		m_start_addr += std::max<u32>(m_disasm->disasm(m_start_addr), 4);
 	}
 
-	if (m_cpu && m_cpu->get_class() == thread_class::rsx && steps < 0)
+	if (cpu && cpu->get_class() == thread_class::rsx && steps < 0)
 	{
 		// If scrolling backwards (upwards), try to obtain the start of commands tail
-		if (auto [count, res] = static_cast<rsx::thread*>(m_cpu)->try_get_pc_of_x_cmds_backwards(-steps, m_start_addr); count == 0u - steps)
+		if (auto [count, res] = static_cast<rsx::thread*>(cpu)->try_get_pc_of_x_cmds_backwards(-steps, m_start_addr); count == 0u - steps)
 		{
 			steps = 0;
 			m_start_addr = res;
@@ -315,6 +324,8 @@ void debugger_list::scroll(s32 steps)
 
 void debugger_list::keyPressEvent(QKeyEvent* event)
 {
+	const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
 	// Always accept event (so it would not bubble upwards, debugger_frame already sees it)
 	struct accept_event_t
 	{
@@ -352,7 +363,7 @@ void debugger_list::keyPressEvent(QKeyEvent* event)
 			return;
 		}
 
-		if (m_cpu && m_cpu->get_class() == thread_class::rsx)
+		if (cpu && cpu->get_class() == thread_class::rsx)
 		{
 			create_rsx_command_detail(m_showing_selected_instruction ? m_selected_instruction : m_pc);
 			return;
@@ -381,7 +392,7 @@ void debugger_list::hideEvent(QHideEvent* event)
 
 void debugger_list::create_rsx_command_detail(u32 pc)
 {
-	RSXDisAsm rsx_dis = *static_cast<RSXDisAsm*>(m_disasm);
+	RSXDisAsm rsx_dis = static_cast<RSXDisAsm&>(*m_disasm);
 	rsx_dis.change_mode(cpu_disasm_mode::list);
 
 	// Either invalid or not a method
@@ -425,7 +436,9 @@ void debugger_list::mouseDoubleClickEvent(QMouseEvent* event)
 
 		u32 pc = m_start_addr;
 
-		for (; m_cpu && m_cpu->get_class() == thread_class::rsx && i; i--)
+		const auto cpu = m_disasm ? m_disasm->get_cpu() : nullptr;
+
+		for (; cpu && cpu->get_class() == thread_class::rsx && i; i--)
 		{
 			// If scrolling forwards (downwards), we can skip entire commands
 			pc += std::max<u32>(m_disasm->disasm(pc), 4);

rpcs3/rpcs3qt/debugger_list.h

@@ -33,7 +33,7 @@ Q_SIGNALS:
 	void BreakpointRequested(u32 loc, bool only_add = false);
 public:
 	debugger_list(QWidget* parent, std::shared_ptr<gui_settings> settings, breakpoint_handler* handler);
-	void UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm);
+	void UpdateCPUData(std::shared_ptr<CPUDisAsm> disasm);
 	void EnableThreadFollowing(bool enable = true);
 public Q_SLOTS:
 	void ShowAddress(u32 addr, bool select_addr = true, bool direct = false);
@@ -58,7 +58,7 @@ private:
 
 	breakpoint_handler* m_ppu_breakpoint_handler;
 	cpu_thread* m_cpu = nullptr;
-	CPUDisAsm* m_disasm = nullptr;
+	std::shared_ptr<CPUDisAsm> m_disasm;
 	QDialog* m_cmd_detail = nullptr;
 	QLabel* m_detail_label = nullptr;
 };