Debugger: Add some error pop-ups for invalid operations

* Show error window when setting breakpoints on these conditions:
- SPU/RSX are selected. (not supported)
- When using non-interpreters decoders.
- Non-executable memory is specified.
* Do not allow instruction stepping for non-interpreters decoders.
* Clear breakpoints when the game is stopped.
* Fix setting breakpoints on HLE functions.

rpcs3/Emu/Cell/PPUThread.cpp

@@ -513,57 +513,45 @@ static bool ppu_break(ppu_thread& ppu, ppu_opcode_t)
 }
 
 // Set or remove breakpoint
-extern void ppu_breakpoint(u32 addr, bool is_adding)
+extern bool ppu_breakpoint(u32 addr, bool is_adding)
 {
-	if (g_cfg.core.ppu_decoder == ppu_decoder_type::llvm)
+	if (!vm::check_addr(addr, vm::page_executable) || g_cfg.core.ppu_decoder == ppu_decoder_type::llvm)
 	{
-		return;
+		return false;
 	}
 
 	const u64 _break = reinterpret_cast<uptr>(&ppu_break);
 
+	// Remove breakpoint parameters
+	u64 to_set = 0;
+	u64 expected = _break;
+
+	if (u32 hle_addr{}; g_fxo->is_init<ppu_function_manager>() && (hle_addr = g_fxo->get<ppu_function_manager>().addr))
+	{
+		// HLE function index
+		const u32 index = (addr - hle_addr) / 8;
+
+		if (addr % 8 == 4 && index < ppu_function_manager::get().size())
+		{
+			// HLE function placement
+			to_set = reinterpret_cast<uptr>(ppu_function_manager::get()[index]);
+		}
+	}
+
+	if (!to_set)
+	{
+		// If not an HLE function use regular instruction function
+		to_set = ppu_cache(addr);
+	}
+
 	if (is_adding)
 	{
-		// Set breakpoint
+		// Swap if adding
-		ppu_ref(addr) = _break;
+		std::swap(to_set, expected);
-	}
-	else
-	{
-		// Remove breakpoint
-		ppu_ref(addr) = ppu_cache(addr);
-	}
 	}
 
-//sets breakpoint, does nothing if there is a breakpoint there already
+	auto& ref = reinterpret_cast<atomic_t<u64>&>(ppu_ref(addr));
-extern void ppu_set_breakpoint(u32 addr)
+	return ref.compare_and_swap_test(expected, to_set);
-{
-	if (g_cfg.core.ppu_decoder == ppu_decoder_type::llvm)
-	{
-		return;
-	}
-
-	const u64 _break = reinterpret_cast<uptr>(&ppu_break);
-
-	if (ppu_ref(addr) != _break)
-	{
-		ppu_ref(addr) = _break;
-	}
-}
-
-//removes breakpoint, does nothing if there is no breakpoint at location
-extern void ppu_remove_breakpoint(u32 addr)
-{
-	if (g_cfg.core.ppu_decoder == ppu_decoder_type::llvm)
-	{
-		return;
-	}
-
-	const auto _break = reinterpret_cast<uptr>(&ppu_break);
-
-	if (ppu_ref(addr) == _break)
-	{
-		ppu_ref(addr) = ppu_cache(addr);
-	}
 }
 
 extern bool ppu_patch(u32 addr, u32 value)

rpcs3/Emu/GDB.cpp

@@ -30,8 +30,7 @@
 #include <regex>
 #include <charconv>
 
-extern void ppu_set_breakpoint(u32 addr);
+extern bool ppu_breakpoint(u32 addr, bool is_adding);
-extern void ppu_remove_breakpoint(u32 addr);
 
 LOG_CHANNEL(GDB);
 
@@ -776,7 +775,7 @@ bool gdb_thread::cmd_set_breakpoint(gdb_cmd& cmd)
 			GDB.warning("Can't parse breakpoint request, data: '%s'.", cmd.data);
 			return send_cmd_ack("E02");
 		}
-		ppu_set_breakpoint(addr);
+		ppu_breakpoint(addr, true);
 		return send_cmd_ack("OK");
 	}
 	//other breakpoint types are not supported
@@ -794,7 +793,7 @@ bool gdb_thread::cmd_remove_breakpoint(gdb_cmd& cmd)
 			GDB.warning("Can't parse breakpoint remove request, data: '%s'.", cmd.data);
 			return send_cmd_ack("E01");
 		}
-		ppu_remove_breakpoint(addr);
+		ppu_breakpoint(addr, false);
 		return send_cmd_ack("OK");
 	}
 	//other breakpoint types are not supported

rpcs3/rpcs3qt/breakpoint_handler.cpp

@@ -1,6 +1,6 @@
 #include "breakpoint_handler.h"
 
-extern void ppu_breakpoint(u32 loc, bool is_adding);
+extern bool ppu_breakpoint(u32 loc, bool is_adding);
 
 bool breakpoint_handler::HasBreakpoint(u32 loc) const
 {
@@ -9,14 +9,22 @@ bool breakpoint_handler::HasBreakpoint(u32 loc) const
 
 bool breakpoint_handler::AddBreakpoint(u32 loc)
 {
-	m_breakpoints.insert(loc);
+	if (!ppu_breakpoint(loc, true))
-	ppu_breakpoint(loc, true);
+	{
+		return false;
+	}
+
+	ensure(m_breakpoints.insert(loc).second);
 	return true;
 }
 
 bool breakpoint_handler::RemoveBreakpoint(u32 loc)
 {
-	m_breakpoints.erase(loc);
+	if (m_breakpoints.erase(loc) == 0)
-	ppu_breakpoint(loc, false);
+	{
+		return false;
+	}
+
+	ensure(ppu_breakpoint(loc, false));
 	return true;
 }

rpcs3/rpcs3qt/breakpoint_list.cpp

@@ -5,9 +5,12 @@
 #include "Emu/Cell/PPUThread.h"
 
 #include <QMenu>
+#include <QMessageBox>
 
 constexpr auto qstr = QString::fromStdString;
 
+extern bool is_using_interpreter(u32 id_type);
+
 breakpoint_list::breakpoint_list(QWidget* parent, breakpoint_handler* handler) : QListWidget(parent), m_breakpoint_handler(handler)
 {
 	setEditTriggers(QAbstractItemView::NoEditTriggers);
@@ -62,9 +65,12 @@ void breakpoint_list::RemoveBreakpoint(u32 addr)
 	Q_EMIT RequestShowAddress(addr);
 }
 
-void breakpoint_list::AddBreakpoint(u32 pc)
+bool breakpoint_list::AddBreakpoint(u32 pc)
 {
-	m_breakpoint_handler->AddBreakpoint(pc);
+	if (!m_breakpoint_handler->AddBreakpoint(pc))
+	{
+		return false;
+	}
 
 	m_disasm->disasm(pc);
 
@@ -78,6 +84,8 @@ void breakpoint_list::AddBreakpoint(u32 pc)
 	addItem(breakpoint_item);
 
 	Q_EMIT RequestShowAddress(pc);
+
+	return true;
 }
 
 /**
@@ -85,9 +93,27 @@ void breakpoint_list::AddBreakpoint(u32 pc)
 */
 void breakpoint_list::HandleBreakpointRequest(u32 loc)
 {
-	if (!m_cpu || m_cpu->id_type() != 1 || !vm::check_addr(loc, vm::page_allocated | vm::page_executable))
+	if (!m_cpu || m_cpu->state & cpu_flag::exit)
+	{
+		return;
+	}
+
+	if (m_cpu->id_type() != 1)
 	{
 		// TODO: SPU breakpoints
+		QMessageBox::warning(this, tr("Unimplemented Breakpoints For Thread Type!"), tr("Cannot set breakpoints on non-PPU thread currently, sorry."));
+		return;
+	}
+
+	if (!vm::check_addr(loc, vm::page_executable))
+	{
+		QMessageBox::warning(this, tr("Invalid Memory For Breakpoints!"), tr("Cannot set breakpoints on non-executable memory!"));
+		return;
+	}
+
+	if (!is_using_interpreter(m_cpu->id_type()))
+	{
+		QMessageBox::warning(this, tr("Interpreters-Only Feature!"), tr("Cannot set breakpoints on non-interpreter decoders."));
 		return;
 	}
 
@@ -97,7 +123,11 @@ void breakpoint_list::HandleBreakpointRequest(u32 loc)
 	}
 	else
 	{
-		AddBreakpoint(loc);
+		if (!AddBreakpoint(loc))
+		{
+			QMessageBox::warning(this, tr("Unknown error while setting breakpoint!"), tr("Failed to set breakpoints."));
+			return;
+		}
 	}
 }
 

rpcs3/rpcs3qt/breakpoint_list.h

@@ -16,7 +16,7 @@ public:
 	breakpoint_list(QWidget* parent, breakpoint_handler* handler);
 	void UpdateCPUData(cpu_thread* cpu, CPUDisAsm* disasm);
 	void ClearBreakpoints();
-	void AddBreakpoint(u32 pc);
+	bool AddBreakpoint(u32 pc);
 	void RemoveBreakpoint(u32 addr);
 
 	QColor m_text_color_bp;

rpcs3/rpcs3qt/debugger_frame.cpp

@@ -39,6 +39,16 @@ constexpr auto s_pause_flags = cpu_flag::dbg_pause + cpu_flag::dbg_global_pause;
 
 extern atomic_t<bool> g_debugger_pause_all_threads_on_bp;
 
+extern bool is_using_interpreter(u32 id_type)
+{
+	switch (id_type)
+	{
+	case 1: return g_cfg.core.ppu_decoder != ppu_decoder_type::llvm;
+	case 2: return g_cfg.core.spu_decoder == spu_decoder_type::fast || g_cfg.core.spu_decoder == spu_decoder_type::precise;
+	default: return true; 
+	}
+}
+
 debugger_frame::debugger_frame(std::shared_ptr<gui_settings> gui_settings, QWidget *parent)
 	: custom_dock_widget(tr("Debugger"), parent)
 	, m_gui_settings(std::move(gui_settings))
@@ -733,17 +743,21 @@ void debugger_frame::UpdateUI()
 			m_last_pc = cia;
 			DoUpdate();
 
-			if (cpu->state & s_pause_flags)
+			const bool paused = !!(cpu->state & s_pause_flags);
+
+			if (paused)
 			{
 				m_btn_run->setText(RunString);
-				m_btn_step->setEnabled(true);
-				m_btn_step_over->setEnabled(true);
 			}
 			else
 			{
 				m_btn_run->setText(PauseString);
-				m_btn_step->setEnabled(false);
+			}
-				m_btn_step_over->setEnabled(false);
+
+			if (is_using_interpreter(cpu->id_type()))
+			{
+				m_btn_step->setEnabled(paused);
+				m_btn_step_over->setEnabled(paused);
 			}
 		}
 	}
@@ -814,6 +828,11 @@ void debugger_frame::UpdateUnitList()
 		if (m_inst_editor) m_inst_editor->close();
 	}
 
+	if (emu_state == system_state::stopped)
+	{
+		ClearBreakpoints();
+	}
+
 	OnSelectUnit();
 
 	m_choice_units->update();
@@ -1094,11 +1113,15 @@ void debugger_frame::EnableUpdateTimer(bool enable) const
 
 void debugger_frame::EnableButtons(bool enable)
 {
-	if (!get_cpu()) enable = false;
+	const auto cpu = get_cpu();
+
+	if (!cpu) enable = false;
+
+	const bool step = enable && is_using_interpreter(cpu->id_type());
  
 	m_go_to_addr->setEnabled(enable);
 	m_go_to_pc->setEnabled(enable);
-	m_btn_step->setEnabled(enable);
+	m_btn_step->setEnabled(step);
-	m_btn_step_over->setEnabled(enable);
+	m_btn_step_over->setEnabled(step);
 	m_btn_run->setEnabled(enable);
 }