[review] mstan #270 - cosim: key checkpoints on retired-instruction count, not guest cycles

[Alexbeav]

Purpose

This is a review-only retrofit for the already-open upstream PR mstan/psxrecomp#270.

Do not merge this PR. The AI reviewer must inspect this exact diff before the next upstream update.

Identity

• Upstream base: 47bda8172e43fdba1879c1a5885904a265c567f6
• Review head: 4aebb349da131e74bab1007e91aab08ab5615055
• Upstream branch: pr/cosim-icount-checkpoints

Review gate

Classify each finding as valid, invalid, or uncertain. A valid finding requires a correction, a regression, and repeated validation. An uncertain finding blocks the next upstream update.

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.

---

Summary by cubic

Cosim checkpoints now key on retired instruction count instead of guest cycles, so both instances always hash the same architectural position and cycle-skew phantom divergences are gone.

• Branch+delay-slot retires as one event in both backends, so checkpoint k is state after k*stride retirements in both instances by construction.
• The guest-cycle clock stays in the state hash, so real cycle-accounting drift is still detected, now pinned to an exact instruction instead of corrupting park alignment.
• cosim.py stride/--max/--start-cycle now count retired instructions; cycle skew at a checkpoint is reported as a real timing divergence.
• launch() gives instance B its own exe (the mods root is exe-dir-relative) and refuses a stale COSIM_CWD_B copy.
• PSX_COSIM_STRIDE and PSX_COSIM_START_CYCLE now use retirement counts, not guest cycles.

^{Written for commit 4aebb34. Summary will update on new commits.}

[cubic-dev-ai]

2 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tools/cosim.py">

<violation number="1" location="tools/cosim.py:57">
P2: When custom ports do not match the `4600`/`4601` suffix convention, this condition assigns B's isolated executable to the wrong instance or neither instance. Pass an explicit B-role flag into `launch()` instead of inferring the role from the numeric port.</violation>

<violation number="2" location="tools/cosim.py:63">
P2: When B's executable is missing or stale, this new exception occurs after A starts but before cleanup begins, leaving A orphaned. Move both launches inside the cleanup scope or terminate A whenever the B launch fails.</violation>
</file>

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

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: When custom ports do not match the 4600/4601 suffix convention, this condition assigns B's isolated executable to the wrong instance or neither instance. Pass an explicit B-role flag into launch() instead of inferring the role from the numeric port.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At tools/cosim.py, line 57:

<comment>When custom ports do not match the `4600`/`4601` suffix convention, this condition assigns B's isolated executable to the wrong instance or neither instance. Pass an explicit B-role flag into `launch()` instead of inferring the role from the numeric port.</comment>

<file context>
@@ -42,6 +46,27 @@ def tail_file(path, max_bytes=8192):
+    cwd = CWD
+    exe = EXE
+    cwd_b = os.environ.get("COSIM_CWD_B", "")
+    if cwd_b and port != 4600 and str(port).endswith("1"):
+        cwd = cwd_b
+        exe_b = os.environ.get("COSIM_EXE_B",
</file context>

[cubic-dev-ai]

P2: When B's executable is missing or stale, this new exception occurs after A starts but before cleanup begins, leaving A orphaned. Move both launches inside the cleanup scope or terminate A whenever the B launch fails.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At tools/cosim.py, line 63:

<comment>When B's executable is missing or stale, this new exception occurs after A starts but before cleanup begins, leaving A orphaned. Move both launches inside the cleanup scope or terminate A whenever the B launch fails.</comment>

<file context>
@@ -42,6 +46,27 @@ def tail_file(path, max_bytes=8192):
+                               os.path.join(cwd_b, os.path.basename(EXE)))
+        if not os.path.isfile(exe_b):
+            raise RuntimeError(
+                f"instance B exe not found: {exe_b} — copy the freshly built "
+                f"exe into COSIM_CWD_B (its mods/ root is exe-dir-relative)")
+        if os.path.getmtime(exe_b) < os.path.getmtime(EXE) - 1:
</file context>

[Alexbeav]

Review complete. The corresponding upstream PR was merged: mstan#270

Closing this fork review without merge. Its immutable base, discussion, and commit history remain available as evidence.