jit/gc: eval-breaker back-edge fold (#517) + registry hardening (#518) + catch-adjacency (#174)#551
jit/gc: eval-breaker back-edge fold (#517) + registry hardening (#518) + catch-adjacency (#174)#551youknowone wants to merge 5 commits into
Conversation
WalkthroughChangesEval-breaker word integration
Shadow-stack STW safety
Canraise adjacency invariant
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd7d8c013
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if value < 0 { | ||
| majit_ir::eval_breaker_word::set_async(); | ||
| } else { | ||
| majit_ir::eval_breaker_word::clear_async(); |
There was a problem hiding this comment.
If a Unix signal is delivered after _ticker is reset to a non-negative value but before this clear_async(), the signal handler writes _ticker = -1 and sets EB_ASYNC, then this line erases the only bit compiled loops now poll. At a loop-header resume, eval_loop_jit can try jit_merge_point_hook before the interpreter decrements the ticker again, so a non-allocating compiled loop can re-enter with _ticker < 0 but bit0 clear and miss the pending signal indefinitely. Recheck the ticker after clearing or make the clear conditional on no signal having rearmed it.
Useful? React with 👍 / 👎.
| } | ||
| let gc = build_gc(); | ||
| majit_gc::gc_sync::store_singleton(gc); | ||
| majit_ir::eval_breaker_word::publish_addr(); |
There was a problem hiding this comment.
When two threads initialize the JIT/GC concurrently, one thread can set GC_INITIALIZED inside store_singleton() and then be preempted before this publish; a second thread entering build_gc_global() during that window returns at the is_initialized() check and may compile traces while eval_breaker_word_addr() is still 0. Those GuardEvalBreaker ops are compiled as inert forever, so the affected traces will not deopt for SIGINT or STW. Ensure the address is published even for the already-initialized path, or publish it before exposing GC initialization.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit bb98173). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
|
Ran the codex parity review cycle on this PR (15 files, base = merge-base with One real finding, fixed (commit No other in-scope findings. The remaining structural-adaptation items (free-threaded async/STW word, the reserved poll-address register, — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16c2a240ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| ready_rx.recv().unwrap(); | ||
| let stw = quiesce_mutators(); |
There was a problem hiding this comment.
When the default Rust test harness runs this unit test in parallel with another unignored test in this module, this direct quiesce_mutators() call can leave the process-global stw_requested flag set without holding the gc_mutex that request_stw() normally holds. If a sibling test has already registered a mutator and then enters gc_op during this window, gc_op_slow can acquire the mutex and hit its !stw_requested debug assertion, making cargo test -p majit-gc timing-dependent; mark this process-global STW test ignored/serial like the other STW tests or drive it through request_stw.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48693de94c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::hint::spin_loop(); | ||
| } | ||
| worker_observed_poll.store(true, Ordering::Release); | ||
| safepoint_poll(); |
There was a problem hiding this comment.
On weakly ordered targets such as aarch64, this test can observe EB_STW in the eval-breaker word before the separate stw_requested store is visible; the eval-breaker module explicitly allows that transient and relies on real compiled/interpreter loops re-polling. Here the worker calls safepoint_poll() only once and then immediately expects bit1 to be cleared, so safepoint_poll() can return without parking and the following assertion becomes a timing-dependent failure. Keep polling until the request is observed/cleared, or synchronize the test on the authoritative STW state rather than a single relaxed bit observation.
Useful? React with 👍 / 👎.
insert_exits guards that a block's catch_exception directly follows the raising op's trailing -live- (the adjacency handle_exception_in_frame and derive_after_call_indices_from_sparse both key on). That guard was a debug_assert!, compiled out in release, so a vable-mirror store serialized between the -live- and catch_exception would silently mis-resume the blackhole in release builds. Promote it to assert! so the invariant holds in every build profile. Pyre's walker does not block-split at each can-raise op the way flowspace does, so a block may legitimately hold several can-raise ops; only the caught (last) op needs the adjacency. The enforced invariant is that adjacency, not a per-block can-raise-op count. gh#174 Assisted-by: Claude Coded-by: Codex
The MUTATOR_REGISTRY foreign-walk and extra-area APIs upheld their soundness invariants by caller discipline only. Encode them at the API boundary (no release-build behavior change): - walk_all_extra_areas / walk_all_roots / walk_all_jf_roots / walk_all_bh_regs / walk_all_resume_ref_roots: debug_assert mutators_quiesced() on entry — these dereference foreign mutator TLS and are correct only under collector-side STW. The collector already gates each call on the same predicate, so the assert is invariant. - register_mutator_extra_area: mark unsafe with a documented data lifetime / walk-derivation contract; wrap the register_thread_root_areas call sites and the unit test in unsafe blocks. - register_mutator: document the GcMutatorRegistration RAII pairing that callers must arm for unregistration. Assisted-by: Claude Coded-by: Codex
Compiled loop back-edges polled two separate words: the async-action ticker address and a GC stop-the-world flag. Replace both with a single process-global AtomicUsize bitmask (bit0 async, bit1 STW) baked into one reserved register, so the back-edge is one load plus one nonzero branch. - majit-ir/eval_breaker_word.rs: the bitmask word, its published address, and a test-only per-thread address override. - executioncontext/signalstate: mirror the ticker sign into bit0 through reset_ticker/decrement_ticker/rearm_ticker. - gc_sync: set/clear bit1 next to stw_requested under the quiesce lock. - eval.rs: publish the address at GC init and call gc_sync::safepoint_poll at the eval_loop_jit head so a deopted non-allocating loop parks. - backends: single load/cmp/branch arm (aarch64 hoists the address into a reserved register, x86 immediate, cranelift condition-only). - remove the superseded gc_stw and eval_breaker address holders. Backend guard tests bake a test-local word via the override; the global-STW multi-thread park test is marked ignore (it drives the process-global STW state and must run serially). Assisted-by: Claude Coded-by: Codex
ActionFlag is per-execution-context, so the fold's reset_ticker and decrement_ticker async-bit mirror fired for every context's ticker. A non-main context clearing its own ticker then cleared the shared bit while the signal-registered ticker was still negative, dropping a pending async from a compiled loop polling that bit. Gate both mirror sites on the registered ticker cell (the single ticker compiled-loop back-edges polled before the fold) so only it drives the shared word. Assisted-by: Claude Coded-by: Codex
The is_registered_ticker helper (introduced with the cross-EC async-bit mirror gate) caused two CI regressions: - wasm32 failed to compile (E0433): it referenced crate::module::signal::signalstate::registered_ticker_ptr(), but module::signal is #[cfg(not(target_arch = "wasm32"))]. Gate the check off on wasm32 (no signal module, hence no registered ticker, so the mirror is inert there). - The Windows build drifted the bh-handler unwired-opname snapshot with an int_eq/ir>i kind shape. The helper runs inside the traced eval loop (decrement_ticker), and std::ptr::eq on *const isize can lower to an int_eq with a ref-typed operand, which has no blackhole handler. Compare the two cell addresses as usize so the equality stays int/int. Behavior is unchanged on native targets. Assisted-by: Claude
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-gc/src/gc_sync.rs`:
- Around line 645-699: Mark the test function
eval_breaker_word_parks_and_resumes_mutator with #[ignore] and add the same
exclusive-process annotation used by neighboring shared-state tests. Preserve
the test body and assertions unchanged.
In `@pyre/pyre-interpreter/src/module/signal/signalstate.rs`:
- Around line 242-295: Serialize signal_during_warmup_sets_async_bit by
acquiring the crate’s existing lock_tests() mutex at the start of the test,
following the locking pattern used by the stack checks. Keep the existing
ResetSignalState guard and test assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e570af66-3a66-4ec3-b7c5-aa6aeccfa11e
📒 Files selected for processing (15)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend-dynasm/Cargo.tomlmajit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/aarch64/registers.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-gc/src/gc_sync.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-ir/src/eval_breaker.rsmajit/majit-ir/src/eval_breaker_word.rsmajit/majit-ir/src/lib.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/signal/signalstate.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/flatten.rs
💤 Files with no reviewable changes (2)
- majit/majit-ir/src/eval_breaker.rs
- pyre/pyre-interpreter/src/module/signal/interp_signal.rs
| fn _call_header(&mut self, inputargs: &[InputArg]) { | ||
| dynasm!(self.mc ; .arch aarch64 | ||
| ; stp x29, x30, [sp, #-48]! | ||
| ; stp x29, x30, [sp, #-64]! | ||
| ; stp x19, x20, [sp, #16] // save callee-saved regs | ||
| ; stp x21, x22, [sp, #32] // save callee-saved regs | ||
| ; str x24, [sp, #56] // save reserved bitmask-addr reg | ||
| ; mov x29, x0 | ||
| ); | ||
| // Bake the process-global eval-breaker-word address into the reserved | ||
| // register once per trace entry; the back-edge poll then loads the word | ||
| // x24-relative. Gated identically to the poll (0 = not published -> | ||
| // no reg, no poll). | ||
| let bitmask_addr = majit_ir::eval_breaker_word::eval_breaker_word_addr(); | ||
| if bitmask_addr != 0 { | ||
| self.emit_mov_imm64( | ||
| crate::aarch64::registers::BITMASK_ADDR_REG.value as u32, | ||
| bitmask_addr as i64, | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Frame-size magic numbers and the reserved register are duplicated as literals instead of using a single source of truth.
The new 64-byte frame size and the x24 save offset (#56) are repeated verbatim across three sites (_call_header prologue, the stack-overflow early-return, and _call_footer), and the reserved register itself is only referenced symbolically once (BITMASK_ADDR_REG at line 1278) — every other occurrence (lines 1265, 1268, 1329, 1353, and the guard load at line 3450) hardcodes the literal x24. If the reserved register is ever renumbered via BITMASK_ADDR_REG, these hardcoded x24 sites and the #56/#64 offsets would silently desync, producing stack corruption or a guard that polls the wrong register.
♻️ Suggested consolidation
-const AARCH64_GEN_REGS: [crate::regloc::RegLoc; 18] = crate::aarch64::registers::ALL_REGS;
+const AARCH64_GEN_REGS: [crate::regloc::RegLoc; 18] = crate::aarch64::registers::ALL_REGS;
+const CALL_HEADER_FRAME_SIZE: u32 = 64;
+const BITMASK_ADDR_REG_SAVE_OFS: u32 = 56;Then use X(crate::aarch64::registers::BITMASK_ADDR_REG.value) instead of the literal x24 in the guard load, and reference CALL_HEADER_FRAME_SIZE/BITMASK_ADDR_REG_SAVE_OFS at all three save/restore sites.
Since BITMASK_ADDR_REG is defined in registers.rs (not in this review batch), please confirm its value is 24 and that dynasm's X(n) macro accepts the constant's .value field directly (as it does for regalloc-provided RegLocs elsewhere in this file).
Also applies to: 1326-1331, 1347-1357, 3445-3458
| #[test] | ||
| fn eval_breaker_word_parks_and_resumes_mutator() { | ||
| majit_ir::eval_breaker_word::clear_async(); | ||
| majit_ir::eval_breaker_word::clear_stw(); | ||
| majit_ir::eval_breaker_word::publish_addr(); | ||
|
|
||
| let observed_poll = Arc::new(AtomicBool::new(false)); | ||
| let worker_observed_poll = observed_poll.clone(); | ||
| let (ready_tx, ready_rx) = mpsc::channel(); | ||
| let (resumed_tx, resumed_rx) = mpsc::channel(); | ||
| let worker = std::thread::spawn(move || { | ||
| register_thread(); | ||
| ready_tx.send(()).unwrap(); | ||
| while load_eval_breaker_word() & majit_ir::eval_breaker_word::EB_STW == 0 { | ||
| std::hint::spin_loop(); | ||
| } | ||
| worker_observed_poll.store(true, Ordering::Release); | ||
| safepoint_poll(); | ||
| assert_eq!( | ||
| load_eval_breaker_word() & majit_ir::eval_breaker_word::EB_STW, | ||
| 0, | ||
| "the resumed mutator must observe bit1 cleared" | ||
| ); | ||
| resumed_tx.send(()).unwrap(); | ||
| unregister_thread(); | ||
| }); | ||
|
|
||
| ready_rx.recv().unwrap(); | ||
| let stw = quiesce_mutators(); | ||
| assert!( | ||
| observed_poll.load(Ordering::Acquire), | ||
| "the bitmask poll must lead the worker into the park gate" | ||
| ); | ||
| assert_ne!( | ||
| load_eval_breaker_word() & majit_ir::eval_breaker_word::EB_STW, | ||
| 0, | ||
| "bit1 must remain armed throughout the STW episode" | ||
| ); | ||
| drop(stw); | ||
| assert_eq!( | ||
| load_eval_breaker_word() & majit_ir::eval_breaker_word::EB_STW, | ||
| 0, | ||
| "bit1 must balance to zero before mutators resume" | ||
| ); | ||
| resumed_rx | ||
| .recv_timeout(Duration::from_secs(5)) | ||
| .expect("the parked mutator must resume cleanly"); | ||
| worker.join().unwrap(); | ||
| assert_eq!( | ||
| load_eval_breaker_word(), | ||
| 0, | ||
| "the eval-breaker word must be balanced after the STW episode" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="majit/majit-gc/src/gc_sync.rs"
echo "=== ignore annotations in file ==="
rg -n '#\[ignore' "$FILE" || true
echo
echo "=== surrounding test block ==="
sed -n '560,740p' "$FILE" | cat -n
echo
echo "=== tests mentioning quiesce_mutators or register_thread ==="
rg -n 'quiesce_mutators|register_thread|unregister_thread|safepoint_poll|eval_breaker_word_parks_and_resumes_mutator|stw_blocks_concurrent_gc_ops|entry_only_safepoint_preserves_fresh_gc_op_return' "$FILE" || trueRepository: youknowone/pyre
Length of output: 9180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="majit/majit-gc/src/gc_sync.rs"
sed -n '760,870p' "$FILE" | cat -n
echo
echo "=== ignore annotations around later tests ==="
rg -n '#\[ignore|fn stw_blocks_concurrent_gc_ops|fn entry_only_safepoint_preserves_fresh_gc_op_return' "$FILE" -A 6 -B 2 || trueRepository: youknowone/pyre
Length of output: 7348
Mark eval_breaker_word_parks_and_resumes_mutator as #[ignore]
This test uses process-global eval-breaker and mutator-registration state, so it can race with other majit-gc tests under the default parallel runner. Add the same exclusive-process annotation used by the neighboring shared-state tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-gc/src/gc_sync.rs` around lines 645 - 699, Mark the test function
eval_breaker_word_parks_and_resumes_mutator with #[ignore] and add the same
exclusive-process annotation used by neighboring shared-state tests. Preserve
the test body and assertions unchanged.
| #[cfg(all(test, unix))] | ||
| mod tests { | ||
| use super::*; | ||
| use std::sync::atomic::{AtomicUsize, Ordering}; | ||
|
|
||
| use crate::executioncontext::{ActionFlag, ActionFlagOps}; | ||
|
|
||
| struct ResetSignalState; | ||
|
|
||
| impl Drop for ResetSignalState { | ||
| fn drop(&mut self) { | ||
| register_ticker(std::ptr::null_mut()); | ||
| pypysig_default(libc::SIGINT); | ||
| while signal_poll() >= 0 {} | ||
| majit_ir::eval_breaker_word::clear_async(); | ||
| } | ||
| } | ||
|
|
||
| fn load_eval_breaker_word() -> usize { | ||
| let addr = majit_ir::eval_breaker_word::eval_breaker_word_addr(); | ||
| assert_ne!(addr, 0); | ||
| unsafe { &*(addr as *const AtomicUsize) }.load(Ordering::Relaxed) | ||
| } | ||
|
|
||
| #[test] | ||
| fn signal_during_warmup_sets_async_bit() { | ||
| let _reset = ResetSignalState; | ||
| let mut actionflag = ActionFlag::new(); | ||
| actionflag.reset_ticker(0); | ||
| register_ticker(actionflag.ticker_addr()); | ||
| majit_ir::eval_breaker_word::publish_addr(); | ||
|
|
||
| assert!(pypysig_setflag(libc::SIGINT)); | ||
| assert_eq!(unsafe { libc::raise(libc::SIGINT) }, 0); | ||
| assert!(actionflag.get_ticker() < 0); | ||
| assert_ne!( | ||
| load_eval_breaker_word() & majit_ir::eval_breaker_word::EB_ASYNC, | ||
| 0, | ||
| "the real signal handler must arm bit0" | ||
| ); | ||
| assert_eq!(signal_poll(), libc::SIGINT); | ||
|
|
||
| actionflag | ||
| .action_dispatcher(std::ptr::null_mut(), std::ptr::null_mut()) | ||
| .unwrap(); | ||
| assert!(actionflag.get_ticker() >= 0); | ||
| assert_eq!( | ||
| load_eval_breaker_word() & majit_ir::eval_breaker_word::EB_ASYNC, | ||
| 0, | ||
| "dispatch must clear bit0 with the non-negative ticker reset" | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for other tests in pyre-interpreter that touch signal/ticker
# global state concurrently with the new test, and check for existing
# serialization conventions for signal tests.
rg -n -B2 -A5 'register_ticker\(|pypysig_setflag|pypysig_default|libc::raise' pyre/pyre-interpreter/src --type rust
rg -n 'test-threads|serial_test|#\[serial\]' pyre/pyre-interpreter Cargo.toml .cargo 2>/dev/nullRepository: youknowone/pyre
Length of output: 10702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the signal module tests and crate-level test configuration.
printf '\n== signalstate.rs tests ==\n'
sed -n '230,340p' pyre/pyre-interpreter/src/module/signal/signalstate.rs
printf '\n== signal-related test usages in pyre-interpreter ==\n'
rg -n -B2 -A4 'register_ticker\(|pypysig_setflag|pypysig_default|pypysig_ignore|signal_poll\(|libc::raise' pyre/pyre-interpreter/src --type rust
printf '\n== crate config / test serialization hints ==\n'
rg -n 'test-threads|serial_test|may_force_test_lock|exception_test_guard|parallel|harness = false' pyre/pyre-interpreter/Cargo.toml pyre/pyre-interpreter/.cargo .cargo Cargo.toml 2>/dev/null || trueRepository: youknowone/pyre
Length of output: 17362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the signal test module itself introduces any guard or if other
# tests in the same crate share the same process-global state.
ast-grep outline pyre/pyre-interpreter/src/module/signal/signalstate.rs --view expandedRepository: youknowone/pyre
Length of output: 1688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all rust tests in the interpreter that manipulate process-wide signal state
# or use the same serializing patterns from other backends.
rg -n -B2 -A6 'exception_test_guard\(|may_force_test_lock\(|register_ticker\(|pypysig_setflag|pypysig_default|pypysig_ignore|libc::raise' \
pyre/pyre-interpreter/src pyre/pyre-check/src pyre/pyre-checker/src --type rust || true
# Show crate test config files if present.
find pyre -path '*/Cargo.toml' -o -path '*/.cargo/config*' | sed 's#^\./##' | sortRepository: youknowone/pyre
Length of output: 12880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any repo-wide setting that serializes tests.
rg -n 'test-threads\s*=\s*1|RUST_TEST_THREADS|cargo test.*--test-threads|serial_test|test harness' . \
--type toml --type rust --type sh --type md || trueRepository: youknowone/pyre
Length of output: 2257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== publish_addr and eval_breaker_word usages ==\n'
rg -n -B2 -A6 'publish_addr\(|eval_breaker_word_addr\(|clear_async\(|EB_ASYNC|load_eval_breaker_word' pyre/pyre-interpreter/src --type rust
printf '\n== signal-related tests outside signalstate.rs ==\n'
rg -n -B2 -A6 '#\[test\]|#\[ignore\]|register_ticker\(|pypysig_setflag|pypysig_default|libc::raise' pyre/pyre-interpreter/src --type rustRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== install_signal_handling / startup paths ==\n'
rg -n -B2 -A8 'install_signal_handling\(|block_async_signals_on_origin_thread\(|unblock_async_signals_on_interp_thread\(' pyre/pyre-interpreter/src --type rust
printf '\n== any tests mentioning signals across the crate ==\n'
rg -n -B2 -A8 '#\[test\].*(signal|SIGINT|SIGSEGV|SIGFPE)|signal|SIGINT|SIGSEGV|SIGFPE' pyre/pyre-interpreter/src --type rustRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the existing test lock helper and inspect its shape.
rg -n -B4 -A20 'fn lock_tests\(|lock_tests\(\)' pyre/pyre-interpreter/src/stack_check.rs pyre/pyre-interpreter/src --type rust
# Also inspect any nearby reset helpers that pair with it.
sed -n '1,120p' pyre/pyre-interpreter/src/stack_check.rsRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Pull only the signal-related test area with line numbers for a precise rewrite.
sed -n '240,290p' pyre/pyre-interpreter/src/module/signal/signalstate.rsRepository: youknowone/pyre
Length of output: 1841
Serialize this signal test. It mutates process-wide signal handlers plus the shared ticker/eval-breaker state, so it can race with other #[test]s. Wrap it with the crate’s existing lock_tests() mutex, like the stack checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/signal/signalstate.rs` around lines 242 -
295, Serialize signal_during_warmup_sets_async_bit by acquiring the crate’s
existing lock_tests() mutex at the start of the test, following the locking
pattern used by the stack checks. Keep the existing ResetSignalState guard and
test assertions unchanged.
1 participant
Three JIT/GC hardening commits from the #517-519 STW epic, on
ssa-reprahead ofmain.1.
jit: fold ticker+STW back-edge polls into one word(#517)Compiled-loop back-edges polled two separate process-global words — the async-action ticker address and a GC stop-the-world flag. This folds both into one
AtomicUsizebitmask (bit0 async mirrors_ticker < 0, bit1 STW mirrorsstw_requested), baked into one reserved register, so the back-edge is one load + one nonzero branch (matching the pre-STW baseline — the two-poll version regressed non-allocating hot loops).majit-ir/eval_breaker_word.rs: the bitmask word, its published address, and a test-only per-thread address override.executioncontext/signalstate: mirror the ticker sign into bit0 (reset_ticker/decrement_ticker/rearm_ticker).gc_sync: set/clear bit1 next tostw_requestedunder the quiesce lock.eval.rs: publish the address at GC init and callsafepoint_poll()at theeval_loop_jithead so a deopted non-allocating loop actually parks (closes a latent park gap where a back-edge STW deopt re-entered the interpreter without a park point).gc_stwandeval_breakeraddress holders → single-word design.2.
majit-gc: harden mutator registry API safety boundary (#518)Encodes the
MUTATOR_REGISTRYforeign-walk / extra-area soundness invariants at the API boundary (no release-build behavior change):debug_assert!(mutators_quiesced())on the fivewalk_all_*foreign walkers,register_mutator_extra_areamarkedunsafewith a documented lifetime contract, and theregister_mutatorRAII pairing documented.3.
jit: enforce catch_exception -live- adjacency in release(#174)Promotes the
insert_exitscatch_exception/-live-adjacency guard fromdebug_assert!toassert!so a vable-mirror store serialized between them cannot silently mis-resume the blackhole in release builds.Verification
python pyre/check.py --backend dynasm→ 180/180;--backend cranelift→ 180/180.--features dynasm/--features cranelift); the one global-STW multi-thread park test is#[ignore]d (drives process-global STW, runs serially)._ticker/bit0 mirror window is closed by the interpreter's direct-_tickergate before any bit0-polling trace is entered, and the reserved-register cross-trace hazard is unreachable because address publication precedes all trace compilation.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Reliability