Eleven defect fixes: a mapdict __dict__ materialisation that invalidated the JIT's shape guard, plus locals() staleness, GC pinning, and narrowing-order bugs - #1075
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (46)
WalkthroughThe PR adds JIT specialization for ChangesJIT frame-local builtins
Interpreter runtime corrections
JIT benchmark statistics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant JIT residual dispatcher
participant locals specialization
participant frame locals helpers
participant dir names helper
JIT residual dispatcher->>locals specialization: recognize zero-argument locals(), vars(), or dir()
locals specialization->>frame locals helpers: read guarded virtualized fastlocals
frame locals helpers-->>locals specialization: build locals mapping
locals specialization->>dir names helper: sort mapping keys for dir()
dir names helper-->>JIT residual dispatcher: return names or null
Possibly related issues
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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit b8ba6a9). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 11433-11442: Update jit_dir_names_from_locals so its Err(_) branch
drains the pending call error from PENDING_CALL_ERROR before returning PY_NULL,
matching the error-handling behavior required for Result failures from
dir_names_from_locals_mapping.
In `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs`:
- Around line 181-188: Update the remaining host signal APIs—raise_signal,
strsignal, siginterrupt, pthread_kill, pthread_sigmask, and pidfd_send_signal—to
call check_signum_in_range on each Python-provided signal argument before
narrowing it to i32 or passing it to host calls. Reuse signum_arg for the
unwrapped value where appropriate, preserving rejection of values outside the
valid signal range and preventing low-bit aliases such as (1 << 32) | SIGINT.
In `@pyre/pyre-object/src/listobject.rs`:
- Around line 1705-1712: Update the lock acquisition in the list mutation path
around w_list_lock so its stripe identity comes from stable list-owned state
rather than the pre-block relocatable obj address. Ensure callers use the same
identity after before_external_block relocation, while preserving the post-lock
shadow_stack_get reload and existing lock coverage.
🪄 Autofix
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: c78b9b73-df69-4c7e-8fc2-544992f7c8bf
📒 Files selected for processing (46)
pyre/bench/synth/foriter_inplace_immutable.cranelift.jitstatspyre/bench/synth/inline_callee_constructs_object.cranelift.jitstatspyre/bench/synth/locals_forced_frame.cranelift.jitstatspyre/bench/synth/locals_forced_frame.dynasm.jitstatspyre/bench/synth/locals_forced_frame.pypyre/bench/synth/locals_forced_frame.wasm.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstatspyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstatspyre/bench/synth/pickle_ctor_args.cranelift.jitstatspyre/bench/synth/pickle_terminal_raise_resume.cranelift.jitstatspyre/bench/synth/pickle_terminal_raise_resume.dynasm.jitstatspyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstatspyre/bench/synth/polymorphic_binary_receiver.cranelift.jitstatspyre/bench/synth/polymorphic_binary_receiver.dynasm.jitstatspyre/bench/synth/polymorphic_binary_receiver.wasm.jitstatspyre/bench/synth/sre_pattern_methods.cranelift.jitstatspyre/bench/synth/sre_pattern_methods.dynasm.jitstatspyre/bench/synth/sre_pattern_methods.wasm.jitstatspyre/bench/synth/sre_wasm_min.cranelift.jitstatspyre/bench/synth/sre_wasm_min.dynasm.jitstatspyre/bench/synth/sre_wasm_min.wasm.jitstatspyre/bench/synth/sre_wasm_min1.cranelift.jitstatspyre/bench/synth/sre_wasm_min1.dynasm.jitstatspyre/bench/synth/sre_wasm_min1.wasm.jitstatspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/launch_env.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/math/interp_math.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/zlib/mod.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/state.rspyre/pyre-jit/tests/gc_stress.rspyre/pyre-object/src/listobject.rs
| pub extern "C" fn jit_dir_names_from_locals(mapping: i64) -> i64 { | ||
| let _roots = pyre_object::gc_roots::push_roots(); | ||
| let mapping_slot = pyre_object::gc_roots::shadow_stack_len(); | ||
| pyre_object::gc_roots::pin_root(mapping as PyObjectRef); | ||
| let mapping = pyre_object::gc_roots::shadow_stack_get(mapping_slot); | ||
| match dir_names_from_locals_mapping(mapping) { | ||
| Ok(names) => names as i64, | ||
| Err(_) => pyre_object::PY_NULL as i64, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the error paths reachable from dir_names_from_locals_mapping publish state beyond the returned PyError.
set -euo pipefail
rg -n -C 6 'pub fn call_and_check' pyre/pyre-interpreter/src
rg -n -C 4 'set_call_error|take_call_error|BH_LAST_EXC_VALUE|store_exception' pyre/pyre-interpreter/src/call.rs
ast-grep run --pattern 'fn builtin_sorted($$$) { $$$ }' --lang rust pyre/pyre-interpreter/src/builtins.rsRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files pyre/pyre-interpreter/src | rg '(^|/)(builtins|call|residual|builtin|gc_roots|pyre_object)' || true
echo "== target function context =="
sed -n '11400,11455p' pyre/pyre-interpreter/src/builtins.rs 2>/dev/null || true
echo "== dir_names_from_locals_mapping occurrences =="
rg -n -C 8 'dir_names_from_locals_mapping|jit_dir_names_from_locals|builtin_sorted|call_and_check|drain_backend_jit_exc' pyre/pyre-interpreter/src || true
echo "== call.rs exception-related symbols =="
fd -a 'call.*\.rs$' pyre/pyre-interpreter/src | while read -r f; do
echo "--- $f"
rg -n -C 3 'set_call_error|take_call_error|BH_LAST_EXC_VALUE|store_exception|last_exc|BackendJit|drain|PendingException|exception|Error' "$f" || true
doneRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call_function definitions/callers =="
rg -n -C 8 '^pub (fn|unsafe fn) call_function|fn call_function|pub fn call_and_check|fn call_function_impl_result|drain_backend_jit_exc|take_call_error|set_call_error|BH_LAST_EXC|last_call|PendingException|stored call' pyre/pyre-interpreter/src/call.rs pyre/pyre-interpreter/src/builtins.rs pyre/pyre-interpreter/src/baseobjspace.rs pyre/pyre-interpreter/src/typedef.rs | head -n 240
echo "== builtin_sorted implementation =="
sed -n '12760,12855p' pyre/pyre-interpreter/src/builtins.rs
echo "== residual_call references =="
find pyre/pyre-interpreter/src -name '*residual*' -type f -print
rg -n -C 5 'drain_backend_jit_exc|PendingException|last.*exc|call_error' pyre/pyre-interpreter/src | head -n 220Repository: youknowone/pyre
Length of output: 42315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pending call error helpers =="
rg -n -C 8 'PENDING_CALL_ERROR|take_call_error|clear_call_error|capture_pending_call_error_area|walk_pending_call_error_area|drain.*call.*exc|CallMode::Bare|call_modelled' pyre/pyre-interpreter/src/call.rs pyre/pyre-interpreter/src/pyframe.rs pyre/pyre-interpreter/src/pyobject.rs | sed -n '1,260p'
echo "== dir_names_from_locals_mapping calls and error handling =="
rg -n -C 5 'jit_dir_names_from_locals|dir_names_from_locals_mapping|builtin_dir\(' pyre/pyre-interpreter/src/builtins.rs pyre/pyre-interpreter/src/
echo "== all call_function result recovery around dir/sorted helpers =="
sed -n '8827,8845p' pyre/pyre-interpreter/src/builtins.rsRepository: youknowone/pyre
Length of output: 21196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== builtin_sorted call path =="
rg -n -C 6 'fn call_builtin_code_positional|pub fn call_function\(|pub fn call_user_function|clear_call_error\(|take_call_error\(|call_function_impl_raw|CallMode' pyre/pyre-interpreter/src/call.rs | sed -n '1,380p'
echo "== inspect call_function_result/call_function_raw definitions =="
sed -n '3161,3310p' pyre/pyre-interpreter/src/call.rs
echo "== inspect call_function in baseobjspace result returns =="
rg -n 'call_function\(|pub fn call_function\(callable: PyObjectRef|pub fn call_method\(' pyre/pyre-interpreter/src/baseobjspace.rs | head -n 20
fd -a 'baseobjspace.rs' pyre/pyre-interpreter/src -x sh -c 'echo "--- $1"; rg -n -C 4 "pub fn call_function\(|pub fn call_method\(" "$1"' sh {}
echo "== jit_dir_names callers =="
rg -n -C 4 'jit_dir_names_from_locals|dir_names_from_locals_mapping' pyre/pyre-interpreter/src/builtins.rs pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 34138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== builtin_sorted exact implementation slice =="
sed -n '12768,12855p' pyre/pyre-interpreter/src/builtins.rs
echo "== sort_rooted_items implementation =="
rg -n -C 8 'fn sort_rooted_items|fn finish_builtin_code_positional|pub fn call_method\(' pyre/pyre-interpreter/src/builtins.rs pyre/pyre-interpreter/src/baseobjspace.rs | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 7632
Drain the stashed call error on the PY_NULL path.
dir_names_from_locals_mapping uses Result-returning paths, and builtin_sorted also returns Result<..., PyError> after collect_iterable. A user override that returns PY_NULL would stash the error in PENDING_CALL_ERROR; returning PY_NULL here would leave that side effect for the residual dir(). Clear the pending call error in the Err(_) path.
🤖 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/builtins.rs` around lines 11433 - 11442, Update
jit_dir_names_from_locals so its Err(_) branch drains the pending call error
from PENDING_CALL_ERROR before returning PY_NULL, matching the error-handling
behavior required for Result failures from dir_names_from_locals_mapping.
| // `w_list_clear` is the twin destructive re-installation and holds the | ||
| // stripe lock across its `drop_object_items`; a concurrent reader must not | ||
| // see the half-installed state this one writes either. Take the lock after | ||
| // the allocating `build_list_storage` so the acquire cannot deadlock behind | ||
| // a collection, and reload `obj` behind it because a contended acquire | ||
| // blocks through `before_external_block`. | ||
| let _list_guard = w_list_lock(obj); | ||
| let obj = crate::gc_roots::shadow_stack_get(obj_slot); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Use a stable lock identity for the list.
At Line 1711, w_list_lock selects its stripe from the pre-block object address. On contention, before_external_block can relocate obj, as Line 1712 acknowledges. A later caller can select a different stripe from the relocated address while this caller reloads and mutates the same list under the old stripe lock. This can concurrently free or replace list storage.
Store or derive the lock identity from non-moving list-owned state. Do not derive it from a relocatable address.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-object/src/listobject.rs` around lines 1705 - 1712, Update the lock
acquisition in the list mutation path around w_list_lock so its stripe identity
comes from stable list-owned state rather than the pre-block relocatable obj
address. Ensure callers use the same identity after before_external_block
relocation, while preserving the post-lock shadow_stack_get reload and existing
lock coverage.
![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: 85fa2df06b
ℹ️ 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".
| // the allocating `build_list_storage` so the acquire cannot deadlock behind | ||
| // a collection, and reload `obj` behind it because a contended acquire | ||
| // blocks through `before_external_block`. | ||
| let _list_guard = w_list_lock(obj); |
There was a problem hiding this comment.
When w_list_init_items builds an Object-strategy replacement and this stripe-lock acquisition contends, w_list_lock enters the before_external_block GC safepoint while storage.block is only an unrooted native pointer. The builder's own contract says this fresh nursery block must be pinned across any subsequent safepoint, and reload_typed_blocks only repairs the int/float blocks, so a collection can move or reclaim the object block before its stale address is installed into the list. Pin and reload storage.block across the acquire as well.
Useful? React with 👍 / 👎.
| let dict_root = pyre_object::gc_roots::shadow_stack_len(); | ||
| pyre_object::gc_roots::pin_root( | ||
| pyre_interpreter::pyframe::jit_locals_dict_new() as pyre_object::PyObjectRef | ||
| ); |
There was a problem hiding this comment.
When an optimized frame already has a non-fast-local entry, such as after sys._getframe().f_locals["extra"] = value, the interpreter's fast2locals updates the cached w_locals mapping without deleting that extra key and frame_locals_snapshot copies the whole mapping. This fold instead starts from an empty dict and adds only code.varnames, so compiled locals()/vars() silently omit the entry and dir() omits its name. The fold must incorporate the frame-owned mapping or decline when it contains entries outside the modeled slots.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
| /// object's `numlocals`; the explicit ceiling here keeps a pathologically | ||
| /// wide frame from turning one `locals()` into hundreds of trace ops. Over | ||
| /// the bound the fold declines and the generic residual runs (SAFE). | ||
| const MAX_MODELLED_FASTLOCALS: usize = 32; |
There was a problem hiding this comment.
For any optimized function with at least 33 entries in co_varnames, every zero-argument locals()/vars()/dir() call is forced onto the generic residual path; that path forces the virtualizable and aborts the trace, so such a hot loop cannot compile even though upstream's @jit.unroll_safe loop has no equivalent numeric cutoff. Translate the upstream loop shape or segment the generated trace rather than rejecting these frames outright.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| if unsafe { crate::objspace::std::mapdict::has_mapdict_storage(obj) } { | ||
| return Ok(unsafe { | ||
| crate::objspace::std::mapdict::instance_node_getdictvalue( | ||
| obj, | ||
| rustpython_wtf8::Wtf8::new(name), | ||
| ) |
There was a problem hiding this comment.
For a devolved native mapdict carrier such as a _random.Random subclass whose object-strategy __dict__ contains a same-hash non-string key with a raising __eq__, this new branch wraps instance_node_getdictvalue in Ok; its devolved terminator uses unchecked w_dict_getitem_wtf8, which turns that comparison error into a miss. The previous getdict_backing plus checked finditem_str path propagated the exception, so attribute lookup can now continue to a class value or AttributeError instead. Preserve the mapdict shape without bypassing the fallible finditem_str semantics.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
`sys.stdout.write` and `print` do not go through the `TextIOWrapper` /
`BufferedWriter` pair `make_std_stream` builds; the stdout `write` is shadowed by
a native builtin (module/sys/vm.rs:2610) so that wasm32, which has no fd 1, and
the sandbox seam can capture the bytes, and so that `print` and
`sys.stdout.write` stay in order. That path reached `std::io::stdout()`, which is
a `LineWriter`, so the buffering flags never applied to it: a written value with
no newline waited in Rust's buffer until process exit.
Measured with stdout on a pipe and SIGKILL, which cannot be caught, so anything
observed had already reached the descriptor:
-u sys.stdout.write("AB") 3.14 "AB" pyre "" -> now "AB"
-u print("AB", end="") 3.14 "AB" pyre "" -> now "AB"
`flush_stdout_when_unbuffered` carries the rule, called from both `emit_stdout`
spellings, from `print_output`, and from the vm write builtin. Under sandbox fd 1
is already written through unbuffered `ll_os_write` requests, so it is a no-op
there.
An unflagged run is unchanged, and still line-buffers a pipe where CPython
block-buffers it.
Assisted-by: Claude
…_str re-entry `base_exception_str_wtf8`'s one-argument branch calls `py_str_wtf8` on the element in tail position. Release builds turn that into a jump, so a self-referential `e.args = (e,)` cycled with a constant stack pointer and a constant frame count, and neither half of `stack_check` could observe it. Take a `call::enter_native_dispatch` unit at the function's entry, as the equally frameless `A.__call__ = A()` chain does, and add the `stack_check` `py_repr_wtf8` already carries to `py_str_wtf8`. `str(e)` now raises RecursionError where it previously ran until killed. Assisted-by: Claude
`app_math.py:factorial` tests `'__index__' not in dir(n)`, which reports membership only. The port used `lookup_special`, which walks the MRO and then calls the descriptor's `__get__`, so a non-function `__index__` descriptor was bound twice per call: once for the discarded check and once inside `get_bigint`. Use `lookup`, the non-binding MRO walk, leaving `get_bigint` as the single binding site. A descriptor whose `__get__` returns a different callable per bind now yields the first one: `math.factorial` on such an object returned 2 where it should return 1. Assisted-by: Claude
…uncating `confname_arg` and the `sysconf` integer arm cast the index to `i32`, so `os.pathconf(fd, 2**40)` reached the syscall as name 0 and came back EINVAL, reporting a failure for a name the caller never passed. Convert with `i32::try_from` and raise OverflowError on a value that does not fit, as `conv_confname` does. Assisted-by: Claude
`locale_implies_utf8_mode` decided from the LC_ALL/LC_CTYPE/LANG strings themselves, so a variable naming a locale the C library cannot install (`LC_ALL=xx_YY.invalid`) was read as a named locale and left utf8_mode 0 while setlocale had in fact kept C. Set LC_CTYPE from the environment, read back what was installed, and test that instead. Restricted to the process-environment case: an embedding that supplied its own table via `set_launch_env` is describing an environment this process does not have, and wasm32 has no locale database, so both keep the string cascade. Assisted-by: Claude
…placement `w_list_clear` is the twin destructive re-installation and takes `w_list_lock` across its `drop_object_items`; `w_list_init_items` dropped and rewrote `length`, `items`, `strategy`, `int_items` and `float_items` with no lock, so a concurrent reader could observe the half-installed state. `list.sort`'s restore path reaches both in sequence holding no outer lock. Take the lock after the allocating `build_list_storage`, and reload `obj` behind the acquire, which blocks through `before_external_block` when contended — the pin/reload/lock/reload order every other mutator in the file uses. Assisted-by: Claude
…bool__ `list.sort` read the receiver and the `key=` callable out of `args`, then decoded `reverse=` by calling the object's `__bool__`, and only pinned afterwards. That call runs Python and can collect, and `args` is a native copy `call_builtin_code_positional` takes before dispatch which the collector does not update, so re-reading it afterwards yields the same stale pointer. Pin both before the call and reload the key from its shadow slot after, the order `tuple_method_index` uses around `eq_w`. Adds a gc_stress case that sorts with a `__bool__` calling `gc.collect()`. Assisted-by: Claude
…e flush's mode through c_int_w `signum_arg` narrowed with `as i32` before `check_signum_in_range` ran, so `(1 << 32) | SIGINT` truncated to `SIGINT` and passed the check. Keep the unwrapped value machine-word wide, as the RPython `int` in `interp_signal.py`'s `@unwrap_spec(signum=int)` is, and narrow after the check bounds it to `1..NSIG`. The `sigwait`/`pthread_sigmask` set walk had the same order and gets the shared `check_signum_in_range` call. `Compress.flush` unwrapped its mode with `int_w(o)? as i32`, truncating an out-of-range value into a different flush mode. `interp_zlib.py:196` spells that argument `@unwrap_spec(mode="c_int")`, so it goes through `c_int_w`. Measured against pypy3: `signal.getsignal((1 << 32) | 2)` and `signal.signal` now raise `ValueError: signal number out of range`, and `Compress.flush((1 << 32) | Z_NO_FLUSH)` raises `OverflowError: expected a 32-bit integer`, each matching upstream's message. `c_int_w`'s doc cited baseobjspace.py:1976-1982; the function is at :2062-2068. It also named `sys.setrecursionlimit` the only caller. Assisted-by: Claude
`vable_after_residual_call` raises `SwitchToBlackhole(Counters.ABORT_ESCAPE, raising_exception=True)` (pyjitpl.py:3389-3390). The walker's `VableEscapedDuringResidualCall` decline staged no reason at all, so the ladder in `jitdriver` fell through to `AbortReason::Generic`, whose `as_int()` is `ABORT_BRIDGE`. The escape was therefore tallied in the slot `jitprof.rs ABORT_COUNTER_KINDS` labels `bridge_or_generic` -- a counter named after bridges, for a decline with no bridge in it. `note_vable_escape_abort` stages `counters::ABORT_ESCAPE` at the single `Err(DispatchError::VableEscapedDuringResidualCall)` site, next to the existing `note_force_quasi_immut_abort`, which was the only `stage_abort_reason` caller in the tree. `raising_exception` needs no counterpart here: pyre derives it from the residual's own `exec_result` rather than from the staged reason, and the escape's `Err` arm already sets it. Measured on recursive_forced_frame_kept_stack (with the pending `locals()` frame-force change in the tree, which is what produces an escape there): `abrt_bridge=1 abrt_escape=0` becomes `abrt_bridge=0 abrt_escape=1`. `loops_compiled=1 bridges_compiled=3 loops_aborted=1 guard_failures=600` are unchanged -- this reclassifies a tally, it does not recover a trace. No `.jitstats` field records an `abrt_*` counter, so no baseline moves. Assisted-by: Claude
…zero-argument form in the walker `topframe_for_locals` resolved the frame with `gettopframe_nohidden()` alone, which forces the vref but never calls `force_frame`. `fast2locals` then read a virtualizable whose `locals_cells_stack_w` was last flushed at the previous deopt, so inside a compiled loop `locals()` and `vars()` returned the right keys with values thousands of iterations stale: a loop over 200000 iterations binding `a = i * 2` reported `a = 2082, i = 1041` at the point the frame held `10000 / 5000`. `sys._getframe().f_locals` was already exact in the same loop because `getframe` calls the forcing `gettopframe()` first. It now calls `force_frame_before_locals_read`. That force alone costs the loop. `locals()` is an opaque residual, so the walker arms the vable token protocol around it (`tracing_before_residual_call`); the force clears the token inside that window, which is the escape signal `tracing_after_residual_call` reads, and the walk declines. Measured on `for i in range(200000): d = locals()`: `loops_compiled=0 loops_aborted=5`. Upstream has no exemption for this — `pyjitpl.py:3373-3390` raises `SwitchToBlackhole(Counters.ABORT_ESCAPE)` unconditionally once a force is detected. What upstream has instead is `pyframe.py:539 fast2locals` decorated `@jit.unroll_safe`, which `codewriter/policy.py:60-61,67` keeps looked-into, so the fastlocals reads lower to `getarrayitem_vable_r` and no residual boundary exists to arm. `try_walker_specialize_builtin_locals` models that shape here. It sits in the walker specialization chain in `residual_call.rs`, which runs before `try_execute_residual_call_via_executor` arms the protocol, and expands a zero-argument `locals()` / `vars()` / `dir()` on the walk's own portal frame into per-slot reads plus a dict-build chain. `jit_locals_dict_new` and `jit_locals_dict_setitem_local` take a code object and a slot index rather than a `PyFrame`, so `force_frame` is unreachable from them. The fold declines — and falls through to the generic residual — on wrong arity, a callable that is not the builtin, a null receiver, an inline subwalk, a frame whose vable pointer is not the portal's, a code object that is not plain fastlocals (`code_locals_are_plain_fastlocals`: OPTIMIZED, no cellvars, no freevars, no hidden locals), more slots than `MAX_MODELLED_FASTLOCALS`, and an unexpected vable info shape. `vars()` folds with `locals()` because zero-argument `vars()` is `locals()` (`app_inspect.py:21-24`). `dir()` shares `topframe_for_locals`, so it shared the regression and shares the fold; `dir(obj)` carries an argument and never reaches it. Measured after, on `recursive_forced_frame_kept_stack`: loops 2, bridges 5, aborted 0, guard_failures 1000, `fbw_rolled_back_with_effects` 0 — unchanged from the committed baselines on all three backends. Hot `locals()` goes from `loops_compiled=0 loops_aborted=5` to `1 / 0`, and hot no-argument `dir()` the same. Sixteen decline paths (closure free-var visible, rebound name, comprehension, class body, the PEP 667 snapshot, hot `vars()`, `sys._getframe(0).f_locals`, args/kwargs, and the eight `dir()` twins) match python3.14 exactly. `locals_forced_frame` covers the stale-value shape behind a cold guard. The doc comments on `topframe_for_locals`, `gettopframe_nohidden` and the typedef frame-walk note claimed an unforced frame yields an empty mapping; it yields correct keys with stale values, which is why key-only probes missed this. Assisted-by: Claude
…outing through getdict
`baseobjspace::getdictvalue`/`setdictvalue` carried only the `W_Root`
defaults (baseobjspace.py:46-50, 52-57) and applied them to every
receiver, resolving the instance dict through `getdict_backing` ->
`getdict` -> `_obj_getdict`. For a mapdict carrier that materialises the
`("dict", SPECIAL)` wrapper, which adds an attribute to the instance's
map: an ordinary `self.a = a` moved the receiver from `Plain("a")` to
`Plain("dict", SPECIAL, back=Plain("a"))`.
mapdict.py:846-850 `MapdictDictSupport` overrides both for a mapdict
carrier and reads/writes the map directly. Port those overrides, gated
on `has_mapdict_storage`.
`instance_node_setdictvalue` now returns `map.write`'s flag rather than
debug_asserting it: a `NoDictTerminator` (`__slots__`) answers false, and
that is the AttributeError signal `object_setattr` raises on. The
`assert flag` moves to `MapDictStrategy::setitem_str`, where
mapdict.py:1172-1175 has it.
The JIT bakes the pre-materialisation map into the `GuardValue` that
`walker_guard_mapdict_instance_shape` emits, so the shape change made
that guard fail on every entry until a bridge attached. Re-recorded
baselines, guard_failures and bridges_compiled, dynasm/cranelift/wasm:
mapdict_frozen_unboxing_fold 203/203/404 -> 2/2/2, bridges 1/1/2 -> 0
sre_pattern_methods 2291/2291/2292 -> 1012/1012/1013, bridges 11 -> 5
sre_wasm_min 1849 -> 803, bridges 8 -> 4
sre_wasm_min1 803 -> 603, bridges 4 -> 3
polymorphic_binary_receiver 1045 -> 788, bridges 4 -> 3
pickle_terminal_raise_resume 463/656/464 -> 338/338/339, loops 36/36/73 -> 35/35/72
pickle_ctor_args (cranelift) 201 -> 1, bridges 1 -> 0
inline_callee_constructs_object 201 -> 1, bridges 1 -> 0
foriter_inplace_immutable 202 -> 1, bridges 1 -> 0, loops 1 -> 2
The three cranelift-only rows are a selection effect, not a
backend-specific cause: `PyFrame::store_attr_cached` (eval.rs:4630) ports
pyopcode.py:920 `if not jit.we_are_jitted():` and skips the mapdict cache
when jitted, falling through to `object_setattr` -> `setdictvalue`, and
`majit-backend-cranelift/src/compiler.rs:7424` is the only setter of that
flag in the tree. Filed as a separate task.
`__dict__` observable behaviour checked against python3.14 on a 13-case
fixture (materialisation, `vars()`, aliasing, `__dict__` assignment,
`del`, `__slots__` rejection, 120-attribute devolve, surrogate-named
attribute): byte-identical.
Assisted-by: Claude
…alled `note_vable_escape_abort` reached the driver through `driver::driver_pair`, which resolves it via `callbacks::get` and panics with "CallJitCallbacks not initialized" when no table is installed. A skeleton walk drives `jitcode_dispatch` directly with no pyre-jit eval behind it, so `jitcode_dispatch::tests::may_force_vable_escape_surfaces_ typed_abort` — which exercises exactly the escape arm the note was added to — panicked instead of asserting its typed error. All three `cargo test` legs failed on it, each 341 passed / 1 failed. Add `driver::try_driver_pair`, the `callbacks::try_get` spelling of the same lookup, and route both abort-reason notes through one `stage_walker_abort_reason` that returns early without a driver. The adjacent `drain_backend_jit_exc` call in the same arm (residual_call.rs:3223-3227) already takes `try_get` for this reason. Staging a reason is accounting only: the abort travels in the `DispatchError` the caller returns and no consumer reads the slot back, so skipping it changes nothing a skeleton walk observes. Production is unaffected — the walker reaches the panicking `driver_pair` at ten other sites, so a callback-less thread cannot get this far. `note_force_quasi_immut_abort` had the same latent panic and is covered by the shared helper. cargo test --all --features dynasm: 7489 passed, 0 failed (101 harnesses). The cranelift subset CI runs after it: 3054 passed, 0 failed (46 harnesses). Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/e855a9ec4d4be34aad7c9abbb3483f7df1731cc5/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L6465-L6466
When an inlined helper calls zero-argument locals(), vars(), or dir(), this gate always declines the fold; the generic residual then reaches force_frame_before_locals_read, and force_pyframe treats the published inline frame as escaping the traced virtualizable, clears the tracing token, and aborts the entire portal trace. Consequently, a hot loop cannot compile merely because an otherwise-inlineable helper inspects its locals, whereas upstream traces the @jit.unroll_safe fast2locals reads. Source the values from the callee shadow by preserving its per-frame red frame rather than restricting the model to the portal virtualizable.
AGENTS.md reference: AGENTS.md:L32-L42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b8ba6a9c500b7196940f0c96f14dabb807e7a37f/pyre-interpreter/src/display.rs#L1006
When execution is already in the deepest permitted Python frame—for example, while handling the RecursionError from one further recursive call—an ordinary str(123) now raises another RecursionError even though it performs no recursive dispatch. execute_frame checks before incrementing the frame depth specifically so builtin/object-space operations remain usable while handling recursion failures, but this unconditional check observes py_recursion_depth == recursionlimit; apply the check only around the recursive BaseException.args re-entry that this change is intended to guard.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ 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".
1 participant
Eleven independent defect fixes, rebased onto current
main. Each commit isself-contained and carries its own upstream citation and measurement; the
grouping is chronological, not thematic.
The one with the largest blast radius
mapdict: implementMapdictDictSupport'sget/setdictvalueinstead ofrouting through
getdict.baseobjspace::getdictvalue/setdictvaluecarried only theW_Rootdefaults(baseobjspace.py:46-50, 52-57) and applied them to every receiver, resolving
the instance dict through
getdict_backing->getdict->_obj_getdict. Fora mapdict carrier that materialises the
("dict", SPECIAL)wrapper, which addsan attribute to the instance's map — so an ordinary
self.a = amoved thereceiver from
Plain("a")toPlain("dict", SPECIAL, back=Plain("a")).mapdict.py:846-850
MapdictDictSupportoverrides both for a mapdict carrier andreads/writes the map directly. The JIT bakes the pre-materialisation map into
the
GuardValuethatwalker_guard_mapdict_instance_shapeemits, so the shapechange made that guard fail on every entry until a bridge attached.
Re-recorded baselines —
guard_failures, andbridges_compiledwhere it moved:mapdict_frozen_unboxing_foldsre_pattern_methodssre_wasm_minsre_wasm_min1polymorphic_binary_receiverpickle_terminal_raise_resumepickle_ctor_args(cranelift)inline_callee_constructs_object(cranelift)foriter_inplace_immutable(cranelift)The three cranelift-only rows are a selection effect, not a backend-specific
cause:
PyFrame::store_attr_cached(eval.rs:4630) ports pyopcode.py:920if not jit.we_are_jitted():and skips the mapdict cache when jitted, fallingthrough to
object_setattr->setdictvalue;majit-backend-cranelift'scompiler.rs:7424is the only setter of that flag in the tree. Filed as aseparate follow-up.
__dict__observable behaviour was checked against python3.14 on a 13-casefixture (materialisation,
vars(), aliasing,__dict__assignment,del,__slots__rejection, 120-attribute devolve, surrogate-named attribute):byte-identical.
The rest
builtins— force the frame behindlocals()/vars()/dir(), and foldthe zero-argument form in the walker.
topframe_for_localsforced the vrefbut never called
force_frame, so inside a compiled looplocals()returnedthe right keys with values thousands of iterations stale.
jit— nameABORT_ESCAPEas the reason for the vable-escape walkerdecline (pyjitpl.py:3389-3390). The escape was being tallied in the counter
jitprof.rslabelsbridge_or_generic. Reclassifies a tally; recovers notrace, and no
.jitstatsfield records anabrt_*counter.signal/zlib— range-check the signal number before narrowing(
(1 << 32) | SIGINTtruncated toSIGINTand passed the check), and routeCompress.flush's mode throughc_int_was interp_zlib.py:196 spells it.type_methods— pinlist.sort's receiver and key across thereverse=__bool__, which runs Python and can collect whileargsis an unupdatednative copy. Adds a gc_stress case.
listobject— hold the stripe lock acrossw_list_init_items' storagereplacement, matching its destructive twin
w_list_clear.launch_env— resolveutf8_modefrom the effectiveLC_CTYPEratherthan the raw env cascade, so a locale the C library cannot install
(
LC_ALL=xx_YY.invalid) no longer reads as a named locale.posix— narrowpathconf/fpathconf/sysconfnames withi32::try_fromand raise OverflowError, instead of truncatingos.pathconf(fd, 2**40)to name 0 and reporting the resulting EINVAL.math— makefactorial's__index__presence check non-binding.app_math.py uses
'__index__' not in dir(n); the port usedlookup_special,binding the descriptor twice per call.
display— spend a recursion unit for the framelessBaseException.descr_strre-entry, so a self-referentiale.args = (e,)raises RecursionError instead of running until killed.
stdio— settle fd 1 after a write when-u/PYTHONUNBUFFEREDis set.The stdout
writebuiltin reachedstd::io::stdout(), aLineWriter, so thebuffering flags never applied to it.
Verification
check.py, all three backends, on the rebased tree:The single cranelift failure is the pre-existing
pickle_ctor_argsperf gate(
exec 0.97s > pypy 0.02s, ratio 44.7x > gate 36x), already tracked separately;it is a ratio over a 0.02s pypy denominator and is not affected by anything here.
Four
.wasm.jitstatsfixtures read +-1/+-2guard_failuresagainst theircommitted baselines. They are
main's own reds, not this branch's, and aredeliberately left un-re-recorded. The base's own
pyre/check.py (ubuntu-24.04)job — the only leg that runs the wasm backend — produces the same four numbers
at
fcf997da0e0, one commit behind this branch's base, with none of thesecommits present:
closure_per_callexception_traceback_frame_linenogc_iterator_source_droprecursive_call_frame_relocationIdentical on both hosts, so this is not host wobble either. Recording them here
would bless a pre-existing
mainregression under an unrelated change. Thisbranch additionally touches no
majit/crate at all — every source change in itis backend-neutral — and dynasm and cranelift show no movement on those four.
Summary by CodeRabbit
locals(),vars(), anddir()now consistently reflect current loop-local values.RecursionErrorinstead of risking stack overflow.math.factorial(), list sorting, dictionary-backed objects, and locale UTF-8 detection.