Repository navigation
review: act on the Codex and CodeRabbit findings on #887 - #911
Conversation
WalkthroughThe PR updates GC root-slot reuse and object publication, adds faulthandler file ownership tracking, propagates stdio codec errors, exposes root-walker wiring, removes a JIT guard, and expands JIT recursion documentation. ChangesGC rooting and object publication
Runtime initialization and handler ownership
JIT control-flow documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: d4226796ea
ℹ️ 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".
| let (fd, _w_file) = faulthandler_get_fileno_and_file( | ||
| args.get(1).copied().unwrap_or(pyre_object::PY_NULL), | ||
| )?; |
There was a problem hiding this comment.
On Unix host builds, when register(file=None) resolves a redirected sys.stderr, the returned owner is discarded here while register_user_signal retains only its descriptor. If sys.stderr is later rebound and the old stream is collected, its descriptor can be closed or reused before the registered signal fires. PyPy stores w_file per signal in Handler.user_w_files and removes it on unregister, so pyre needs an equivalent GC-rooted per-signal owner rather than _w_file.
AGENTS.md reference: AGENTS.md:L184-L192
Useful? React with 👍 / 👎.
| if let Ok(flush) = crate::baseobjspace::getattr_str(resolved, "flush") { | ||
| let _ = crate::call::call_function_impl_result(flush, &[]); |
There was a problem hiding this comment.
When a file's flush attribute lookup or call raises SystemExit, these two ignored results swallow it and allow faulthandler.enable or register to continue. The cited PyPy implementation ignores only ordinary OperationErrors and explicitly re-raises asynchronous exceptions; pyre already represents this distinction with PyErrorKind::SystemExit, so this path should propagate that case.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
Drop the `CriticalCodeGuard` from the `bound_reached` compile path (`eval.rs`). It spanned the whole trace-compile-and-dispatch region, so `report_error = 0` covered the walk's residual calls into user code — `stack.c:111` returns that flag in place of the overflow verdict, turning a RecursionError into an unreported native overflow. Upstream marks only regions that run no user code (`compile.py:976` `cpu.force` + `handle_async_forcing`, `pyjitpl.py:3305` `rebuild_state_after_failure`, `resume.py:1318`). Measured on the fixture it was added for, `synth/wasm_ca_trampoline_decline`: with the guard suppressed the overflow no longer occurs at all, so the region is inert. Record why `fbw_inline_recursion_count` matching `w_code` alone is the element-wise greenkey compare of `pyjitpl.py:1396-1401`: `MIFrame.setup` (`pyjitpl.py:74-80`) assigns `greenkey` once, so a frame's greenkey stays its entry greens, and every `InlineFrame` is pushed at a callee entry with the `(w_code, 0)` identity `reconstruct_inline_recipe` also stamps. Write the tuple header before publishing its root (`tupleobject.rs`). `try_gc_alloc_stable_raw` returns uninitialized payload and `tuple_object_custom_trace` reads `ob_header.w_class` and `wrappeditems`, so a collection between `pin_root` and the items-block allocation read garbage. The header now goes in first with a null items block — the state the trace hook returns early on — and the block is stored after. Restore `FAULTHANDLER_FD` when `enable_fatal_handlers` fails; a failed re-enable was redirecting the handlers already installed. Give `OwnerRootGuard` a `PhantomData<*mut ()>`: its `index` names a slot in the acquiring thread's `OWNER_ROOTS`, and a bare `usize` left the guard auto-`Send`. Delete the `type_id == 9` backtrace capture from `remember_young_pointer`. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) 3/3 both with and without this commit, and passes on the Linux CI runner. Assisted-by: Claude
`W_TextIOWrapper` published its reference fields with one write barrier at the end of `attach_buffer`, but every `w_str_new` in between allocates and `set_newline` / `getattr_str` / `set_encoder_decoder` run Python, so a collection could land after a store and before the barrier that would have told it to trace the wrapper. Barrier after each reference store instead, through a `publish_refs` helper, in `attach_buffer` and in `set_encoder_decoder`. `attach_stdio_codec` dropped both the codec lookup and the incremental encoder/decoder construction errors, leaving a stream that reports itself unreadable however readable its buffer is. Propagate them: `init_stream_codecs` and `init_importlib_bootstrap` now return the failure, with a bootstrap failure still winning as the more fundamental one. `allocate_stdio` still cannot propagate — it runs inside `sys` module creation, before there is an interpreter to raise into — but it no longer discards the result blind: with a null codec the only fallible step `attach_buffer` reaches is `buffer.seekable()`, so finish the two fields that early return skipped rather than leaving them at `Default` and forcing `STATE_OK` over a half-applied init. `faulthandler.enable` resolved `file=None` to a hard-coded fd 2 and dropped the file object. `get_fileno_and_file` (`handler.py:35-49`) resolves the CURRENT `sys.stderr`, asks it for `fileno()`, and flushes it ignoring an ordinary flush error; `enable` then parks it as `fatal_error_w_file` (`handler.py:145`) and `disable` clears it (`:150`), because the descriptor the installed handler writes to belongs to that object. pyre has no Handler instance, so the owner is a process-global slot walked as a GC root alongside the other interpreter globals — not a module attribute, which would show up in `dir(faulthandler)` where upstream has none. `acquire_owner_root` scanned `OWNER_ROOTS` from 0 for a free slot, so rooting the frames of a recursion `depth` deep cost O(depth²). Released interior slots now go on a free stack; a released tail still shrinks the walked range, and the shrink drops the queued indices it took out of range. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) with and without this commit, and passes on the Linux CI runner. cargo test: pyre-interpreter 432, majit-gc 215. Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 27e7baf). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
… resolution `interp_return_log_enabled` was defined twice in `pyre-interpreter/src/eval.rs` — #874 and #907 each added a copy under the same `cfg`, and neither PR's CI saw the other's, so `main` does not compile (E0428). Delete the later duplicate. `register` discarded the file `get_fileno_and_file` resolved while the installed handler kept only its descriptor, so a `register(sig, f)` whose `f` was later collected left the handler writing through a closed or reused fd. `handler.py:125` keeps `user_w_files[signum] = w_file` and `:132` pops it on `unregister`; hold the same per-signal owners in a process-global table walked by the faulthandler root walker, which now covers both it and the fatal-error file. A `fileno()` returning a negative value reached `FAULTHANDLER_FD` and the handler registration unchecked. Measured: PyPy accepts it (`enable()` succeeds), 3.14 raises `RuntimeError: file.fileno() is not a valid file descriptor` — distinct from the direct-int arm's `ValueError: file is not a valid file descriptor`. Follow 3.14. Resolving the file argument runs a user `fileno()` and `flush()`; `enable` and `register` ran it before the support gate, so a build that can only answer `NotImplementedError` executed those side effects first. Move both inside the gate — which also puts `register`'s integer coercions ahead of the file resolution, as its argument parsing does. Not changed, after measuring: a `SystemExit` raised out of `flush` does NOT abort `enable`. `handler.py:44-48` re-raises asynchronous errors (`error.py:62-65`) and PyPy does propagate it, but 3.14 clears the flush error unconditionally and swallows it; 3.14 is the behaviour target. check.py: dynasm 345/345, cranelift 345/345, wasm 341/341 — all three green; `synth/mutate_then_raise_caught`, which panicked in `collector.rs` (`minor_custom_trace_target`) 3/3 before, now passes 5/5. cargo test pyre-interpreter 438. faulthandler probes (enable/register file retention across GC, unregister, negative fileno, missing fileno, flush errors) match python3.14 line for line. Assisted-by: Claude
d422679 to
27e7baf
Compare
|
Pushed
|
enable(obj) where obj.flush() raises SystemExit(7) |
|
|---|---|
| PyPy 3.11 | SystemExit propagated: 7 |
| CPython 3.14.5 | SystemExit SWALLOWED |
I had implemented the propagation before checking, then reverted it — applying it would have moved pyre away from its target. The divergence is now recorded in a comment at the call site.
Also fixed while in there
A fileno() returning a negative value reached FAULTHANDLER_FD and the handler registration unchecked. PyPy accepts it; 3.14 raises RuntimeError: file.fileno() is not a valid file descriptor, distinct from the direct-int arm's ValueError: file is not a valid file descriptor. Follows 3.14 now.
Resolving the file argument runs a user fileno() and flush(), and both enable and register ran it before the support gate — so a build that can only answer NotImplementedError executed those side effects first. Both moved inside the gate, which also puts register's integer coercions ahead of the file resolution, as its argument parsing does.
Verification
ALL PASSED: dynasm 345/345
ALL PASSED: cranelift 345/345
ALL PASSED: wasm 341/341
All three backends green. synth/mutate_then_raise_caught, which panicked in collector.rs (minor_custom_trace_target, invalid type_id) 3/3 on macOS before — including on a rebuild of the base with none of this branch applied — now passes 5/5. cargo test: pyre-interpreter 438.
Every faulthandler probe (file retention across GC for both enable and register, unregister, negative fileno, missing fileno, ordinary and async flush errors, dir() surface) matches python3.14 line for line.
— commented by Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/27e7baf70e28dd406dae074ea85a0338a338b45b/pyre-interpreter/src/module/faulthandler/handler.rs#L174-L177
When a supplied file's flush() raises SystemExit, this unconditional discard lets faulthandler.enable() or register() continue and install the handler instead of propagating the exception; pypy/module/faulthandler/handler.py:43-48 explicitly re-raises asynchronous errors. The fresh evidence in this revision is the added comment explicitly confirming that the PyPy behavior is being bypassed in favor of CPython 3.14, despite the repository's strict structural-parity requirement.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@pyre/pyre-interpreter/src/module/faulthandler/handler.rs`:
- Around line 27-36: Update the intra-doc link in the FAULTHANDLER_FILE
documentation to reference the existing walk_faulthandler_roots function instead
of the nonexistent walk_fatal_error_file symbol.
- Around line 146-153: Re-read the normalized pinned object via
gc_roots::shadow_stack_get(file_slot) after pin_root and before calling
getattr_str for "fileno". Use this refreshed value instead of the original
resolved when obtaining and invoking the fileno method, matching the later flush
path.
- Around line 192-238: Add a process-wide faulthandler mutex and acquire its
guard across the complete fd/file update sequences in the enable, register, and
unregister implementations. Hold the guard through handler installation or
removal and the corresponding FAULTHANDLER_FD, FAULTHANDLER_FILE, and per-signum
table updates, including rollback paths, so concurrent calls cannot observe
mismatched ownership.
In `@pyre/pyre-object/src/tupleobject.rs`:
- Around line 200-216: Update the null-raw fallback around
try_gc_alloc_stable_raw and alloc_tuple_items_block_gc to root header.w_class
before the allocation call, then reload the rooted class pointer when
constructing W_TupleObject. Ensure the fallback uses the refreshed shadow-stack
value rather than the potentially stale local header.w_class.
🪄 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 Plus
Run ID: a032245b-1e5f-4037-9681-08b1c0f2ee75
📒 Files selected for processing (10)
majit/majit-gc/src/collector.rsmajit/majit-gc/src/shadow_stack.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/faulthandler/handler.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/tupleobject.rspyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (2)
- majit/majit-gc/src/collector.rs
- pyre/pyre-jit/src/eval.rs
| /// `handler.py:145` `self.fatal_error_w_file = w_file` / `handler.py:150` | ||
| /// `self.fatal_error_w_file = None`: the descriptor the handler writes to | ||
| /// belongs to this object, so it has to outlive the installed handlers rather | ||
| /// than be collected and have its finalizer close the fd under them. pyre has | ||
| /// no Handler instance and the signal callback is a bare `extern "C" fn` that | ||
| /// cannot capture one, so the owner is a process-global slot, walked as a GC | ||
| /// root by [`walk_fatal_error_file`]. | ||
| #[cfg(all(unix, feature = "host_env"))] | ||
| static FAULTHANDLER_FILE: std::sync::atomic::AtomicPtr<pyre_object::PyObject> = | ||
| std::sync::atomic::AtomicPtr::new(std::ptr::null_mut()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Stale intra-doc reference: walk_fatal_error_file doesn't exist.
The doc comment links to [walk_fatal_error_file], but the function actually defined below (line 79/100) is walk_faulthandler_roots.
✏️ Proposed fix
-/// than be collected and have its finalizer close the fd under them. pyre has
-/// no Handler instance and the signal callback is a bare `extern "C" fn` that
-/// cannot capture one, so the owner is a process-global slot, walked as a GC
-/// root by [`walk_fatal_error_file`].
+/// than be collected and have its finalizer close the fd under them. pyre has
+/// no Handler instance and the signal callback is a bare `extern "C" fn` that
+/// cannot capture one, so the owner is a process-global slot, walked as a GC
+/// root by [`walk_faulthandler_roots`].📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// `handler.py:145` `self.fatal_error_w_file = w_file` / `handler.py:150` | |
| /// `self.fatal_error_w_file = None`: the descriptor the handler writes to | |
| /// belongs to this object, so it has to outlive the installed handlers rather | |
| /// than be collected and have its finalizer close the fd under them. pyre has | |
| /// no Handler instance and the signal callback is a bare `extern "C" fn` that | |
| /// cannot capture one, so the owner is a process-global slot, walked as a GC | |
| /// root by [`walk_fatal_error_file`]. | |
| #[cfg(all(unix, feature = "host_env"))] | |
| static FAULTHANDLER_FILE: std::sync::atomic::AtomicPtr<pyre_object::PyObject> = | |
| std::sync::atomic::AtomicPtr::new(std::ptr::null_mut()); | |
| /// `handler.py:145` `self.fatal_error_w_file = w_file` / `handler.py:150` | |
| /// `self.fatal_error_w_file = None`: the descriptor the handler writes to | |
| /// belongs to this object, so it has to outlive the installed handlers rather | |
| /// than be collected and have its finalizer close the fd under them. pyre has | |
| /// no Handler instance and the signal callback is a bare `extern "C" fn` that | |
| /// cannot capture one, so the owner is a process-global slot, walked as a GC | |
| /// root by [`walk_faulthandler_roots`]. | |
| #[cfg(all(unix, feature = "host_env"))] | |
| static FAULTHANDLER_FILE: std::sync::atomic::AtomicPtr<pyre_object::PyObject> = | |
| std::sync::atomic::AtomicPtr::new(std::ptr::null_mut()); |
🤖 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/faulthandler/handler.rs` around lines 27 -
36, Update the intra-doc link in the FAULTHANDLER_FILE documentation to
reference the existing walk_faulthandler_roots function instead of the
nonexistent walk_fatal_error_file symbol.
| // `fileno` and `flush` both run Python; keep the file addressable across | ||
| // them so the caller receives the relocated pointer, not a stale one. | ||
| pyre_object::gc_roots::pin_root(resolved); | ||
| let file_slot = pyre_object::gc_roots::shadow_stack_len() - 1; | ||
| let method = crate::baseobjspace::getattr_str(resolved, "fileno")?; | ||
| let res = crate::call::call_function_impl_result(method, &[])?; | ||
| if !unsafe { pyre_object::is_int(res) } { | ||
| return Err(crate::PyError::type_error("fileno() returned non-integer")); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Stale pointer reused after pin_root — missing re-read via shadow_stack_get.
After pin_root(resolved), resolved is still the un-normalized value the caller passed in; pin_root only fixes up the shadow-stack slot in place (it does not return the normalized address). The established pattern elsewhere in this codebase (call_function_impl_result, FrameLocalsProxy::new) is to re-read the pinned value via shadow_stack_get before using it in a call that can trigger GC. Here, getattr_str(resolved, "fileno") uses the original resolved, not the normalized value — yet a few lines below (line 178), the same function correctly does let resolved = pyre_object::gc_roots::shadow_stack_get(file_slot); before the flush call. This inconsistency inside the same function indicates the first use was missed.
As pin_root's own doc explains, a foreign mutator can have moved the object between the caller obtaining resolved (e.g. via getattr_str(sys, "stderr")) and this pin_root call, leaving resolved a forwarding stub that must be normalized before use.
🐛 Proposed fix
pyre_object::gc_roots::pin_root(resolved);
let file_slot = pyre_object::gc_roots::shadow_stack_len() - 1;
- let method = crate::baseobjspace::getattr_str(resolved, "fileno")?;
+ let resolved = pyre_object::gc_roots::shadow_stack_get(file_slot);
+ let method = crate::baseobjspace::getattr_str(resolved, "fileno")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `fileno` and `flush` both run Python; keep the file addressable across | |
| // them so the caller receives the relocated pointer, not a stale one. | |
| pyre_object::gc_roots::pin_root(resolved); | |
| let file_slot = pyre_object::gc_roots::shadow_stack_len() - 1; | |
| let method = crate::baseobjspace::getattr_str(resolved, "fileno")?; | |
| let res = crate::call::call_function_impl_result(method, &[])?; | |
| if !unsafe { pyre_object::is_int(res) } { | |
| return Err(crate::PyError::type_error("fileno() returned non-integer")); | |
| // `fileno` and `flush` both run Python; keep the file addressable across | |
| // them so the caller receives the relocated pointer, not a stale one. | |
| pyre_object::gc_roots::pin_root(resolved); | |
| let file_slot = pyre_object::gc_roots::shadow_stack_len() - 1; | |
| let resolved = pyre_object::gc_roots::shadow_stack_get(file_slot); | |
| let method = crate::baseobjspace::getattr_str(resolved, "fileno")?; | |
| let res = crate::call::call_function_impl_result(method, &[])?; | |
| if !unsafe { pyre_object::is_int(res) } { | |
| return Err(crate::PyError::type_error("fileno() returned non-integer")); |
🤖 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/faulthandler/handler.rs` around lines 146 -
153, Re-read the normalized pinned object via
gc_roots::shadow_stack_get(file_slot) after pin_root and before calling
getattr_str for "fileno". Use this refreshed value instead of the original
resolved when obtaining and invoking the fileno method, matching the later flush
path.
| // Resolving the argument runs a user `fileno()` and `flush()`; | ||
| // keep those side effects behind the support gate, so a build | ||
| // that can only answer NotImplementedError does not run them | ||
| // first. | ||
| #[cfg(all(unix, feature = "host_env"))] | ||
| { | ||
| FAULTHANDLER_FD.store(fd, std::sync::atomic::Ordering::Relaxed); | ||
| let (fd, w_file) = faulthandler_get_fileno_and_file( | ||
| args.first().copied().unwrap_or(pyre_object::PY_NULL), | ||
| )?; | ||
| // `set_fatal_error_file` allocates its key, so the file has | ||
| // to stay addressable from here to the store below. | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let file_slot = (!w_file.is_null()).then(|| { | ||
| pyre_object::gc_roots::pin_root(w_file); | ||
| pyre_object::gc_roots::shadow_stack_len() - 1 | ||
| }); | ||
| // `pypy_faulthandler_enable(fileno, all_threads)` takes the | ||
| // descriptor as an argument, so the handler never observes | ||
| // a descriptor the install did not commit to. Split across | ||
| // two statements here, the new fd has to be visible before | ||
| // the handlers go in — a fatal signal in between would | ||
| // otherwise dump to the old one — and has to be rolled back | ||
| // when the install fails, or a failed re-enable would | ||
| // redirect the handlers already installed. | ||
| let previous_fd = | ||
| FAULTHANDLER_FD.swap(fd, std::sync::atomic::Ordering::Relaxed); | ||
| let ok = rustpython_host_env::faulthandler::enable_fatal_handlers( | ||
| faulthandler_signal_handler, | ||
| libc::SA_NODEFER | libc::SA_ONSTACK, | ||
| ); | ||
| if ok { | ||
| FAULTHANDLER_ENABLED.store(true, std::sync::atomic::Ordering::Relaxed); | ||
| // `handler.py:145` `self.fatal_error_w_file = w_file`. | ||
| set_fatal_error_file(file_slot.map_or( | ||
| pyre_object::PY_NULL, | ||
| pyre_object::gc_roots::shadow_stack_get, | ||
| )); | ||
| return Ok(pyre_object::w_none()); | ||
| } | ||
| FAULTHANDLER_FD.store(previous_fd, std::sync::atomic::Ordering::Relaxed); | ||
| return Err(crate::PyError::runtime_error( | ||
| "faulthandler.enable: sigaction failed", | ||
| )); | ||
| } | ||
| #[cfg(not(all(unix, feature = "host_env")))] | ||
| { | ||
| let _ = fd; | ||
| let _ = args; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any global interpreter lock / serialization primitive guarding
# builtin-function dispatch, which would make concurrent enable()/register()
# calls impossible in practice.
rg -n -C3 --type=rust '\bGIL\b|global_interpreter_lock|InterpLock' pyre majitRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== faulthandler outline =="
ast-grep outline pyre/pyre-interpreter/src/module/faulthandler/handler.rs || true
echo "== relevant faulthandler lines =="
cat -n pyre/pyre-interpreter/src/module/faulthandler/handler.rs | sed -n '1,440p'
echo "== globals definitions =="
rg -n 'FAULTHANDLER_(FD|ENABLED|FILE)|register_user_signal|set_user_signal_file|enable_fatal_handlers' pyre/pyre-interpreter/src/module/faulthandlerRepository: youknowone/pyre
Length of output: 27492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== global lock / interpreter run guards =="
rg -n --type=rust -C4 '\bInterp(?:Lock|Guard)\b|\bInterpreter(?:Lock|Guard)\b|\bGIL\b|with_lock|lock\(|mutex\(|Mutex|RwLock|SingleThread|single.*thread|freethread|FreeThreaded' pyre/pyre-interpreter src 2>/dev/null | head -n 300
echo "== module dispatch / builtin maker =="
rg -n --type=rust -C5 'fn make_builtin_function_with|pub fn module_ns_store|call_function_impl_result|PyObjectRef' pyre/pyre-interpreter/src | head -n 350
echo "== faulthandler references =="
rg -n -C2 '\bfaulthandler\b|enable_fau|register_user_signal|unregister_user_signal' pyre majit --type=rust --type=py --type=text | head -n 240Repository: youknowone/pyre
Length of output: 24578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate builtin/function wrappers =="
rg -n --type=rust -C4 'make_builtin_function|make_builtin_function_with|fn call_function_impl_result|struct builtin|BuiltinFunction' pyre/pyre-interpreter/src | head -n 260
echo "== exact builtin symbol references =="
python3 - <<'PY'
from pathlib import Path
for p in Path('pyre/pyre-interpreter/src').rglob('*.rs'):
s=p.read_text(errors='replace')
if 'fn make_builtin_function' in s or 'pub fn make_builtin_function' in s or 'call_function_impl_result' in s:
print(f'--- {p}')
for i,line in enumerate(s.splitlines(),1):
if 'make_builtin_function' in line or 'call_function_impl_result' in line:
print(f'{i}: {line}')
PYRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== gateway.rs make_builtin definitions =="
cat -n pyre/pyre-interpreter/src/gateway.rs | sed -n '1000,1155p'
echo "== call.rs call_function_impl_result definition and thread docs =="
cat -n pyre/pyre-interpreter/src/call.rs | sed -n '1,90p;200,330p;380,410p'
echo "== module storage ownership semantics =="
cat -n pyre/pyre-interpreter/src/gateway.rs | sed -n '1295,1340p'
rg -n -C3 'fn module_ns_store|FAULTHANDLER_REGISTERED|register_module' pyre/pyre-interpreter/src pyreRepository: youknowone/pyre
Length of output: 50373
Guard the faulthandler fd/file ownership updates with a local mutex.
There is no process-wide interpreter lock serializing these builtin calls, and enable/register update shared globals across separate stores: enable swaps FAULTHANDLER_FD before installing/clearing FAULTHANDLER_FILE; register stores the file through the shared per-signum table only after register_user_signal; unregister can clear an entry while another thread has already installed for that signal. A per-faulthandler mutex around the fd-install and file-statement sequences prevents mismatched ownership across concurrent calls.
🤖 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/faulthandler/handler.rs` around lines 192 -
238, Add a process-wide faulthandler mutex and acquire its guard across the
complete fd/file update sequences in the enable, register, and unregister
implementations. Hold the guard through handler installation or removal and the
corresponding FAULTHANDLER_FD, FAULTHANDLER_FILE, and per-signum table updates,
including rollback paths, so concurrent calls cannot observe mismatched
ownership.
| let raw_slot = if raw.is_null() { | ||
| None | ||
| } else { | ||
| unsafe { | ||
| std::ptr::write( | ||
| raw as *mut W_TupleObject, | ||
| W_TupleObject { | ||
| ob_header: PyObject { | ||
| ob_type: header.ob_type, | ||
| w_class: header.w_class, | ||
| }, | ||
| wrappeditems: std::ptr::null_mut(), | ||
| }, | ||
| ); | ||
| } | ||
| crate::gc_roots::pin_root(raw as PyObjectRef); | ||
| Some(crate::gc_roots::shadow_stack_len() - 1) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Root header.w_class on the raw-allocation fallback.
When try_gc_alloc_stable_raw returns null, header.w_class remains an unrooted local across alloc_tuple_items_block_gc, which may move GC objects. Reload that class pointer from a shadow-stack root before constructing the fallback Box<W_TupleObject>; otherwise the fallback can retain a stale GC address.
🤖 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-object/src/tupleobject.rs` around lines 200 - 216, Update the
null-raw fallback around try_gc_alloc_stable_raw and alloc_tuple_items_block_gc
to root header.w_class before the allocation call, then reload the rooted class
pointer when constructing W_TupleObject. Ensure the fallback uses the refreshed
shadow-stack value rather than the potentially stale local header.w_class.
Follow-up to #887, acting on the automated review findings that landed on it after the merge.
Removed
CriticalCodeGuardin thebound_reachedcompile path (pyre-jit/src/eval.rs). It spanned the whole trace-compile-and-dispatch region, soreport_error = 0covered the walk's residual calls into user code —stack.c:111returns that flag in place of the overflow verdict, turning aRecursionErrorinto an unreported native overflow. Upstream marks only regions that run no user code (compile.py:976,pyjitpl.py:3305,resume.py:1318). Measured on the fixture it was added for,synth/wasm_ca_trampoline_decline: with the guard suppressed the overflow no longer occurs at all, so the region is inert.type_id == 9backtrace capture inremember_young_pointer.GC correctness
w_tuple_new_array_backedpublished an uninitialized tuple.try_gc_alloc_stable_rawreturns uninitialized payload andtuple_object_custom_tracereadsob_header.w_classandwrappeditems, so a collection betweenpin_rootand the items-block allocation read garbage. The header now goes in first with a null items block — the state the trace hook returns early on — and the block is stored after.W_TextIOWrapperpublished its reference fields with one barrier at the end ofattach_buffer, but everyw_str_newin between allocates andset_newline/getattr_str/set_encoder_decoderrun Python. Barrier after each reference store instead.OwnerRootGuardwas auto-Send. Itsindexnames a slot in the acquiring thread'sOWNER_ROOTS; aPhantomData<*mut ()>pins it to that thread at compile time.Error handling
attach_stdio_codecdropped both the codec lookup and the encoder/decoder construction errors, leaving a stream that reports itself unreadable however readable its buffer is.init_stream_codecsandinit_importlib_bootstrapnow propagate; a bootstrap failure still wins as the more fundamental one.allocate_stdiostill cannot propagate — it runs insidesysmodule creation — but no longer discards the result blind: it finishes the two fields the early return skipped instead of forcingSTATE_OKover a half-applied init.faulthandler.enablerestoresFAULTHANDLER_FDwhenenable_fatal_handlersfails; a failed re-enable was redirecting the handlers already installed.faulthandler file ownership
enableresolvedfile=Noneto a hard-coded fd 2 and dropped the file object.get_fileno_and_file(handler.py:35-49) resolves the currentsys.stderr, asks it forfileno(), and flushes it ignoring an ordinary flush error;enablethen parks it asfatal_error_w_file(:145) anddisableclears it (:150), because the descriptor the installed handler writes to belongs to that object. pyre has no Handler instance, so the owner is a process-global slot walked as a GC root alongside the other interpreter globals — not a module attribute, which would show up indir(faulthandler)where upstream has none.Performance
acquire_owner_rootscannedOWNER_ROOTSfrom 0 for a free slot, so rooting the frames of a recursiondepthdeep cost O(depth²). Released interior slots now go on a free stack; a released tail still shrinks the walked range.Documentation
Recorded why
fbw_inline_recursion_countmatchingw_codealone is the element-wise greenkey compare ofpyjitpl.py:1396-1401:MIFrame.setup(pyjitpl.py:74-80) assignsgreenkeyonce, so a frame's greenkey stays its entry greens, and everyInlineFrameis pushed at a callee entry with the(w_code, 0)identityreconstruct_inline_recipealso stamps. Codex had flagged this as a mismatch; it is not one.Verification
cargo test: pyre-interpreter 432 passed, majit-gc 215 passed.The one cranelift failure,
synth/mutate_then_raise_caught, panics incollector.rs(minor_custom_trace_target, invalid type_id). It reproduces 3/3 on a rebuild ofmainwith none of these changes applied, and passes on the Linux CI runner — it is a pre-existing macOS-only failure, not introduced here.Summary by CodeRabbit
Bug Fixes
Reliability