Review: preserve registers in ExitCriticalSection

[Alexbeav]

Summary

Preserve the caller's registers in SYS(02h) ExitCriticalSection. The handler currently clears v0, although the BIOS contract gives this service no return value. A wrapper can return an earlier BIOS result through ExitCriticalSection; clearing that value turns success into a false failure.

Ghost in the Shell PAL SCES-01050 exposed this when movie startup entered cleanup while disc audio was already streaming. Removing the unwanted write lets the caller enter its video decode loop. The interrupt-enable update and host continuation behavior remain unchanged.

Reference: PSX-SPX SYS(02h).

Scope

• Upstream base: 17f49ad3b20dc30917a881a02baaa25374c13d18.
• Review head: 49d5f6233c330c39346fda008c82c59675c8cbc2.
• Two commits, four files: runtime/src/traps.c, runtime/tests/test_exit_critical_section.c, runtime/CMakeLists.txt, docs/accuracy/bios_exit_critical_section.md.
• Runtime-only correction; consumers need a framework update and rebuild, without game-code regeneration.
• No title-specific hook, game data, BIOS data, generated retail code, private paths or private tooling in the diff.

Validation

• GCC 16.1, Windows x64: the real-handler regression fails on clean upstream and passes with this patch. It covers 16 combinations of incoming v0 and status-register state and compares the full CPU state. It uses no BIOS or retail fixture.
• The standalone runtime and oracle targets build with no BIOS backend linked. All 64 enabled runtime tests pass; two existing tests remain disabled.
• The recompiler build passes. Its suite passes 58 of 61 enabled tests, with three existing tests disabled. All three failures reproduce on clean upstream source: the stale dirty_text_continuation_guards expectation, aot_overlay_discovery, and the Windows newline expectation in release_zip.
• The AOT test first hits a mixed MinGW compiler/DLL environment issue. Isolating the unchanged emitter beside matching DLLs and placing the fixture compiler first in PATH advances both base and candidate to the same existing check_interior_fragment_contract assertion. No AOT source is changed here.
• git diff --check and the accuracy-document link check pass.

The same runtime correction was tested in a local GITS PAL SCES-01050 build using retail SCPH5552: the opening, menu and New Game movie render in digital and analog routes; the digital route reaches the first mission and its normal death/menu return. A Silent Bomber PAL SLES-02792 frontend control passes before and after. The operator accepted the corrected GITS build. These retail observations belong to that tested local integration; they are not a new gameplay run on this upstream-base build.

The real-handler test runs on its verified Windows x64 MinGW GCC 16.1.0 toolchain. Other configurations register an explicit CTest skip, with the compiler and reason. Clang/MSVC register-preservation coverage and full-campaign compatibility are not claimed.

Review corrections

• Restricted the LTO-dependent link recipe to the documented, verified toolchain instead of assuming that every GCC variant will prune the unrelated syscall paths.
• Registered a visible CTest skip on unsupported compilers/platforms instead of silently omitting the test.
• Reconfigured and reran the real test on GCC 16.1.0: PASS, all 16 combinations.
• Configured Clang 22.1.8: CTest lists the test and reports Skipped with the reason. This validates the coverage report, not the register contract on Clang.
• The runtime correction and synthetic fixture are unchanged by these review fixes.

Review request

Please review the SYS(02h) register contract, host continuation behavior, and the real-handler regression. This fork PR is the review step before submitting the exact reviewed commit upstream.

Developed with AI assistance; validated as described (test evidence in PR body). AI writes the code and the PR, but I always test before I send something up. Happy to iterate on this process with your feedback.

[Alexbeav]

@cubic-dev-ai review this PR

[cubic-dev-ai]

@cubic-dev-ai review this PR

@Alexbeav I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

All reported issues were addressed across 4 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 4 files

<sub>Requires human review: Auto-approval blocked by 1 unresolved issue from a previous review of this commit.

Re-trigger cubic</sub>

[Alexbeav]

Both compiler-coverage comments are addressed in 49d5f6233c330c39346fda008c82c59675c8cbc2.

I chose the documented, verified-toolchain gate requested in the second comment: Windows x64 MinGW GCC 16.1.0. The test still calls the real psx_syscall implementation; it provides no fake scheduler/exception behavior. Its link recipe relies on LTO pruning, and the accuracy note now states that dependency explicitly.

Every other configuration registers the same CTest name with SKIP_REGULAR_EXPRESSION, so the missing coverage is visible. I tested both sides: GCC 16.1.0 builds and passes all 16 state combinations; an actual Clang 22.1.8 configuration registers the test and reports Skipped with the compiler and reason. No Clang/MSVC correctness coverage is claimed.

The runtime fix and the source-owned fixture are unchanged. Please re-review the updated head.

[Alexbeav]

Merged upstream as RetroPortingToolKit#322 (Alexbeav:codex/fix-exit-critical-section, merged 2026-09-05T17:16:15Z). Closing this internal review PR as complete; the branch is preserved.