Repository navigation
jit: exact-resume FOR_ITER gate widening and JIT traceback fidelity - #563
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
WalkthroughThe PR updates JIT frame-slot translation, FOR_ITER eligibility and resume recovery, operand-stack mirroring, blackhole exception traceback handling, GC rooting, container hash validation, class namespace relocation, and checker interpreter selection. ChangesJIT execution and resume handling
Blackhole exception propagation
Interpreter runtime safety
Interpreter selection
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 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: 2a593d5c90
ℹ️ 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".
| | I::StoreAttr { .. } | ||
| | I::StoreName { .. } | ||
| | I::StoreGlobal { .. } | ||
| | I::StoreDeref { .. } |
There was a problem hiding this comment.
Allowing STORE_DEREF admits loop bodies in frames with closure/freevar cells, but the forward-resume path still restores the outer CALL result at locals_cells_stack_w[nlocals] and sets valuestackdepth = nlocals + 1 in flush_walk_end_state_after_outer_call, rather than using nlocals + ncells. For a nonlocal loop body where an inlined call aborts before the STORE_DEREF (or before the tail), this overwrites the first cell/freevar slot instead of the operand stack slot, corrupting the resumed frame; keep these shapes interpreted or make the flush path use the physical stack base.
Useful? React with 👍 / 👎.
| let caught_reraise = exc_value != 0 && last_caught_exception_value == exc_value; | ||
| if caught_reraise { | ||
| err.attach_tb = false; |
There was a problem hiding this comment.
This treats any exception object that was caught in the blackhole frame as a bare RERAISE, but an except ... as e: raise e path raises the same object through handler_raise, not handler_reraise. In that case Python should append the current raise site to the traceback; setting attach_tb = false skips that recording when the frame exits, so JIT blackhole exits lose the frame for explicit re-raises of the caught object. Track whether the raising opcode was actually reraise/ instead of using object identity.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit c99871b). 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)None. 4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a5f92bae8
ℹ️ 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 Some(merge_point) = crate::jitcode_runtime::decoded_ops(code) | ||
| .find(|op| op.opname == "jit_merge_point") | ||
| .map(|op| op.pc) |
There was a problem hiding this comment.
When a function has sibling hot loops, full_body_walk_trace is called with the selected loop's start_pc, but this helper now anchors the scan to the first jit_merge_point in the whole JitCode. For a later loop, back_edge_end is computed from the first loop and an abort_permanent inside the later loop lies beyond scan_end, so the defensive decline is skipped and the authoritative walker can hit the in-loop abort corruption this guard is meant to avoid. Use the merge point corresponding to start_pc, not the first one in the function.
Useful? React with 👍 / 👎.
| ( | ||
| bank, | ||
| color, | ||
| frame_layout_slot(slot, nlocals, stack_base_absolute), |
There was a problem hiding this comment.
Publishing pcdep_color_slots with operand-stack slots shifted by ncells breaks bridge and inline-resume paths that still build compact [locals | operand stack] mirrors. For example, setup_bridge_sym bounds its mirror by nlocals + stack_depth, and reconstruct_inline_recipe sizes registers_r the same way; in a frame with a pure cell/freevar, a live stack slot is now published as stack_base_absolute + depth, so it is dropped as out-of-prefix and guard-fail resumes lose kept operand values. Either keep this metadata compact for those consumers or update their indexing/bounds to physical frame slots.
Useful? React with 👍 / 👎.
| let frame_ptr = bh.virtualizable_ptr as *mut PyFrame; | ||
| let jitcode_index = bh.jitcode.try_index().map(|v| v as i32); | ||
| let last_opcode_position = bh.last_opcode_position; |
There was a problem hiding this comment.
In a multi-frame blackhole chain where the exception has no handler in the current frame, this captures the frame info but then releases the frame and moves to the caller; the new traceback recording only runs in the bottommost None arm. An exception raised in an inlined/blackholed callee and escaping through compiled callers therefore records only the outermost frame, omitting the callee frames that Python tracebacks must include. Record the current frame before the Some(caller) propagation as well.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit-trace/src/jitcode_dispatch.rs (1)
4717-4768: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the physical stack base here.
STORE_DEREFandMAKE_CELLalso write throughsetarrayitem_vable_r, but they target the cell/freevar region inlocals_cells_stack_w, not an operand-stack push. Comparing against rawnlocalscan still fold those closure writes intoctx.vstack_last_ref/ctx.vstack_boxes; gate this on the stack base (nlocals + ncells) instead.🤖 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-jit-trace/src/jitcode_dispatch.rs` around lines 4717 - 4768, Update the vstack write handling around ctx.vstack_last_ref and ctx.vstack_boxes to use the physical operand-stack base, nlocals + ncells, rather than raw nlocals. Ensure STORE_DEREF and MAKE_CELL writes in the cell/freevar region are excluded, while retaining the existing method-form LOAD_ATTR mirroring and last-reference behavior for true operand-stack writes.
🤖 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/check.py`:
- Around line 23-30: The PYTHON3 selection can accept an older or shimmed
interpreter, causing version-sensitive baseline and stdlib mismatches. Update
the candidate selection around PYRE_CHECK_PYTHON3 and PYTHON3 to validate each
executable by probing sys.implementation.name and sys.version_info, accepting
only CPython 3.14; apply the same requirement to the environment override or
reject it when invalid, while preserving the existing fallback behavior.
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 3222-3233: Update the class-body execution flow around
run_with_jit() and the earlier __prepare__ call to explicitly pin every
PyObjectRef that remains live afterward, including w_namespace, bases,
w_metaclass, extra_kwargs, and w_orig_bases. Re-read each reference from its
shadow-stack/frame slot after the allocating call and before later uses, while
preserving the existing body_ns refresh and mapping_namespace handling.
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 2289-2330: Update loop_body_has_abort_permanent to select the
jit_merge_point associated with start_pc rather than the first decoded merge
point. Use start_pc to identify the correct loop header and then restrict
back-edge and abort_permanent scanning to that loop, preserving the existing
exception-table and full-scan behavior.
In `@pyre/pyre-jit/src/call_jit.rs`:
- Around line 588-590: The last_instruction calculations in
pyre/pyre-jit/src/call_jit.rs at lines 588-590, 1948-1955, and 2080-2087 eagerly
dereference frame_ptr through map_or; replace map_or with map_or_else at all
three sites so the unsafe (*frame_ptr).last_instr read occurs only for None,
while preserving i64::from for Some values.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 4717-4768: Update the vstack write handling around
ctx.vstack_last_ref and ctx.vstack_boxes to use the physical operand-stack base,
nlocals + ncells, rather than raw nlocals. Ensure STORE_DEREF and MAKE_CELL
writes in the cell/freevar region are excluded, while retaining the existing
method-form LOAD_ATTR mirroring and last-reference behavior for true
operand-stack writes.
🪄 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: 1091b3cf-6272-4254-8217-27ee8d44f7e9
📒 Files selected for processing (11)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/blackhole.rspyre/check.pypyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
| # pyre's compat target is CPython 3.14; its native modules (`_sre.MAGIC`) and the | ||
| # vendored `lib-python/3` are coupled to that version. Prefer a version-matched | ||
| # oracle so a stale `python3` on PATH (an older system CPython) does not diverge | ||
| # from pypy on version-sensitive error text and trip a spurious cpython-vs-pypy | ||
| # baseline mismatch. | ||
| PYTHON3 = os.environ.get("PYRE_CHECK_PYTHON3") or next( | ||
| (cand for cand in ("python3.14", "python3", "python") if shutil.which(cand)), | ||
| "python3", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
for candidate in python3.14 python3 python; do
path="$(command -v "$candidate" 2>/dev/null || true)"
if [ -n "$path" ]; then
"$path" -c 'import sys; print(sys.executable, sys.implementation.name, sys.version_info[:3])'
fi
doneRepository: youknowone/pyre
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers.
sed -n '1,120p' pyre/check.py | cat -n
# Find where PYTHON3 is used.
rg -n "\bPYTHON3\b|PYRE_CHECK_PYTHON3|python3\.14|version-sensitive|compat target" pyre/check.py pyre -g '!**/__pycache__/**'Repository: youknowone/pyre
Length of output: 7599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact uses of PYTHON3/PYPY3 and any version checks nearby.
sed -n '730,1045p' pyre/check.py | cat -n
# Look for any explicit CPython/version validation in the repo.
rg -n "sys\.implementation|version_info|CPython 3\.14|3\.14|PYRE_CHECK_PYTHON3|PYRE_CHECK_PYPY3" pyre -g '!**/__pycache__/**'Repository: youknowone/pyre
Length of output: 35967
Pin the CPython 3.14 oracle more tightly
shutil.which() still allows an older python3 or shim to be selected here, and that path feeds both the baseline comparisons and stdlib lookup. Probe the candidate with sys.implementation.name and sys.version_info before accepting it, or require PYRE_CHECK_PYTHON3 to point at a known 3.14 interpreter.
🤖 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/check.py` around lines 23 - 30, The PYTHON3 selection can accept an
older or shimmed interpreter, causing version-sensitive baseline and stdlib
mismatches. Update the candidate selection around PYRE_CHECK_PYTHON3 and PYTHON3
to validate each executable by probing sys.implementation.name and
sys.version_info, accepting only CPython 3.14; apply the same requirement to the
environment override or reject it when invalid, while preserving the existing
fallback behavior.
| // A minor collection during the body (a hot class-level loop compiles | ||
| // and runs through the JIT portal) can promote the namespace young -> | ||
| // old, relocating it. The frame's `debugdata.w_locals` slot and the | ||
| // shadow-stack pin are forwarded, keeping the object alive at its new | ||
| // address, but this stack-local `body_ns` copy is not — it is left | ||
| // pointing at the freed nursery slot. Re-read the forwarded object | ||
| // from the frame before any downstream use; a stale read would | ||
| // dereference reclaimed memory (`resolve_dict_backing` / `w_dict_items` | ||
| // below, then `__set_name__`). | ||
| let body_ns = frame.get_w_locals(); | ||
| let mapping_namespace = mapping_namespace.map(|_| body_ns); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Pin and re-read all live object references to prevent use-after-free.
While this change correctly re-reads body_ns from the frame to survive GC relocation during run_with_jit(), several other local PyObjectRef variables remain unrooted and are not updated. Specifically, w_namespace, bases, w_metaclass, extra_kwargs, and w_orig_bases are held by value and used after frame.run_with_jit() (e.g., w_namespace at line 3397, bases at line 3366).
Because these variables are not explicitly pinned and re-read from the GC shadow stack, a minor collection during the JIT portal execution will leave them pointing to reclaimed nursery slots, causing memory corruption when later accessed.
All PyObjectRef state that lives across run_with_jit() (and other prior allocating calls like the __prepare__ invocation) must be explicitly pinned to the shadow stack and re-read before subsequent use.
🤖 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/call.rs` around lines 3222 - 3233, Update the
class-body execution flow around run_with_jit() and the earlier __prepare__ call
to explicitly pin every PyObjectRef that remains live afterward, including
w_namespace, bases, w_metaclass, extra_kwargs, and w_orig_bases. Re-read each
reference from its shadow-stack/frame slot after the allocating call and before
later uses, while preserving the existing body_ns refresh and mapping_namespace
handling.
| fn loop_body_has_abort_permanent(w_code: *const (), start_pc: usize) -> bool { | ||
| let Some(pjc) = crate::state::pyjitcode_for_code(w_code) else { | ||
| return false; | ||
| }; | ||
| let code = pjc.jitcode.code.as_slice(); | ||
| let mut seen_merge_point = false; | ||
| for op in crate::jitcode_runtime::decoded_ops(code) { | ||
| if op.opname == "jit_merge_point" { | ||
| seen_merge_point = true; | ||
| } else if seen_merge_point && op.opname == "abort_permanent" { | ||
| return true; | ||
| let Some(merge_point) = crate::jitcode_runtime::decoded_ops(code) | ||
| .find(|op| op.opname == "jit_merge_point") | ||
| .map(|op| op.pc) | ||
| else { | ||
| return false; | ||
| }; | ||
|
|
||
| let mut back_edge_end: Option<usize> = None; | ||
| let mut first_abort_permanent = None; | ||
| for op in crate::jitcode_runtime::decoded_ops(code).filter(|op| op.pc > merge_point) { | ||
| if op.opname == "abort_permanent" && first_abort_permanent.is_none() { | ||
| first_abort_permanent = Some(op.pc); | ||
| } | ||
| if op.opname.starts_with("goto") && op.argcodes.ends_with('L') { | ||
| let target = u16::from_le_bytes([code[op.next_pc - 2], code[op.next_pc - 1]]) as usize; | ||
| if target <= merge_point { | ||
| back_edge_end = Some(back_edge_end.map_or(op.pc, |end| end.max(op.pc))); | ||
| } | ||
| } | ||
| } | ||
| false | ||
|
|
||
| // `start_pc` is a code-unit index; the exception table lookup takes byte offsets (×2). | ||
| let loop_in_try = unsafe { | ||
| pyre_interpreter::pycode::w_code_lookup_exceptiontable( | ||
| w_code as pyre_object::PyObjectRef, | ||
| (start_pc as u32) * 2, | ||
| ) | ||
| } | ||
| .is_some(); | ||
|
|
||
| let scan_end = if loop_in_try || std::env::var_os("PYRE_FBW_LOOPBODY_SCAN_FULL").is_some() { | ||
| code.len() | ||
| } else { | ||
| back_edge_end.unwrap_or(code.len()) | ||
| }; | ||
| first_abort_permanent.is_some_and(|pc| pc < scan_end) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'fn find_loop_header_pcs' -A 25 pyre/pyre-jit/src/jit/codewriter.rs
rg -n 'jit_merge_point' pyre/pyre-jit/src/jit/codewriter.rs
rg -n 'fn resume_jitcode_pc_for' -A 15 pyre/pyre-jit-trace/src/state.rsRepository: youknowone/pyre
Length of output: 3584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper in trace.rs and nearby context.
sed -n '2260,2355p' pyre/pyre-jit-trace/src/trace.rs | cat -n
# Inspect resume_jitcode_pc_for and surrounding state handling.
sed -n '1,260p' pyre/pyre-jit-trace/src/state.rs | cat -n
# Inspect loop-header emission context in codewriter.
sed -n '7390,7565p' pyre/pyre-jit/src/jit/codewriter.rs | cat -nRepository: youknowone/pyre
Length of output: 32295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for call sites / uses of loop_body_has_abort_permanent and start_pc.
rg -n 'loop_body_has_abort_permanent|resume_jitcode_pc_for\(|start_pc' pyre/pyre-jit-trace/src pyre/pyre-jit/src | sed -n '1,220p'
# Search for the surrounding doc comment about corruption scenario.
rg -n 'corruption scenario|abort_permanent|loop body' pyre/pyre-jit-trace/src/trace.rs -A 10 -B 10Repository: youknowone/pyre
Length of output: 31662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every call site and inspect the surrounding logic.
rg -n 'loop_body_has_abort_permanent\(' pyre/pyre-jit-trace/src/trace.rs -A 20 -B 20
# Inspect the helper definition with line numbers in context.
sed -n '2280,2345p' pyre/pyre-jit-trace/src/trace.rs | cat -n
# Inspect any nearby doc comments that describe the intended loop-body semantics.
sed -n '2220,2289p' pyre/pyre-jit-trace/src/trace.rs | cat -nRepository: youknowone/pyre
Length of output: 12674
Resolve the merge point from start_pc
decoded_ops(...).find(|op| op.opname == "jit_merge_point") always picks the first loop header in the jitcode. In a code object with multiple loop headers, that can scan the wrong back-edge and miss an abort_permanent in the loop named by start_pc.
🤖 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-jit-trace/src/trace.rs` around lines 2289 - 2330, Update
loop_body_has_abort_permanent to select the jit_merge_point associated with
start_pc rather than the first decoded merge point. Use start_pc to identify the
correct loop header and then restrict back-edge and abort_permanent scanning to
that loop, preserving the existing exception-table and full-scan behavior.
| let last_instruction = | ||
| pyre_jit_trace::state::python_pc_for_jitcode_pc_public(jitcode_index, opcode_position) | ||
| .map_or(unsafe { (*frame_ptr).last_instr as i64 }, i64::from); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Defer raw pointer dereference using map_or_else.
The map_or method evaluates its default argument eagerly, causing an unnecessary memory read of (*frame_ptr).last_instr even when the Option resolves to Some. Use map_or_else to defer the unsafe memory read until it is actually needed.
pyre/pyre-jit/src/call_jit.rs#L588-L590: replacemap_orwithmap_or_else(|| unsafe { (*frame_ptr).last_instr as i64 }, i64::from)pyre/pyre-jit/src/call_jit.rs#L1948-L1955: replacemap_orwithmap_or_else(|| unsafe { (*frame_ptr).last_instr as i64 }, i64::from)pyre/pyre-jit/src/call_jit.rs#L2080-L2087: replacemap_orwithmap_or_else(|| unsafe { (*frame_ptr).last_instr as i64 }, i64::from)
📍 Affects 1 file
pyre/pyre-jit/src/call_jit.rs#L588-L590(this comment)pyre/pyre-jit/src/call_jit.rs#L1948-L1955pyre/pyre-jit/src/call_jit.rs#L2080-L2087
🤖 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-jit/src/call_jit.rs` around lines 588 - 590, The last_instruction
calculations in pyre/pyre-jit/src/call_jit.rs at lines 588-590, 1948-1955, and
2080-2087 eagerly dereference frame_ptr through map_or; replace map_or with
map_or_else at all three sites so the unsafe (*frame_ptr).last_instr read occurs
only for None, while preserving i64::from for Some values.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/cd2c480cebda04c5d7fe5e00fcdf8e01402f443d/pyre-interpreter/src/opcode_ops.rs#L356
When this path is reached from the interpreter, map_add has already popped both key and value off the Python frame before calling this helper, so this new try_hash_value(key) can run a user __hash__ and trigger a moving GC while those objects are only raw Rust locals. If either object moves, the following w_dict_store(dict, key, value) uses stale pointers. Pin dict/key/value across the hash call, or keep the operands rooted until after the store.
https://github.com/youknowone/pyre/blob/cd2c480cebda04c5d7fe5e00fcdf8e01402f443d/pyre-interpreter/src/opcode_ops.rs#L328
For SET_UPDATE over an arbitrary iterable, collect_iterable roots yielded objects only until it returns the Vec; this new per-item try_hash_value can then execute Python and trigger GC while the current item and the remaining Vec entries are unrooted raw pointers. A generator yielding young user objects whose __hash__ allocates can leave w_set_add or later iterations using stale references. Keep the shadow-stack roots alive through the hash/add loop or pin each item before hashing it.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d58c7c0a3d
ℹ️ 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".
| key: PyObjectRef, | ||
| value: PyObjectRef, | ||
| ) -> Result<(), PyError> { | ||
| crate::builtins::try_hash_value(key)?; |
There was a problem hiding this comment.
With a user-defined key in a MAP_ADD path, this preflight calls __hash__, and the following w_dict_store computes object_key_for(key), invoking the same hash hook a second time through the infallible store path. That makes dict displays/comprehensions run side-effecting or stateful __hash__ methods twice (and a second hash error is swallowed by the raw store) instead of once as in PyPy/CPython; use w_dict_store_checked or an equivalent single checked insertion, and apply the same pattern to the other new pre-hash-before-raw-store sites.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pyre/check.py (1)
23-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate that the selected executable is actually CPython 3.14.
shutil.which()only proves that a command exists;python3/pythonmay resolve to an older CPython, PyPy, or a shim, andPYRE_CHECK_PYTHON3bypasses validation entirely. That can still produce version-mismatched baseline comparisons and stdlib discovery. Probe candidates withsys.implementation.nameandsys.version_infobefore accepting them, or fail clearly when the override is not CPython 3.14.#!/bin/bash set -euo pipefail for candidate in "${PYRE_CHECK_PYTHON3:-}" python3.14 python3 python; do [ -n "$candidate" ] || continue path="$(command -v "$candidate" 2>/dev/null || true)" [ -n "$path" ] || continue "$path" -c 'import sys; print(sys.executable, sys.implementation.name, sys.version_info[:3])' done🤖 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/check.py` around lines 23 - 30, Validate the executable selected by PYRE_CHECK_PYTHON3 or the fallback candidates before assigning PYTHON3: probe each candidate using sys.implementation.name and sys.version_info, accept only CPython 3.14, and reject an invalid explicit override with a clear failure. Continue searching fallback candidates when an unqualified candidate is unavailable or mismatched, and fail clearly if none qualify.pyre/pyre-interpreter/src/call.rs (1)
3222-3233: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPin and re-read all live object references to prevent use-after-free.
While this change correctly re-reads
body_nsfrom the frame to survive GC relocation duringrun_with_jit(), several other localPyObjectRefvariables remain unrooted and are not updated. Specifically,w_namespace,bases,w_metaclass,extra_kwargs, andw_orig_basesare held by value and used afterframe.run_with_jit()(e.g.,w_namespaceat line 3397,basesat line 3366).Because these variables are not explicitly pinned and re-read from the GC shadow stack, a minor collection during the JIT portal execution will leave them pointing to reclaimed nursery slots, causing memory corruption when later accessed.
All
PyObjectRefstate that lives acrossrun_with_jit()(and other prior allocating calls like the__prepare__invocation) must be explicitly pinned to the shadow stack and re-read before subsequent use.🤖 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/call.rs` around lines 3222 - 3233, Update the class-creation flow surrounding frame.run_with_jit() and the earlier __prepare__ call to explicitly pin every live PyObjectRef that survives those allocations, including w_namespace, bases, w_metaclass, extra_kwargs, and w_orig_bases. Re-read each reference from its shadow-stack/frame slot before later use, alongside the existing body_ns refresh, preserving the current behavior while preventing stale relocated-object references.
🤖 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-backend-cranelift/src/compiler.rs`:
- Around line 24740-24806: Add the #[ignore] attribute to both
guard_eval_breaker_deopts_when_bitmask_set and
guard_eval_breaker_cross_trace_inherits_bitmask, which mutate the process-global
eval_breaker_word address override. Leave their test logic unchanged and mark
them for explicit serial execution.
In `@pyre/pyre-interpreter/src/opcode_ops.rs`:
- Around line 349-356: Prevent moving-GC use-after-free around every
try_hash_value call by pinning referenced PyObjectRef locals and each unrooted
vector element on the shadow stack, then re-reading them with shadow_stack_get
before subsequent stores or uses. Apply this in
pyre/pyre-interpreter/src/opcode_ops.rs lines 349-356 (map_add_value), 310-310,
and 322-328; pyre/pyre-interpreter/src/builtins.rs lines 5748-5748 and
5766-5766; and pyre/pyre-interpreter/src/typedef.rs lines 2057-2063, 2129-2134,
3493-3499, 13895-13901, 13987-13993, and 14067-14073, preserving the existing
w_dict_store and w_set_add behavior.
---
Duplicate comments:
In `@pyre/check.py`:
- Around line 23-30: Validate the executable selected by PYRE_CHECK_PYTHON3 or
the fallback candidates before assigning PYTHON3: probe each candidate using
sys.implementation.name and sys.version_info, accept only CPython 3.14, and
reject an invalid explicit override with a clear failure. Continue searching
fallback candidates when an unqualified candidate is unavailable or mismatched,
and fail clearly if none qualify.
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 3222-3233: Update the class-creation flow surrounding
frame.run_with_jit() and the earlier __prepare__ call to explicitly pin every
live PyObjectRef that survives those allocations, including w_namespace, bases,
w_metaclass, extra_kwargs, and w_orig_bases. Re-read each reference from its
shadow-stack/frame slot before later use, alongside the existing body_ns
refresh, preserving the current behavior while preventing stale relocated-object
references.
🪄 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: a12676fe-141a-4daf-948e-bc39ebac628d
📒 Files selected for processing (13)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/blackhole.rspyre/check.pypyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pyre/check.py (1)
23-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate that the selected executable is actually CPython 3.14.
shutil.which()only proves that a command exists;python3/pythonmay resolve to an older CPython, PyPy, or a shim, andPYRE_CHECK_PYTHON3bypasses validation entirely. That can still produce version-mismatched baseline comparisons and stdlib discovery. Probe candidates withsys.implementation.nameandsys.version_infobefore accepting them, or fail clearly when the override is not CPython 3.14.#!/bin/bash set -euo pipefail for candidate in "${PYRE_CHECK_PYTHON3:-}" python3.14 python3 python; do [ -n "$candidate" ] || continue path="$(command -v "$candidate" 2>/dev/null || true)" [ -n "$path" ] || continue "$path" -c 'import sys; print(sys.executable, sys.implementation.name, sys.version_info[:3])' done🤖 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/check.py` around lines 23 - 30, Validate the executable selected by PYRE_CHECK_PYTHON3 or the fallback candidates before assigning PYTHON3: probe each candidate using sys.implementation.name and sys.version_info, accept only CPython 3.14, and reject an invalid explicit override with a clear failure. Continue searching fallback candidates when an unqualified candidate is unavailable or mismatched, and fail clearly if none qualify.pyre/pyre-interpreter/src/call.rs (1)
3222-3233: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPin and re-read all live object references to prevent use-after-free.
While this change correctly re-reads
body_nsfrom the frame to survive GC relocation duringrun_with_jit(), several other localPyObjectRefvariables remain unrooted and are not updated. Specifically,w_namespace,bases,w_metaclass,extra_kwargs, andw_orig_basesare held by value and used afterframe.run_with_jit()(e.g.,w_namespaceat line 3397,basesat line 3366).Because these variables are not explicitly pinned and re-read from the GC shadow stack, a minor collection during the JIT portal execution will leave them pointing to reclaimed nursery slots, causing memory corruption when later accessed.
All
PyObjectRefstate that lives acrossrun_with_jit()(and other prior allocating calls like the__prepare__invocation) must be explicitly pinned to the shadow stack and re-read before subsequent use.🤖 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/call.rs` around lines 3222 - 3233, Update the class-creation flow surrounding frame.run_with_jit() and the earlier __prepare__ call to explicitly pin every live PyObjectRef that survives those allocations, including w_namespace, bases, w_metaclass, extra_kwargs, and w_orig_bases. Re-read each reference from its shadow-stack/frame slot before later use, alongside the existing body_ns refresh, preserving the current behavior while preventing stale relocated-object references.
🤖 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-backend-cranelift/src/compiler.rs`:
- Around line 24740-24806: Add the #[ignore] attribute to both
guard_eval_breaker_deopts_when_bitmask_set and
guard_eval_breaker_cross_trace_inherits_bitmask, which mutate the process-global
eval_breaker_word address override. Leave their test logic unchanged and mark
them for explicit serial execution.
In `@pyre/pyre-interpreter/src/opcode_ops.rs`:
- Around line 349-356: Prevent moving-GC use-after-free around every
try_hash_value call by pinning referenced PyObjectRef locals and each unrooted
vector element on the shadow stack, then re-reading them with shadow_stack_get
before subsequent stores or uses. Apply this in
pyre/pyre-interpreter/src/opcode_ops.rs lines 349-356 (map_add_value), 310-310,
and 322-328; pyre/pyre-interpreter/src/builtins.rs lines 5748-5748 and
5766-5766; and pyre/pyre-interpreter/src/typedef.rs lines 2057-2063, 2129-2134,
3493-3499, 13895-13901, 13987-13993, and 14067-14073, preserving the existing
w_dict_store and w_set_add behavior.
---
Duplicate comments:
In `@pyre/check.py`:
- Around line 23-30: Validate the executable selected by PYRE_CHECK_PYTHON3 or
the fallback candidates before assigning PYTHON3: probe each candidate using
sys.implementation.name and sys.version_info, accept only CPython 3.14, and
reject an invalid explicit override with a clear failure. Continue searching
fallback candidates when an unqualified candidate is unavailable or mismatched,
and fail clearly if none qualify.
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 3222-3233: Update the class-creation flow surrounding
frame.run_with_jit() and the earlier __prepare__ call to explicitly pin every
live PyObjectRef that survives those allocations, including w_namespace, bases,
w_metaclass, extra_kwargs, and w_orig_bases. Re-read each reference from its
shadow-stack/frame slot before later use, alongside the existing body_ns
refresh, preserving the current behavior while preventing stale relocated-object
references.
🪄 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: a12676fe-141a-4daf-948e-bc39ebac628d
📒 Files selected for processing (13)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-metainterp/src/blackhole.rspyre/check.pypyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-jit/src/jit/codewriter.rs
🛑 Comments failed to post (2)
majit/majit-backend-cranelift/src/compiler.rs (1)
24740-24806: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add
#[ignore]to tests mutating process-global state to prevent CI flakiness and SIGSEGVs.These tests override the global
eval_breaker_wordaddress to point to a stack-localAtomicUsize. Ifcargo testruns them in parallel with other tests compiling JIT traces, the other traces will bake in a dangling pointer, leading to undefined behavior (SIGSEGVs) or flaky test failures.Consistent with other process-global tests in this file (e.g.,
test_host_loop_external_jump_to_middle_label_uses_label_selector), please add an#[ignore]attribute instructing them to be run serially.🛠️ Proposed fix to isolate test execution
+ #[ignore = "sets the process-global eval_breaker_word override; run serially: \ + `cargo test -p majit-backend-cranelift guard_eval_breaker_ \ + --features dynasm -- --ignored --test-threads=1`"] #[test] fn guard_eval_breaker_deopts_when_bitmask_set() { assert_inert_eval_breaker_guard(); let test_word = AtomicUsize::new(0); majit_ir::eval_breaker_word::set_addr_override_for_test( &test_word as *const AtomicUsize as usize, ); let _clear_override = ClearEvalBreakerAddrOverride; let (backend, token) = compile_eval_breaker_guard_trace(518); let frame = backend.execute_token(&token, &[]); assert!( backend.get_latest_descr(&frame).is_finish(), "a zero eval-breaker word must stay on the guard fast path" ); test_word.fetch_or(majit_ir::eval_breaker_word::EB_STW, Ordering::Relaxed); assert_ne!( test_word.load(Ordering::Relaxed) & majit_ir::eval_breaker_word::EB_STW, 0 ); let frame = backend.execute_token(&token, &[]); assert!( !backend.get_latest_descr(&frame).is_finish(), "the STW bit must use the existing guard-failure exit" ); test_word.fetch_and(!majit_ir::eval_breaker_word::EB_STW, Ordering::Relaxed); let (backend_after_clear, token_after_clear) = compile_eval_breaker_guard_trace(519); let frame = backend_after_clear.execute_token(&token_after_clear, &[]); assert!( backend_after_clear.get_latest_descr(&frame).is_finish(), "a freshly compiled trace must pass after the STW bit clears" ); test_word.fetch_or(majit_ir::eval_breaker_word::EB_ASYNC, Ordering::Relaxed); let frame = backend_after_clear.execute_token(&token_after_clear, &[]); assert!( !backend_after_clear.get_latest_descr(&frame).is_finish(), "the async bit must use the same guard-failure exit" ); } + #[ignore = "sets the process-global eval_breaker_word override; run serially: \ + `cargo test -p majit-backend-cranelift guard_eval_breaker_ \ + --features dynasm -- --ignored --test-threads=1`"] #[test] fn guard_eval_breaker_cross_trace_inherits_bitmask() {📝 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.#[ignore = "sets the process-global eval_breaker_word override; run serially: \ `cargo test -p majit-backend-cranelift guard_eval_breaker_ \ --features dynasm -- --ignored --test-threads=1`"] #[test] fn guard_eval_breaker_deopts_when_bitmask_set() { assert_inert_eval_breaker_guard(); let test_word = AtomicUsize::new(0); majit_ir::eval_breaker_word::set_addr_override_for_test( &test_word as *const AtomicUsize as usize, ); let _clear_override = ClearEvalBreakerAddrOverride; let (backend, token) = compile_eval_breaker_guard_trace(518); let frame = backend.execute_token(&token, &[]); assert!( backend.get_latest_descr(&frame).is_finish(), "a zero eval-breaker word must stay on the guard fast path" ); test_word.fetch_or(majit_ir::eval_breaker_word::EB_STW, Ordering::Relaxed); assert_ne!( test_word.load(Ordering::Relaxed) & majit_ir::eval_breaker_word::EB_STW, 0 ); let frame = backend.execute_token(&token, &[]); assert!( !backend.get_latest_descr(&frame).is_finish(), "the STW bit must use the existing guard-failure exit" ); test_word.fetch_and(!majit_ir::eval_breaker_word::EB_STW, Ordering::Relaxed); let (backend_after_clear, token_after_clear) = compile_eval_breaker_guard_trace(519); let frame = backend_after_clear.execute_token(&token_after_clear, &[]); assert!( backend_after_clear.get_latest_descr(&frame).is_finish(), "a freshly compiled trace must pass after the STW bit clears" ); test_word.fetch_or(majit_ir::eval_breaker_word::EB_ASYNC, Ordering::Relaxed); let frame = backend_after_clear.execute_token(&token_after_clear, &[]); assert!( !backend_after_clear.get_latest_descr(&frame).is_finish(), "the async bit must use the same guard-failure exit" ); } #[ignore = "sets the process-global eval_breaker_word override; run serially: \ `cargo test -p majit-backend-cranelift guard_eval_breaker_ \ --features dynasm -- --ignored --test-threads=1`"] #[test] fn guard_eval_breaker_cross_trace_inherits_bitmask() { let test_word = AtomicUsize::new(0); majit_ir::eval_breaker_word::set_addr_override_for_test( &test_word as *const AtomicUsize as usize, ); let _clear_override = ClearEvalBreakerAddrOverride; let (backend_a, token_a) = compile_eval_breaker_guard_trace(520); test_word.fetch_or(majit_ir::eval_breaker_word::EB_ASYNC, Ordering::Relaxed); let (backend_b, token_b) = compile_eval_breaker_guard_trace(521); let frame_a = backend_a.execute_token(&token_a, &[]); assert!( !backend_a.get_latest_descr(&frame_a).is_finish(), "the first trace must observe the stable test-local word" ); let frame_b = backend_b.execute_token(&token_b, &[]); assert!( !backend_b.get_latest_descr(&frame_b).is_finish(), "a trace compiled after arming must bake the same test-local word 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 `@majit/majit-backend-cranelift/src/compiler.rs` around lines 24740 - 24806, Add the #[ignore] attribute to both guard_eval_breaker_deopts_when_bitmask_set and guard_eval_breaker_cross_trace_inherits_bitmask, which mutate the process-global eval_breaker_word address override. Leave their test logic unchanged and mark them for explicit serial execution.pyre/pyre-interpreter/src/opcode_ops.rs (1)
349-356: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Pin local object references and vector elements before
try_hash_valueto prevent use-after-free.The addition of
try_hash_valueintroduces an allocating call (which can trigger a moving minor GC) immediately before using localPyObjectRefvariables (dict,set,key,value,item) and iterating over unrooted nativeVecelements. Because Pyre uses a shadow-stack GC, local Rust variables and heapVecs are not automatically updated when nursery objects relocate. These references must be explicitly pinned to the shadow stack beforetry_hash_valueand re-read viashadow_stack_getbefore subsequent use; otherwise, they will point to freed memory.
pyre/pyre-interpreter/src/opcode_ops.rs#L349-L356: pindict,key, andvalueto the shadow stack beforetry_hash_value, then re-read them beforew_dict_store.pyre/pyre-interpreter/src/opcode_ops.rs#L310-L310: pinsetandvalueto the shadow stack beforetry_hash_value, then re-read them beforew_set_add.pyre/pyre-interpreter/src/opcode_ops.rs#L322-L328: pinsetand every element in the unrooteditemsvector, re-reading the active item andsetacrosstry_hash_value.pyre/pyre-interpreter/src/builtins.rs#L5748-L5748: pindict,val, and all unrootedkeyselements, re-reading the active key/dict/val acrosstry_hash_value.pyre/pyre-interpreter/src/builtins.rs#L5766-L5766: pindictand all unrootedelemselements, re-reading them acrosstry_hash_value.pyre/pyre-interpreter/src/typedef.rs#L2057-L2063: pinobjand all unrooteditemselements, re-reading them acrosstry_hash_value.pyre/pyre-interpreter/src/typedef.rs#L2129-L2134: pinset_objand all unrooteditemselements, re-reading them acrosstry_hash_value.pyre/pyre-interpreter/src/typedef.rs#L3493-L3499: pind,value, and all unrooteditemselements, re-reading them acrosstry_hash_value.pyre/pyre-interpreter/src/typedef.rs#L13895-L13901: pinargs[0]andargs[1]beforetry_hash_value, and re-read them beforew_set_add.pyre/pyre-interpreter/src/typedef.rs#L13987-L13993: pinargs[0]and all unrootedother_itemselements, re-reading them acrosstry_hash_value.pyre/pyre-interpreter/src/typedef.rs#L14067-L14073: pinargs[0]and all unrootedother_itemselements, re-reading them acrosstry_hash_value.📍 Affects 3 files
pyre/pyre-interpreter/src/opcode_ops.rs#L349-L356(this comment)pyre/pyre-interpreter/src/opcode_ops.rs#L310-L310pyre/pyre-interpreter/src/opcode_ops.rs#L322-L328pyre/pyre-interpreter/src/builtins.rs#L5748-L5748pyre/pyre-interpreter/src/builtins.rs#L5766-L5766pyre/pyre-interpreter/src/typedef.rs#L2057-L2063pyre/pyre-interpreter/src/typedef.rs#L2129-L2134pyre/pyre-interpreter/src/typedef.rs#L3493-L3499pyre/pyre-interpreter/src/typedef.rs#L13895-L13901pyre/pyre-interpreter/src/typedef.rs#L13987-L13993pyre/pyre-interpreter/src/typedef.rs#L14067-L14073🤖 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/opcode_ops.rs` around lines 349 - 356, Prevent moving-GC use-after-free around every try_hash_value call by pinning referenced PyObjectRef locals and each unrooted vector element on the shadow stack, then re-reading them with shadow_stack_get before subsequent stores or uses. Apply this in pyre/pyre-interpreter/src/opcode_ops.rs lines 349-356 (map_add_value), 310-310, and 322-328; pyre/pyre-interpreter/src/builtins.rs lines 5748-5748 and 5766-5766; and pyre/pyre-interpreter/src/typedef.rs lines 2057-2063, 2129-2134, 3493-3499, 13895-13901, 13987-13993, and 14067-14073, preserving the existing w_dict_store and w_set_add behavior.
The full-body walker discarded w_type and version_tag from load_attr_fast_path, so its folded instance-storage read had only class and map guards. On wasm this left trace re-entry blind to class mutation and kept the stale instance slot after a property was installed. Record the live version_tag getfield and GuardValue before the map/storage read, then replace the guarded box in the heap cache so promotion state matches the existing trait tracer path. Assisted-by: Claude
Remove the #57 gate's LOAD_ATTR exclusion because the landed exact-resume path uses forward-exception-delivery / CALL-forward to resume a mid-body method-call abort exactly instead of the FBW refuse-drop. The method-load-then-call partial-drop hazard therefore no longer applies. Only LOAD_ATTR is admitted; heap-store widening remains a separate later slice. Assisted-by: Claude
Remove the #57 gate's abort-free restriction on the STORE_SUBSCR, STORE_ATTR, STORE_NAME, STORE_GLOBAL, and LOAD_NAME group. The landed exact-resume path uses forward-exception-delivery or CALL-forward to resume a mid-body abort after a committed un-journaled store exactly instead of taking the FBW refuse-drop path, so the partial-iteration drop hazard no longer applies. Assisted-by: Claude
The codewriter published operand-stack color mappings in its compact fast-locals-plus-stack space, while runtime resume consumers treated those slots as physical locals_cells_stack_w indices. Pure cell and free-variable slots therefore shifted every resumed operand by ncells; argument-cells did not because they overlap fast locals. Translate runtime-facing stack metadata through stack_base, use the same nlocals + ncells base in the callee rebuild, and keep dependent slot lookups aligned. This clears the range_iterator-as-method-target crash in JIT-compiled closure-frame FOR_ITER loops surfaced by LOAD_ATTR admission. Assisted-by: Claude
…-flush for nested loops The kept-stack Unsupported branch-flush plain-flushed at the inner FOR_ITER header without re-delivering the consumed item. For a nested inner FOR_ITER over a non-inlinable iterator, this dropped v=0 before its body ran. Select delivery versus plain flush from the existing body-completed predicates, mirroring Shape A's item-delivering flush when the body has not run. Assisted-by: Claude
…ss_inner build_class_inner holds `body_ns` (the class namespace object) in a stack-local across `frame.run_with_jit()`. A hot class-level loop compiles and runs through the JIT portal; a minor collection during it can promote the namespace young -> old, relocating it. The frame's `debugdata.w_locals` slot and the shadow-stack pin are forwarded — the object stays alive at its new address — but the stack-local `body_ns` copy is not, so it is left pointing at the freed nursery slot. The downstream `resolve_dict_backing` / `w_dict_items` mirror into class_ns then reads reclaimed memory, and the stranded value later surfaces as a SIGSEGV in `__set_name__` / getattr. Re-read the forwarded namespace from `frame.get_w_locals()` after the run, and rebind `mapping_namespace` to it. A large nursery (no minor collection) hid the bug. Assisted-by: Claude
collect_iterable accumulated yielded elements into a raw Vec<PyObjectRef> on the malloc heap across baseobjspace::next calls. When the iterable is a generator whose body allocates (e.g. a genexpr calling a JIT callee that boxes a fresh int), each next can trigger a moving minor collection. The raw Vec is not a GC root and conservative C-stack scanning does not reach its heap buffer, so earlier nursery-boxed elements were stranded / not forwarded; a consumer such as builtin_sum then read stale pointers, giving SIGSEGV or a wrong answer (a big nursery masked it). Pin the iterator and every yielded element on the gc_roots shadow stack — a real root the collector walks and updates on relocation — reloading the iterator from its slot before each next, then read the forwarded element slots back out into the result Vec after the loop. Mirrors the translator's push_roots/pop_roots bracket around a collecting call. collect_iterable is a residual loop-bearing helper (not trace-inlined); full check.py stays 181/181 on both dynasm and cranelift. Assisted-by: Claude
The guard-failure bridge entry seeding in `run_perfn_walk` gated its EC-color-collision operand reseed behind `CAN_BE_TAGGED`. That gate was added with the tagged-int machinery and is inert on flag-false builds, so the reseed never ran there. Under free register coloring a leaf callee (`def cap(): return CELL + 1`) places its LOAD_DEREF result in the portal EC register color. When the cell payload's class flips mid-loop (float->int, int->str, int->bignum) the deref-class guard fails and a bridge resumes at the guard offset. The base entry seed writes the stale `ec_box` (a `ConstPtr` to the trace-time cell payload) into that color; the gated-off reseed left it there instead of overwriting it with the live resume operand (`bridge_registers_r` names the real deref result). The residual add then read the stale `ConstPtr` as its LHS and dereferenced the now-dead trace-time payload object -> SIGSEGV in the eager `bh_binary_op`. The reseed of a non-frame reserved color that the bridge names as a genuine operand is a general correctness fix (the frame color keeps its unconditional skip to preserve the standard-virtualizable identity), so drop the tagged-int gate. Assisted-by: Claude
…ception The interpreter records each propagating frame through handle_exception and record_application_traceback. The compiled deopt ExitFrameWithExceptionRef path omitted that boundary record, dropping the materialized JIT frame from uncaught exception tracebacks. Mirror the interpreter record at both bottommost blackhole exits, using the faulting JitCode position mapped back to the Python PC so the traceback line remains exact. This does not reconstruct unmaterialized inlined callees; that remains separate work. Assisted-by: Claude
… the frame A raising DELETE residual left the backend _store_exception cell holding the exception after blackhole propagation returned ExitFrameWithExceptionRef and the outer interpreter caught it. The next JIT entry's GUARD_NO_EXCEPTION then resurfaced that stale value. Drain the backend cell at the bottommost unhandled-exception exit in blackhole_resume_via_rd_numb, after in-frame handler and caller routing have failed. Assisted-by: Claude
…bodies Admit the delete opcodes through the same forward-exact-resume path as the slice-2 stores. A committed un-journaled delete resumes forward without dropping the tail; a raising delete propagates with a bit-exact traceback via the landed exit-frame recording. STORE_DEREF intentionally remains declined because its separate resume bug is still open. Assisted-by: Claude
The post-call guard snapshot sourced stack slot 0 from the stale pre-call mirror [callable, NULL, arg0, arg1], so deep-recursion guard-fail recovery substituted the callable for the result. Advance the caller mirror explicitly after the CALL_ASSEMBLER result register writeback and before the paired guards; the register file was already correct. Preserve the returned callee JitFrame for Cranelift exception recovery, and leave exception-guard failures to blackhole recovery because the bridge callback has no exception-value channel. Assisted-by: Claude
Method-form LOAD_ATTR left only its final self box in the walk vstack mirror. After a committed STORE_DEREF, a later guard refilled the lower method slot from the stale receiver, so resumed CALL invoked the list itself. Mirror both exact LOAD_ATTR method slots at the vable stack-write chokepoint, then admit STORE_DEREF loop bodies and cover the branch-plus-call gate shape. Assisted-by: Claude
…me reraise/chain escape route_to_catch transferred a residual exception into a zero-cost exception-table handler without recording the compiled frame's traceback entry. A handler that reraised (bare raise) or raised a new exception (implicit __context__ chaining) then escaped with the faulting line missing, and for chaining the __context__ exception carried no compiled-frame traceback; Fix A only recorded the cleanup RERAISE, yielding the except-clause line instead of the fault/raise lines. Add a record_caught_exception callback on the blackhole interpreter (generic, raw frame/exception refs only). pyre-jit installs record_caught_blackhole_traceback, which translates the faulting JitCode PC through the JitCode map and records via record_application_traceback on handler entry. Track the caught exception value so that when the same object later escapes through the handler cleanup, the bottommost exit suppresses the duplicate entry (attach_tb=false); Fix A's uncaught path is unchanged. Assisted-by: Claude
The cpython baseline oracle resolved to the first `python3` on PATH. On a host with an older system CPython that emits version-sensitive error text (e.g. list.insert/pop with a float index: "integer argument expected, got float" on 3.9 vs "'float' object cannot be interpreted as an integer" on 3.14/pypy), it diverged from the pypy oracle and tripped a spurious cpython-vs-pypy baseline mismatch (BASEFAIL) on synth/list_insert_pop_index even though pyre matches 3.14. Prefer python3.14 (the compat target) when present, falling back to python3 then python; the PYRE_CHECK_PYTHON3 override is unchanged. Assisted-by: Claude
MAP_ADD, SET_ADD, and several direct builtin insertion paths bypassed the existing hashability gate and stored list keys or elements. Check try_hash_value before mutating comprehension accumulators, set methods and constructors, dict construction, and dict.fromkeys. Assisted-by: Claude
…nt encoding unsupported_jit_shape returns a new ConstEncodingOverflow decline when an upper bound on a frame's constant pool and register file cannot fit the single-byte register-or-constant index encoding (assembler.py:72; check_result count_regs[kind] + len(constants) <= 256). Such a frame runs in the interpreter before tracing; the decline is recorded in the frame-shape census. The constant-pool bound flattens nestable Tuple/Frozenset/Slice constants — a module body like html.entities expands a few code.constants entries into thousands of ConstRef pool slots — while Code constants count as one ref (they belong to a separate callee frame). The register bound is max_stackdepth plus local/cell/free slots plus headroom for the vable red args and per-opcode temporaries. Both are upper bounds, so no unencodable frame is admitted. Without the gate, assembling such a frame overruns the encoding and the majit assembler asserts mid-emission (touch_ref_reg_or_pool_slot); the FOR_ITER gate widening on this branch admitted the <module> frames of test_html and test_sundry, which crashed there. Assisted-by: Claude
…ey hash check 416dc05 added `try_hash_value(...)?` calls to several dict/set insert paths (SET_ADD, SET_UPDATE, MAP_ADD, the set/frozenset/dict constructors, dict.fromkeys, set.add/update/symmetric_difference_update). `try_hash_value` can run a user `__hash__`, which can allocate and trigger a moving minor collection that relocates young nursery objects. The new call sites held the container (dict/set) and key/value/item as raw `PyObjectRef` locals across that collection point without shadow-stack rooting, then passed the (potentially stale, post-relocation) pointers to `w_set_add`/`w_dict_store`. Before 416dc05 these paths had no collection point between receiving the pointers and the store, so this was a genuine regression. Root every such site with `pyre_object::gc_roots::{push_roots, pin_root, shadow_stack_len, shadow_stack_get}` (the existing pattern used by `collect_iterable` / `sort_rooted_items` / the memoryview constructors): pin the container and key/value/item before the hash check, then reload from the shadow stack before the store. For a loop over a `Vec` returned by `collect_iterable` (which pops its own roots before returning), the whole vec is re-pinned for the loop's duration, not just the current element, since an earlier iteration's hash call can also strand later elements that were never rooted. Sites fixed: - opcode_ops.rs: set_add_value, set_update_value (SET branch), map_add_value, dict_update_value (generic-mapping tail loop) - builtins.rs: builtin_set_from_items, builtin_dict_ctor (both the keys()-mapping loop and the pairs loop) - typedef.rs: frozenset_descr_new, set_descr_init, dict.fromkeys (init_dict_type), and the three init_set_type sites (add, update, symmetric_difference_update) Also closes a second, related gap in dict_update_value (used by the DICT_UPDATE bytecode, e.g. `{**mapping}`): the generic-mapping tail loop called raw `getitem` + `w_dict_store` with no hash check at all, silently accepting an unhashable key from a non-dict mapping. Added the same try_hash_value + rooting there, matching the sibling paths' reject-before- mutate behavior and error message. Added a set_update_value unit test mirroring the existing map_add_value/set_add_value unhashable-reject test. Verified: cargo test -p pyre-interpreter --features dynasm (380 passed); cargo test -p pyre-jit --features dynasm --test gc_stress (6 passed); CI's clippy sandbox check (CLIPPY_CONF_DIR=ci/clippy-sandbox cargo clippy -p pyre-interpreter --no-deps --features sandbox,dynasm -- -A clippy::all -D clippy::disallowed_methods -D clippy::disallowed_types) clean; python3 pyre/check.py --backend dynasm,cranelift (188/188 both, no BASEFAIL); all touched-site unhashable repros still raise `TypeError: unhashable type: '<type>'` and hashable controls stay byte-exact vs a python3.14 oracle on both backends with JIT on and off; a dict/set-comprehension and set.update/symmetric_difference_update repro over instances whose `__hash__` allocates and calls `gc.collect()` mid-hash stays byte-exact under a forced full collection. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdccc162bc
ℹ️ 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".
| // dereference reclaimed memory (`resolve_dict_backing` / `w_dict_items` | ||
| // below, then `__set_name__`). | ||
| let body_ns = frame.get_w_locals(); | ||
| let mapping_namespace = mapping_namespace.map(|_| body_ns); |
There was a problem hiding this comment.
When __prepare__ returns a namespace and the class body runs a hot JIT loop that triggers a moving GC, this only refreshes body_ns/mapping_namespace from the frame; the later metaclass path still reads the original w_namespace local (if let Some(w_prepared_dict) = w_namespace), which can still point at the pre-forwarding address. In that scenario a custom metaclass (or an exact prepared dict that is replayed after the body) can dereference a stale namespace even though the frame’s debugdata.w_locals was updated, so the prepared namespace used downstream needs to be refreshed/rooted as well.
Useful? React with 👍 / 👎.
Widens the #57 FOR_ITER JIT decline gate as exact-resume coverage is proven per opcode group, and closes the JIT traceback-fidelity gaps uncovered along the way. Every increment is gated on adversarial byte-exact parity vs the interpreter on both backends.
FOR_ITER gate widening (exact-resume)
A committed un-journaled heap effect resumes forward exactly (the loop tail is not dropped); a raising effect propagates via forward-exception-delivery. The gate now admits, alongside the base no-heap-mutation set:
STORE_SUBSCR/STORE_ATTR/STORE_NAME/STORE_GLOBAL(slice-2) andLOAD_ATTR/LOAD_NAMEDELETE_SUBSCR/DELETE_ATTR(slice-3a)STORE_DEREF(slice-3b)Supporting exact-resume fixes: FOR_ITER resume slots for closure frames; in-flight FOR_ITER item delivery on the kept-stack Unsupported branch-flush for nested loops; class-body namespace local refresh after a JIT run.
JIT traceback fidelity (#9)
A JIT-compiled frame that exits with a residual exception previously dropped its own Python traceback entry.
route_to_catchtransferred a residual into a zero-cost handler without recording the frame; a reraise (raise),raise ... from, or implicit__context__chain then escaped with the fault/raise line missing and the chained exception carrying no compiled-frame traceback. A genericrecord_caught_exceptioncallback on the blackhole (raw refs only — no interpreter dependency) records at handler entry; the bottommost exit suppresses the duplicate cleanup entry when the escaping exception is the already-recorded caught object.Correctness fixes uncovered by the wider gate
[callable, ...], so deep-recursion guard-fail recovery substituted the callable for the result. Advance the mirror to the post-call state before the guard; decline the exception-guard bridge compile from the post-call resume state.LOAD_ATTRpushes[method, self]but the mirror kept onlyself, so a cell-related guard reconstructed the method slot from a stale value and the tail call invoked the wrong object. Mirror both method-load slots at the vable stack-write chokepoint._store_exceptioncell populated after propagating out, so the next JIT'd function resurfaced it. Drain the cell at the bottommost unhandled frame exit.Loop admission
scope loop abort_permanent decline to the loop body: anexcept X as ehandler lowers its cleanup to a post-loopDELETE_FAST(abort_permanent) that wrongly declined the hot loop; scope the scan to the loop body so such loops compile (and their caught-reraise tracebacks are exercised).Verification
pyre/check.py --backend dynasm,craneliftgreen across the increments (the sole failure on the current base issynth/list_insert_pop_index, a pre-existing objspace error-message parity issue from #552 that fails at interpreter level, unrelated to this branch — fixed as a follow-up commit here). Adversarial byte-exact repros (values, final container state, exception type/message, and full multi-frame tracebacks incl.__cause__/__context__chains) pass on both backends.🤖 Generated with Claude Code
Summary by CodeRabbit
dict/setoperations validate hashability up-front: unhashable keys/values now raiseTypeErrorwithout partially mutating the result.FOR_ITERloop bodies, including method-call patterns and commonSTORE_*/DELETE_*cases.