Review: preserve registers in ExitCriticalSection

docs/accuracy/bios_exit_critical_section.md

@@ -0,0 +1,30 @@
+# ExitCriticalSection register contract
+
+`psx_syscall` handles BIOS SYS(02h) directly, including when optional BIOS HLE
+services are disabled. It enables the current interrupt bit and the hardware
+interrupt mask in COP0 SR. It preserves the caller's general registers.
+In particular, it must not assign a return value to `v0`.
+
+An SDK wrapper can call another BIOS service, then ExitCriticalSection, and
+return the earlier result. Clearing `v0` turns a successful operation into a
+false failure. This can prevent a caller from entering its video decode loop
+even though disc streaming and audio have already started.
+
+The hardware contract is documented in
+[PSX-SPX, SYS(02h)](https://psx-spx.consoledev.net/kernelbios/#sys02h-exitcriticalsection-syscall-with-r402h).
+The specification allows K0 to change; this direct handler preserves it too.
+The host-only `cpu->pc = 0` continuation and C return value remain unchanged.
+
+`exit_critical_section_test` compiles the real `traps.c` implementation with
+LTO and tests 16 combinations of incoming `v0` and SR. It checks the full CPU
+state, allowing only the documented SR update and host continuation change.
+The test uses no BIOS, generated retail code, or game data.
+
+The fixture links only the real trap unit and its test driver. It relies on
+interprocedural optimization removing syscall paths that the driver never
+calls, rather than providing fake scheduler or exception implementations.
+This link contract is verified with Windows x64 MinGW GCC 16.1.0 and is enabled
+only for that toolchain. Other configurations still register the test, but
+CTest reports it as **Skipped** with the unsupported compiler and reason.
+They do not provide register-preservation coverage until the fixture's link
+contract is validated there or a portable real-dependency harness replaces it.

runtime/CMakeLists.txt

@@ -183,6 +183,27 @@ if(BUILD_TESTING)
     add_test(NAME mod_gpu_dma_aperture_test
              COMMAND mod_gpu_dma_aperture_test)
 
+    # The real-handler fixture relies on LTO pruning its unused syscall paths.
+    # Enable only the toolchain on which that link contract is verified; keep
+    # the coverage gap visible in CTest for every other compiler/platform.
+    if(WIN32 AND MINGW AND CMAKE_SIZEOF_VOID_P EQUAL 8
+       AND CMAKE_C_COMPILER_ID STREQUAL "GNU"
+       AND CMAKE_C_COMPILER_VERSION VERSION_EQUAL "16.1.0")
+        add_executable(exit_critical_section_test
+            tests/test_exit_critical_section.c src/traps.c)
+        target_include_directories(exit_critical_section_test PRIVATE include)
+        target_compile_definitions(exit_critical_section_test PRIVATE PSX_NO_DEBUG_TOOLS=1)
+        target_compile_options(exit_critical_section_test PRIVATE -O3 -flto -ffunction-sections)
+        target_link_options(exit_critical_section_test PRIVATE -flto -Wl,--gc-sections)
+        add_test(NAME exit_critical_section_test COMMAND exit_critical_section_test)
+    else()
+        add_test(NAME exit_critical_section_test
+            COMMAND "${CMAKE_COMMAND}" -E echo
+                "SKIP: real-handler LTO fixture is verified only on Windows x64 MinGW GCC 16.1.0; current compiler is ${CMAKE_C_COMPILER_ID} ${CMAKE_C_COMPILER_VERSION}")
+        set_tests_properties(exit_critical_section_test PROPERTIES
+            SKIP_REGULAR_EXPRESSION "^SKIP:")
+    endif()
+
     add_executable(dma_gpu_linked_list_timing_test
         tests/test_dma_gpu_linked_list_timing.c
         src/dma_gpu_ll.c)

runtime/src/traps.c

@@ -924,7 +924,7 @@ int psx_syscall(CPUState* cpu, uint32_t code) {
     /*
      * PS1 BIOS SYSCALL convention:
      *   $a0 = 1: EnterCriticalSection — disable interrupts, return old SR
-     *   $a0 = 2: ExitCriticalSection  — enable interrupts, return old SR
+     *   $a0 = 2: ExitCriticalSection  — enable interrupts, preserve GPRs
      *   $a0 = 3: ReturnFromException  — restore full TCB state + RFE
      *
      * Syscalls 1 and 2 are always handled directly — they only touch IEc
@@ -953,7 +953,9 @@ int psx_syscall(CPUState* cpu, uint32_t code) {
 
         case 2: /* ExitCriticalSection: enable interrupts */
             cpu->cop0[12] = sr | 0x0401u; /* set IEc (bit 0) + IM[2] (bit 10) */
-            cpu->gpr[2] = 0;
+            /* SYS(02h) has no return value: it preserves the caller's v0.
+             * SDK wrappers can return an earlier BIOS result through this
+             * call, so assigning zero here changes their guest contract. */
             g_pc0_reason = PSX_PC0_CRIT_SECTION;
             cpu->pc = 0;
             return 0;

runtime/tests/test_exit_critical_section.c

@@ -0,0 +1,38 @@
+/* Exercise the real psx_syscall case, with no BIOS or retail fixture.
+ * LTO removes the unused scheduler/exception cases so this focused executable
+ * does not need to stub them. The syscall selector remains a constant 2. */
+#include "cpu_state.h"
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+
+int psx_syscall(CPUState *cpu, uint32_t code);
+
+int main(void) {
+    const uint32_t results[] = {0u, 1u, 0x80010000u, UINT32_MAX};
+    const uint32_t statuses[] = {0u, 1u, 0x40000400u, 0x40000401u};
+    for (unsigned r = 0; r < sizeof(results) / sizeof(results[0]); ++r) {
+        for (unsigned s = 0; s < sizeof(statuses) / sizeof(statuses[0]); ++s) {
+            CPUState cpu = {0};
+            for (unsigned i = 1; i < 32; ++i)
+                cpu.gpr[i] = 0x98760000u + i;
+            cpu.gpr[2] = results[r];
+            cpu.gpr[4] = 2u;
+            cpu.pc = 0x80012340u;
+            cpu.hi = 0x12345678u;
+            cpu.lo = 0xFEDCBA98u;
+            cpu.cop0[12] = statuses[s];
+            CPUState expected = cpu;
+            expected.cop0[12] |= 0x401u;
+            expected.pc = 0u; /* host continuation: resume after SYSCALL */
+            if (psx_syscall(&cpu, 0u) != 0 ||
+                memcmp(&cpu, &expected, sizeof(cpu)) != 0) {
+                fprintf(stderr, "SYS(02h) changed preserved state: v0=%08X SR=%08X\n",
+                        results[r], statuses[s]);
+                return 1;
+            }
+        }
+    }
+    puts("exit_critical_section_test: PASS (16 state combinations)");
+    return 0;
+}