From e2ab84087ea3e1aba14bdd964c018791e2e08f15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Mon, 31 Aug 2026 12:27:24 +0200 Subject: [PATCH] Fix five ways to end up stuck, crashed, or silently degraded System.cpp stamped BootState::Complete unconditionally after InitGPU(), overwriting the Failed that InitGPU sets when GPU_Init() fails - after it has already run CPU_Shutdown(). PSP_InitUpdate then took the success path on a core that no longer existed, down to a null Memory::base, and the first guest access dereferenced it. InitGPU now reports failure and both callers honor it. (The libretro path had the same problem from the other direction: it calls InitGPU after the Failed check.) HandleAssert called g_assertCancelCallback directly on the IDCANCEL path, without the null check its own BreakIntoPSPDebugger() helper does - and EmuScreen clears the callback when a game is unloaded. So any assert after returning to the menu turned "Cancel: skip and break into PPSSPP debugger" into a null jump, from the one button whose entire purpose is surviving the assert. __CheatDoState registered the cheat event type when the savestate had no CwCheat section, but never scheduled it. CoreTiming::DoState has already swapped in the state's event queue by then, which doesn't contain one either - so loading an old savestate silently killed cheats, and the enable/disable polling with them, for the rest of the session. Achievements::ChangeUMD set g_isIdentifying and returned without clearing it when hashing failed, leaving IsBlockingExecution() true forever - EmuScreen stops running the CPU and the game is frozen until restart. Reachable from a disc swap on any ISO whose PARAM.SFO or EBOOT.BIN can't be read. x64Analyzer routed opcode 0x88 into the write path but had no case for it, so it hit the default, logged from inside the crash handler, and failed. 0x88 is exactly what the x64 JIT emits for a guest sb, so MemFault could never skip or ignore a bad byte store the way it can a word one. Handle the 8-bit forms, and drop the 0x8a/0x8b cases in the read path that the same 0xF0 mask made unreachable. Covered by a new CheckAnalyze case, which fails without this change. 314 pspautotests pass, all unit tests pass. --- Common/Log.cpp | 5 ++++- Common/x64Analyzer.cpp | 25 +++++++++++++++---------- Common/x64Analyzer.h | 2 ++ Core/CwCheat.cpp | 6 ++++++ Core/RetroAchievements.cpp | 3 +++ Core/System.cpp | 21 ++++++++++++++++++--- unittest/TestX64Emitter.cpp | 10 ++++++++++ 7 files changed, 58 insertions(+), 14 deletions(-) diff --git a/Common/Log.cpp b/Common/Log.cpp index 3d589e6ea5..183a1721f7 100644 --- a/Common/Log.cpp +++ b/Common/Log.cpp @@ -148,7 +148,10 @@ bool HandleAssert(bool isDebugAssert, const char *function, const char *file, in } return false; // Break into the native debugger. case IDCANCEL: - g_assertCancelCallback(formatted, g_assertCancelCallbackUserData); + // Via the helper, which null-checks. EmuScreen clears the callback when a game is + // unloaded, so an assert after returning to the menu was jumping through null - from + // the one button whose whole purpose is surviving the assert. + BreakIntoPSPDebugger(formatted); return true; // don't crash! } } diff --git a/Common/x64Analyzer.cpp b/Common/x64Analyzer.cpp index 06689f86ec..c19e989b1c 100644 --- a/Common/x64Analyzer.cpp +++ b/Common/x64Analyzer.cpp @@ -195,6 +195,20 @@ bool X86AnalyzeMOV(const unsigned char *codePtr, LSInstructionInfo &info) info.isMemoryWrite = false; break; + // The 8-bit forms land here too - (codeByte & 0xF0) == 0x80 covers 0x88..0x8B - and + // without them a guest sb, which the x64 JIT emits as 0x88, hit the default below and + // failed to decode. That left MemFault unable to skip or ignore a bad byte store the way + // it can a word one. + case MOVE_REG_TO_MEM8: + info.isMemoryWrite = true; + info.operandSizeInBytes = 1; + break; + + case MOVE_MEM_TO_REG8: + info.isMemoryWrite = false; + info.operandSizeInBytes = 1; + break; + default: ERROR_LOG(Log::CPU, "Unhandled disasm case in write handler!\n\nPlease implement or avoid."); return false; @@ -249,16 +263,7 @@ bool X86AnalyzeMOV(const unsigned char *codePtr, LSInstructionInfo &info) return false; } break; - case 0x8a: - if (info.operandSizeInBytes == 4) - { - info.operandSizeInBytes = 1; - break; - } - else - return false; - case 0x8b: - break; //it's OK don't need to do anything + // NOTE: 0x88..0x8B never reach here - they're all accessType 1, handled above. default: return false; } diff --git a/Common/x64Analyzer.h b/Common/x64Analyzer.h index 4bcdb7bd78..3db1f056f8 100644 --- a/Common/x64Analyzer.h +++ b/Common/x64Analyzer.h @@ -62,7 +62,9 @@ enum { MOVSX_SHORT = 0xBF, //movsx on short MOVE_8BIT = 0xC6, //move 8-bit immediate MOVE_16_32BIT = 0xC7, //move 16 or 32-bit immediate + MOVE_REG_TO_MEM8 = 0x88, //move 8-bit reg to memory MOVE_REG_TO_MEM = 0x89, //move reg to memory + MOVE_MEM_TO_REG8 = 0x8A, //move 8-bit memory to reg MOVE_MEM_TO_REG = 0x8B, //move memory to reg // These two opcodes are shared between MOVUPS (no mandatory prefix) and MOVSS (mandatory 0xF3 prefix). MOVUPS_MOVSS_FROM_RM = 0x10, //movups/movss xmm, xmm/m diff --git a/Core/CwCheat.cpp b/Core/CwCheat.cpp index e96efc8bf5..2b02f85bd8 100644 --- a/Core/CwCheat.cpp +++ b/Core/CwCheat.cpp @@ -266,8 +266,14 @@ void __CheatShutdown() { void __CheatDoState(PointerWrap &p) { auto s = p.Section("CwCheat", 0, 2); if (!s) { + // A savestate from before this section existed. Registering the event type isn't enough - + // CoreTiming::DoState has already replaced the queue with the state's, which has no cheat + // event in it, so without scheduling one here hleCheat never runs again and cheats (plus + // the enable/disable polling) stay dead for the rest of the session. CheatEvent = -1; CoreTiming::RestoreRegisterEvent(CheatEvent, "CheatEvent", &hleCheat); + CoreTiming::RemoveEvent(CheatEvent); + CoreTiming::ScheduleEvent(msToCycles(GetRefreshMs()), CheatEvent, 0); return; } diff --git a/Core/RetroAchievements.cpp b/Core/RetroAchievements.cpp index a9567c4c0c..a85f2fc2bf 100644 --- a/Core/RetroAchievements.cpp +++ b/Core/RetroAchievements.cpp @@ -1178,6 +1178,9 @@ void ChangeUMD(const Path &path, FileLoader *fileLoader) { s_game_hash = ComputePSPISOHash(blockDevice); if (s_game_hash.empty()) { ERROR_LOG(Log::Achievements, "Failed to hash - can't identify"); + // Leaving this set makes IsBlockingExecution() true forever, so EmuScreen stops running + // the CPU and the game is frozen until restart. SetGame's equivalent path clears it too. + g_isIdentifying = false; return; } diff --git a/Core/System.cpp b/Core/System.cpp index 6a97027acb..36d64ddb0d 100644 --- a/Core/System.cpp +++ b/Core/System.cpp @@ -664,7 +664,9 @@ void PSP_ForceDebugStats(bool enable) { _assert_(g_coreCollectDebugStatsCounter >= 0); } -static void InitGPU(std::string *error_string) { +// Returns false if the GPU couldn't be brought up - in which case it has already set +// BootState::Failed and torn the CPU back down, so the caller must not carry on. +static bool InitGPU(std::string *error_string) { if (!gpu) { // should be! INFO_LOG(Log::Loader, "Starting graphics..."); Draw::DrawContext *draw = g_CoreParameter.graphicsContext ? g_CoreParameter.graphicsContext->GetDrawContext() : nullptr; @@ -675,8 +677,10 @@ static void InitGPU(std::string *error_string) { *error_string = "Unable to initialize rendering engine."; CPU_Shutdown(false); g_bootState = BootState::Failed; + return false; } } + return true; } bool PSP_InitStart(const CoreParameter &coreParam) { @@ -752,7 +756,12 @@ bool PSP_InitStart(const CoreParameter &coreParam) { // Initialize the GPU as far as we can here (do things like load cache files). _dbg_assert_(!gpu); #ifndef __LIBRETRO__ - InitGPU(errorString); + // Must not stamp Complete over the Failed that InitGPU sets - it has already run + // CPU_Shutdown(), so PSP_InitUpdate would take the success path on a core that no longer + // exists, right down to a null Memory::base. + if (!InitGPU(errorString)) { + return; + } #endif g_bootState = BootState::Complete; }); @@ -784,7 +793,13 @@ BootState PSP_InitUpdate(std::string *error_string) { } #ifdef __LIBRETRO__ - InitGPU(error_string); + if (!InitGPU(error_string)) { + // Same as the Failed branch above - the core is already gone. + Core_NotifyLifecycle(CoreLifecycle::START_COMPLETE); + *error_string = g_CoreParameter.errorString; + g_bootState = BootState::Off; + return BootState::Failed; + } #endif // Ok, async part of the boot completed, let's finish up things on the main thread. diff --git a/unittest/TestX64Emitter.cpp b/unittest/TestX64Emitter.cpp index a846461deb..6230742a8e 100644 --- a/unittest/TestX64Emitter.cpp +++ b/unittest/TestX64Emitter.cpp @@ -75,6 +75,16 @@ bool TestX64Emitter() { emitter.MOV(32, MDisp(RCX, 4), R(EAX)); RET(CheckAnalyze(emitter, true, InstructionClass::GPR, 4)); + // The 8-bit forms, which the JIT emits for a guest sb/lb. These used to fail to decode + // entirely, so MemFault couldn't skip or ignore a bad byte access the way it can a word one. + prevStart = emitter.GetCodePointer(); + emitter.MOV(8, MDisp(RCX, 4), R(AL)); + RET(CheckAnalyze(emitter, true, InstructionClass::GPR, 1)); + + prevStart = emitter.GetCodePointer(); + emitter.MOV(8, R(AL), MDisp(RCX, 4)); + RET(CheckAnalyze(emitter, false, InstructionClass::GPR, 1)); + prevStart = emitter.GetCodePointer(); emitter.MOVZX(32, 8, EAX, MDisp(RCX, 4)); RET(CheckAnalyze(emitter, false, InstructionClass::GPR, 1, true, false));