Review: port freeze heartbeat stack capture to POSIX - #31
Open
Alexbeav wants to merge 8 commits into
Open
Conversation
…ast-boot PAL vsync+pacer double-block)
… semantics; cosim: per-instance cwd
The cycle-keyed design hashed+parked at the first cosim_instr boundary AFTER the stride cycle was crossed; the two backends can reach that crossing at different retirement positions, so the oracle compared different architectural states (observed as a +5-cycle park skew and phantom CPU-register divergences mid-CPS). Checkpoints are now taken inside cosim_instr, keyed on the retirement count itself (a branch and its delay slot retire as one event in both backends). Checkpoint k = state after k*stride retirements in BOTH instances by construction. The guest-cycle clock stays inside the state hash, so real cycle-accounting drift is still detected -- now pinned to an exact instruction window instead of corrupting the alignment. cosim.py: stride/--max/--start-cycle now in retirement units; the cycle skew warning is now reported as a real timing divergence. launch() gives instance B its own exe (the mods root is exe-dir-relative, so a shared exe races on mods/state.toml publish regardless of cwd separation) and refuses a stale COSIM_CWD_B exe. Gates on CMR2 (SLUS-01222): compiled/compiled and interp/interp both clean over 100M retirements with identical cross-pair chains; injected 1-bit RAM fault detected at the next checkpoint.
A game that overlays its own text at runtime (CMR2 streams transform-
loop variants with patched projection constants over the boot EXE)
kept executing the stale static translation through inter-piece host
transfers: dispatch validates an entry's emitted ranges, but split
function pieces chain via direct host calls (fallthrough to split
piece / next function, legacy non-CPS split branch/jump/continuation
transfers, non-CPS direct jal) with no revalidation. A clean entry
piece falls through natively into a stale sibling whose live bytes an
overlay replaced -- the CMR2 attract/gameplay wedge corruption
(first divergence: one ADDI immediate, screen-center 160 static vs
256 live, at 0x800199D4, retirement 3,203,596,289, frame ~11963).
Emit at every such transfer:
if (!psx_game_text_native_ok(T)) { cpu->pc = T; return; }
publishing the PC and unwinding to the trampoline, whose dispatch
takes the sanctioned dirty-RAM-interpreter fallback over live bytes.
The non-CPS direct jal falls back to call_by_address instead.
dirty_ram_text_native_ok_ranges_from gains a page-clean fast path
(skip the memcmp when no touched page is guard-modified or runtime-
dirty) so the guards are near-free on pristine pages.
cosim.py: print full A/B cpu dumps in cpudiff mode (values, not just
diffs).
dirty_ram_text_native_ok_ranges_from clipped validation to [exec_pc, end), assuming a continuation never fetches bytes behind its resume PC. False for any entry with a backward edge: a post-call continuation re-enters mid-function and loops back into the clipped region, executing stale static code. CMR2's overlay engine carries a second copy of the vertex-transform kernel at 0x80016B34 that forked exactly this way (r16 +0xA0 static vs +0x100 live, the same 0x60 delta as the first locus) after the inter-piece transfer guards closed the fallthrough edge. Validation now covers every emitted range of the entry regardless of resume PC. A genuinely patched prologue blocks the whole entry to the dirty-RAM interpreter -- correct, merely slower.
Linux/macOS builds kept no freeze observability: freeze_heartbeat_start was a Windows-only no-op (Linux smoke finding 5, 2026-08-29). This ports the heartbeat thread to pthreads and the main-thread stack capture to a signal-based walker. POSIX design: heartbeat thread = pthread + nanosleep. Stack capture = SIGUSR2 delivered to the main thread; the handler runs on the main thread, so backtrace() walks the wedged stack directly into a preallocated static buffer; the heartbeat thread polls a sig_atomic flag with a 64ms bound, then symbolizes via dladdr (symbol, module, RVA for offline addr2line). Barrier before the capture flag; uninterruptible wedges return an empty array. Self-capture guard matches the Windows rule (fatal path never walks itself). Dump mutex and atomic rename were already dual-path. Windows path unchanged; compiles clean on MinGW (object check). POSIX path compiles on glibc targets only (execinfo/dladdr); Linux build and gate validation deferred to users and Linux CI per owner decision 2026-09-10.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 8 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="runtime/src/memory.c">
<violation number="1" location="runtime/src/memory.c:541">
P2: This change makes the registered `dirty_text_continuation_guards` test fail because it removes the `exec_pc` clipping that the test explicitly requires. Update the regression test with the new full-range contract, or retain clipping if that contract is not intended.</violation>
<violation number="2" location="runtime/src/memory.c:576">
P1: After a savestate restores self-modified game text, this fast path can treat the page as clean and dispatch stale native code. Preserve and restore the text-dirty state, or force range comparisons after `overlay_watch_invalidate_after_ram_restore()`.</violation>
</file>
<file name="recompiler/src/code_generator.cpp">
<violation number="1" location="recompiler/src/code_generator.cpp:115">
P1: When a stale split piece is reached through a nested non-CPS call, this `return` unwinds only the current generated function and the caller resumes its continuation as if the callee returned normally. Set the call-bail flag before returning so enclosing generated frames stop and the trampoline dispatches `cpu->pc` through the interpreter fallback.</violation>
</file>
<file name="tools/cosim.py">
<violation number="1" location="tools/cosim.py:57">
P2: When custom ports do not match the hard-coded `4600`/`*1` pattern, `launch()` assigns B’s runtime directory to the wrong process or neither process. Pass an explicit A/B role from the call sites instead of inferring it from the port number.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| break; | ||
| } | ||
| } | ||
| if (clean) continue; |
There was a problem hiding this comment.
P1: After a savestate restores self-modified game text, this fast path can treat the page as clean and dispatch stale native code. Preserve and restore the text-dirty state, or force range comparisons after overlay_watch_invalidate_after_ram_restore().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/memory.c, line 576:
<comment>After a savestate restores self-modified game text, this fast path can treat the page as clean and dispatch stale native code. Preserve and restore the text-dirty state, or force range comparisons after `overlay_watch_invalidate_after_ram_restore()`.</comment>
<file context>
@@ -547,12 +555,26 @@ int dirty_ram_text_native_ok_ranges_from(const uint32_t *lo_len_pairs,
+ break;
+ }
+ }
+ if (clean) continue;
+ }
if (memcmp(ram + phys, text_ref_image + (phys - text_ref_lo), len) != 0) {
</file context>
| * sanctioned dirty-RAM-interpreter fallback over the live bytes. */ | ||
| static std::string emit_stale_static_guard(uint32_t target, const std::string& indent) { | ||
| return fmt::format( | ||
| "{0}if (!psx_game_text_native_ok(0x{1:08X}u)) {{ cpu->pc = 0x{1:08X}u; return; }} /* stale-static guard */\n", |
There was a problem hiding this comment.
P1: When a stale split piece is reached through a nested non-CPS call, this return unwinds only the current generated function and the caller resumes its continuation as if the callee returned normally. Set the call-bail flag before returning so enclosing generated frames stop and the trampoline dispatches cpu->pc through the interpreter fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At recompiler/src/code_generator.cpp, line 115:
<comment>When a stale split piece is reached through a nested non-CPS call, this `return` unwinds only the current generated function and the caller resumes its continuation as if the callee returned normally. Set the call-bail flag before returning so enclosing generated frames stop and the trampoline dispatches `cpu->pc` through the interpreter fallback.</comment>
<file context>
@@ -101,6 +101,33 @@ CodeGenerator::CodeGenerator(const PS1Executable& exe, const CodeGenConfig& conf
+ * sanctioned dirty-RAM-interpreter fallback over the live bytes. */
+static std::string emit_stale_static_guard(uint32_t target, const std::string& indent) {
+ return fmt::format(
+ "{0}if (!psx_game_text_native_ok(0x{1:08X}u)) {{ cpu->pc = 0x{1:08X}u; return; }} /* stale-static guard */\n",
+ indent, target);
+}
</file context>
Suggested change
| "{0}if (!psx_game_text_native_ok(0x{1:08X}u)) {{ cpu->pc = 0x{1:08X}u; return; }} /* stale-static guard */\n", | |
| "{0}if (!psx_game_text_native_ok(0x{1:08X}u)) {{ cpu->pc = 0x{1:08X}u; g_psx_call_bail = 1; return; }} /* stale-static guard */\n", |
| cwd = CWD | ||
| exe = EXE | ||
| cwd_b = os.environ.get("COSIM_CWD_B", "") | ||
| if cwd_b and port != 4600 and str(port).endswith("1"): |
There was a problem hiding this comment.
P2: When custom ports do not match the hard-coded 4600/*1 pattern, launch() assigns B’s runtime directory to the wrong process or neither process. Pass an explicit A/B role from the call sites instead of inferring it from the port number.
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 hard-coded `4600`/`*1` pattern, `launch()` assigns B’s runtime directory to the wrong process or neither process. Pass an explicit A/B role from the call sites instead of inferring it from the port number.</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>
| g_text_native_blocked++; | ||
| return 0; | ||
| } | ||
| /* NOTE (2026-08-30): ranges are validated IN FULL regardless of exec_pc. |
There was a problem hiding this comment.
P2: This change makes the registered dirty_text_continuation_guards test fail because it removes the exec_pc clipping that the test explicitly requires. Update the regression test with the new full-range contract, or retain clipping if that contract is not intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/memory.c, line 541:
<comment>This change makes the registered `dirty_text_continuation_guards` test fail because it removes the `exec_pc` clipping that the test explicitly requires. Update the regression test with the new full-range contract, or retain clipping if that contract is not intended.</comment>
<file context>
@@ -538,6 +538,14 @@ int dirty_ram_text_native_ok_ranges_from(const uint32_t *lo_len_pairs,
g_text_native_blocked++;
return 0;
}
+ /* NOTE (2026-08-30): ranges are validated IN FULL regardless of exec_pc.
+ * The former [exec_pc, end) clip assumed a continuation "never fetches
+ * the patched bytes" behind its resume PC — false for any entry with a
</file context>
…the handler Cubic review finding (P2, PR #31): calling backtrace() from the SIGUSR2 handler can deadlock when the interrupted code holds a loader or allocator lock. The handler now only copies the interrupted ucontext_t and raises a flag (async-signal-safe: no unwinding, allocation, or locking); the heartbeat thread polls with the existing 64ms bound and walks the saved PC/FP/SP itself, rejecting frame pointers outside the main-thread stack bounds recorded at start. Linux and macOS x86-64 use the FP chain; other POSIX targets emit the interrupted PC. _GNU_SOURCE precedes the system headers for pthread_getattr_np. Windows path unchanged and object-compiled on MinGW.
Owner
Author
|
@cubic-dev-ai review this PR |
@Alexbeav I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 8 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="runtime/src/freeze_heartbeat.c">
<violation number="1" location="runtime/src/freeze_heartbeat.c:407">
P2: In the repository's optimized builds, `RBP` is not guaranteed to be a frame pointer, so Linux captures will usually lack the requested call stack or contain unreliable frames. Preserve frame pointers for the relevant runtime objects or use an unwinder that does not require them.</violation>
<violation number="2" location="runtime/src/freeze_heartbeat.c:408">
P2: If the interrupted frame pointer is corrupted, this addition can wrap and make the heartbeat thread dereference an invalid address, losing freeze capture and potentially invoking the process crash path. Check that `fp <= s_stack_hi` before subtracting the required frame size.</violation>
<violation number="3" location="runtime/src/freeze_heartbeat.c:1305">
P2: On macOS, this POSIX port does not provide the promised stack capture: Intel builds stop after one PC, while Apple Silicon builds produce no frames. Add macOS stack-bound discovery and the arm64 `ucontext_t` register extraction before treating macOS as supported.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| sa.sa_flags = SA_RESTART | SA_SIGINFO; | ||
| sigemptyset(&sa.sa_mask); | ||
| if (sigaction(HB_SIG, &sa, NULL) == 0) s_sig_installed = 1; | ||
| #if defined(__linux__) |
There was a problem hiding this comment.
P2: On macOS, this POSIX port does not provide the promised stack capture: Intel builds stop after one PC, while Apple Silicon builds produce no frames. Add macOS stack-bound discovery and the arm64 ucontext_t register extraction before treating macOS as supported.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/freeze_heartbeat.c, line 1305:
<comment>On macOS, this POSIX port does not provide the promised stack capture: Intel builds stop after one PC, while Apple Silicon builds produce no frames. Add macOS stack-bound discovery and the arm64 `ucontext_t` register extraction before treating macOS as supported.</comment>
<file context>
@@ -1124,5 +1291,37 @@ void freeze_heartbeat_start(const char *backend_label) {
+ sa.sa_flags = SA_RESTART | SA_SIGINFO;
+ sigemptyset(&sa.sa_mask);
+ if (sigaction(HB_SIG, &sa, NULL) == 0) s_sig_installed = 1;
+#if defined(__linux__)
+ {
+ /* Stack bounds make the saved-context walk safe (rejecting any frame
</file context>
| (void)sp; | ||
| int n = 0; | ||
| out[n++] = pc; | ||
| while (n < max && fp && s_stack_lo && s_stack_hi) { |
There was a problem hiding this comment.
P2: In the repository's optimized builds, RBP is not guaranteed to be a frame pointer, so Linux captures will usually lack the requested call stack or contain unreliable frames. Preserve frame pointers for the relevant runtime objects or use an unwinder that does not require them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/freeze_heartbeat.c, line 407:
<comment>In the repository's optimized builds, `RBP` is not guaranteed to be a frame pointer, so Linux captures will usually lack the requested call stack or contain unreliable frames. Preserve frame pointers for the relevant runtime objects or use an unwinder that does not require them.</comment>
<file context>
@@ -308,6 +344,114 @@ static void freeze_dump_main_stack_samples_json(FILE *f, int n) {
+ (void)sp;
+ int n = 0;
+ out[n++] = pc;
+ while (n < max && fp && s_stack_lo && s_stack_hi) {
+ if (fp < s_stack_lo || fp + 16 > s_stack_hi) break;
+ uintptr_t next_fp = *(const uintptr_t *)fp;
</file context>
| int n = 0; | ||
| out[n++] = pc; | ||
| while (n < max && fp && s_stack_lo && s_stack_hi) { | ||
| if (fp < s_stack_lo || fp + 16 > s_stack_hi) break; |
There was a problem hiding this comment.
P2: If the interrupted frame pointer is corrupted, this addition can wrap and make the heartbeat thread dereference an invalid address, losing freeze capture and potentially invoking the process crash path. Check that fp <= s_stack_hi before subtracting the required frame size.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/freeze_heartbeat.c, line 408:
<comment>If the interrupted frame pointer is corrupted, this addition can wrap and make the heartbeat thread dereference an invalid address, losing freeze capture and potentially invoking the process crash path. Check that `fp <= s_stack_hi` before subtracting the required frame size.</comment>
<file context>
@@ -308,6 +344,114 @@ static void freeze_dump_main_stack_samples_json(FILE *f, int n) {
+ int n = 0;
+ out[n++] = pc;
+ while (n < max && fp && s_stack_lo && s_stack_hi) {
+ if (fp < s_stack_lo || fp + 16 > s_stack_hi) break;
+ uintptr_t next_fp = *(const uintptr_t *)fp;
+ uintptr_t ret = *(const uintptr_t *)(fp + sizeof(uintptr_t));
</file context>
Suggested change
| if (fp < s_stack_lo || fp + 16 > s_stack_hi) break; | |
| if (fp < s_stack_lo || fp > s_stack_hi || s_stack_hi - fp < 16) break; |
There was a problem hiding this comment.
3 existing issues remain and 1 new issue found across 1 file (changes from recent commits).
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="runtime/src/freeze_heartbeat.c">
<violation number="1" location="runtime/src/freeze_heartbeat.c:354">
P1: On macOS, this shallow-copies `ucontext_t` while retaining a pointer into the signal frame, which the heartbeat thread reads after the handler returns. Copy the Darwin machine context or register values into mailbox-owned storage before raising `s_sig_captured`.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Re-trigger cubic
| /* Async-signal-safe: copy the interrupted context and raise the flag. | ||
| * No unwinding, allocation, or locking — see the mailbox comment. */ | ||
| if (uctx) { | ||
| s_sig_uctx = *(const ucontext_t *)uctx; |
There was a problem hiding this comment.
P1: On macOS, this shallow-copies ucontext_t while retaining a pointer into the signal frame, which the heartbeat thread reads after the handler returns. Copy the Darwin machine context or register values into mailbox-owned storage before raising s_sig_captured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At runtime/src/freeze_heartbeat.c, line 354:
<comment>On macOS, this shallow-copies `ucontext_t` while retaining a pointer into the signal frame, which the heartbeat thread reads after the handler returns. Copy the Darwin machine context or register values into mailbox-owned storage before raising `s_sig_captured`.</comment>
<file context>
@@ -332,60 +346,97 @@ static void freeze_dump_main_stack_samples_json(FILE *f, int n) {
+ /* Async-signal-safe: copy the interrupted context and raise the flag.
+ * No unwinding, allocation, or locking — see the mailbox comment. */
+ if (uctx) {
+ s_sig_uctx = *(const ucontext_t *)uctx;
+ __sync_synchronize(); /* context visible before the capture flag */
+ s_sig_captured = 1;
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linux and macOS builds have zero freeze observability:
freeze_heartbeat_start()is a Windows-only no-op (Linux smoke finding 5, 2026-08-29), so Linux/Deck users get no freeze dumps, no stall forensics, and the boot gate sees nothing on a hang.Change:
backtrace()walks the wedged stack directly into a preallocated static buffer. The heartbeat thread polls asig_atomicflag with a 64 ms bound, then symbolizes viadladdr(symbol, module, RVA for offline addr2line), matching the Windows dump contract. Barrier before the capture flag; a main thread wedged inside an uninterruptible syscall yields an empty array (honest result). Self-capture guard mirrors the Windows rule: the fatal path never walks itself.Evidence and limits:
psx_freeze_heartbeat.jsonneed no changes.Developed with AI assistance (Cline session, 2026-09-10); tested as described above before pushing.
Summary by cubic
Ports freeze heartbeat stack capture to Linux/macOS so wedged builds produce the same freeze dumps as Windows, and fixes several correctness bugs found while validating the port. Windows path is unchanged; the POSIX path requires glibc (
execinfo/dladdr), and Linux build and boot-gate validation is deferred to Linux CI.New Features
dladdr.Bug Fixes
gte_read_data()andgte_read_ctrl()treats specially.PSX_COSIM_STRIDEandPSX_COSIM_START_CYCLEare now in retired instructions, and instance B needs its own exe copy.game_idmanually since no serial is recoverable.Written for commit b053ed0. Summary will update on new commits.