From 47614daa67deeaa6857dcff8213b7eca669d4a06 Mon Sep 17 00:00:00 2001 From: Brian Degenhardt Date: Sat, 25 Jul 2026 16:06:14 -0700 Subject: [PATCH] IOP: scan past non-overlapping neighbours when hunting straddlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit psxRecClearMem walks down through recBlocks from the block containing the written word to pick up straddlers — blocks that start below it but whose bodies reach into the range being cleared. recBlocks is ordered by startpc, not by end address, so stopping at the first block that ends before the write is unsound: a longer block lower down can jump clean over a short one and still cover the write, and the scan quits before it is ever examined. The straddler is then neither removed from recBlocks nor LUT-reset, so it keeps executing stale code. This is the IOP half of the same blind spot 3ac41e05bd fixed on the EE; both stops came from 801d71f7f0 (2009), which added them to iR3000A and iR5900 in one commit and is still upstream in both. Scan past a non-overlapping neighbour instead of stopping on it, bounded by s_maxBlockBytes — the longest guest extent compiled since the last reset, so the walk still terminates as soon as nothing below could reach. The scan cursor is split from the removal start, so a skipped survivor never becomes the base of the removal range. Unlike the EE, IOP blocks have no 4KB page break (psxRecRecompile ends them only at a branch or an already-compiled head), so the mark has no structural cap beyond the 0xffff-instruction assert. IOP blocks are short in practice; the cost is flat-array reads either way. New gate ClearScansPastNonOverlappingNeighborToStraddlerBelow builds the geometry the bug needs — A long, H compiled mid-A, then B truncated against H's compiled head, which is the only way to get a block that both starts inside A and ends before A does. It pokes a word inside A past B's end and requires A to run the new instruction. Red before this change with the JIT returning the stale 0x11 against the interpreter's 0x22, green after. recompiler_tests 1439/1439. --- pcsx2/arm64/iR3000A-arm64.cpp | 33 +++++++- .../ctest/core/recompilers/iop_smc_tests.cpp | 82 +++++++++++++++++++ 2 files changed, 112 insertions(+), 3 deletions(-) diff --git a/pcsx2/arm64/iR3000A-arm64.cpp b/pcsx2/arm64/iR3000A-arm64.cpp index c24717f758..63fd6c8cfd 100644 --- a/pcsx2/arm64/iR3000A-arm64.cpp +++ b/pcsx2/arm64/iR3000A-arm64.cpp @@ -56,6 +56,10 @@ static u32 s_nEndBlock = 0; static u32 s_branchTo; static bool s_nBlockFF; +// Longest guest extent, in bytes, of any block compiled since the last reset. +// Bounds how far back the straddler walk in psxRecClearMem must scan. +static u32 s_maxBlockBytes = 0; + static u32 s_saveConstRegs[32]; static u32 s_saveHasConstReg = 0, s_saveFlushedConstReg = 0; static EEINST* s_psaveInstInfo = nullptr; @@ -1193,13 +1197,31 @@ static __fi u32 psxRecClearMem(u32 pc) u32 lowerextent = pc, upperextent = pc + 4; - while (BASEBLOCKEX* pexblock = recBlocks[blockidx - 1]) + // Walk down for straddlers: blocks starting below the merged range whose + // bodies reach into it. `scanidx` is the cursor; `blockidx` only follows it + // down onto blocks we actually absorb, so a skipped survivor never becomes + // the start of the removal range below. + int scanidx = blockidx - 1; + while (BASEBLOCKEX* pexblock = recBlocks[scanidx]) { if (pexblock->startpc + pexblock->size * 4 <= HWADDR(lowerextent)) - break; + { + // Not overlapping — but recBlocks is ordered by startpc, NOT by end + // address, so a block further down can still be long enough to reach + // the merged range; this one is no proof the rest miss too. Skip it + // and carry on until even the longest block compiled since the last + // reset could not span the gap. (Upstream x86 breaks here — + // iR3000A.cpp — and silently leaves the straddler below compiled.) + if (pexblock->startpc + s_maxBlockBytes <= HWADDR(lowerextent)) + break; + + scanidx--; + continue; + } lowerextent = std::min(lowerextent, pexblock->startpc); - blockidx--; + blockidx = scanidx; + scanidx--; } while (BASEBLOCKEX* pexblock = recBlocks[blockidx]) @@ -1371,6 +1393,7 @@ void recResetIOP() recBlocks.Reset(); memset(g_iopCodeCov, 0, sizeof(g_iopCodeCov)); g_psxMaxRecMem = 0; + s_maxBlockBytes = 0; psxbranch = 0; } @@ -1601,6 +1624,10 @@ StartRecomp: s_pCurBlockEx->size = (psxpc - startpc) >> 2; iopCovAdjust(s_pCurBlockEx->startpc, s_pCurBlockEx->size * 4, +1); + // High-water mark of any compiled block's guest extent, for the straddler + // walk's scan-back bound in psxRecClearMem. Reset with the block array. + s_maxBlockBytes = std::max(s_maxBlockBytes, psxpc - startpc); + if (!(psxpc & 0x10000000)) g_psxMaxRecMem = std::max((psxpc & ~0xa0000000), g_psxMaxRecMem); diff --git a/tests/ctest/core/recompilers/iop_smc_tests.cpp b/tests/ctest/core/recompilers/iop_smc_tests.cpp index 03097c997a..c7b32bf7dd 100644 --- a/tests/ctest/core/recompilers/iop_smc_tests.cpp +++ b/tests/ctest/core/recompilers/iop_smc_tests.cpp @@ -378,3 +378,85 @@ TEST(IopSmc, OverwriteLastWordOfBlockBeforeTerminator) EXPECT_EQ(h.GetGprInterp(reg::v1), 30u); EXPECT_EQ(h.GetGprInterp(reg::v0), 0xDEADu); } + +TEST(IopSmc, ClearScansPastNonOverlappingNeighborToStraddlerBelow) +{ + // psxRecClearMem walks DOWN from the block containing the written word to + // pick up straddlers that start below it. recBlocks is ordered by startpc, + // NOT by end address, so stopping at the first block that ends before the + // write is unsound: a longer block lower down can jump clean over a short + // one and still cover the write. The scan quits before it is ever examined, + // the straddler is neither removed nor LUT-reset, and it keeps executing + // stale code. (Upstream x86 carries the same stop — iR3000A.cpp — and it + // is the IOP half of the same 2009 commit, 801d71f7f0, that put the + // matching stop in the EE.) + // + // Geometry, built by compile order (block ends are set by the boundary + // scan in psxRecRecompile): + // + // A = [S, S+0x48) long — S to the `jr ra` terminator + // B = [S+0x10, S+0x20) short — truncated against H's compiled head + // H = [S+0x20, S+0x48) + // + // A must compile FIRST (nothing above it to truncate against), then H, + // then B — B's scan stops at H because H's LUT slot no longer holds + // iopJITCompile. That is the only way to get a block that both starts + // inside A and ends before A does. + // + // The write lands at S+0x30: inside A, inside H, and past B's end. The + // downward walk starts at H, sees B ending at S+0x20 <= S+0x30, and stops + // — never reaching A. + JitTestHarness h; + constexpr u32 kS = kProgramPc; + constexpr u32 kU = kProgramPc + 0x10; // B's entry + constexpr u32 kH = kProgramPc + 0x20; // H's entry + constexpr u32 kP = kProgramPc + 0x30; // the word we overwrite + + // Straight-line body — no branches before the terminator, or A would end + // early and the geometry collapses. + h.LoadProgramAt(kS, { + ADDIU(reg::v0, reg::zero, 1), // S+0x00 A entry + NOP, NOP, NOP, + ADDIU(reg::a0, reg::zero, 4), // S+0x10 B entry + NOP, NOP, NOP, + ADDIU(reg::a1, reg::zero, 8), // S+0x20 H entry + NOP, NOP, NOP, + ADDIU(reg::v1, reg::zero, 0x11),// S+0x30 overwritten below + NOP, NOP, NOP, + JR(reg::ra), // S+0x40 + NOP, // S+0x44 delay slot; A ends at S+0x48 + }, /*append_jr_ra_term=*/false); + + // 1) A — enters at kS, runs to the terminator. + h.Run(); + ASSERT_EQ(h.GetGprInterp(reg::v0), 1u); + ASSERT_EQ(h.GetGprInterp(reg::v1), 0x11u); + + // 2) H — mid-A, so its LUT slot is still iopJITCompile and it compiles + // as its own block running to the same terminator. + h.SetPc(kH); + h.SetRa(kParkingPc); + h.RunResume(); + ASSERT_EQ(h.GetGprInterp(reg::a1), 8u); + + // 3) B — its scan now hits H's compiled head and truncates there, so B + // ends at S+0x20 and falls through into H. + h.SetPc(kU); + h.SetRa(kParkingPc); + h.RunResume(); + ASSERT_EQ(h.GetGprInterp(reg::a0), 4u); + + // The SMC write: inside A, past B's end. + iopMemWrite32(kP, ADDIU(reg::v1, reg::zero, 0x22)); + + // Re-enter at A. If the walk stopped at B, A is still cached with the old + // 0x11 baked in — the JIT returns 0x11 while the interpreter reads the + // patched word and returns 0x22, so DiffJitVsInterp fires as well. + h.SetGpr(reg::v1, 0xDEAD); + h.SetPc(kS); + h.SetRa(kParkingPc); + h.RunResume(); + EXPECT_EQ(h.GetGprJit(reg::v1), 0x22u) << "block A executed stale code — " + "the downward walk in psxRecClearMem stopped at the short block B"; + EXPECT_EQ(h.GetGprInterp(reg::v1), 0x22u); +}