jit: resolve the jd1 novable unpackiterable-drain live blackhole resume and enable it by default - #751
Conversation
WalkthroughThe PR updates iterator-drain lowering and execution, adds JD1 live-path resume handling, threads driver-specific frame metadata, hardens blackhole indirect calls and descriptor dispatch, preserves pending exceptions, and updates interpreter cache and virtualizable-state behavior. ChangesDrain execution and JIT safety
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Iterator
participant eval
participant CompiledDrain
participant Blackhole
participant InterpreterBoundary
Iterator->>eval: trigger compiled drain path
eval->>CompiledDrain: run drain loop
CompiledDrain->>Blackhole: resume novable state
Blackhole-->>eval: StopIteration or pending error
eval->>InterpreterBoundary: park non-StopIteration error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4d6f731a0
ℹ️ 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".
| // cached element. Key on descr.index() (the canonical resolved | ||
| // descr index the getarrayitem read path uses), not the raw | ||
| // bytecode operand descr_idx. | ||
| ctx.heapcache_setarrayitem(array_opref, index_opref, descr_index, value_opref); |
There was a problem hiding this comment.
In the PYRE_JD1 trace-walk path, this new cache write mirrors only the final heapcache.setarrayitem step, but upstream execute_setarrayitem_gc reaches that step through execute_and_record, which first runs heapcache.invalidate_caches/mark_escaped for SETARRAYITEM_GC. When an escaped array stores a freshly allocated ref and a later getarrayitem hits this cache, the cached value_opref can still be marked unescaped, so a subsequent residual call can preserve heap caches for an object that is now reachable through the escaped array and may be mutated. Run the Setarrayitem invalidation/escape step before this cache update, then cache the store.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8f10cc2). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1a4248f2e
ℹ️ 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".
| // loop-exit StopIteration on its next `next()`. A genuine | ||
| // drain-time exception is left for `ln` to re-raise (the | ||
| // exhaustion-stable iterators jd1 triggers on re-raise it). | ||
| let _ = err; |
There was a problem hiding this comment.
When PYRE_JD1_ENTER is set and the compiled jd1 drain resumes in blackhole, this arm treats every ExitFrameWithExceptionRef as if it were StopIteration and drops the PyError. The interpreter loop just below only breaks for PyErrorKind::StopIteration and returns all other errors, so a stateful iterator whose __next__ raises ValueError or TypeError once can have that exception consumed and the baseline loop will call next() again instead of propagating it. Check err.kind and surface non-StopIteration rather than discarding it.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b80f82235
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if is_finish { | ||
| break; |
There was a problem hiding this comment.
When PYRE_JD1_ENTER is enabled and the compiled drain exits via ExitFrameWithExceptionRef (for example, a non-StopIteration raised by next() in the drain), the backend still reports a finish, but this branch breaks as if the drain completed normally. The code below then clears the pending exception channels and returns to the interpreter loop, so one-shot iterator errors can be swallowed instead of propagated; handle the exception-finish state before taking the normal is_finish path.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
youknowone
changed the title
|
Added commit
Verification (default env, no — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/9faa576a5af08c03451f9d33f08ce0ca27cb93c1/pyre-jit/src/eval.rs#L4723
With this default true, any hot unknown-length unpack now runs drive_unpack_iterable_trace, which installs the build-time drain into METAINTERP_SD.jitcodes at its baked index; install_build_time_jitcode_at explicitly overwrites the existing runtime slot when the drain index is 0. In a thread where a live jd0 PyCode or guard snapshot already owns that slot, later jd0 resume/traceback resolves index 0 to the code_ptr-less drain instead of the original PyCode, so deopts can fail or use null/raw wrong code. Keep jd1 opt-in, or separate the build-time and runtime jitcode index spaces before enabling it by default.
ℹ️ 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 descr = entry.as_bh_descr().unwrap_or_else(|| { | ||
| panic!("d-arg descrs[{descr_idx}] is not a BhDescr entry: {entry:?}") |
There was a problem hiding this comment.
When a jd1 blackhole resume reaches an inline_call_*, read_inline_call_jitcode calls this helper. For build-time jitcodes the new descr_at fallback returns RuntimeBhDescr::JitCode for j operands, but this unwrap accepts only RuntimeBhDescr::Descr, so the first inlined call in _unpackiterable_unknown_length panics instead of returning the callee fnaddr/calldescr. Use a separate reader for j operands, or have the inline-call reader handle as_jitcode.
Useful? React with 👍 / 👎.
|
Rebased on 1.
|
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/2906d4cd83097675c0d29f5e6acbd0b3b2d48d4a/pyre-jit/src/eval.rs#L5123
When a jd1 compiled entry detects stack overflow, pyre_stack_check_for_jit_prologue stores a RecursionError in this same pending-exception slot. This unconditional drain consumes and ignores that error whenever the compiled-run loop breaks or finishes, unlike the other compiled-run boundaries that propagate it, so a hot star-unpack entered near the stack limit can continue execution instead of raising and may proceed toward the guard page. Inspect the drained error and discard only the expected loop-exit StopIteration.
https://github.com/youknowone/pyre/blob/2906d4cd83097675c0d29f5e6acbd0b3b2d48d4a/pyre-jit/src/eval.rs#L3308-L3311
Under a stop-the-world collection initiated by another thread, this global extra-root callback runs on the collector thread and therefore visits only that thread's TL_JIT_PENDING_EXCEPTION; foreign mutator TLS is reached exclusively through walk_all_extra_areas. A thread stopped after park_jit_pending_error can consequently have its sole exception reference missed and later read a moved or collected object. Expose a capture/area walker for this TLS cell and register it in register_thread_root_areas, as is already done for the other per-mutator root sources.
AGENTS.md reference: AGENTS.md:L148-L162
ℹ️ 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: 1
🤖 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-translate/src/front/exc_from_raise.rs`:
- Around line 189-205: Centralize the etype-reuses-evalue invariant in a helper
alongside set_raise_values, such as set_raise_values_from_evalue, which performs
the clone-and-set operation. Update exc_from_raise.rs:189-205,
result_exc.rs:537-542, and result_exc.rs:2094-2100 to call this helper instead
of hand-rolling graph.set_raise_values(block, v.clone(), v); all three sites
require the replacement.
🪄 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: 8aee35e2-f2e9-44a4-a377-241eab686f98
📒 Files selected for processing (14)
majit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-translate/src/front/exc_from_raise.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/tests/test_result_exc_lowering.rspyre/bench/synth/unpack_drain_star_raise.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/listobject.rs
… store majit-metainterp/pyjitpl/dispatch.rs: after recording SetarrayitemGc in the trace walker, call ctx.heapcache_setarrayitem (execute_setarrayitem_gc, pyjitpl.py:2744) so a later getarrayitem of the same array and constant index reads the stored value rather than a stale cached element. Key the store on descr.index() -- the canonical resolved descr index the getarrayitem read path also keys on -- not the raw bytecode operand descr_idx. Assisted-by: Claude
resolve_active_jitdriver_sd_with_vinfo returns the elected active driver's own virtualizable_info (None when the driver is novable) instead of scanning for the first vinfo-bearing slot; the scan stays only for the unelected init-time path. A novable driver's trace then captures no vable section. blackhole_resume_via_rd_numb and resume_in_blackhole_from_exit_layout take a `novable` flag: when set, the resume passes no vinfo (skipping the vable section decode) and leaves the virtualizable pointer/info handle unset. Every jd0 caller and the CALL_ASSEMBLER path pass false. The jd1 unpackiterable_driver live-path enter (gated PYRE_JD1_ENTER) runs the compiled drain over the shared (w_iterator, items) reds and, on a guard failure carrying resume storage, resumes in the blackhole interpreter; ContinueRunningNormally re-enters the compiled loop. Assisted-by: Claude
…/descr stores The jd1 (`unpackiterable_driver`) live-path resume decodes a frame chain whose jitcode_index, `-live-` offsets, and `d`-arg descrs are numbered in the build-time `jitcode_runtime` artifacts (the extracted `_unpackiterable_unknown_length` body and its inlined build-time callees), not the runtime CodeObject-keyed stores jd0 uses. The prior code resolved all three against the runtime stores, so the drain's 2-ref frame decoded against an unrelated jd0 PyCode jitcode and mistyped the refs as ints (`Const::getint on non-Int variant: Ref`). - blackhole_resume_via_rd_numb resolve_jitcode: for a novable driver, resolve jitcode_index via jitcode_runtime::get_jitcode_by_index instead of pyjitcode_for_jitcode_index, mirroring the driver's own bridge resume (run_compiled_detailed_with_bridge_keyed), which resolves through the flat build-time jitcode_registry. - Same function: decode `-live-` offsets against jitcode_runtime::all_liveness() for a novable driver, not the jd0-accumulated metainterp_sd.liveness_info. - read_descr: resolve `d`-arg descrs via JitCode::descr_at (per-jitcode exec.descrs then the process-global build-time pool) instead of exec.descrs alone, so a build-time jitcode's empty exec.descrs falls back to the shared pool the drain body names by index. jd0 unaffected: the resolve/liveness branches are novable-gated, and descr_at checks exec.descrs first (jd0's runtime jitcodes hit it before the new fallback). Assisted-by: Claude
build_multi_frame_miframe read `last_caught_exception_value` off the frame stack, a field no longer present on the rebased base — multi-frame exception state now travels via the metainterp `last_exc_value`. Delete the two orphaned reads and the now-unused innermost `current` binding so the file compiles; `LatchedMultiFrameBlackhole` already carries the resumed exception value. raw_code_for_jitcode_index returned `jc.raw_code()` unconditionally. The novable drain portal (jd1 unpackiterable driver) is a native function with no Python CodeObject; its degenerate PyJitCode carries a null code_ptr. Return None for a null raw pointer so the instruction-decoding consumers (bare-reraise probe, traceback lineno) skip it instead of dereferencing null. Assisted-by: Claude
…de seam The jd1 unpackiterable driver compiles `_unpackiterable_unknown_length`'s drain loop and blackhole-executes it on a guard failure. Inlining `w_list_append` into that jitcode surfaced each of append's strategy/grow helpers (object_push, switch_to_correct_strategy, typed-array grow, …) as a separate residual funcptr the blackhole could not resolve — unregistered paths fell back to a symbolic hash and faulted (SIGBUS) at the first append. - Add `drain_list_append`, a `dont_look_inside` wrapper over `w_list_append`, and route only the drain through it; register it (plus the already-residual `w_list_new_empty`/`w_list_new_object` prologue and `drain_collect_items` epilogue) in `jit_trace_fnaddrs`. The global `list.append` keeps calling `w_list_append` directly and stays traced, so the append fold and the escape-flush replay are unaffected. - Return the grown `W_List` ref from `_unpackiterable_unknown_length` (`Type::Ref`, matching the driver's result) and move the `Vec` readback into the caller-side `drain_collect_items`, out of the traced/blackholed drain body so its blackhole epilogue is a plain ref-return. - Add `int_is_true/i>i` to the curated inline-call blackhole builder; the drain's back-edge guard emits it and it was absent from the set, panicking on the first back-edge test of a blackhole-executed drain. Assisted-by: Claude
Lower `_unpackiterable_unknown_length` from the production LLBC and assert `try_fuse_drain_match` fired: the synthesized `exc_kind_discriminant` kind-test call is present, no `StopIteration` SyntheticTransparentCtor survives, and the next() site carries a `LastException` edge. The recognizer is fail-safe — any unrecognised shape silently falls back to `catch_and_rewrap`, which leaves the unwalkable `StopIteration` ctor / eq residuals in place — and the default non-jd1 run never executes the fusion (the `unpack_drain_exact_kind` parity test only guards the default path). A drain-source rework or a recognizer regression that stops the fusion from firing was therefore invisible to the suite while reopening the jd1 walk SIGBUS. Assisted-by: Claude
`jd1_experiment_enabled` changes from opt-in (`PYRE_JD1=1`) to on by default: it returns false only for `PYRE_NO_JD1`, `PYRE_JD1=0`, or the master JIT off-switches `PYRE_NO_JIT` / `PYRE_JIT=0` (so "no JIT" also means no jd1, matching jd0's kill-switch at eval.rs). A new `jd1_enter_enabled` gates the live enter of the compiled drain loop on the `RunCompiled` back-edge action. It is on by default with a `PYRE_JD1_NO_ENTER` opt-out, replacing the former `PYRE_JD1_ENTER` opt-in. With both gates default-on, the default runtime traces, compiles, and live-enters the `unpackiterable_driver` drain on hot non-tuple unpack sites; the previous behavior (jd1 inert) is reachable via `PYRE_NO_JD1`. Assisted-by: Claude
…return `cargo fmt --all -- --check` broke the CI fmt gate on three call sites added by the jd1 drain commits: the `push_fnaddr` registrations for `drain_list_append` and `w_list_new_empty` in `jit_fnaddr.rs`, and the `ResolvedJitCode::new` return in `blackhole_resume_via_rd_numb`. All three exceed rustfmt's `fn_call_width` and are broken across lines. Assisted-by: Claude
`PyErrorKind::StopIteration` reaches `try_fuse_drain_match`'s `eq` operand as a `ConstInt` once the fieldless enum lowers to its discriminant, not only as a niladic `SyntheticTransparentCtor`. The ctor-only lookup declined, and the fail-safe fallback to `catch_and_rewrap` left `StopIteration` ctor/eq residuals carrying `symbolic_fnaddr_for_path` addresses. Assisted-by: Claude
`resolve_active_jitdriver_sd_with_vinfo` returned None whenever any slot carried a `virtualizable_info`, including the placeholder `ensure_default_driver_sd` pushes before any host registration (`set_virtualizable_info` broadcasts to it while no driver is elected). Gate the check on `jd.index.is_some()`, which `register_jitdriver_sd` stamps (call.py:46-47), so only host-registered drivers veto the linear scan. Fixes the i64env `COMPILES >= 1` assertion. Assisted-by: Claude
The live-enter path discarded every `ExitFrameWithExceptionRef` and cleared the pending-exception slots, leaving the interpreter drain loop to re-derive the error by calling `next()` again. That only works for an exhaustion-stable iterator; a generator is closed once the exception escapes, and a plain `__next__` need not re-raise, so both report StopIteration and the error is lost. Resuming the guard exit in the blackhole instead reached the drain's re-raise arm, whose `inline_call_r_r` byte the production blackhole builder does not register (`dispatch_step` unwired-opcode panic). Take the exception off the guard exit (and off `ExitFrameWithExceptionRef`) and park it in `TL_JIT_PENDING_EXCEPTION` after the trailing clears; `drain_jit_pending_exception` re-raises it at the caller loop's next call dispatch, before `__next__` re-runs. `park_jit_pending_error` is the second producer for that slot next to the prologue stack check, and `walk_jit_pending_exception` roots the parked object across the collecting code the drain runs before it. Adds `pyre/bench/synth/unpack_drain_star_raise.py`, which drives `_unpackiterable_unknown_length` through `f(*it)` for the plain, re-raising, and raise-once iterators; no existing test reached the drain on purpose. Assisted-by: Claude
`make_bytecode_block` hands the exceptblock's `inputargs` to `make_return` (`flatten.py:106-108`), whose 2-arg arm emits `-live-` + `raise self.getcolor(args[1])` and never reads `args[0]` (`flatten.py:139-143`); `make_exception_link` drops both args for a bare `reraise` when the link targets the re-raising exceptblock (`flatten.py:157-173`). `flatten.rs:770-793` mirrors both. Three front sites filled that slot with a synthesized `CallTarget::function_path(["type"])` Call whose result register no emitted bytecode reads: `lower_result_exc_returns`' `return Err(e)` rewrite, `try_fuse_drain_match`'s R block, and `lower_exc_from_raise`. Pass `evalue` for both slots instead. The jd1 drain's `return Err(e)` arm was one of those sites. It now flattens to `live` + `raise r0`; the `inline_call_r_r` it previously carried targeted a callee whose path the host never published, so its fnaddr stayed a `symbolic_fnaddr_for_path` hash. Assisted-by: Claude
…bolic call targets `wire_handler` resolves an opname through `_insns` and returns `false` for a key `setup_insns` never registered; both call sites discard the bool. The curated `build_inline_call_only_bh_builder` map omitted the ten canonical `inline_call_*` keys, so the matching `wire_handler` calls in `wire_bhimpl_handlers` were no-ops and a build-time jitcode's `inline_call_*` reached `dispatch_step`'s unwired panic. `unwired_opnames()` does not cover this: an absent key has no table slot to hold a placeholder. Register the ten keys. `read_inline_call_jitcode` now reads the operand as the `j` argcode it is: `descr_at` + `as_jitcode`, taking `fnaddr` / `calldescr` off the `Arc<JitCode>`. `blackhole.py:150-157` resolves `d` and `j` from the same `descrs` table and differs only in `assert isinstance(value, JitCode)`, since upstream's `JitCode` is an `AbstractDescr` (`jitcode.py:9`). The flattened `BhDescr::JitCode.fnaddr` in `ALL_DESCRS` is never rewritten by `runtime_fnaddr_patch`, so it always carries the build-script process's value; the `Arc` the runtime pool wraps carries the patched address. Add `is_symbolic_fnaddr` / `is_callable_fnaddr` and decline paths for both call families. The gate is the walker's — `(func as u64) >> 47 != 0` → `ResidualDecline::Symbolic` (`jitcode_dispatch/residual_call.rs:1117`). The blackhole has no decline channel, so it sets `aborted` + `LeaveFrame`, handing the continuation back to the interpreter. `residual_call_*` deliberately does not reject `func == 0`: the backends' `bh_call_*` treat it as a no-op returning 0/null. Assisted-by: Claude
… trampoline
`bh_call_*_dispatch` transmutes the callee to an `extern "C" fn(i64, ...)`
guessed from the bucketed `(int_args.len(), float_args.len())` arity. That
holds only on a C ABI that tolerates a signature mismatch: SysV/AAPCS pass the
surplus in registers the callee ignores, and a pointer parameter is
register-width either way. wasm32 has neither property — `call_indirect`
type-checks the callee's declared type on every call, and a pointer parameter
is `i32` where the transmute says `i64` — so a mistyped guess traps with
`indirect call type mismatch`.
`ResidualHostCallFn` (`call_stub.rs`) exists for that case and reflects the
callee's real signature; `set_residual_host_call` installs it on wasm32. The
trait-default `Backend::bh_call_{i,r,f,v}` consulted it, but the eight inline
residual sites in `pyjitpl/dispatch.rs`'s `JitCodeMachine` walker called
`bh_call_{i,f,v}_dispatch` directly and bypassed it.
Add `bh_call_{i,f,v}_by_classes`, which take `arg_classes` and hold the
hook-vs-transmute choice, and route both the walker's eight sites and the four
trait defaults through them. The choice has to be made where `arg_classes` is
still in hand: `collect_call_args` buckets into `(int, float)` and drops the
interleaving, so the positional list the trampoline takes cannot be
reconstructed from the bucketed pair.
On dynasm and cranelift `residual_host_call()` is `None`, so both keep the
existing transmute path.
Assisted-by: Claude
…n store
The compile-time `rd_numb` decoders — `build_guard_metadata` and the cranelift
backend's `collect_guards` — asked the process-global
`majit_ir::resumedata::set_frame_value_count_fn` callback for each resume
frame's `enumerate_vars` box count. pyre registers
`pyre_jit_trace::state::frame_value_count_at` there, which decodes against
`MetaInterpStaticData.jitcodes` + `liveness_info`: the CodeObject-keyed store jd0
grows as tracing interns `-live-` triples.
jd1 (`unpackiterable_driver`) numbers its frames elsewhere. Its jitcode is the
`_unpackiterable_unknown_length` body extracted from LLBC, with `-live-` offsets
baked at extraction into `jitcode_runtime::all_liveness()`. This is the store
split `4e1b74866b3` resolved for `blackhole_resume_via_rd_numb`, at the
compile-time sites it did not cover.
Decoding jd1's coordinates against the runtime store does not fail when that
store is populated. Measured on dynasm for `unpack_drain_star_raise`:
jitcode 0 pc=96 build-time [i=0, r=4, f=0] = 4 runtime [i=14, r=0, f=4] = 18
jitcode 0 pc=115 build-time [i=0, r=3, f=0] = 3 runtime [i=32, r=0, f=3] = 35
so every jd1 guard's metadata was built from an 18- or 35-slot int/float frame
that is really 4 or 3 refs. On wasm the runtime store was empty instead, and the
decode hit `frame_value_count_at`'s fail-loud panic, aborting `except_star`,
`exception_group_type`, and `unpack_drain_star_raise`.
The global callback carries no driver identity, so put the choice on
`JitDriverStaticData` (`frame_value_count_fn`) and thread it from `MetaInterp`'s
callers via the trace-bound `active_jitdriver_sd` — directly into
`build_guard_metadata`, and into the backend through
`Backend::set_next_frame_value_count_fn` alongside the existing
`set_next_trace_id` / `set_next_header_pc` pre-compile setters. `None` keeps the
global callback, so jd0 and every non-pyre host are unchanged. jd1's descriptor
installs `state::build_time_frame_value_count_at`, which resolves the jitcode
through `jitcode_runtime::get_jitcode_by_index` and decodes against
`jitcode_runtime::all_liveness()` using the build-time `live/` opcode.
The store is picked per driver rather than tried-then-retried because a decode
against the wrong store succeeds, as measured above.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/8f10cc2976103f2716d435a3d5f251d16236be33/pyre-jit/src/eval.rs#L5162
When a Cranelift JD1 entry hits the stack-check overflow path, compiler.rs:8694-8707 parks a RecursionError and returns the untouched JIT frame; the RunCompiled fallback then reaches this unconditional drain and discards that exception. An unknown-length unpack near the recursion limit therefore continues into next() instead of raising and can consume more native stack. Preserve or surface the pending error here rather than clearing it.
AGENTS.md reference: AGENTS.md:L14-L19
https://github.com/youknowone/pyre/blob/8f10cc2976103f2716d435a3d5f251d16236be33/pyre-interpreter/src/baseobjspace.rs#L10439-L10440
When a translated caller reaches unpackiterable's unknown-length branch, this annotation forces drain_collect_items through the residual-call ABI. dont_look_inside_return_token classifies the opaque Vec<PyObjectRef> ADT as a one-word Ref, while the registered Rust function actually returns a three-word Vec via an sret ABI; the single-register call stubs therefore omit the hidden return pointer and can corrupt memory or crash. Keep the W_List across the generated boundary and perform the Vec conversion only in genuinely untraced host code, or add real aggregate-return ABI support.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ 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".
| // raise tail in the corpus, and made the raise arm unwalkable in the | ||
| // blackhole (canonical `inline_call_*` on a callee with no runtime | ||
| // address). Reuse `evalue`: same ref kind, no new op. | ||
| graph.set_raise_values(block, evalue_var.clone(), evalue_var); |
There was a problem hiding this comment.
This intentionally replaces upstream w_type = op.type(w_value) with the exception value itself solely to avoid an unresolved generated call. Even if the current flattener ignores the first except-block argument, the annotator and rtyper still consume the flow graph, so deleting the operation is a structural shortcut rather than the required fix to the generated call/address path. Restore the canonical type(evalue) operation and fix its translation/runtime binding instead.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
majit/majit-metainterp/src/jitdriver.rs (1)
410-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeftover "remove before commit" debug scaffold still wired into production guard-failure paths.
bridge_only_allowsis explicitly labeledTEMP DIAGNOSTIC (remove before commit)and gated by an undocumentedMAJIT_BRIDGE_ONLYenv var, yet it's actually called from both guard-failure hot paths (should_bridgeinback_edge_internalandrun_back_edge_generic). Harmless when the env var is unset, but it's dead debug code that was supposed to be removed before this PR merged.Also applies to: 3526-3529, 5860-5863
🤖 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-metainterp/src/jitdriver.rs` around lines 410 - 430, Remove the temporary bridge_only_allows diagnostic helper and all invocations from the guard-failure paths, including should_bridge in back_edge_internal and run_back_edge_generic. Restore both paths to their normal bridge-formation behavior without MAJIT_BRIDGE_ONLY filtering or related diagnostic logging.majit/majit-metainterp/src/pyjitpl.rs (1)
1069-1075: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winReplace the green-key inversion scan with an indexed lookup
compiled_key_for_greenswalksloop_header_greenslinearly and callshas_compiled_targetsfor each candidate;compiled_key_for_greensis the only Rust caller, so the scan cannot be amortized across calls. Store an inverse map from header greens to green key (keeping parity by not treating alias-only entries as jumpable) instead of keeping duplicates onloop_header_greens.🤖 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-metainterp/src/pyjitpl.rs` around lines 1069 - 1075, Replace the linear scan in compiled_key_for_greens with an indexed inverse map from header-green tuples to their green key. Add and maintain this map wherever loop_header_greens is populated, while ensuring alias-only entries are not considered jumpable. Remove the duplicate lookup data from loop_header_greens and preserve has_compiled_targets behavior for valid compiled targets.
🤖 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 7693-7695: Update the next-compilation handling for
next_frame_value_count_fn so collect_guards consumes it with Option::take rather
than borrowing or retaining the stored callback. Preserve
set_next_frame_value_count_fn’s setter behavior while ensuring the override is
cleared after one compilation and cannot affect subsequent compilations.
In `@majit/majit-backend/src/lib.rs`:
- Around line 2707-2717: Extend the regression tests for the shared
residual-call path around the dispatch branches invoking bh_call_i_by_classes,
bh_call_r, and the other return handlers. Cover mixed I/R/F argument classes,
Int, Ref, Float, and Void returns, empty or None argument buckets, and a real
GC-pointer return through bh_call_r; verify class ordering and pointer-return
handling remain correct.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 6095-6096: Extract the repeated backend priming and active frame
value count wiring into a private helper such as prime_backend_for_compile on
the relevant interpreter type, then replace the duplicated compile-site calls
with it, including loop, retrace, bridge, and entry paths. Preserve the existing
ordering and compile behavior, and retain the compile_bridge borrow-safe capture
before any mutable compiled_loops borrow.
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 10439-10448: Update drain_collect_items to reload the rooted items
reference through the shadow-stack slot after any potentially allocating getitem
call, rather than continuing to use the stale local pointer. Also obtain the
list length from the reloaded rooted reference immediately before iteration, and
use that refreshed reference for each w_list_getitem call.
In `@pyre/pyre-interpreter/src/stack_check.rs`:
- Around line 447-452: Update park_jit_pending_error to handle a null result
from err.to_exc_object() instead of silently returning. Surface the original
parked error as an internal failure when object-space construction fails, while
preserving set_jit_pending_exception for non-null exception objects.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 5068-5072: Scope the pins in the compiled-run path around the
logic following the pin operations in the relevant evaluator function, using a
local root scope that pushes before pinning w_iterator and items and pops when
the compiled run finishes. Ensure every break path, including no compiled loop,
fail_index == u32::MAX, and BlackholeResult::Failed, drains this local scope
before returning to the caller, while preserving the existing pin coverage
during execution.
- Around line 5160-5167: Update the exception cleanup around pending_err to
discard only StopIteration values from drain_jit_pending_exception() and
take_ca_exception(). Preserve and re-propagate any other error, including
stack-overflow RecursionError and callback/FFI errors, while keeping the
existing pending_err parking behavior.
---
Outside diff comments:
In `@majit/majit-metainterp/src/jitdriver.rs`:
- Around line 410-430: Remove the temporary bridge_only_allows diagnostic helper
and all invocations from the guard-failure paths, including should_bridge in
back_edge_internal and run_back_edge_generic. Restore both paths to their normal
bridge-formation behavior without MAJIT_BRIDGE_ONLY filtering or related
diagnostic logging.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 1069-1075: Replace the linear scan in compiled_key_for_greens with
an indexed inverse map from header-green tuples to their green key. Add and
maintain this map wherever loop_header_greens is populated, while ensuring
alias-only entries are not considered jumpable. Remove the duplicate lookup data
from loop_header_greens and preserve has_compiled_targets behavior for valid
compiled targets.
🪄 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: 3cb47d2d-5416-4fec-a48e-80ddcfc1aa1d
📒 Files selected for processing (20)
majit/majit-backend-cranelift/src/compiler.rsmajit/majit-backend/src/call_stub.rsmajit/majit-backend/src/lib.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-translate/src/front/exc_from_raise.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/tests/test_result_exc_lowering.rspyre/bench/synth/unpack_drain_star_raise.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/unpack_state.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/listobject.rs
| /// `Backend::set_next_frame_value_count_fn` — the compiling driver's | ||
| /// `-live-` decoder for the `rd_numb` reads below. | ||
| next_frame_value_count_fn: Option<fn(i32, i32) -> usize>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every call site of set_next_frame_value_count_fn to confirm whether
# it is reset to None before every compile for a driver that has no override,
# or only ever set once for jd1 without a matching reset for jd0.
rg -n -B3 -A3 'set_next_frame_value_count_fn' --type=rustRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
fd -a 'compiler\.rs$|jitdriver\.rs$' . | sed 's#^\./##' | head -100
echo "== symbols =="
rg -n -B2 -A2 'next_frame_value_count_fn|set_next_frame_value_count_fn|next_trace_id|next_header_pc|collect_guards|JitDriverStaticData|frame_value_count_fn' --type=rust . | head -300Repository: youknowone/pyre
Length of output: 25337
🏁 Script executed:
#!/bin/bash
set -u
echo "== exact occurrences =="
rg -n -B3 -A3 'next_frame_value_count_fn|set_next_frame_value_count_fn|frame_value_count_fn' --type=rust .
echo "== compiler.rs relevant implementation =="
sed -n '7680,7705p' majit/majit-backend-cranelift/src/compiler.rs
sed -n '7915,7928p' majit/majit-backend-cranelift/src/compiler.rs
sed -n '8405,8496p' majit/majit-backend-cranelift/src/compiler.rs
sed -n '15380,15410p' majit/majit-backend-cranelift/src/compiler.rs
echo "== jitdriver.rs relevant doc/field =="
sed -n '410,470p' majit/majit-metainterp/src/jitdriver.rsRepository: youknowone/pyre
Length of output: 38243
🏁 Script executed:
#!/bin/bash
set -u
echo "== call sites near cranelift calls =="
sed -n '8390,8505p' majit/majit-backend-cranelift/src/compiler.rs
echo "== pyjitpl call sites excerpts =="
sed -n '6080,6098p' majit/majit-metainterp/src/pyjitpl.rs
sed -n '7140,7150p' majit/majit-metainterp/src/pyjitpl.rs
echo "== backend trait definition context =="
sed -n '1750,1782p' majit/majit-backend/src/lib.rsRepository: youknowone/pyre
Length of output: 9310
Consume next_frame_value_count_fn for the next compilation.
set_next_frame_value_count_fn stores the value and collect_guards reads self.next_frame_value_count_fn; unlike the sibling one-shot fields, it is not cleared. Pass .take() instead of keeping it in the CraneliftBackend state so a driver override cannot persist into later compilations.
🤖 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 7693 - 7695,
Update the next-compilation handling for next_frame_value_count_fn so
collect_guards consumes it with Option::take rather than borrowing or retaining
the stored callback. Preserve set_next_frame_value_count_fn’s setter behavior
while ensuring the override is cleared after one compilation and cannot affect
subsequent compilations.
| // SAFETY: `func` is a valid funcptr matching the ABI recovered from | ||
| // `calldescr.arg_classes`. | ||
| unsafe { | ||
| crate::call_stub::bh_call_i_by_classes( | ||
| func as usize, | ||
| &calldescr.arg_classes, | ||
| args_i, | ||
| args_r, | ||
| args_f, | ||
| ); | ||
| return hook(func as usize, &args); | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add ABI regression coverage for the shared residual-call path.
Please add or verify tests covering mixed I/R/F argument classes and Int, Ref, Float, and Void returns, including a real GC-pointer return through bh_call_r and empty/None argument buckets. This should validate class ordering and pointer-return handling after replacing the previous dispatch path.
Also applies to: 2733-2742, 2759-2768, 2784-2793
🤖 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/src/lib.rs` around lines 2707 - 2717, Extend the
regression tests for the shared residual-call path around the dispatch branches
invoking bh_call_i_by_classes, bh_call_r, and the other return handlers. Cover
mixed I/R/F argument classes, Int, Ref, Float, and Void returns, empty or None
argument buckets, and a real GC-pointer return through bh_call_r; verify class
ordering and pointer-return handling remain correct.
| self.backend | ||
| .set_next_frame_value_count_fn(self.active_frame_value_count_fn()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Repeated set_next_frame_value_count_fn + active_frame_value_count_fn() wiring across ~8 compile sites.
Same two-line pattern (set_next_frame_value_count_fn(self.active_frame_value_count_fn()) then passing self.active_frame_value_count_fn() into build_guard_metadata) is duplicated verbatim at every compile path (loop, retrace, finish, simple-loop, entry-bridge, bridge). Functionally correct at each site (traced the borrow-checker workaround in compile_bridge where fvc is captured before the compiled_loops mutable borrow), but a tiny private helper (e.g. fn prime_backend_for_compile(&mut self)) would remove the duplication. Given this file's established convention of literal per-site duplication for audit/parity purposes, this is a low-reward cleanup.
Also applies to: 6280-6285, 7148-7149, 7213-7218, 7676-7677, 7756-7761, 8046-8047, 8122-8127, 10355-10356, 10391-10391, 11004-11005, 11118-11132, 20225-20236
🤖 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-metainterp/src/pyjitpl.rs` around lines 6095 - 6096, Extract the
repeated backend priming and active frame value count wiring into a private
helper such as prime_backend_for_compile on the relevant interpreter type, then
replace the duplicated compile-site calls with it, including loop, retrace,
bridge, and entry paths. Preserve the existing ordering and compile behavior,
and retain the compile_bridge borrow-safe capture before any mutable
compiled_loops borrow.
| #[majit_macros::dont_look_inside] | ||
| pub(crate) fn drain_collect_items(items: PyObjectRef) -> Vec<PyObjectRef> { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| pyre_object::gc_roots::pin_root(items); | ||
| let n = unsafe { pyre_object::listobject::w_list_len(items) }; | ||
| let mut out: Vec<PyObjectRef> = Vec::with_capacity(n); | ||
| for i in 0..n as i64 { | ||
| out.push(unsafe { pyre_object::listobject::w_list_getitem(items, i).unwrap() }); | ||
| } | ||
| Ok(out) | ||
| out |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
drain_collect_items pins items but never reloads the relocated pointer.
The doc comment states an Integer/Float-strategy getitem boxes through the moving collector and "can relocate items" — but the loop keeps dereferencing the original items local. pin_root updates the shadow-stack slot, not the local, so after a relocation both w_list_getitem(items, i) and the already-read n refer to from-space memory. Every other reduce/readback in this file re-reads through shadow_stack_get after each allocating call (e.g. enumerate_reduce_method, Lines 2521-2524).
🐛 Proposed fix: read the accumulator back through the rooted slot
pub(crate) fn drain_collect_items(items: PyObjectRef) -> Vec<PyObjectRef> {
let _roots = pyre_object::gc_roots::push_roots();
pyre_object::gc_roots::pin_root(items);
- let n = unsafe { pyre_object::listobject::w_list_len(items) };
+ let items_slot = pyre_object::gc_roots::shadow_stack_len() - 1;
+ let n = unsafe {
+ pyre_object::listobject::w_list_len(pyre_object::gc_roots::shadow_stack_get(items_slot))
+ };
let mut out: Vec<PyObjectRef> = Vec::with_capacity(n);
for i in 0..n as i64 {
- out.push(unsafe { pyre_object::listobject::w_list_getitem(items, i).unwrap() });
+ let items = pyre_object::gc_roots::shadow_stack_get(items_slot);
+ out.push(unsafe { pyre_object::listobject::w_list_getitem(items, i).unwrap() });
}
out
}📝 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.
| #[majit_macros::dont_look_inside] | |
| pub(crate) fn drain_collect_items(items: PyObjectRef) -> Vec<PyObjectRef> { | |
| let _roots = pyre_object::gc_roots::push_roots(); | |
| pyre_object::gc_roots::pin_root(items); | |
| let n = unsafe { pyre_object::listobject::w_list_len(items) }; | |
| let mut out: Vec<PyObjectRef> = Vec::with_capacity(n); | |
| for i in 0..n as i64 { | |
| out.push(unsafe { pyre_object::listobject::w_list_getitem(items, i).unwrap() }); | |
| } | |
| Ok(out) | |
| out | |
| #[majit_macros::dont_look_inside] | |
| pub(crate) fn drain_collect_items(items: PyObjectRef) -> Vec<PyObjectRef> { | |
| let _roots = pyre_object::gc_roots::push_roots(); | |
| pyre_object::gc_roots::pin_root(items); | |
| let items_slot = pyre_object::gc_roots::shadow_stack_len() - 1; | |
| let n = unsafe { | |
| pyre_object::listobject::w_list_len(pyre_object::gc_roots::shadow_stack_get(items_slot)) | |
| }; | |
| let mut out: Vec<PyObjectRef> = Vec::with_capacity(n); | |
| for i in 0..n as i64 { | |
| let items = pyre_object::gc_roots::shadow_stack_get(items_slot); | |
| out.push(unsafe { pyre_object::listobject::w_list_getitem(items, i).unwrap() }); | |
| } | |
| out |
🤖 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/baseobjspace.rs` around lines 10439 - 10448, Update
drain_collect_items to reload the rooted items reference through the
shadow-stack slot after any potentially allocating getitem call, rather than
continuing to use the stale local pointer. Also obtain the list length from the
reloaded rooted reference immediately before iteration, and use that refreshed
reference for each w_list_getitem call.
| pub fn park_jit_pending_error(mut err: PyError) { | ||
| let obj = err.to_exc_object(); | ||
| if !obj.is_null() { | ||
| set_jit_pending_exception(obj); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Can to_exc_object() return null, and for which PyErrorKind values?
ast-grep run --pattern 'pub fn to_exc_object(&mut self) -> $_ { $$$ }' --lang rust pyre/pyre-interpreter/src/error.rs
# All producers of park_jit_pending_error.
rg -nP -C5 'park_jit_pending_error' --type=rustRepository: youknowone/pyre
Length of output: 6614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== stack_check relevant lines =="
sed -n '420,475p' pyre/pyre-interpreter/src/stack_check.rs
echo "== to_exc_object context =="
sed -n '1000,1090p' pyre/pyre-interpreter/src/error.rs
echo "== w_exception_new definition/usages =="
rg -n --type=rust 'fn\s+w_exception_new|w_exception_new\(' pyre/pyre-interpreter/src -C 4
echo "== exc_object initialization sites around error construction =="
rg -n --type=rust '\bexc_object\b|PyErrorKind::|PyError\{|PyError::' pyre/pyre-interpreter/src/error.rs pyre/pyre-interpreter/src/stack_check.rs -C 2Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parse Python interp_exceptions W_BaseException.__new__ and _exceptions_w =="
sed -n '1,220p' pyre/pyre-interpreter/src/majit_ir.py
echo "== interp_exceptions.rs signature and implementation =="
rg -n --type=rust 'pub fn w_exception_new|w_exception_new' pyre/pyre-interpreter/src/interp_exceptions.rs -C 5 || true
sed -n '1,220p' pyre/pyre-interpreter/src/interp_exceptions.rs
echo "== C PyObjectRef null semantics =="
rg -n 'PyObjectRef|typedef |pub type PyObjectRef|is_null' pyre/pyre-interpreter/src/typedef.rs pyre/pyre-interpreter/src/*.rs -C 2 | head -n 120Repository: youknowone/pyre
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidates with exception constructors =="
git ls-files pyre/pyre-interpreter/src | grep -E '\.(rs|py|toml)$' | xargs grep -l 'BaseException\|exc_object\|W_BaseException\|w_exception_new' | head -n 100
echo "== interp_exceptions.rs outline/contents if present =="
if [ -f pyre/pyre-interpreter/src/interp_exceptions.rs ]; then
sed -n '1,260p' pyre/pyre-interpreter/src/interp_exceptions.rs
fi
echo "== Python exception definitions =="
rg -n 'class W_BaseException|def w_exception_new|_exceptions_w|class W_(Runtime|Recursion|StopIteration|StopAsyncIteration|MemoryError|ImportError|ModuleNotFound|NameError|AttributeError|KeyError|IndexError|ValueError|TypeError|ZeroDivision|AssertionError|ReferenceError|OSError|FileNotFoundError|GeneratorExit|GeneratorReturn)' pyre -S -g '*.py' -C 4 | head -n 200
echo "== PyObjectRef definition =="
rg -n 'PyObjectRef|is_null' pyre/pyre-interpreter/src/typedef.rs pyre/pyre-interpreter/src -g '*.rs' -C 2 | head -n 80Repository: youknowone/pyre
Length of output: 2038
Catch a null exc_object in park_jit_pending_error.
to_exc_object() can return null when object-space construction fails after allocation; in that case an error from the jit-driver path is lost and the drain loop returns a short result instead of raising. Treat the null as a surfaced internal failure rather than silently discarding the parked error.
🤖 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/stack_check.rs` around lines 447 - 452, Update
park_jit_pending_error to handle a null result from err.to_exc_object() instead
of silently returning. Surface the original parked error as an internal failure
when object-space construction fails, while preserving set_jit_pending_exception
for non-null exception objects.
| // Root the shared reds across the compiled run (it may collect). | ||
| // `items` is already pinned by `ln`; re-pinning is a harmless dup that | ||
| // pops with `ln`'s root scope. `w_iterator` is a bare `ln` local. | ||
| pyre_object::gc_roots::pin_root(w_iterator); | ||
| pyre_object::gc_roots::pin_root(items); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbalanced pin_root grows the shadow stack once per enter attempt.
These two pins have no matching push_roots() scope in this function; they only pop when the caller's _roots guard in _unpackiterable_unknown_length drops. On the paths that break without draining (no compiled loop, fail_index == u32::MAX, BlackholeResult::Failed) the caller keeps iterating and re-enters the hook every jd1_trace_threshold() iterations, pushing another two entries each time — so a long drain that never hands off accumulates roots proportional to iterations / threshold. A local scope is self-balancing and covers exactly the compiled run that needs the pins.
🔒️ Proposed fix: scope the pins to the compiled run
- // Root the shared reds across the compiled run (it may collect).
- // `items` is already pinned by `ln`; re-pinning is a harmless dup that
- // pops with `ln`'s root scope. `w_iterator` is a bare `ln` local.
- pyre_object::gc_roots::pin_root(w_iterator);
- pyre_object::gc_roots::pin_root(items);
+ // Root the shared reds across the compiled run (it may collect), in a
+ // scope of our own so repeated enter attempts do not stack up roots.
+ // `items` is already pinned by `ln`; re-pinning is a harmless dup.
+ let _enter_roots = pyre_object::gc_roots::push_roots();
+ pyre_object::gc_roots::pin_root(w_iterator);
+ pyre_object::gc_roots::pin_root(items);📝 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.
| // Root the shared reds across the compiled run (it may collect). | |
| // `items` is already pinned by `ln`; re-pinning is a harmless dup that | |
| // pops with `ln`'s root scope. `w_iterator` is a bare `ln` local. | |
| pyre_object::gc_roots::pin_root(w_iterator); | |
| pyre_object::gc_roots::pin_root(items); | |
| // Root the shared reds across the compiled run (it may collect), in a | |
| // scope of our own so repeated enter attempts do not stack up roots. | |
| // `items` is already pinned by `ln`; re-pinning is a harmless dup. | |
| let _enter_roots = pyre_object::gc_roots::push_roots(); | |
| pyre_object::gc_roots::pin_root(w_iterator); | |
| pyre_object::gc_roots::pin_root(items); |
🤖 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/eval.rs` around lines 5068 - 5072, Scope the pins in the
compiled-run path around the logic following the pin operations in the relevant
evaluator function, using a local root scope that pushes before pinning
w_iterator and items and pops when the compiled run finishes. Ensure every break
path, including no compiled loop, fail_index == u32::MAX, and
BlackholeResult::Failed, drains this local scope before returning to the caller,
while preserving the existing pin coverage during execution.
| // Discard any pending compiled-side StopIteration; `ln` re-derives its | ||
| // own loop exit. | ||
| let _ = pyre_interpreter::stack_check::drain_jit_pending_exception(); | ||
| let _ = crate::call_jit::take_ca_exception(); | ||
| // Parked last, so the clears above cannot swallow it. | ||
| if let Some(err) = pending_err { | ||
| pyre_interpreter::stack_check::park_jit_pending_error(err); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Blanket-clearing both exception slots can swallow a non-StopIteration error.
drain_jit_pending_exception() is also the delivery channel for the backend's prologue stack-overflow RecursionError (see stack_check.rs), and take_ca_exception() holds whatever a CALL_ASSEMBLER / blackhole callback stashed. Discarding them unconditionally means a stack overflow or an FFI-propagated error raised during the compiled drain vanishes and the caller loop keeps running as if nothing happened — the comment only justifies dropping a compiled-side StopIteration.
Filter by kind so only the loop-exit StopIteration is dropped.
🐛 Proposed fix: keep non-StopIteration errors
- // Discard any pending compiled-side StopIteration; `ln` re-derives its
- // own loop exit.
- let _ = pyre_interpreter::stack_check::drain_jit_pending_exception();
- let _ = crate::call_jit::take_ca_exception();
- // Parked last, so the clears above cannot swallow it.
- if let Some(err) = pending_err {
- pyre_interpreter::stack_check::park_jit_pending_error(err);
- }
+ // Drop only a compiled-side StopIteration (`ln` re-derives its own loop
+ // exit); anything else — a prologue RecursionError, an FFI-propagated
+ // error — must still reach the caller.
+ let drained = pyre_interpreter::stack_check::drain_jit_pending_exception().err();
+ let stashed = crate::call_jit::take_ca_exception();
+ let carried = pending_err
+ .or(drained)
+ .or(stashed)
+ .filter(|e| e.kind != pyre_interpreter::PyErrorKind::StopIteration);
+ // Parked last, so the clears above cannot swallow it.
+ if let Some(err) = carried {
+ pyre_interpreter::stack_check::park_jit_pending_error(err);
+ }📝 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.
| // Discard any pending compiled-side StopIteration; `ln` re-derives its | |
| // own loop exit. | |
| let _ = pyre_interpreter::stack_check::drain_jit_pending_exception(); | |
| let _ = crate::call_jit::take_ca_exception(); | |
| // Parked last, so the clears above cannot swallow it. | |
| if let Some(err) = pending_err { | |
| pyre_interpreter::stack_check::park_jit_pending_error(err); | |
| } | |
| // Drop only a compiled-side StopIteration (`ln` re-derives its own loop | |
| // exit); anything else — a prologue RecursionError, an FFI-propagated | |
| // error — must still reach the caller. | |
| let drained = pyre_interpreter::stack_check::drain_jit_pending_exception().err(); | |
| let stashed = crate::call_jit::take_ca_exception(); | |
| let carried = pending_err | |
| .or(drained) | |
| .or(stashed) | |
| .filter(|e| e.kind != pyre_interpreter::PyErrorKind::StopIteration); | |
| // Parked last, so the clears above cannot swallow it. | |
| if let Some(err) = carried { | |
| pyre_interpreter::stack_check::park_jit_pending_error(err); | |
| } |
🤖 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/eval.rs` around lines 5160 - 5167, Update the exception
cleanup around pending_err to discard only StopIteration values from
drain_jit_pending_exception() and take_ca_exception(). Preserve and re-propagate
any other error, including stack-overflow RecursionError and callback/FFI
errors, while keeping the existing pending_err parking behavior.
1 participant
Resolves the jd1 (
unpackiterable_driver) novable-drain live blackholeresume, then makes the fixed driver the default: entering the compiled drain
live, blackhole-resuming it on a guard failure against the build-time
jitcode/liveness/descr stores, and draining into a
W_List— end-to-endOK builtin 3000.The final commit flips jd1 from opt-in (
PYRE_JD1=1/PYRE_JD1_ENTER) toon by default, so the default runtime now traces, compiles, and
live-enters the
unpackiterable_driverdrain on hot non-tuple unpack sites.The previous inert behavior is reachable with
PYRE_NO_JD1; jd1 also followsthe master JIT off-switches (
PYRE_NO_JIT/PYRE_JIT=0).Commits
setarrayitem_gctrace heap cache — after the trace-walk store, callctx.heapcache_setarrayitemkeyed ondescr.index()so the siblinggetarrayitem read cannot return a value cached before the store (Codex P2 on
jit: register the jd1 unpackiterable drain jitcode and compile its drain loop #741).
vinfoso the drain's blackhole frame reconstructs its reds(
[w_iterator, items], bothType::Ref) from the right jitdriver.drain has no Python
CodeObject; resolve its resumerd_numb/ liveness /descr against the build-time jitcode/liveness/descr stores instead of a
frame-chain walk.
novable portal's degenerate
PyJitCodecarries a nullcode_ptr, soraw_code_for_jitcode_indexreturnsNonefor a null pointer rather thanhanding it to the instruction-decoding consumers (bare-reraise probe,
traceback lineno). This commit also originally deleted the orphaned
build_multi_frame_miframelast_caught_exception_valuereads (#763referenced a field
#756had removed → E0609); after rebasing onto currentmainthat deletion converged with upstream#765("drop jit(fbw): multi-frame blackhole-resume build path + input-arg _resref seed (adoption gated) #763'sresidual last_caught_exception_value propagation"), which made the identical
removal, so the rebase absorbed it and this commit now carries only the
state.rsnull-guard.dont_look_insideseam —inlining
w_list_appendinto the drain jitcode surfaced each of append'sstrategy/grow helpers (
object_push,switch_to_correct_strategy,typed-array grow, …) as a separate unresolved residual funcptr → symbolic
hash → SIGBUS. Route the drain's append through a
dont_look_insidedrain_list_appendwrapper and register it (with the already-residualw_list_new_emptyprologue anddrain_collect_itemsepilogue). The globallist.appendkeeps callingw_list_appenddirectly and stays traced, sothe append fold and the escape-flush replay are unaffected. Also adds
int_is_true/i>ito the curated inline-call blackhole builder — the drain'sback-edge guard emits it and it was missing, panicking a blackhole-executed
drain at the first back-edge test.
test_result_exc_lowering.rslowers the real
_unpackiterable_unknown_lengthfrom the production LLBC andasserts the Facet A (jit: fuse jd1 _unpackiterable_unknown_length drain-match into an exception-edge handler #703)
try_fuse_drain_matchfusion actually fires:the synthesized
exc_kind_discriminantkind-test is present, noStopIterationctor survives, and thenext()site is aLastExceptionedge. The fusion is fail-safe (silent decline →
catch_and_rewrap), thedefault non-jd1 run never executed it, and the
unpack_drain_exact_kindparity test only guards the default path — so this drain rework (commit 5)
or any recognizer regression that silently stopped the fusion was previously
invisible while reopening the jd1 SIGBUS.
jd1_experiment_enabledflipsfrom opt-in (
PYRE_JD1=1) to on by default, returning false only forPYRE_NO_JD1/PYRE_JD1=0/PYRE_NO_JIT/PYRE_JIT=0. A newjd1_enter_enabledgates theRunCompiledlive enter, on by default with aPYRE_JD1_NO_ENTERopt-out, replacing the formerPYRE_JD1_ENTERopt-in.Verification
check.py --backend dynasm, default env (jd1 now on by default):303/303 ALL PASSED — including the perf gates.
Compiled) and live-enters the compiled drain (4 enters, 200→3000 items
per enter) →
OK 24000.PYRE_NO_JD1=1makes it fully inert (0 counter/compile/enter) and still
OK 24000.cargo test -p majit-translate --test test_result_exc_lowering: 6/6(incl. the drain-fusion firing guard:
exc_kind_discriminant=1,stopiteration_ctors=0,lastexc_blocks=2on the real drain).899880005 30 73411).synth/getframe_force_cancel_journal→20000 20000 989403(this testbriefly regressed to
19995under an earlier append-dont_look_insideapproach that residualized the global append; commit 5 keeps the global
append folded, restoring it).
Follow-up (not in this PR)
Making the global append a residual exposes a separate pre-existing latent
bug: a journaled-append residual under a FOR_ITER item + a
sys._getframeframe-force makes the escape-flush withdraw and the resume disagree — the resume
adopts a forward continuation past
STORE_FASTinstead of the full legacyreplay, dropping the store. That path (post-withdraw resume/journal, touched by
#756/#749/#763) is out of jd1 scope; this PR sidesteps it by keeping the global
append traced/folded.
🤖 Generated with Claude Code
Summary by CodeRabbit