Thread.cpp: Fix game exit on access violation

Fixes game closing in Twisted Metal.
This commit also aloows to debug such states by using cpu_flag::req_exit as a broker

Utilities/Thread.cpp

@@ -107,7 +107,7 @@ thread_local u64 g_tls_fault_rsx = 0;
 thread_local u64 g_tls_fault_spu = 0;
 thread_local u64 g_tls_wait_time = 0;
 thread_local u64 g_tls_wait_fail = 0;
-thread_local bool g_tls_access_violation_recovered = false;
+thread_local u64 g_tls_access_violation_recovered = umax;
 extern thread_local std::string(*g_tls_log_prefix)();
 
 namespace stx
@@ -1269,7 +1269,7 @@ namespace rsx
 	extern std::function<bool(u32 addr, bool is_writing)> g_access_violation_handler;
 }
 
-bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noexcept
+bool handle_access_violation(u32 addr, bool is_writing, bool is_exec, ucontext_t* context) noexcept
 {
 	g_tls_fault_all++;
 
@@ -1305,7 +1305,7 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 		}
 	} spu_protection{cpu};
 
-	if (addr < RAW_SPU_BASE_ADDR && vm::check_addr(addr) && rsx::g_access_violation_handler)
+	if (!is_exec && addr < RAW_SPU_BASE_ADDR && vm::check_addr(addr) && rsx::g_access_violation_handler)
 	{
 		bool state_changed = false;
 
@@ -1371,7 +1371,7 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 	{
 		auto thread = idm::get_unlocked<named_thread<spu_thread>>(spu_thread::find_raw_spu((addr - RAW_SPU_BASE_ADDR) / RAW_SPU_OFFSET));
 
-		if (!thread)
+		if (!thread || is_exec)
 		{
 			break;
 		}
@@ -1503,7 +1503,9 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 	static_cast<void>(context);
 #endif /* ARCH_ */
 
-	if (vm::check_addr(addr, is_writing ? vm::page_writable : vm::page_readable))
+	const auto required_page_perms = (is_writing ? vm::page_writable : vm::page_readable) + (is_exec ? vm::page_executable : 0);
+
+	if (vm::check_addr(addr, required_page_perms))
 	{
 		return true;
 	}
@@ -1511,9 +1513,7 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 	// Hack: allocate memory in case the emulator is stopping
 	const auto hack_alloc = [&]()
 	{
-		g_tls_access_violation_recovered = true;
+		if (vm::check_addr(addr, required_page_perms))
-
-		if (vm::check_addr(addr, is_writing ? vm::page_writable : vm::page_readable))
 		{
 			return true;
 		}
@@ -1525,17 +1525,45 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 			return false;
 		}
 
+		extern void ppu_register_range(u32 addr, u32 size);
+
+		bool reprotected = false;
+
 		if (vm::writer_lock mlock; area->flags & vm::preallocated || vm::check_addr(addr, 0))
 		{
 			// For allocated memory with protection lower than required (such as protection::no or read-only while writing to it)
 			utils::memory_protect(vm::base(addr & -0x1000), 0x1000, utils::protection::rw);
+			reprotected = true;
+		}
+
+		if (reprotected)
+		{
+			if (is_exec && !vm::check_addr(addr, vm::page_executable))
+			{
+				ppu_register_range(addr & -0x10000, 0x10000);
+			}
+
+			g_tls_access_violation_recovered = addr;
 			return true;
 		}
 
-		return area->falloc(addr & -0x10000, 0x10000) || vm::check_addr(addr, is_writing ? vm::page_writable : vm::page_readable);
+		const bool allocated = area->falloc(addr & -0x10000, 0x10000);
+
+		if (allocated)
+		{
+			if (is_exec && !vm::check_addr(addr, vm::page_executable))
+			{
+				ppu_register_range(addr & -0x10000, 0x10000);
+			}
+
+			g_tls_access_violation_recovered = addr;
+			return true;
+		}
+
+		return false;
 	};
 
-	if (cpu && (cpu->get_class() == thread_class::ppu || cpu->get_class() == thread_class::spu))
+	if (cpu && (cpu->get_class() == thread_class::ppu || cpu->get_class() == thread_class::spu) && !is_exec)
 	{
 		vm::temporary_unlock(*cpu);
 		u32 pf_port_id = 0;
@@ -1678,7 +1706,7 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 
 		if (cpu->get_class() == thread_class::spu)
 		{
-			if (!g_tls_access_violation_recovered)
+			if (g_tls_access_violation_recovered != addr)
 			{
 				vm_log.notice("\n%s", dump_useful_thread_info());
 				vm_log.always()("[%s] Access violation %s location 0x%x (%s)", cpu->get_name(), is_writing ? "writing" : "reading", addr, (is_writing && vm::check_addr(addr)) ? "read-only memory" : "unmapped memory");
@@ -1714,10 +1742,10 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 
 	// Note: a thread may access violate more than once after hack_alloc recovery
 	// Do not log any further access violations in this case.
-	if (!g_tls_access_violation_recovered)
+	if (g_tls_access_violation_recovered != addr)
 	{
 		vm_log.notice("\n%s", dump_useful_thread_info());
-		vm_log.fatal("Access violation %s location 0x%x (%s)", is_writing ? "writing" : (cpu && cpu->get_class() == thread_class::ppu && cpu->get_pc() == addr ? "executing" : "reading"), addr, (is_writing && vm::check_addr(addr)) ? "read-only memory" : "unmapped memory");
+		vm_log.fatal("Access violation %s location 0x%x (%s)", is_writing ? "writing" : (is_exec ? "executing" : "reading"), addr, (is_writing && vm::check_addr(addr)) ? "read-only memory" : "unmapped memory");
 	}
 
 	while (Emu.IsPausedOrReady())
@@ -1766,8 +1794,13 @@ bool handle_access_violation(u32 addr, bool is_writing, ucontext_t* context) noe
 		}
 	}
 
-	if (Emu.IsStopped() && !hack_alloc())
+	if (Emu.IsStopped())
 	{
+		while (!hack_alloc())
+		{
+			thread_ctrl::wait_for(1000);
+		}
+
 		return false;
 	}
 
@@ -1806,6 +1839,7 @@ static LONG exception_handler(PEXCEPTION_POINTERS pExp) noexcept
 	if (pExp->ExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION && !is_executing)
 	{
 		u32 addr = 0;
+		bool is_exec = false;
 
 		if (auto [addr0, ok] = vm::try_get_addr(ptr); ok)
 		{
@@ -1813,14 +1847,21 @@ static LONG exception_handler(PEXCEPTION_POINTERS pExp) noexcept
 		}
 		else if (const usz exec64 = (ptr - vm::g_exec_addr) / 2; exec64 <= u32{umax})
 		{
+			is_exec = true;
 			addr = static_cast<u32>(exec64);
 		}
-		else
+		else if (const usz exec64 = (ptr - vm::g_exec_addr - vm::g_exec_addr_seg_offset); exec64 <= u32{umax})
 		{
+			is_exec = true;
+			addr = static_cast<u32>(exec64);
+		}
+		else 
+		{
+			std::this_thread::sleep_for(1ms);
 			return EXCEPTION_CONTINUE_SEARCH;
 		}
 
-		if (thread_ctrl::get_current() && handle_access_violation(addr, is_writing, pExp->ContextRecord))
+		if (thread_ctrl::get_current() && handle_access_violation(addr, is_writing, is_exec, pExp->ContextRecord))
 		{
 			return EXCEPTION_CONTINUE_EXECUTION;
 		}
@@ -2027,12 +2068,13 @@ static void signal_handler(int /*sig*/, siginfo_t* info, void* uct) noexcept
 #endif
 
 	const u64 exec64 = (reinterpret_cast<u64>(info->si_addr) - reinterpret_cast<u64>(vm::g_exec_addr)) / 2;
+	const u64 exec64_2 = (reinterpret_cast<u64>(info->si_addr) - reinterpret_cast<u64>(vm::g_exec_addr)) - vm::g_exec_addr_seg_offset;
 	const auto cause = is_executing ? "executing" : is_writing ? "writing" : "reading";
 
 	if (auto [addr, ok] = vm::try_get_addr(info->si_addr); ok && !is_executing)
 	{
 		// Try to process access violation
-		if (thread_ctrl::get_current() && handle_access_violation(addr, is_writing, context))
+		if (thread_ctrl::get_current() && handle_access_violation(addr, is_writing, false, context))
 		{
 			return;
 		}
@@ -2040,7 +2082,14 @@ static void signal_handler(int /*sig*/, siginfo_t* info, void* uct) noexcept
 
 	if (exec64 < 0x100000000ull && !is_executing)
 	{
-		if (thread_ctrl::get_current() && handle_access_violation(static_cast<u32>(exec64), is_writing, context))
+		if (thread_ctrl::get_current() && handle_access_violation(static_cast<u32>(exec64), is_writing, true, context))
+		{
+			return;
+		}
+	}
+	else if (exec64_2 < 0x100000000ull && !is_executing)
+	{
+		if (thread_ctrl::get_current() && handle_access_violation(static_cast<u32>(exec64_2), is_writing, true, context))
 		{
 			return;
 		}
@@ -2359,7 +2408,7 @@ thread_base::native_entry thread_base::finalize(u64 _self) noexcept
 	g_tls_fault_spu = 0;
 	g_tls_wait_time = 0;
 	g_tls_wait_fail = 0;
-	g_tls_access_violation_recovered = false;
+	g_tls_access_violation_recovered = umax;
 
 	g_tls_log_prefix = []() -> std::string { return {}; };
 

rpcs3/Emu/CPU/CPUThread.cpp

@@ -888,6 +888,14 @@ bool cpu_thread::check_state() noexcept
 				store = true;
 			}
 
+			if (flags & cpu_flag::req_exit)
+			{
+				// A request for the thread to quit has been made
+				flags -= cpu_flag::req_exit;
+				flags += cpu_flag::exit;
+				store = true;
+			}
+
 			// Can't process dbg_step if we only paused temporarily
 			if (cpu_can_stop && flags & cpu_flag::dbg_step)
 			{
@@ -1157,13 +1165,13 @@ void cpu_thread::notify()
 
 cpu_thread& cpu_thread::operator=(thread_state)
 {
-	if (state & cpu_flag::exit)
+	if (state & (cpu_flag::exit + cpu_flag::req_exit))
 	{
 		// Must be notified elsewhere or self-raised
 		return *this;
 	}
 
-	const auto old = state.fetch_add(cpu_flag::exit);
+	const auto old = state.fetch_add(cpu_flag::req_exit);
 
 	if (old & cpu_flag::wait && old.none_of(cpu_flag::again + cpu_flag::exit))
 	{

rpcs3/Emu/CPU/CPUThread.h

@@ -29,6 +29,7 @@ enum class cpu_flag : u32
 	yield, // Thread is being requested to yield its execution time if it's running
 	preempt, // Thread is being requested to preempt the execution of all CPU threads
 
+	req_exit, // Request the thread to exit
 	dbg_global_pause, // Emulation paused
 	dbg_pause, // Thread paused
 	dbg_step, // Thread forced to pause after one step (one instruction, etc)
@@ -39,7 +40,7 @@ enum class cpu_flag : u32
 // Test stopped state
 constexpr bool is_stopped(bs_t<cpu_flag> state)
 {
-	return !!(state & (cpu_flag::stop + cpu_flag::exit + cpu_flag::again));
+	return !!(state & (cpu_flag::stop + cpu_flag::exit + cpu_flag::again + cpu_flag::req_exit));
 }
 
 // Test paused state

rpcs3/Emu/Cell/Modules/cellGem.cpp

@@ -1774,6 +1774,12 @@ public:
 
 	shared_mutex mutex;
 
+	gem_tracker& operator=(thread_state) noexcept
+	{
+		wake_up_tracker();
+		return *this;
+	}
+
 private:
 	atomic_t<u32> m_wake_up_tracker = 0;
 	atomic_t<u32> m_tracker_done = 0;

rpcs3/Emu/System.cpp

@@ -49,6 +49,7 @@
 
 #include <memory>
 #include <regex>
+#include <shared_mutex>
 
 #include "Utilities/JIT.h"
 
@@ -3075,7 +3076,7 @@ void Emulator::GracefulShutdown(bool allow_autoexit, bool async_op, bool savesta
 		std::vector<stx::shared_ptr<named_thread<ppu_thread>>> ppu_thread_list;
 
 		// If EXITGAME signal is not read, force kill after a second.
-		constexpr int loop_timeout_ms = 50;
+		constexpr int loop_timeout_ms = 16;
 		int kill_timeout_ms = 1000;
 		int elapsed_ms = 0;
 
@@ -3092,8 +3093,10 @@ void Emulator::GracefulShutdown(bool allow_autoexit, bool async_op, bool savesta
 				Resume();
 			}, nullptr, true, read_counter);
 
+			std::shared_lock rlock(id_manager::g_mutex, std::defer_lock); 
+
 			// Check if the EXITGAME signal was read. We allow the game to terminate itself if that's the case.
-			if (!read_sysutil_signal && read_counter != get_sysutil_cb_manager_read_count())
+			if (!read_sysutil_signal && read_counter != get_sysutil_cb_manager_read_count() && rlock.try_lock())
 			{
 				sys_log.notice("The game received the exit request. Waiting for it to terminate itself...");
 				kill_timeout_ms += 5000; // Grant a couple more seconds
@@ -3103,7 +3106,12 @@ void Emulator::GracefulShutdown(bool allow_autoexit, bool async_op, bool savesta
 				idm::select<named_thread<ppu_thread>>([&](u32 id, cpu_thread&)
 				{
 					ppu_thread_list.emplace_back(idm::get_unlocked<named_thread<ppu_thread>>(id));
-				});
+				}, idm::unlocked);
+			}
+
+			if (rlock)
+			{
+				rlock.unlock();
 			}
 
 			if (static_cast<u64>(info) != m_stop_ctr || Emu.IsStopped())

rpcs3/rpcs3qt/debugger_frame.cpp

@@ -872,7 +872,7 @@ std::function<cpu_thread*()> debugger_frame::make_check_cpu(cpu_thread* cpu, boo
 {
 	constexpr cpu_thread* null_cpu = nullptr;
 
-	if (Emu.IsStopped())
+	if (Emu.IsStopped(true))
 	{
 		return []() { return null_cpu; };
 	}
@@ -921,7 +921,7 @@ std::function<cpu_thread*()> debugger_frame::make_check_cpu(cpu_thread* cpu, boo
 
 	return [cpu, type, shared = std::move(shared), emulation_id = Emu.GetEmulationIdentifier()]() mutable -> cpu_thread*
 	{
-		if (emulation_id != Emu.GetEmulationIdentifier() || Emu.IsStopped())
+		if (emulation_id != Emu.GetEmulationIdentifier() || Emu.IsStopped(true))
 		{
 			// Invalidate all data after Emu.Kill()
 			shared.reset();
@@ -1040,7 +1040,7 @@ void debugger_frame::UpdateUnitList()
 	const u64 emulation_id = static_cast<std::underlying_type_t<Emulator::stop_counter_t>>(Emu.GetEmulationIdentifier());
 	const u64 threads_created = cpu_thread::g_threads_created;
 	const u64 threads_deleted = cpu_thread::g_threads_deleted;
-	const system_state emu_state = Emu.GetStatus();
+	const system_state emu_state = Emu.GetStatus(false);
 
 	std::unique_lock<shared_mutex> lock{id_manager::g_mutex, std::defer_lock};
 
@@ -1077,6 +1077,11 @@ void debugger_frame::UpdateUnitList()
 	{
 		std::function<cpu_thread*()> func_cpu = make_check_cpu(std::addressof(cpu), true);
 
+		if (cpu.state & cpu_flag::exit)
+		{
+			return;
+		}
+
 		// Space at the end is to pad a gap on the right
 		cpu_list.emplace_back(QString::fromStdString((id >> 24 == 0x55 ? "RSX[0x55555555]" : cpu.get_name()) + ' '), std::move(func_cpu));
 

rpcs3/rpcs3qt/main_window.cpp

@@ -128,7 +128,7 @@ extern void qt_events_aware_op(int repeat_duration_ms, std::function<bool()> wra
 			}
 			else
 			{
-				QTimer::singleShot(repeat_duration_ms, *check_iteration);
+				QTimer::singleShot(repeat_duration_ms, event_loop, *check_iteration);
 			}
 		});
 
@@ -138,7 +138,7 @@ extern void qt_events_aware_op(int repeat_duration_ms, std::function<bool()> wra
 			event_loop = new QEventLoop();
 
 			// Queue event initially
-			QTimer::singleShot(0, *check_iteration);
+			QTimer::singleShot(0, event_loop, *check_iteration);
 
 			// Event loop
 			event_loop->exec();