Debugger: Fix and use centered PC by default

* Fix instruction-selection dependent functionality.
* Remove odd instruction position jumps in add/remove breakpoint.
* Make PC fixate at 1/3 of the frame because knowing future instructions is more important than knowing the previous.

rpcs3/rpcs3qt/breakpoint_list.cpp

@@ -68,8 +68,6 @@ void breakpoint_list::RemoveBreakpoint(u32 addr)
 		}
 	}
 
-	Q_EMIT RequestShowAddress(addr);
-
 	if (!count())
 	{
 		hide();
@@ -94,8 +92,6 @@ bool breakpoint_list::AddBreakpoint(u32 pc)
 	breakpoint_item->setData(Qt::UserRole, pc);
 	addItem(breakpoint_item);
 
-	Q_EMIT RequestShowAddress(pc);
-
 	show();
 
 	return true;

rpcs3/rpcs3qt/debugger_frame.cpp

@@ -360,7 +360,8 @@ void debugger_frame::keyPressEvent(QKeyEvent* event)
 	}
 
 	const u32 address_limits = (cpu->id_type() == 2 ? 0x3fffc : ~3);
-	const u32 pc = (row >= 0 ? m_debugger_list->m_pc + row * 4 : cpu->get_pc()) & address_limits;
+	const u32 pc = (m_debugger_list->m_pc & address_limits);
+	const u32 selected = (m_debugger_list->m_showing_selected_instruction ? m_debugger_list->m_selected_instruction : cpu->get_pc()) & address_limits;
 
 	const auto modifiers = QApplication::keyboardModifiers();
 
@@ -568,7 +569,7 @@ void debugger_frame::keyPressEvent(QKeyEvent* event)
 			{
 				if (!m_inst_editor)
 				{
-					m_inst_editor = new instruction_editor_dialog(this, pc, m_disasm.get(), make_check_cpu(cpu));
+					m_inst_editor = new instruction_editor_dialog(this, selected, m_disasm.get(), make_check_cpu(cpu));
 					connect(m_inst_editor, &QDialog::finished, this, [this]() { m_inst_editor = nullptr; });
 					m_inst_editor->show();
 				}
@@ -625,19 +626,21 @@ void debugger_frame::keyPressEvent(QKeyEvent* event)
 			// Indirect branches (unknown targets, such as function return) do not proceed to any instruction
 			std::array<u32, 2> res{umax, umax};
 
+			const u32 selected = (m_debugger_list->m_showing_selected_instruction ? m_debugger_list->m_selected_instruction : cpu->get_pc()) & address_limits;
+
 			switch (cpu->id_type())
 			{
 			case 2:
 			{
-				res = op_branch_targets(pc, spu_opcode_t{static_cast<spu_thread*>(cpu)->_ref<u32>(pc)});
+				res = op_branch_targets(selected, spu_opcode_t{static_cast<spu_thread*>(cpu)->_ref<u32>(selected)});
 				break;
 			}
 			case 1:
 			{
 				be_t<ppu_opcode_t> op{};
 
-				if (vm::check_addr(pc, vm::page_executable) && vm::try_access(pc, &op, 4, false))
-					res = op_branch_targets(pc, op);
+				if (vm::check_addr(selected, vm::page_executable) && vm::try_access(selected, &op, 4, false))
+					res = op_branch_targets(selected, op);
 
 				break;
 			}
@@ -645,7 +648,7 @@ void debugger_frame::keyPressEvent(QKeyEvent* event)
 			}
 
 			if (const usz pos = std::basic_string_view<u32>(res.data(), 2).find_last_not_of(umax); pos != umax)
-				m_debugger_list->ShowAddress(res[pos] - std::max(row, 0) * 4, true, true);
+				m_debugger_list->ShowAddress(res[pos] - std::max(row, 0) * 4, true);
 
 			return;
 		}

rpcs3/rpcs3qt/debugger_list.cpp

@@ -34,6 +34,26 @@ debugger_list::debugger_list(QWidget* parent, std::shared_ptr<gui_settings> gui_
 
 	setSizeAdjustPolicy(QListWidget::AdjustToContents);
 	setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff);
+
+	connect(this, &QListWidget::currentRowChanged, this, [this](int row)
+	{
+		if (row < 0)
+		{
+			m_selected_instruction = -1;
+			m_showing_selected_instruction = false;
+			return;
+		}
+
+		u32 pc = m_start_addr;
+
+		for (; m_cpu && m_cpu->id_type() == 0x55 && row; row--)
+		{
+			// If scrolling forwards (downwards), we can skip entire commands
+			pc += std::max<u32>(m_disasm->disasm(pc), 4);
+		}
+
+		m_selected_instruction = pc + row * 4;
+	});
 }
 
 void debugger_list::UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm)
@@ -42,17 +62,40 @@ void debugger_list::UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm)
 	{
 		m_cpu = cpu;
 		m_selected_instruction = -1;
+		m_showing_selected_instruction = false;
 	}
 
 	m_disasm = disasm;
 }
 
-u32 debugger_list::GetCenteredAddress(u32 address) const
+u32 debugger_list::GetStartAddress(u32 address)
 {
-	return address - ((m_item_count / 2) * 4);
+	const u32 steps = m_item_count / 3;
+
+	u32 result = address;
+
+	if (m_cpu && m_cpu->id_type() == 0x55)
+	{
+		if (auto [count, res] = static_cast<rsx::thread*>(m_cpu)->try_get_pc_of_x_cmds_backwards(steps, address); count == steps)
+		{
+			result = res;
+		}
+	}
+	else
+	{
+		result = address - (steps * 4);
+	}
+
+	if (address > m_pc || m_start_addr > address)
+	{
+		m_pc = address;
+		m_start_addr = result;
+	}
+
+	return m_start_addr;
 }
 
-void debugger_list::ShowAddress(u32 addr, bool select_addr, bool force)
+void debugger_list::ShowAddress(u32 addr, bool select_addr, bool direct)
 {
 	const decltype(spu_thread::local_breakpoints)* spu_bps_list;
 
@@ -71,32 +114,24 @@ void debugger_list::ShowAddress(u32 addr, bool select_addr, bool force)
 		}
 	};
 
-	bool center_pc = m_gui_settings->GetValue(gui::d_centerPC).toBool();
-
-	// How many spaces addr can move down without us needing to move the entire view
-	const u32 addr_margin = (m_item_count / (center_pc ? 2 : 1) - 4); // 4 is just a buffer of 4 spaces at the bottom
-
-	if (select_addr || force)
+	if (select_addr || direct)
 	{
 		// The user wants to survey a specific memory location, do not interfere from this point forth 
 		m_follow_thread = false;
 	}
 
-	if (force || ((m_follow_thread || select_addr) && addr - m_pc > addr_margin * 4)) // 4 is the number of bytes in each instruction
+	u32 pc = m_start_addr;
+
+	if (!direct && (m_follow_thread || select_addr))
 	{
-		if (center_pc)
-		{
-			m_pc = GetCenteredAddress(addr);
-		}
-		else
-		{
-			m_pc = addr;
-		}
+		pc = GetStartAddress(addr);
 	}
 
 	const auto& default_foreground = palette().color(foregroundRole());
 	const auto& default_background = palette().color(backgroundRole());
 
+	m_showing_selected_instruction = false;
+
 	if (select_addr)
 	{
 		m_selected_instruction = addr;
@@ -124,14 +159,14 @@ void debugger_list::ShowAddress(u32 addr, bool select_addr, bool force)
 	{
 		const bool is_spu = m_cpu->id_type() == 2;
 		const u32 address_limits = (is_spu ? 0x3fffc : ~3);
-		m_pc &= address_limits;
-		u32 pc = m_pc;
+		m_start_addr &= address_limits;
+		u32 pc = m_start_addr;
 
 		for (uint i = 0, count = 4; i < m_item_count; ++i, pc = (pc + count) & address_limits)
 		{
 			QListWidgetItem* list_item = item(i);
 
-			if (m_cpu->is_paused() && pc == m_cpu->get_pc())
+			if (pc == m_cpu->get_pc())
 			{
 				list_item->setForeground(m_text_color_pc);
 				list_item->setBackground(m_color_pc);
@@ -143,6 +178,8 @@ void debugger_list::ShowAddress(u32 addr, bool select_addr, bool force)
 				{
 					list_item->setSelected(true);
 				}
+
+				m_showing_selected_instruction = true;
 			}
 			else if (IsBreakpoint(pc))
 			{
@@ -204,26 +241,25 @@ void debugger_list::EnableThreadFollowing(bool enable)
 
 void debugger_list::scroll(s32 steps)
 {
-	while (m_cpu && m_cpu->id_type() == 0x55 && steps > 0)
+	for (; m_cpu && m_cpu->id_type() == 0x55 && steps > 0; steps--)
 	{
 		// If scrolling forwards (downwards), we can skip entire commands
-		// Backwards is impossible though
-		m_pc += std::max<u32>(m_disasm->disasm(m_pc), 4);
-		steps--;
+		m_start_addr += std::max<u32>(m_disasm->disasm(m_start_addr), 4);
 	}
 
 	if (m_cpu && m_cpu->id_type() == 0x55 && 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_pc); count == 0u - steps)
+		if (auto [count, res] = static_cast<rsx::thread*>(m_cpu)->try_get_pc_of_x_cmds_backwards(-steps, m_start_addr); count == 0u - steps)
 		{
 			steps = 0;
-			m_pc = res;
+			m_start_addr = res;
 		}
 	}
 
 	EnableThreadFollowing(false);
-	ShowAddress(m_pc + (steps * 4), false, true);
+	m_start_addr += steps * 4;
+	ShowAddress(0, false, true);
 }
 
 void debugger_list::keyPressEvent(QKeyEvent* event)
@@ -247,7 +283,7 @@ void debugger_list::keyPressEvent(QKeyEvent* event)
 		}
 		if (m_cpu && m_cpu->id_type() == 0x55)
 		{
-			create_rsx_command_detail(m_pc, currentRow());
+			create_rsx_command_detail(m_showing_selected_instruction ? m_selected_instruction : m_pc);
 			return;
 		}
 		return;
@@ -269,19 +305,8 @@ void debugger_list::hideEvent(QHideEvent* event)
 	QListWidget::hideEvent(event);
 }
 
-void debugger_list::create_rsx_command_detail(u32 pc, int row)
+void debugger_list::create_rsx_command_detail(u32 pc)
 {
-	if (row < 0)
-	{
-		return;
-	}
-
-	for (; row > 0; row--)
-	{
-		// Skip methods
-		pc += std::max<u32>(m_disasm->disasm(pc), 4);
-	}
-
 	RSXDisAsm rsx_dis = *static_cast<RSXDisAsm*>(m_disasm);
 	rsx_dis.change_mode(cpu_disasm_mode::list);
 
@@ -321,10 +346,19 @@ void debugger_list::mouseDoubleClickEvent(QMouseEvent* event)
 {
 	if (event->button() == Qt::LeftButton)
 	{
-		const int i = currentRow();
+		int i = currentRow();
 		if (i < 0) return;
 
-		const u32 pc = m_pc + i * 4;
+		u32 pc = m_start_addr;
+
+		for (; m_cpu && m_cpu->id_type() == 0x55 && i; i--)
+		{
+			// If scrolling forwards (downwards), we can skip entire commands
+			pc += std::max<u32>(m_disasm->disasm(pc), 4);
+		}
+
+		pc += i * 4;
+		m_selected_instruction = pc;
 
 		// Let debugger_frame know about breakpoint.
 		// Other option is to add to breakpoint manager directly and have a signal there instead.

rpcs3/rpcs3qt/debugger_list.h

@@ -18,9 +18,11 @@ class debugger_list : public QListWidget
 
 public:
 	u32 m_pc = 0;
+	u32 m_start_addr = 0;
 	u32 m_item_count = 30;
-	bool m_follow_thread = true; // If true, follow the selected thread to wherever it goes in code
 	u32 m_selected_instruction = -1;
+	bool m_follow_thread = true; // If true, follow the selected thread to wherever it goes in code
+	bool m_showing_selected_instruction = false;
 	QColor m_color_bp;
 	QColor m_color_pc;
 	QColor m_text_color_bp;
@@ -33,7 +35,7 @@ public:
 	void UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm);
 	void EnableThreadFollowing(bool enable = true);
 public Q_SLOTS:
-	void ShowAddress(u32 addr, bool select_addr = true, bool force = false);
+	void ShowAddress(u32 addr, bool select_addr = true, bool direct = false);
 	void RefreshView();
 protected:
 	void keyPressEvent(QKeyEvent* event) override;
@@ -43,12 +45,12 @@ protected:
 	void showEvent(QShowEvent* event) override;
 	void hideEvent(QHideEvent* event) override;
 	void scroll(s32 steps);
-	void create_rsx_command_detail(u32 pc, int row);
+	void create_rsx_command_detail(u32 pc);
 private:
 	/**
 	* It really upsetted me I had to copy this code to make debugger_list/frame not circularly dependent.
 	*/
-	u32 GetCenteredAddress(u32 address) const;
+	u32 GetStartAddress(u32 address);
 
 	std::shared_ptr<gui_settings> m_gui_settings;
 

rpcs3/rpcs3qt/gui_settings.h

@@ -196,7 +196,6 @@ namespace gui
 	const gui_save l_limit_tty = gui_save(logger, "TTY_limit", 1000);
 
 	const gui_save d_splitterState = gui_save(debugger, "splitterState", QByteArray());
-	const gui_save d_centerPC      = gui_save(debugger, "centerPC",      false);
 
 	const gui_save rsx_geometry = gui_save(rsx, "geometry", QByteArray());
 	const gui_save rsx_states   = gui_save(rsx, "states",   QVariantMap());