FBW guard-snapshot py_pc derivation, exception-resume routing, and residual-call opcode lowering batch - #183
Merged
Conversation
WalkthroughThis PR expands JIT support for many Python opcodes (StoreAttr, DeleteAttr, BinarySlice, ContainsOp, IsOp, BuildMap, BuildSet, BuildString, ImportName, LoadSuperAttr, LoadDeref, LoadFastCheck, FormatSimple, FormatWithSpec, ConvertValue, UnaryInvert, UnaryNot) by wiring new runtime_ops helpers, blackhole residuals, Cpu function pointers, and codewriter graph lowering. It also fixes GC exception rooting in the dynasm backend, rewrites blackhole exception-handler dispatch, adds a cooperative trace-abort mechanism for unencodable resume PCs, integrates GC write barriers for set objects, and adds synthetic benchmarks for each new opcode. ChangesNew opcode JIT support, trace safety, and GC correctness
Sequence Diagram(s)sequenceDiagram
participant JIT_Guard
participant handle_fail_resume_guard
participant gc_add_root
participant blackhole_interpreter
participant BH_LAST_EXC_VALUE
participant jit_exc_raise
JIT_Guard->>handle_fail_resume_guard: guard failure with jf_guard_exc
handle_fail_resume_guard->>gc_add_root: root guard_exc as GcRef (if non-null)
handle_fail_resume_guard->>blackhole_interpreter: invoke with rooted exc as i64
blackhole_interpreter->>BH_LAST_EXC_VALUE: write exception on residual error
blackhole_interpreter->>jit_exc_raise: publish_residual_call_exception
blackhole_interpreter-->>handle_fail_resume_guard: result
handle_fail_resume_guard->>gc_add_root: gc_remove_root
sequenceDiagram
participant codewriter
participant FnPtrIndices
participant LoweringContext
participant JitCode
participant bh_store_attr_fn
participant publish_residual_call_exception
codewriter->>FnPtrIndices: register_helper_fn_pointers (store_attr_fn, etc.)
codewriter->>LoweringContext: thread new indices into flatten-time lowering
codewriter->>JitCode: emit store_attr HLOp (obj, value, code_const, name_idx_const)
JitCode->>bh_store_attr_fn: call on guard failure / residual
bh_store_attr_fn->>publish_residual_call_exception: on error → BH_LAST_EXC_VALUE + jit_exc_raise
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
`handle_fail_resume_guard` reads `jf_guard_exc` (a GCREF visited by `jitframe_trace`) and clears the JitFrame slot before invoking the bridge trace/compile hook, which can allocate and trigger a moving nursery collection. With the slot cleared the exception object is no longer rooted, so a collection during the bridge hook can relocate it and leave the bare `usize` copy stale before it reaches the blackhole resume. Hold the value in a `GcRef` local and register it with `gc_add_root` for the duration of the bridge hook, reading the possibly-relocated pointer back from the rooted slot before passing it to the blackhole and dropping the root. This reproduces the shadowstack rooting RPython gives the `grab_exc_value` result automatically (llmodel.py:240). Assisted-by: Claude
…nstr publish - call_jit.rs: publish_residual_call_exception writes both BH_LAST_EXC_VALUE and the backend jit_exc_raise stores; all four Err branches of bh_call_fn_impl use it. - blackhole.rs: handle_exception_in_frame scans forward then backward for the catch_exception; route_to_catch sets exception_last_value, position, clears BH. - codewriter.rs: emit_abort_permanent! publishes setfield_vable_i(last_instr=py_pc-1) before the abort marker so the interpreter resumes at the unsupported opcode. - jitcode_dispatch.rs: PYRE_51C_RELAX toggle on walker_abort_if_protected_may_force gate plus PYRE_51C_DIAG diagnostics. - pyre-interpreter/eval.rs: PYRE_51C_DIAG underflow diagnostic in pop_value. Assisted-by: Claude
…bort The emit_abort_permanent! macro requires a $py_pc:expr argument (it publishes last_instr = py_pc - 1 to the vable before the bail). The STORE_GLOBAL match arm still called it with no argument, which does not match the single-arm macro and breaks the build. Pass py_pc, matching the sibling abort sites in the same dispatch. Assisted-by: Claude
FunctionGraph::new created the exceptblock's two inputargs with fresh_untyped_variable (kind=None). make_dependencies registers a block inputarg for coloring only when its kind matches the bank, so the pair was skipped by every kind bank and left uncolored. A bare `raise` in a nested inner handler produces a non-reraise propagate edge into the exceptblock that falls to the generic make_link / generate_last_exc / make_return path, which colors those inputargs and hits regalloc_color's missing-color panic during portal build (make_jitcodes). Assign the kinds the inputargs are read back with: etype via last_exception/>i (Int), evalue via last_exc_value/>r (Ref). Matches the typed pair exception_edge_vars already mints for explicit catch edges. Assisted-by: Claude
The full-body walk declined any jitcode body containing a `catch_exception/L` whenever it reached a may-force can-raise residual call (`walker_abort_if_protected_may_force` → `MayForceProtectedByExceptionHandlerUnsupported` → trait fallback). The gap that motivated the gate — the `GUARD_NO_EXCEPTION` deopt routing into a handler whose standing exception value the walker resume could not seed — is now closed (exception-resume publication + handler routing), so these bodies walk and resume correctly. Remove the gate function, its three residual-call call sites, the `MayForceProtectedByExceptionHandlerUnsupported` DispatchError variant and its trace.rs decline arm, and the `PYRE_51C_RELAX` toggle. Keep `jitcode_has_exception_handler` for the CALL_ASSEMBLER / LOAD_GLOBAL fast paths, which still conservatively decline a handler-bearing body to the generic residual walk rather than to their concrete fold, and update their comments to drop the removed trait-fallback rationale. Strip the `PYRE_51C_DIAG` diagnostics. Verified byte-exact against CPython on try/except, multi-except, else, bare-reraise, nested-reraise, e.args[0]-reading handlers, and self-recursive raise-in-try loops. Gates: check.py dynasm 55/55 + cranelift 55/55; pyre-jit-trace 265/0, pyre-jit 270/0, pyre-interpreter 360/0, majit-metainterp 0 failed. Assisted-by: Claude
…nsert `alloc_set_with_type` allocated `W_SetObject` through `malloc_typed` (`alloc_with_gc_header`, flags=0), so the body lacked TRACK_YOUNG_PTRS. `w_set_add` stores possibly-young elements (e.g. nursery ints produced by a JIT-compiled loop) into `items`, but with no TRACK_YOUNG_PTRS the write barrier was a no-op, the set never entered the remembered set, and the minor collector never ran `set_object_custom_trace` to forward those elements -- they were collected, leaving dangling pointers whose later `ob_type` read segfaulted. Allocate the body via `try_gc_alloc_stable` (old-gen, mark-sweep, TRACK_YOUNG_PTRS) with a `malloc_typed` fallback, mirroring `w_list_new` / `w_tuple_new`, and fire `set_write_barrier` after each insert, mirroring `dict_write_barrier`. Assisted-by: Claude
CONTAINS_OP previously hit `emit_abort_permanent` in the codewriter, so
any hot loop with `in` / `not in` declined to the interpreter. Reuse the
existing compare-residual path: `compare_op_tag_for_opname` maps
`contains` -> 6 and `not_contains` -> 7, the codewriter lowers
`ContainsOp { invert }` to a `contains` / `not_contains` graph op over
`[item, container]` (mirroring `CompareOp`), and both the
`compare_value_from_tag` interpreter helper and the `bh_compare_fn`
blackhole/compiled-trace helper dispatch tags 6/7 to
`baseobjspace::contains` (tag 7 negates). `not_contains` joins
`contains` in simplify's CAN_REMOVE set.
set_membership's membership loop now compiles and stays byte-exact
(61259104618); check.py 57/57 on both backends.
Assisted-by: Claude
STORE_ATTR previously emitted the elidable `setattr` HLOp (rewritten to setfield_gc) and then aborted the full-body walk. Add a `store_attr` HLOp and `bh_store_attr_fn(obj, value, code, name_idx)` residual, symmetric to LoadAttr's `load_attr_fn`, lowered to `residual_call_ir_v`. The codewriter StoreAttr arm now records `store_attr` with the code-object / name-index surrogate operands and no longer aborts. - call_jit.rs: bh_store_attr_fn resolves the name via the code object and runs baseobjspace::setattr_str; publishes exceptions through BH_LAST_EXC_VALUE for the trailing GuardNoException. - cpu.rs: store_attr_fn field + binding. - codewriter.rs: register store_attr_fn (MayForce, appended last to preserve fn-ptr indices); emit_frontend_store_attr; StoreAttr arm rewrite. - flatten.rs: LoweringContext.store_attr_fn_idx; lower_setattr_hlop_to_insn; dispatch arm; lowering test. attr_store_cache now JIT-compiles its loop (no FBW abort) and stays byte-exact; instance_dict_reassign and type_name_setter stay byte-exact with their remaining non-STORE_ATTR aborts. Assisted-by: Claude
BUILD_MAP previously aborted the full-body walk. Record the BuildTuple-style
`new_array_clear` + unrolled `setarrayitem_gc_r` pair array, then a single
`build_map_from_array(array)` residual consuming the forced
`[k0, v0, k1, v1, ...]` array. The array-build machinery is reused as-is;
only the final call helper is new.
- call_jit.rs: bh_build_map_from_array reads the length-prefixed array and
runs runtime_ops::build_map_from_refs. Key insertion hashes (user
__hash__ / __eq__ may run), so the call is MayForce.
- cpu.rs: build_map_from_array_fn field + binding.
- codewriter.rs: register build_map_from_array_fn (MayForce, appended last);
BuildMap arm builds the array + records build_map_from_array. count == 0
({}) declines — the only corpus site is `type(name, (), {})`, whose raise
exercises the unsupported exception-resume-through-call path.
- flatten.rs: LoweringContext.build_map_from_array_fn_idx;
lower_tuple_build_hlop_to_insn build_map_from_array arm
(residual_call_r_r, MayForce); lowering test.
Non-empty dict literals JIT-compile byte-exact (synth + instance_dict_reassign,
both backends); instance_dict_reassign's BUILD_MAP abort is replaced by its
remaining portal-exit decline.
Assisted-by: Claude
Extract the interpreter binary_slice body into runtime_ops::binary_slice_values(obj, start, stop); both the interpreter method and the new bh_binary_slice_fn residual now call it. The codewriter BinarySlice arm records a binary_slice(obj, start, stop) HLOp (Ref result) instead of emit_abort_permanent!. flatten::lower_binary_slice_hlop_to_insn lowers it to residual_call_r_r(binary_slice_fn_idx, [obj, start, stop]) with CallFlavor::MayForce. The helper is bound through cpu.binary_slice_fn and register_helper_fn_pointers (appended last to preserve fn-ptr indices); on error it publishes through BH_LAST_EXC_VALUE for the trailing GuardNoException, matching bh_load_attr_fn. Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Assisted-by: Claude
Also extracts the shared format evaluation into runtime_ops::format_value, called by both the interpreter format_simple/format_with_spec and the residual. Assisted-by: Claude
Reuses runtime_ops::format_value (extracted for FORMAT_SIMPLE), passing the popped spec instead of the empty spec. Assisted-by: Claude
Builds the fragment array (new_array_clear + setarrayitem_gc_r, the BuildSet template) and concatenates it through the shared runtime_ops::build_string_from_refs. Plain flavor: fragments are already strings, so no user code runs. Assisted-by: Claude
Bakes the conversion kind (str/repr/ascii) as a compile-time constant and
threads it through a residual_call_ir_r. Extracts the shared conversion into
runtime_ops::convert_value{,_code}, called by both the interpreter
convert_value and the residual.
Assisted-by: Claude
Route IS_OP through the same compare_fn residual as COMPARE_OP and CONTAINS_OP: add op_code 8 (`is`) and 9 (`is not`) to bh_compare_fn (pointer identity, infallible), map "is"/"is_not" in compare_op_tag_for_opname, and emit an is/is_not compare graph-op from the codewriter IsOp arm in place of emit_abort_permanent!. Assisted-by: Claude
Drain the IMPORT_NAME emit_abort_permanent arm: the codewriter records an `import_name(fromlist, level, code, name_idx)` HLOp lowered to `residual_call_ir_r(import_name_fn, ListI[name_idx], ListR[fromlist, level, code])` — three Ref operands plus one Int, the result-producing counterpart of the void STORE_ATTR 3-Ref residual. `bh_import_name_fn` resolves the module name from the jitcode's code object, reads the active frame's wrapped globals through getexecutioncontext().gettopframe() .get_w_globals_obj() for relative-import package resolution, and runs __import__ (MayForce). Assisted-by: Claude
Replace the LoadSuperAttr emit_abort_permanent arm with two residuals: load_super_attr(self, cls, code, name_idx) resolving getattr(super(cls, self), name) via residual_call_ir_r (3-Ref + 1-Int, MayForce), and super_attr_unwrap(raw, which) for the method form's func/self split via residual_call_ir_r (1-Ref + 1-Int). bh_load_super_attr_fn builds the super proxy with w_super_new and runs baseobjspace::getattr_str; bh_super_attr_unwrap_fn extracts the bound method's func/self or pushes the raw attr plus NULL. The codewriter branches on the compile-time oparg (name_idx = oparg >> 2, is_method = oparg & 1): is_method=false pushes the raw result; is_method=true pushes super_attr_unwrap(raw, 0) then super_attr_unwrap(raw, 1). bench synth/load_super_attr exercises super(Child, self).val() in a hot loop. check.py dynasm 67/67 + cranelift 67/67. Assisted-by: Claude
Split LoadDeref out of the grouped emit_abort_permanent arm. The cell object lives in the same vable locals_cells_stack_w array as the plain locals, so the unified deref index is read exactly like LOAD_FAST through emit_load_fast_ref! (the getarrayitem_vable_r path, frame-relative and inlining-safe); the cell pointer then feeds a load_deref_value(cell) HLOp lowered to residual_call_r_r (single-Ref, the FORMAT_SIMPLE shape). bh_load_deref_value_fn dereferences the cell and raises on an unbound free variable. It reads mutable heap but runs no user code, so CallFlavor::Plain (default_effect_info, CanRaise, no virtualizable force) rather than MayForce. bench synth/load_deref exercises a closure reading a captured variable in a hot loop. check.py dynasm 68/68 + cranelift 68/68. Assisted-by: Claude
Split UnaryInvert out of the grouped emit_abort_permanent arm (UnaryNot and GetYieldFromIter still abort). The unary_invert(value) HLOp lowers to residual_call_r_r (single-Ref, the FORMAT_SIMPLE shape); bh_unary_invert_fn computes ~value through opcode_ops::unary_invert_value. A user __invert__ may run Python, so CallFlavor::MayForce. bench synth/unary_invert exercises ~i in a hot loop. check.py dynasm 69/69 + cranelift 69/69. Assisted-by: Claude
Split UnaryNot out of the grouped emit_abort_permanent arm (GetYieldFromIter still aborts). The unary_not(value) HLOp lowers to residual_call_r_r (single-Ref, the FORMAT_SIMPLE shape); bh_unary_not_fn returns `not value` as a bool through opcode_ops::truth_value. A user __bool__ / __len__ may run Python, so CallFlavor::MayForce; truth_value does not surface an error (the JIT and the interpreter share is_true), so the helper is infallible. bench synth/unary_not exercises `not (i & 1)` in a hot loop. check.py dynasm 70/70 + cranelift 70/70. Assisted-by: Claude
Replace the LOAD_FAST_CHECK emit_abort_permanent arm with a load_fast_check HLOp. The local is read from the vable like LOAD_FAST (emit_load_fast_ref) and handed to bh_load_fast_check_fn(value, code, name_idx), which returns the value when bound or raises NameError resolving the name from co_varnames via name_idx. Lowered to residual_call_ir_r (ListR[value, code], ListI[name_idx]) with CallFlavor::Plain — the helper reads no heap and runs no user code. Add the synth/load_fast_check bench (a conditionally-bound local read in a hot loop, always bound at runtime). Assisted-by: Claude
The authoritative full-body walk mis-handles a loop whose body contains an `abort_permanent` marker (e.g. the SWAP an `a < b < c` chained comparison lowers to). The unported in-loop op breaks the loop-input register seeding, so the walk evaluates the loop guard against garbage, exits the loop on the first pass, and concretely executes the post-loop tail — double-running its side effects (a doubled `print`) and leaving the frame positioned past the loop. The reactive in-walk `abort_permanent` decline never fires because the corrupted guard exits before reaching the marker. `full_body_walk_trace` now scans the JitCode for an `abort_permanent` at or after the first `jit_merge_point` (inner loop header) and declines to the trait tracer up front, reaching the same outcome the reactive decline would without the frame corruption. The scan is scoped past the prologue so a prologue-only marker (e.g. COPY_FREE_VARS ahead of a clean hot loop) does not over-decline. bench/synth/chained_comparison.py pins the case (was wrong + doubled, both backends; now byte-exact). Assisted-by: Claude
The full-body walk's top-level `void_return/` arm recorded FINISH([]) via `trace_ctx.finish` and returned Terminate without stashing a finish payload, so `full_body_walk_trace` read no payload and mapped Terminate to Abort, declining the void portal exit to the trait tracer. Under the PYRE_FBW_CALL_ASSEMBLER gate, route the arm through a new `fbw_terminate_void_with_finish` that stores the assembler token in the vable and stashes a Type::Void-marked payload, mirroring the three value-returning arms. `full_body_walk_trace` maps that payload to `TraceAction::Finish` with empty args; the compile pipeline resolves the empty finish_arg_types to done_with_this_frame_descr_void, matching the trait tracer's void-return action. Assisted-by: Claude
… of panicking The cross-frame guard-snapshot path (get_list_of_active_boxes, marker_aware_resume_pc, marker_aware_parent_resume_pc) panicked when a frame reported a resume pc missing from the jitcode pc_map or outside the bit-14 after-residual-call marker range. The comments claimed the recording loop's catch_unwind converted these to AbortPermanent, but the pyre tracer runs metainterp::interpret, which has no such catch, so they crashed the process — reachable for an inlined callee + exception-resume shape (a try-protected call that raises in a hot loop). Replace the .expect / assert! sites with state::request_trace_abort, a thread-local flag that metainterp::interpret polls after each step and full_body_walk_trace polls after the walk; both return TraceAction::Abort. The guard is discarded with the pre-install trace, so the clamped/empty resume data is never decoded and the location interprets. Assisted-by: Claude
The walker's goto_if_not arm unconditionally ran replace_box on the guarded condbox (to CONST_0/CONST_1) and rewrote every matching registers_i slot. The condbox feeding GOTO_IF_NOT is always an int_is_* family result (the switchcase 0/1 invariant); RPython dispatches those — opimpl_goto_if_not_int_is_true / _int_is_zero and the int_lt..float_ge fusions — with replace=False (pyjitpl.py:529-556, "does not make sense to replace condbox"). Only the bare opimpl_goto_if_not(replace=True) promotes its operand. Drop the replace_box / register rewrite to match; the guard still establishes the value for the optimizer. Assisted-by: Claude
The can-raise residual helpers in call_jit.rs that run inside a compiled trace set only BH_LAST_EXC_VALUE on error, so the trailing GUARD_NO_EXCEPTION read a stale backend cell and the helper's null result flowed to the consumer. Route them through publish_residual_call_exception (BH_LAST_EXC_VALUE + jit_exc_raise on both backends), matching the generic CALL helper: store_attr, delete_attr, import_name, load_super_attr, binary_slice, delete_subscr, build_set, format_simple, convert_value, format_with_spec, load_deref, unary_invert, load_fast_check, compare, binary_op, load_global, and the self-recursive portal call. The blackhole-only helpers (getattr, load_attr, load_name, store_name) keep the BH_LAST_EXC_VALUE-only path; they never run in a compiled trace so publishing would leave a stale backend cell. Assisted-by: Claude
…pagate `build_map_from_refs` stored each pair through the infallible `w_dict_store`, silently swallowing an unhashable-key or `__eq__`-raising error. Switch it to `w_dict_store_checked` and return `Result<PyObjectRef, PyError>`, converting the dict error via `take_pending_hash_error`. Propagate the new fallibility through all three callers: - interpreter `build_map` (eval.rs) returns the Result directly. - MIFrame `build_map` (opcode_handler_impls template + snapshot) `?`-forwards. - JIT array residual `bh_build_map_from_array` publishes the exception through `publish_residual_call_exception` for the trailing GuardNoException. - legacy fixed-arity `build_map_from_args` (blackhole-only) signals through `BH_LAST_EXC_VALUE` and returns PY_NULL. Assisted-by: Claude
Both the interpreter (`load_deref`) and the JIT residual
(`bh_load_deref_value_fn`) raised a bare TypeError "free variable referenced
before assignment" for an empty deref slot. Replace it with the named
unbound-variable error via a shared `pyframe::deref_unbound_error(code, idx)`
helper: a cell variable (captured local or pure cellvar) reports "local
variable '{name}' referenced before assignment"; a free variable reports
"free variable '{name}' referenced before assignment in enclosing scope".
Both use NameError, matching `load_local_checked_value`. The name is resolved
through the `npure_cellvars` deref-slot layout.
Thread `code` + `deref_idx` into the JIT residual so it can resolve the name:
`bh_load_deref_value_fn(cell, code, deref_idx)`, lowering the `load_deref_value`
HLOp from the 1-arg `residual_call_r_r` to the 3-arg `residual_call_ir_r`
(ListR[cell, code], ListI[deref_idx]) shape, mirroring LOAD_FAST_CHECK. The
codewriter LOAD_DEREF arm bakes the `w_code` and `deref_idx` constants; cpu.rs
widens the fn pointer signature.
Assisted-by: Claude
`bh_import_name_fn` resolved the importing frame's `__name__`/`__package__` (relative-import package resolution) through `getexecutioncontext().gettopframe()`, which collapses to the wrong frame for an inlined non-portal callee. Thread the live red frame as an explicit Ref argument instead, mirroring `bh_load_global_fn`'s frame pointer: read globals via `(*frame).get_w_globals_obj()`, keeping the execution context only for the `importhook` call. Extend the `import_name` HLOp to `import_name(fromlist, level, code, frame, name_idx)`; the codewriter passes `frame_var`, the flatten lowering grows to a 4-Ref `residual_call_ir_r` (ListR[fromlist, level, code, frame]), and cpu.rs widens the fn pointer signature. Assisted-by: Claude
`collect_outer_active_boxes` read the liveness register banks at the snapshot's resume `py_pc` but bounded the operand-stack-slot classification window with `sym.valuestackdepth` — the walker's CURRENT position, not the resume target's. The two coincide for the per-opcode entry caller and for a guard resuming at its own opcode, but a guard resuming at a not-taken branch target that still carries a live operand-stack temp resumes at a py_pc whose stack depth differs from the walker's. With the smaller current depth, `semantic_ref_slot_for_reg_color` truncates `stack_color_map` and fails to map the kept temp's color to its stack slot. Compute the window from `liveness_for(code_ptr).depth_at_py_pc()` at the same `entry_py_pc` the banks are read at, so the slot classification and the liveness banks share one coordinate. Assisted-by: Claude
`walker_capture_snapshot_for_last_guard_impl` published `last_instr` from the resolved resume py_pc but left the `valuestackdepth` vable scalar at whatever the walk seed wrote. The walker never crosses `set_orgpc`, so that scalar keeps the loop-entry inputarg (a loop-invariant the optimizer folds to a constant). A guard resuming at a shallower depth than the loop seed — the `while` condition branch of a loop whose body keeps an operand-stack temp — then encodes the seed depth into its snapshot. The first-level blackhole resume tolerates the stale scalar, but `setup_bridge_sym` derives `stack_only = valuestackdepth - nlocals` from it; an over-count fabricates a phantom operand-stack slot seeded from the wrong vable position (the `pycode` scalar reads back as a `code` object), corrupting any bridge compiled from that guard. Publish `valuestackdepth` from `sym.valuestackdepth` the same way `last_instr` is published. check.py 72/72 dynasm + cranelift; pyre-jit 284, pyre-jit-trace 252. Assisted-by: Claude
youknowone
marked this pull request as ready for review
June 15, 2026 01:58
youknowone
changed the title
![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: 78f67845dd
ℹ️ 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".
Comment on lines
+496
to
+499
| #[cfg(feature = "cranelift")] | ||
| majit_backend_cranelift::jit_exc_raise(exc_obj); | ||
| #[cfg(feature = "dynasm")] | ||
| majit_backend_dynasm::jit_exc_raise(exc_obj); |
There was a problem hiding this comment.
When an authoritative walker concretely executes these bh_* helpers via execute_residual_call, the same helper now writes the backend jit_exc cells even though no compiled GuardNoException will consume and reset them; execute_residual_call only drains BH_LAST_EXC_VALUE, and the walker success/exception paths only update WalkContext state. If a residual raises while tracing and is caught or the trace aborts, the next compiled trace can see the stale backend exception value and side-exit as if its own call raised. Gate the backend publish to actual compiled execution or clear the backend cells around trace-time executor calls.
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit/src/jit/codewriter.rs (1)
5253-5300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAnchor
abort_permanentto the same PC as thelast_instrpublish.The macro records
setfield_vable_i(last_instr)atpy_pc, then recordsabort_permanentwith offset-1. The canonical flattener uses offsets for PC placement, so the abort can be placed before or outside the PC-locallast_instrstore; if that happens, the blackhole bails before publishing the resume coordinate this macro is trying to fix.🐛 Proposed fix
- // `abort_permanent` is pyre-specific (no upstream - // RPython counterpart); use `offset = -1` matching - // `emit_vsd!`'s synthetic-op convention since - // `abort_permanent` is an emission-time bail-out - // marker, not tied to a single Python bytecode PC. + // `abort_permanent` belongs to the unsupported bytecode PC. + // Keep it anchored after the `last_instr` vable write for the + // same PC so the bail observes the published resume coordinate. record_graph_op( ¤t_block.block(), "abort_permanent", Vec::new(), None, - -1, + ($py_pc) as i64, );🤖 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/jit/codewriter.rs` around lines 5253 - 5300, The abort_permanent operation is recorded with offset -1, which allows the canonical flattener to place it before the setfield_vable_i operation that publishes last_instr. Change the last parameter in the second record_graph_op call (the one recording abort_permanent) from -1 to ($py_pc) as i64 to anchor abort_permanent to the same PC as the last_instr publish, ensuring the resume coordinate is properly set before the blackhole bails out.pyre/pyre-jit-trace/src/trace_opcode.rs (1)
3990-4023:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish converting all resume-PC bound failures to trace aborts.
The marker-aware helpers now fall back through
abort_unencodable_resume_pc, but branch and exception guard snapshots stillassert!on the same bit-14 limit at Line 5099 and Line 8552. A large function can still panic instead of aborting the trace and falling back to the interpreter.Suggested fix
- assert!( - resume_pc < majit_ir::resumedata::AFTER_RESIDUAL_CALL_PC_FLAG as usize, - "branch-guard resume pc {resume_pc} >= AFTER_RESIDUAL_CALL_PC_FLAG; \ - function too large for bit-14 resume encoding" - ); + let resume_pc = + if resume_pc >= majit_ir::resumedata::AFTER_RESIDUAL_CALL_PC_FLAG as usize { + crate::state::abort_unencodable_resume_pc(resume_pc) + } else { + resume_pc + }; let snapshot = self.build_framestack_snapshot( ctx, resume_pc,- assert!( - resume_pc < majit_ir::resumedata::AFTER_RESIDUAL_CALL_PC_FLAG as usize, - "exception-guard resume pc {resume_pc} >= AFTER_RESIDUAL_CALL_PC_FLAG; \ - function too large for bit-14 resume encoding" - ); + let resume_pc = + if resume_pc >= majit_ir::resumedata::AFTER_RESIDUAL_CALL_PC_FLAG as usize { + crate::state::abort_unencodable_resume_pc(resume_pc) + } else { + resume_pc + }; let snapshot = this.build_framestack_snapshot(ctx, resume_pc, &active_boxes, &fail_arg_types);Also applies to: 4052-4059, 5099-5103, 8552-8556
🤖 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_opcode.rs` around lines 3990 - 4023, Multiple locations in the file still use assert! statements to check the bit-14 limit for resume PC encoding, which causes panics instead of gracefully falling back to the interpreter. At lines 4052-4059, 5099-5103, and 8552-8556, replace the assert! statements that check for the bit-14 flag limit with calls to abort_unencodable_resume_pc (similar to how marker_aware_resume_pc now handles these failures), ensuring large functions trigger trace aborts and interpreter fallback rather than panicking. This should follow the same pattern shown in the marker_aware_resume_pc function where bit-14 limit violations are detected and handled via abort_unencodable_resume_pc instead of assertions.
🤖 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-metainterp/src/blackhole.rs`:
- Around line 1093-1116: The issue is that when exception handling is triggered
from residual-call handlers through check_residual_call_exception_after(), the
self.position used as resume_live_pos is still pointing to the first operand
rather than the decoded post-op position. This causes Line 1100 to potentially
inspect operand bytes as opcodes and Line 1115 to fail finding the correct
catch_exception. To fix this, before each call to handle_exception_in_frame from
residual-call handlers, either set bh.position to the decoded post-op position
prior to the call, or pass the correct decoded post-op PC as a parameter into
the helper method. For void residual calls specifically, use p instead of p + 1
when computing the post-op position.
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 923-937: The abort flag is checked after run_perfn_walk() has
already executed and committed side effects (end-flush/store-journal state). If
an unencodable resume coordinate requested abort during the walk, those side
effects persist even though the trace is discarded. Move the abort flag check to
occur before run_perfn_walk() completes its side effect commits. This may
require restructuring run_perfn_walk to check the abort flag before committing
state changes (around lines 691-774 where side effects are persisted), or
deferring the side effect commits until after the caller has confirmed the trace
is valid by checking crate::state::take_trace_abort_requested().
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 7113-7116: The unsupported-path abort needs to occur before
runtime stack mutations to prevent incorrect interpreter state on re-execution.
Move the emit_abort_permanent! call that checks if nargs > 8 to execute BEFORE
the push_and_bump! call in the codewriter.rs function, ensuring that
stack-modifying operations do not execute in the unsupported code path. This
same pattern issue exists at multiple locations in the file and should be fixed
consistently wherever unsupported opcode aborts are followed by stack mutations
like push_and_bump! or similar emit calls.
- Around line 8815-8825: The emit_load_fast_ref! macro currently falls back to
current_state.locals_w for non-portal graphs, which excludes cell/free slots and
synthesizes fresh Refs for missing values. This causes load_deref_value and
load_fast_check opcodes to receive synthetic values instead of actual localsplus
slot values, losing closure values or unbound-variable errors in non-portal
jitcodes. For both the LOAD_DEREF case (around line 8815-8825) and the
LOAD_FAST_CHECK case (around line 8854-8864), modify the code to use a
frame-backed localsplus read whenever FrameInputs::Frame is present, or
alternatively keep the non-portal cases abort-only until the frame-backed read
path is properly wired.
- Around line 8646-8659: The `global_super` operand is being popped from the
stack and stored in `_global_super` but then discarded before being passed to
`emit_frontend_load_super_attr`. This causes loss of critical runtime
information needed to distinguish between built-in `super()` and
shadowed/rebound `super`. Pass the `global_super` value (instead of discarding
it as `_global_super`) through the `emit_frontend_load_super_attr` function
signature and any related HLOp structures to preserve this distinction. Apply
the identical fix in the interpreter at `pyre/pyre-interpreter/src/eval.rs:3109`
where `_global_super` is also being discarded in the equivalent code path.
---
Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace_opcode.rs`:
- Around line 3990-4023: Multiple locations in the file still use assert!
statements to check the bit-14 limit for resume PC encoding, which causes panics
instead of gracefully falling back to the interpreter. At lines 4052-4059,
5099-5103, and 8552-8556, replace the assert! statements that check for the
bit-14 flag limit with calls to abort_unencodable_resume_pc (similar to how
marker_aware_resume_pc now handles these failures), ensuring large functions
trigger trace aborts and interpreter fallback rather than panicking. This should
follow the same pattern shown in the marker_aware_resume_pc function where
bit-14 limit violations are detected and handled via abort_unencodable_resume_pc
instead of assertions.
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 5253-5300: The abort_permanent operation is recorded with offset
-1, which allows the canonical flattener to place it before the setfield_vable_i
operation that publishes last_instr. Change the last parameter in the second
record_graph_op call (the one recording abort_permanent) from -1 to ($py_pc) as
i64 to anchor abort_permanent to the same PC as the last_instr publish, ensuring
the resume coordinate is properly set before the blackhole bails out.
🪄 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: 16f6c9a9-2b3b-4084-a49e-642b335c8fc2
⛔ Files ignored due to path filters (1)
pyre/pyre-jit-trace/tests/snapshots/opcode_handler_impls.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
majit/majit-backend-dynasm/src/lib.rsmajit/majit-metainterp/src/blackhole.rspyre/bench/synth/attr_delete.pypyre/bench/synth/chained_comparison.pypyre/bench/synth/convert_value.pypyre/bench/synth/dict_delete.pypyre/bench/synth/fstring_multi.pypyre/bench/synth/fstring_simple.pypyre/bench/synth/fstring_spec.pypyre/bench/synth/import_name.pypyre/bench/synth/is_op.pypyre/bench/synth/load_deref.pypyre/bench/synth/load_fast_check.pypyre/bench/synth/load_super_attr.pypyre/bench/synth/set_literal.pypyre/bench/synth/unary_invert.pypyre/bench/synth/unary_not.pypyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/runtime_ops.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/metainterp.rspyre/pyre-jit-trace/src/opcode_handler_impls_pre.template.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/jit/codewriter.rspyre/pyre-jit/src/jit/cpu.rspyre/pyre-jit/src/jit/flatten.rspyre/pyre-jit/src/jit/flow.rspyre/pyre-jit/src/jit/simplify.rspyre/pyre-object/src/setobject.rs
Comment on lines
+1093
to
+1116
| let resume_live_pos = position; | ||
| if code[position] == self.op_live { | ||
| position += majit_translate::liveness::OFFSET_SIZE + 1; | ||
| if position >= code.len() { | ||
| return false; | ||
| } | ||
| if opcode == self.op_catch_exception { | ||
| self.exception_last_value = exc_value; | ||
| if position + 2 >= code.len() { | ||
| return false; | ||
| } | ||
| let target = (code[position + 1] as usize) | ((code[position + 2] as usize) << 8); | ||
| self.position = target; | ||
| // blackhole.py:407 parity: once the handler is dispatched the | ||
| // residual-call TLS slot is stale. Clear it so a subsequent | ||
| // opcode that reads `BH_LAST_EXC_VALUE` without issuing a new | ||
| // call can't pick up this already-caught exception. | ||
| BH_LAST_EXC_VALUE.with(|c| c.set(0)); | ||
| return true; | ||
| } | ||
| let opcode = code[position]; | ||
| // Forward case (explicit `raise`, `emit_raise!`): the `catch_exception` | ||
| // is directly after the resume `-live-` (blackhole.py:396 parity). | ||
| if opcode == self.op_catch_exception { | ||
| return self.route_to_catch(position, exc_value); | ||
| } | ||
| // Backward case (after-residual-call guard): pyre resumes the post-call | ||
| // `GUARD_NO_EXCEPTION` at the next opcode's `-live-` | ||
| // (`pc_map[fallthrough_pc]`, jitcode_dispatch.rs / capture_resumedata), | ||
| // because the raising op's vable-mirror stores (flatten.rs:1832-1857) | ||
| // sit between the call's own post-call `-live-` and its | ||
| // `catch_exception` — there is no Python PC that resolves onto the | ||
| // catch. The catch therefore lies BEHIND `resume_live_pos`; scan op | ||
| // boundaries backward, bounded by the call's own post-call `-live-`, | ||
| // so only the just-executed opcode's catch can match. | ||
| if let Some(catch_pos) = self.find_catch_before_resume_live(resume_live_pos) { | ||
| return self.route_to_catch(catch_pos, exc_value); |
There was a problem hiding this comment.
Synchronize the resume PC before the backward catch scan.
Line 1093 now treats self.position as the resume-live boundary, but residual-call handlers reach this path through check_residual_call_exception_after() before dispatch_step stores the handler’s decoded post-op PC. In those handlers, self.position is still at the first operand, so Line 1100 may inspect operand bytes as opcodes and Line 1115 cannot find the catch_exception that belongs to the just-executed residual call.
Set bh.position to the decoded post-op position before calling handle_exception_in_frame for residual calls, or pass the resume PC into the helper.
🐛 Proposed fix direction
fn check_residual_call_exception_after(
bh: &mut BlackholeInterpreter,
+ resume_pos: usize,
) -> Result<Option<usize>, DispatchError> {
let exc_val = BH_LAST_EXC_VALUE.with(|c| c.get());
if exc_val == 0 {
return Ok(None);
}
+ bh.position = resume_pos;
if bh.handle_exception_in_frame(exc_val) {
return Ok(Some(bh.position));
}
bh.exception_last_value = exc_val;
bh.got_exception = true;
Err(DispatchError::LeaveFrame)
}Then pass the decoded post-op PC from each residual-call handler:
- if let Some(handler_pc) = check_residual_call_exception_after(bh)? {
+ if let Some(handler_pc) = check_residual_call_exception_after(bh, p + 1)? {
return Ok(handler_pc);
}For void residual calls, use p instead of p + 1.
🤖 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/blackhole.rs` around lines 1093 - 1116, The issue
is that when exception handling is triggered from residual-call handlers through
check_residual_call_exception_after(), the self.position used as resume_live_pos
is still pointing to the first operand rather than the decoded post-op position.
This causes Line 1100 to potentially inspect operand bytes as opcodes and Line
1115 to fail finding the correct catch_exception. To fix this, before each call
to handle_exception_in_frame from residual-call handlers, either set bh.position
to the decoded post-op position prior to the call, or pass the correct decoded
post-op PC as a parameter into the helper method. For void residual calls
specifically, use p instead of p + 1 when computing the post-op position.
Comment on lines
+923
to
+937
| let walk_result = run_perfn_walk(ctx, sym, w_code, start_pc, cf_addr, true); | ||
| // A guard snapshot emitted during the walk may have hit a resume | ||
| // coordinate the jitcode pc_map cannot encode (#124/#130) and requested | ||
| // an abort (`state::request_trace_abort`). The walker does not poll the | ||
| // flag mid-walk, so honor it here before mapping the outcome — otherwise a | ||
| // walk that reaches a terminator would compile a trace carrying the bad | ||
| // guard. Discarding the trace matches the trait leg's `interpret()` poll. | ||
| if crate::state::take_trace_abort_requested() { | ||
| if crate::jitcode_dispatch::fbw_debug_abort_enabled() { | ||
| eprintln!( | ||
| "[fbw-abort] start_pc={start_pc} unencodable cross-frame resume coordinate (#124/#130)" | ||
| ); | ||
| } | ||
| return TraceAction::Abort; | ||
| } |
There was a problem hiding this comment.
Abort is checked after run_perfn_walk can already commit walk side effects.
At Line 923, run_perfn_walk(...) completes before the abort flag is polled at Line 930. But run_perfn_walk can commit end-flush/store-journal state (Line 691-774). If an unencodable resume coordinate requested abort during the walk, this path can persist authoritative walk effects even though the trace is discarded.
Suggested fix direction
fn run_perfn_walk(...) -> Option<(usize, usize, PerfnWalkResult)> {
...
let mut walk_result = crate::jitcode_dispatch::dispatch_via_miframe(...);
+ // Honor trace-abort requests before any end-of-walk commit path.
+ // Use a non-consuming peek so full_body_walk_trace can still consume
+ // and map to TraceAction::Abort.
+ if authoritative && crate::state::trace_abort_requested() {
+ WALK_END_FLUSH_COMMITTED.with(|c| c.set(false));
+ crate::jitcode_dispatch::fbw_store_journal_rollback();
+ return Some((entry, code_len, walk_result));
+ }
+
if authoritative {
...
}🤖 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 923 - 937, The abort flag is
checked after run_perfn_walk() has already executed and committed side effects
(end-flush/store-journal state). If an unencodable resume coordinate requested
abort during the walk, those side effects persist even though the trace is
discarded. Move the abort flag check to occur before run_perfn_walk() completes
its side effect commits. This may require restructuring run_perfn_walk to check
the abort flag before committing state changes (around lines 691-774 where side
effects are persisted), or deferring the side effect commits until after the
caller has confirmed the trace is valid by checking
crate::state::take_trace_abort_requested().
Comment on lines
7113
to
7116
| if nargs > 8 { | ||
| emit_abort_permanent!(); | ||
| emit_abort_permanent!(py_pc); | ||
| } | ||
| push_and_bump!(call_result_value, py_pc); |
There was a problem hiding this comment.
Move unsupported-path aborts before runtime stack mutations.
Line 7114 and Line 7222 now publish last_instr = py_pc - 1, so the interpreter re-executes the current unsupported opcode after abort_permanent. These branches already emitted emit_popvalue_ref! calls before aborting, which clear vable stack slots and publish a reduced stack depth; the resumed interpreter then sees a post-pop stack while running the opcode from the beginning.
🐛 Suggested shape
Instruction::Call { argc } => {
let nargs = argc.get(op_arg) as usize;
+ if nargs > 8 {
+ emit_abort_permanent!(py_pc);
+ for _ in 0..(nargs + 2) {
+ pop_and_decr_depth(&mut current_state, &mut current_depth);
+ }
+ push_fresh_ref(&mut current_state, &mut graph);
+ current_depth += 1;
+ continue;
+ }
let mut graph_arg_values_rev = Vec::with_capacity(nargs);
...
- if nargs > 8 {
- emit_abort_permanent!(py_pc);
- }
push_and_bump!(call_result_value, py_pc);
} if argc > 3 {
- for _ in 0..argc {
- let _ = emit_popvalue_ref!(current_depth, py_pc);
- let _ = pop_ref_or_fresh(&mut current_state, &mut graph);
- }
emit_abort_permanent!(py_pc);
+ for _ in 0..argc {
+ pop_and_decr_depth(&mut current_state, &mut current_depth);
+ }
push_fresh_ref(&mut current_state, &mut graph);
current_depth += 1;
- emit_vsd!(current_depth, py_pc);
continue;
}Also applies to: 7217-7226
🤖 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/jit/codewriter.rs` around lines 7113 - 7116, The
unsupported-path abort needs to occur before runtime stack mutations to prevent
incorrect interpreter state on re-execution. Move the emit_abort_permanent! call
that checks if nargs > 8 to execute BEFORE the push_and_bump! call in the
codewriter.rs function, ensuring that stack-modifying operations do not execute
in the unsupported code path. This same pattern issue exists at multiple
locations in the file and should be fixed consistently wherever unsupported
opcode aborts are followed by stack mutations like push_and_bump! or similar
emit calls.
Comment on lines
+8646
to
+8659
| let _ = emit_popvalue_ref!(current_depth, py_pc); | ||
| let self_value = pop_ref_or_fresh(&mut current_state, &mut graph); | ||
| let _ = emit_popvalue_ref!(current_depth, py_pc); | ||
| let cls_value = pop_ref_or_fresh(&mut current_state, &mut graph); | ||
| let _ = emit_popvalue_ref!(current_depth, py_pc); | ||
| let _global_super = pop_ref_or_fresh(&mut current_state, &mut graph); | ||
| let raw_value = emit_frontend_load_super_attr( | ||
| &mut graph, | ||
| ¤t_block.block(), | ||
| self_value, | ||
| cls_value, | ||
| code_const, | ||
| name_idx_const, | ||
| py_pc as i64, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether the interpreter/runtime LOAD_SUPER_ATTR path consumes
# the dynamic global_super operand, and whether the JIT helper signature can preserve it.
rg -n -C4 'LoadSuperAttr|LOAD_SUPER_ATTR|load_super_attr|bh_load_super_attr|super_attr_unwrap'Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
# Search for the specific code section mentioned in the review
sed -n '8646,8659p' pyre/pyre-jit/src/jit/codewriter.rsRepository: youknowone/pyre
Length of output: 980
🏁 Script executed:
# Look at the full context around that code
sed -n '8600,8700p' pyre/pyre-jit/src/jit/codewriter.rs | head -100Repository: youknowone/pyre
Length of output: 6306
🏁 Script executed:
# Search for any guards or checks on global_super in load_super_attr handling
rg -n "global_super" pyre/pyre-jit/src/jit/codewriter.rs -A3 -B3Repository: youknowone/pyre
Length of output: 1058
🏁 Script executed:
# Check the bh_load_super_attr_fn implementation in call_jit.rs
sed -n '3504,3545p' pyre/pyre-jit/src/call_jit.rsRepository: youknowone/pyre
Length of output: 1666
🏁 Script executed:
# Search for any comments or documentation about global_super in the context
rg -n "global_super\|built-in.*super\|shadowed.*super" pyre/pyre-jit/src/ -i | head -30Repository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Look at CPython bytecode definition to understand LOAD_SUPER_ATTR semantics
# Search for any references to Python 3.12+ bytecode specs
rg -n "LOAD_SUPER_ATTR" pyre/ --type rust | grep -v "test\|Load" | head -20Repository: youknowone/pyre
Length of output: 1055
🏁 Script executed:
# Look at the eval.rs comment about LOAD_SUPER_ATTR to understand the CPython semantics
sed -n '3100,3120p' pyre/pyre-interpreter/src/eval.rsRepository: youknowone/pyre
Length of output: 987
🏁 Script executed:
# Search for any mention of "super" builtin or shadowed super in interpreter
rg -n "super\(" pyre/pyre-interpreter/src/ -B2 -A2 | head -50Repository: youknowone/pyre
Length of output: 4222
🏁 Script executed:
# Check if there's any logic related to checking if super is shadowed/built-in
rg -n "builtin.*super\|shadowed\|rebound" pyre/ -i | head -20Repository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Search for tests related to shadowed or builtin super
rg -n "shadowed\|rebound.*super\|builtin.*super" pyre/ -iRepository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Check if there's any reference to checking against the builtin super
rg -n "builtins.*super\|__builtins__.*super" pyre/ -iRepository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Look at what w_super_new does to understand if it checks global_super
rg -n "w_super_new" pyre/pyre-object/src/ -A10 | head -40Repository: youknowone/pyre
Length of output: 942
🏁 Script executed:
# Search for any fast-path logic related to super or builtin checks
rg -n "fast.*path\|slow.*path" pyre/pyre-interpreter/src/eval.rs -i -B2 -A2 | grep -A5 -B5 "super"Repository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Check CPython spec or comments about why global_super is in the stack
rg -n "3.12\|fast.*super\|slow.*super\|builtin" pyre/pyre-interpreter/src/ -i | grep -i superRepository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Look at the full load_super_attr_with implementation
sed -n '3106,3140p' pyre/pyre-interpreter/src/eval.rsRepository: youknowone/pyre
Length of output: 1644
🏁 Script executed:
# Search for any use of _global_super after it's popped in the interpreter
rg -n "_global_super" pyre/pyre-interpreter/src/ -A5 -B2Repository: youknowone/pyre
Length of output: 694
🏁 Script executed:
# Check if there's CPython documentation or comments about global_super usage
rg -n "global_super" pyre/ -B3 -A3Repository: youknowone/pyre
Length of output: 3873
🏁 Script executed:
# Search for any comments or TODOs about shadowed super
rg -n "TODO\|FIXME\|XXX\|fast.?path" pyre/pyre-interpreter/src/eval.rs | grep -i superRepository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Check the CPython 3.12 bytecode docs or comments to understand global_super purpose
web_search
CPython 3.12 LOAD_SUPER_ATTR bytecode global_super operand purposeRepository: youknowone/pyre
Length of output: 156
Pass global_super through to distinguish built-in from shadowed super().
The current implementation pops the global_super operand from the bytecode stack (correctly tracking it with emit_popvalue_ref!) but then discards it before lowering to load_super_attr(self, cls, code, name_idx). The CPython 3.12 bytecode LOAD_SUPER_ATTR carries global_super to enable runtime discrimination between the built-in super (for the fast path) and a shadowed/rebound super in the local scope. Dropping it prevents correct handling of code where super is rebound:
# This should use the bound super, not the built-in fast path
super = lambda *args: ...
super().attr # Incorrectly executes as built-in super().attrThe same issue exists in the interpreter (pyre/pyre-interpreter/src/eval.rs:3109), where _global_super is also popped and discarded. Either pass global_super through the HLOp and helper function signatures to preserve the distinction, or keep LOAD_SUPER_ATTR on the abort path until the distinction can be modeled.
🤖 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/jit/codewriter.rs` around lines 8646 - 8659, The
`global_super` operand is being popped from the stack and stored in
`_global_super` but then discarded before being passed to
`emit_frontend_load_super_attr`. This causes loss of critical runtime
information needed to distinguish between built-in `super()` and
shadowed/rebound `super`. Pass the `global_super` value (instead of discarding
it as `_global_super`) through the `emit_frontend_load_super_attr` function
signature and any related HLOp structures to preserve this distinction. Apply
the identical fix in the interpreter at `pyre/pyre-interpreter/src/eval.rs:3109`
where `_global_super` is also being discarded in the equivalent code path.
Comment on lines
+8815
to
+8825
| emit_load_fast_ref!(current_depth, deref_idx, py_pc); | ||
| let cell_reg = emit_popvalue_ref!(current_depth, py_pc); | ||
| let cell_value = pop_ref_or_fresh(&mut current_state, &mut graph); | ||
| if let super::flow::FlowValue::Variable(v) = &cell_value { | ||
| pin!(Some(*v), cell_reg); | ||
| } | ||
| let result_value = emit_graph_op_with_result( | ||
| &mut graph, | ||
| ¤t_block.block(), | ||
| "load_deref_value", | ||
| vec![cell_value.into(), code_const.into(), deref_idx_const.into()], |
There was a problem hiding this comment.
Avoid synthesizing fresh refs for checked/cell loads in non-portal graphs.
Both LOAD_DEREF and LOAD_FAST_CHECK read through emit_load_fast_ref!, but that macro only emits a frame-backed getarrayitem_vable_r when is_portal; the non-portal branch falls back to current_state.locals_w, which excludes cell/free slots and uses a fresh Ref for missing/unbound locals. That can feed load_deref_value / load_fast_check a synthetic value instead of the actual localsplus slot, losing closure values or unbound-variable errors in non-portal callee jitcodes.
Use a frame-backed localsplus read for these opcodes whenever FrameInputs::Frame is present, or keep the non-portal cases abort-only until that read path is wired.
Also applies to: 8854-8864
🤖 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/jit/codewriter.rs` around lines 8815 - 8825, The
emit_load_fast_ref! macro currently falls back to current_state.locals_w for
non-portal graphs, which excludes cell/free slots and synthesizes fresh Refs for
missing values. This causes load_deref_value and load_fast_check opcodes to
receive synthetic values instead of actual localsplus slot values, losing
closure values or unbound-variable errors in non-portal jitcodes. For both the
LOAD_DEREF case (around line 8815-8825) and the LOAD_FAST_CHECK case (around
line 8854-8864), modify the code to use a frame-backed localsplus read whenever
FrameInputs::Frame is present, or alternatively keep the non-portal cases
abort-only until the frame-backed read path is properly wired.
4 tasks
youknowone
added a commit
that referenced
this pull request
Jul 1, 2026
Flip ProducedShortOp.res and ProducedShortOp.same_as_source from BoxRef to Operand, along with the coupled PreambleOp (AbstractShortOp) same_as_source field. res is always a producer-bound or const operand (materialize_operand_at on the preview; res.bound_op()-rooted exported entries per #173), never position-only. Retype the record/add channel to carry Operand: record_imported_preamble_use, record_preamble_use, add_tracked_preamble_op, and ShortPreambleBuilder::new's short_boxes parameter. Replace materialize_box_at with materialize_operand_at at the five ProducedShortOp construction sites in mod.rs. Contain at the principled BoxRef boundaries via to_boxref (Rc::ptr_eq-stable on the bound producer): exported_infos (#158), info::PreambleOp / ImportedShortPureOp (#182/#183), the position-only PreambleOp.res channel, and label_args: Vec<BoxRef>. Delete the optimizer same_as_source position-remap block: an Operand live-tracks its producer's already-remapped Op.pos through the carried Rc<Op>, so there is no separate position Cell to rewrite; the position-only else-branch was dead since #173 roots the producer. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
Flip ProducedShortOp.res and ProducedShortOp.same_as_source from BoxRef to Operand, along with the coupled PreambleOp (AbstractShortOp) same_as_source field. res is always a producer-bound or const operand (materialize_operand_at on the preview; res.bound_op()-rooted exported entries per #173), never position-only. Retype the record/add channel to carry Operand: record_imported_preamble_use, record_preamble_use, add_tracked_preamble_op, and ShortPreambleBuilder::new's short_boxes parameter. Replace materialize_box_at with materialize_operand_at at the five ProducedShortOp construction sites in mod.rs. Contain at the principled BoxRef boundaries via to_boxref (Rc::ptr_eq-stable on the bound producer): exported_infos (#158), info::PreambleOp / ImportedShortPureOp (#182/#183), the position-only PreambleOp.res channel, and label_args: Vec<BoxRef>. Delete the optimizer same_as_source position-remap block: an Operand live-tracks its producer's already-remapped Op.pos through the carried Rc<Op>, so there is no separate position Cell to rewrite; the position-only else-branch was dead since #173 roots the producer. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
Flip ProducedShortOp.res and ProducedShortOp.same_as_source from BoxRef to Operand, along with the coupled PreambleOp (AbstractShortOp) same_as_source field. res is always a producer-bound or const operand (materialize_operand_at on the preview; res.bound_op()-rooted exported entries per #173), never position-only. Retype the record/add channel to carry Operand: record_imported_preamble_use, record_preamble_use, add_tracked_preamble_op, and ShortPreambleBuilder::new's short_boxes parameter. Replace materialize_box_at with materialize_operand_at at the five ProducedShortOp construction sites in mod.rs. Contain at the principled BoxRef boundaries via to_boxref (Rc::ptr_eq-stable on the bound producer): exported_infos (#158), info::PreambleOp / ImportedShortPureOp (#182/#183), the position-only PreambleOp.res channel, and label_args: Vec<BoxRef>. Delete the optimizer same_as_source position-remap block: an Operand live-tracks its producer's already-remapped Op.pos through the carried Rc<Op>, so there is no separate position Cell to rewrite; the position-only else-branch was dead since #173 roots the producer. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Jul 2, 2026
…xVar.var_box to Operand (#321) * optimizeopt: ProducedShortOp res/same_as_source to Operand Flip ProducedShortOp.res and ProducedShortOp.same_as_source from BoxRef to Operand, along with the coupled PreambleOp (AbstractShortOp) same_as_source field. res is always a producer-bound or const operand (materialize_operand_at on the preview; res.bound_op()-rooted exported entries per #173), never position-only. Retype the record/add channel to carry Operand: record_imported_preamble_use, record_preamble_use, add_tracked_preamble_op, and ShortPreambleBuilder::new's short_boxes parameter. Replace materialize_box_at with materialize_operand_at at the five ProducedShortOp construction sites in mod.rs. Contain at the principled BoxRef boundaries via to_boxref (Rc::ptr_eq-stable on the bound producer): exported_infos (#158), info::PreambleOp / ImportedShortPureOp (#182/#183), the position-only PreambleOp.res channel, and label_args: Vec<BoxRef>. Delete the optimizer same_as_source position-remap block: an Operand live-tracks its producer's already-remapped Op.pos through the carried Rc<Op>, so there is no separate position Cell to rewrite; the position-only else-branch was dead since #173 roots the producer. Assisted-by: Claude * optimizeopt: ImportedShortAlias.same_as_source to Operand Flip ImportedShortAlias.same_as_source from BoxRef to Operand. Every construction site is producer-bound: production reads op.arg(0) (already an Operand op-arg) and the three test mints use rooted_resop_operand, so the field never carries a position-only box. This deletes a round-trip that the BoxRef field forced: used_imported_short_aliases stored op.arg(0).to_boxref(), and emit_alias_same_as_for_imports rebuilt the SameAs arg via Operand::from_boxref(&same_as_source). With the field typed Operand both conversions are dropped — op.arg(0) is stored directly and cloned into the emitted op. The assert on .to_opref() is unchanged (Operand carries it). Assisted-by: Claude * optimizeopt: bind IndexVar.var_box + rename_op to producers Flip `IndexVar.var_box: Option<BoxRef>` to `Option<Operand>` and bind the dormant-vectorizer producer sites, draining the last E5b (#175) residual. dependency.rs: - `var_box` field + `new_boxed` param BoxRef -> Operand; `get_or_create` passes the bound `arg_box.clone()` instead of `arg_box.to_boxref()`. - `get_operations` carries `var` as an Operand directly: `first_var` and the chained references bind a synthetic producer via `Operand::bound_from_opref` (to_opref-identical) instead of `BoxRef::from_opref`, dropping the three `Operand::from_boxref(&var_box(..))` wraps. Removes the BoxRef import (dependency.rs is now BoxRef-free). The constant arg stays `from_opref` (a ConstInt, sheds inline). guard.rs `rename_op`: `Operand::from_opref(replacement)` -> `bound_from_opref`. The renamer replacement is a producer position, so `from_opref` would panic on it; bind a synthetic producer carrying the same pos, matching the sibling guard-strengthening sites. Both paths are vectorizer-only (vec_all default false), so check.py cannot exercise them; correct by construction, mirroring the landed `bound_from_opref` siblings. Gate: dynasm-lib 1370 / cranelift-lib 1368 / ir 333 / check.py 167/167 both backends. Assisted-by: Claude * optimizeopt: flip util::args_eq/args_hash to Operand `args_eq` / `args_hash` / `hash_arg` take `&[Option<Operand>]` instead of `&[Option<BoxRef>]` (util.py:100-122 parity port). `Operand::same_box` carries the same const-by-value / producer-by-identity semantics, and `hash_arg` keeps the value special-case (`Operand::Const` hashes by pointer, so equal fresh consts must hash via `const_value` to preserve the args_eq/args_hash contract). Tests use `Operand::const_` / `bound_from_opref`. Drops the last BoxRef use in util.rs (import removed). These helpers have no production caller (the optimizer's CSE compares via `same_box` directly); kept as a faithful port. Lib-authoritative: dynasm-lib 1370 / cranelift-lib 1368 / ir 333. Assisted-by: Claude * optimizeopt: short-box map keys via materialize_operand_at `produce_arg` and `materialize_one` resolved the produced_short_boxes / boxes_in_production key with `let key = ctx.materialize_box_at(x); let okey = Operand::from_boxref(&key)`. Collapse both to `let okey = ctx.materialize_operand_at(x)` — byte-identical (`materialize_operand_at` is `from_boxref(&materialize_box_at(..))` with the same mutation side-effect), dropping two `from_boxref` bridges and the intermediate BoxRef locals. Gate: dynasm-lib 1370 / cranelift-lib 1368 / check.py 168/168 both backends. Assisted-by: Claude * optimizeopt: ShortPreamble position fields to OpRef Flip ShortPreamble.{inputargs, used_boxes, jump_args, phase1_inputargs} from Vec<BoxRef>/Option<Vec<BoxRef>> to flat Vec<OpRef>. These are cross-phase position-domain channels whose payload is a bare producer or const position. Re-home walk_const_ptr_refs_mut to forward inline OpRef::ConstPtr GcRefs directly over &mut slices instead of BoxRef::walk_const_ptr_refs; the old path round-tripped the same GcRef through a BoxKind::Const cell, so forwarding is unchanged. Drop the from_opref re-mints at producers and the to_opref extraction at consumers across shortpreamble.rs / unroll.rs / optimizer.rs. The ExtendedShortPreambleBuilder.used_boxes field stays BoxRef, so re-mint the positions at that boundary. Assisted-by: Claude * optimizeopt: ShortPreamble builder position fields to OpRef Flip the ShortPreambleBuilder-feeding position channels from BoxRef to flat OpRef: - ShortPreambleBuilder.known_constants (VecSet) and .used_boxes; ExtendedShortPreambleBuilder.used_boxes. - OptContext.exported_short_inputargs and ExportedState.short_inputargs. - initialize_imported_short_preamble_builder[_from_short_boxes] short_inputargs parameter. Re-home the const GC walk to inline OpRef::ConstPtr forwarding (visit_oprefs / visit_opref_set over &mut). Retire the temporary re-mint bridge C1 left: used_boxes assigns short_preamble.used_boxes directly; the produce loop pushes produced.preamble_op.pos.get(). Assisted-by: Claude * optimizeopt: PreambleOp.res to Operand PreambleOp.res is identity-bearing at the export boundary — its value feeds ProducedShortOp.res through Operand::from_boxref, which panics on a position-only box, so production res is always producer-bound or const. Carry it as Operand (matching ProducedShortOp.res) rather than a flat OpRef, preserving the producer Rc identity. - Export reads (build_short_preamble_struct, optimizer preview-to-export) become entry.res.clone() / produced.res.clone(). - Drop the optimizer res position-remap block: a bound/const operand live-tracks its producer's already-remapped op.pos, like same_as_source. - Construction: ctx sites use materialize_operand_at; the ctx-less collector builder uses Operand::bound_from_opref. - GC walk routes through Operand::walk_const_ptr_refs (Const cell get/visit/set), consistent with the sibling same_as_source field. - Tests use rooted_resop_operand / Operand::from_boxref. Assisted-by: Claude * optimizeopt: ExportedState position fields to OpRef Flip the three pure position-domain ExportedState fields end_args, renamed_inputargs, and runtime_boxes from Vec<BoxRef> to flat Vec<OpRef>. Their readers already resolve via to_opref (trace inputarg reconstruction in pyjitpl, high-water scan, remap), so storing OpRef drops the constructor from_opref re-mints and the reader round-trips. Re-home the const GC walk to inline OpRef::ConstPtr forwarding (visit_oprefs over &mut); high-water and export remap read the position directly (remap_opref). next_iteration_args and the exported_infos keys stay BoxRef — they carry cross-peel ptr_eq identity and convert together in a later slice. Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#73
Summary
Self-review
Prompt & Model
Model:
Prompt:
Answer
Summary by CodeRabbit
New Features
Bug Fixes
Performance