From 5ac94197031d11545c4b68bfe4c0d3fe3da842d6 Mon Sep 17 00:00:00 2001
From: Connor McLaughlin <stenzek@gmail.com>
Date: Mon, 4 Apr 2022 22:19:39 +1000
Subject: [PATCH] R5900: Make CPU exits consistent and safe

Previously, we would either throw an exception (ints), or longjmp out of
the recompiler when the execution state was checked. Unfortunately for
our stability, this happened at the end of the frame, just before it was
pushed to the GS, and in the middle of processing EE events (!).

Doing so not only meant that we executed a bunch of event
testing/exception code twice (once after we paused, again when we
resumed), but it also could potentially leave things in an inconsistent
state.

So instead, let's do it safely with a flag, replacing the old
iopBreakpoint flag, so there's no additional overhead on the hot path.
---
 pcsx2/Interpreter.cpp           | 23 +++++++++++++++++++----
 pcsx2/x86/ix86-32/iR5900-32.cpp | 28 +++++++++++++++++-----------
 2 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/pcsx2/Interpreter.cpp b/pcsx2/Interpreter.cpp
index 1b14ebb1b3..540f45f987 100644
--- a/pcsx2/Interpreter.cpp
+++ b/pcsx2/Interpreter.cpp
@@ -38,6 +38,7 @@ extern int vu0branch, vu1branch;
 static int branch2 = 0;
 static u32 cpuBlockCycles = 0;		// 3 bit fixed point version of cycle count
 static std::string disOut;
+static bool intExitExecution = false;
 
 static void intEventTest();
 
@@ -498,6 +499,12 @@ static void intEventTest()
 {
 	// Perform counters, ints, and IOP updates:
 	_cpuEventTest_Shared();
+	
+	if (intExitExecution)
+	{
+		intExitExecution = false;
+		throw Exception::ExitCpuExecute();
+	}
 }
 
 static void intExecute()
@@ -577,12 +584,20 @@ static void intExecute()
 static void intCheckExecutionState()
 {
 #ifndef PCSX2_CORE
-	if( GetCoreThread().HasPendingStateChangeRequest() )
-		throw Exception::ExitCpuExecute();
+	const bool interrupted = GetCoreThread().HasPendingStateChangeRequest();
 #else
-	if (VMManager::Internal::IsExecutionInterrupted())
-		throw Exception::ExitCpuExecute();
+	const bool interrupted = VMManager::Internal::IsExecutionInterrupted();
 #endif
+
+	if (interrupted)
+	{
+		// If we're currently processing events, we can't safely jump out of the interpreter here, because we'll
+		// leave things in an inconsistent state. So instead, we flag it for exiting once cpuEventTest() returns.
+		if (eeEventTestIsActive)
+			intExitExecution = true;
+		else
+			throw Exception::ExitCpuExecute();
+	}
 }
 
 static void intStep()
diff --git a/pcsx2/x86/ix86-32/iR5900-32.cpp b/pcsx2/x86/ix86-32/iR5900-32.cpp
index 871e1c30f4..d42d8ef3b2 100644
--- a/pcsx2/x86/ix86-32/iR5900-32.cpp
+++ b/pcsx2/x86/ix86-32/iR5900-32.cpp
@@ -48,6 +48,13 @@
 using namespace x86Emitter;
 using namespace R5900;
 
+static std::atomic<bool> eeRecIsReset(false);
+static std::atomic<bool> eeRecNeedsReset(false);
+static bool eeCpuExecuting = false;
+static bool eeRecExitRequested = false;
+static bool g_resetEeScalingStats = false;
+static int g_patchesNeedRedo = 0;
+
 #define PC_GETBLOCK(x) PC_GETBLOCK_(x, recLUT)
 
 u32 maxrecmem = 0;
@@ -368,9 +375,9 @@ static void recEventTest()
 {
 	_cpuEventTest_Shared();
 
-	if (iopBreakpoint)
+	if (eeRecExitRequested)
 	{
-		iopBreakpoint = false;
+		eeRecExitRequested = false;
 		recExitExecution();
 	}
 }
@@ -619,12 +626,6 @@ static void recAlloc()
 alignas(16) static u16 manual_page[Ps2MemSize::MainRam >> 12];
 alignas(16) static u8 manual_counter[Ps2MemSize::MainRam >> 12];
 
-static std::atomic<bool> eeRecIsReset(false);
-static std::atomic<bool> eeRecNeedsReset(false);
-static bool eeCpuExecuting = false;
-static bool g_resetEeScalingStats = false;
-static int g_patchesNeedRedo = 0;
-
 ////////////////////////////////////////////////////
 static void recResetRaw()
 {
@@ -713,12 +714,17 @@ static void recExitExecution()
 static void recCheckExecutionState()
 {
 #ifndef PCSX2_CORE
-	if (m_cpuException || m_Exception || eeRecNeedsReset || GetCoreThread().HasPendingStateChangeRequest())
+	if (m_cpuException || m_Exception || eeRecNeedsReset || iopBreakpoint || GetCoreThread().HasPendingStateChangeRequest())
 #else
-	if (m_cpuException || m_Exception || eeRecNeedsReset || VMManager::Internal::IsExecutionInterrupted())
+	if (m_cpuException || m_Exception || eeRecNeedsReset || iopBreakpoint || VMManager::Internal::IsExecutionInterrupted())
 #endif
 	{
-		recExitExecution();
+		// If we're currently processing events, we can't safely jump out of the recompiler here, because we'll
+		// leave things in an inconsistent state. So instead, we flag it for exiting once cpuEventTest() returns.
+		if (eeEventTestIsActive)
+			eeRecExitRequested = true;
+		else
+			recExitExecution();
 	}
 }