locals() drops the frame's own extra locals, plus five defect fixes and a wasm codegen change - #1083
Merged
Conversation
`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
…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
…t walk before narrowing
The earlier pass gave `signal`/`getsignal` and `sigwait`'s set walk a
`check_signum_in_range` before the `as i32`, but left three siblings
narrowing first, so `(1 << 32) | SIGINT` still truncated to `SIGINT` and
acted on the signal it aliases.
Upstream authority, per entry point:
siginterrupt interp_signal.py:388 `check_signum_in_range(space, signum)`
pthread_sigmask interp_signal.py:492 `SignalMask.__enter__` checks every
element -- the same helper `sigwait`
already goes through here
strsignal interp_signal.py:593 spells the bound inline
`raise_signal`, `pthread_kill` and `pidfd_send_signal` are left alone:
the first two have no upstream range check (`pthread_kill` passes signum
straight to `c_pthread_kill` and reports through errno; the
`check_signum_in_range` at :492 belongs to `SignalMask.__enter__`, not to
`pthread_kill`), and `pidfd_send_signal` has no upstream counterpart.
`strsignal` does not reuse upstream's own bound. interp_signal.py:593
writes `signalnum > NSIG`, which admits `NSIG`; 3.14 rejects it, and its
`pthread_sigmask` reports the range as `[1; NSIG - 1]`. Take the
half-open `check_signum_in_range` the other entry points use, so
`strsignal(NSIG)` raises where pypy returns `'Unknown signal: 32'`.
Measured, `BIG = (1 << 32) | 2`:
before after 3.14
siginterrupt(BIG, 1) acted on SIGINT ValueError OverflowError
strsignal(BIG) 'Interrupt' ValueError OverflowError
pthread_sigmask(_, [BIG]) blocked SIGINT ValueError ValueError
strsignal(NSIG) 'Unknown 32' ValueError ValueError
strsignal(0) ValueError ValueError ValueError
The remaining delta is the exception class for the out-of-C-int case:
pyre raises `ValueError` where 3.14 raises `OverflowError`, matching pypy
and matching what `signal`/`getsignal` already do here. Moving that to
an `OverflowError`-raising unwrap is module-wide and is tracked
separately.
test.test_signal: SiginterruptTest passes 3/3. The suite's
`StressTest.test_stress_delivery_simultaneous` segfaults, on this commit
and on a control binary built with these edits reverted alike, so it is
untouched by this change and filed on its own.
Assisted-by: Claude
… acquire Taking `w_list_lock` in `w_list_init_items` put a safepoint between `build_list_storage` and the store that installs its result. `obj` and the two typed blocks were bracketed across it; the Object items block was not. `build_list_storage` returns that block young: `alloc_list_items_block_gc` pins it only inside its own `push_roots` scope, which ends at its return, and the block takes its heap edge from `list.items = storage.block` below. Between the two it is a bare Rust local, and the root walker is shadow-stack-only. A contended `w_list_lock` goes through `before_external_block`, which releases the GIL and drops this thread from the RUNNING census -- that is precisely the state in which another mutator takes the GIL and collects without waiting for this one. The block is unreachable to that collection, so the address stored at the install is dead, and `list_write_barrier` then hands it to the collector to trace as a live varsize array. `reload_typed_blocks` cannot cover it: `ListStorage` carries `int_block_root` and `float_block_root` and no slot for `block`. Pin it before the acquire and re-read it where the typed blocks are re-read -- after `drop_object_items`, whose `try_gc_owns_object` query is the other safepoint in the window. This is the bracket `w_list_new_with_strategy` already puts around its header allocation. Reachability needs two registered mutators whose lists collide on the 256-way stripe `(obj >> 4) & 255`, with the loser blocking while the winner allocates. Folding `block_root` into `ListStorage` so `reload_typed_blocks` covers all three, which would let `w_list_new_with_strategy` drop its hand-rolled copy, is a refactor and is left out. cargo test -p pyre-jit --test gc_stress: 32 passed, 0 failed. Assisted-by: Claude
…dict probe
`instance_node_getdictvalue` returned `Option<PyObjectRef>`, so the devolved
terminator's `space.finditem_str` equivalent (mapdict.py:383-388) reported a
raising comparison as a miss. A stored non-string key whose hash collides with
the probed name can reach a user `__eq__`; the attribute then read as absent
and the class attribute won as the value.
Add checked variants down the read path -- `w_dict_getitem_wtf8_checked`,
`terminator_read_checked`, `node_read_checked`,
`instance_node_getdictvalue_checked` -- and write each swallowing spelling as
`checked(..).unwrap_or(None)`, whose `unwrap_or` also consumes the pending
error slot. `W_Root.getdictvalue` and the two getattr step-3 sites take the
checked spelling. The `dont_look_inside` residual `instance_node_getdictvalue`
keeps its signature and its JIT helper caller.
class K:
def __hash__(self): return hash("zz")
def __eq__(self, other): raise ValueError("boom")
class R(_random.Random):
zz = "CLASSVALUE"
r = R()
for i in range(90): setattr(r, "a%d" % i, i)
r.__dict__[K()] = 1
r.zz
printed `CLASSVALUE`; python3.14 and pypy3 both raise `ValueError: boom`, and
now so does pyre.
Assisted-by: Claude
…ntity JUMP self-moves
`llsupport/regalloc.py:873 next_op_can_accept_cc` lets a comparison hand its
condition straight to the next op instead of materialising a boolean; x86
leaves it in the flags (`x86/regalloc.py:265 force_allocate_reg_or_cc`) and the
dynasm sibling ports it at `regalloc.rs:3665`. wasm's operand stack plays the
same role, so the comparison's i32 now stays on the stack and the guard's `if`
tests it, dropping the `i64.extend_i32_u`/`local.set` and the guard's own
`local.get` re-test. The wasm port takes only `GuardTrue`/`GuardFalse`, and
inverts with `i32.eqz` rather than the mirrored comparison, which would differ
on an unordered float operand.
The condition producers -- the six signed and four unsigned integer compares,
the pointer compares, the six float compares, `IntIsTrue` and `IntIsZero` --
move behind one `CondKind`, with `push_cond` leaving the i32 on the stack and
`emit_cond` the widening spelling. `next_op_can_accept_cc` refuses when the
result has another reader (`HomeLiveness::last_use`), when it is one of the
guard's own fail args, when a LABEL resume loader captures it, or when it has a
Ref home.
Separately, the loop-closing JUMP's parallel move emitted `local.get v` /
`local.set v` for a pair whose jump arg is its label arg; the ref-home refresh
nine lines below already skipped that case. Filter those pairs out of the move
as well.
Emitted module bytes over pyre/bench/{fib_loop,int_loop,nbody,fannkuch}
(PYRE_WASM_DUMP_ALL_TRACES, 40 trace modules): 166985 -> 164034, -1.77%
(-2.47% / -3.53% / -2.27% / -1.51%), with the same module count per bench.
`i64.extend_i32_u` occurrences 375 -> 261 on nbody and 695 -> 522 on fannkuch.
check.py: dynasm 389/389, cranelift 389/389, wasm 385/385, no jitstats change.
Assisted-by: Claude
The checked wrapper called the unchecked `w_dict_getitem_wtf8` and then drained
the key-error flag. The strategy leaf it reaches,
`w_dict_lookup_object_strategy` (dictmultiobject.rs:2663-2669), is itself
`w_dict_lookup_object_strategy_checked(..).unwrap_or(None)`, so the flag was
already taken by the time the unchecked spelling returned and the trailing
`take_dict_key_error()` always read `false`.
That made 06cec233f0a's mapdict path inert: a devolved terminator's read of
`r.zz` against an instance dict holding a hash-colliding key with a raising
`__eq__` still reported a miss, and the class attribute won. Verified on
`target/release/pyre-dynasm` (not `target/release/pyre`, which is a stale
leftover no build in the loop refreshes):
RESULT: CLASSVALUE before
RAISED: ValueError boom after, matching python3.14 and pypy3
check.py: dynasm 389/389, cranelift 389/389, wasm 385/385.
`cargo test --all --no-default-features --features dynasm`: 0 failed.
Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 39 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 (10)
WalkthroughThe PR adds WASM comparison-condition fusion, checked mapdict lookups, frame-owned JIT locals materialization, signal-range validation, and GC-safe list storage initialization. ChangesWASM condition lowering
Checked mapdict reads
JIT locals materialization
Signal range validation
List storage rooting
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant JITSpecializer
participant FrameDebugData
participant LocalsDictionary
participant ResultHelper
JITSpecializer->>FrameDebugData: resolve debugdata.w_locals
FrameDebugData-->>JITSpecializer: existing mapping or absence
JITSpecializer->>LocalsDictionary: update bound or delete unbound local
LocalsDictionary-->>JITSpecializer: updated mapping or failure
JITSpecializer->>ResultHelper: create snapshot or sorted names
ResultHelper-->>JITSpecializer: selected result or failure
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 536a619). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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
https://github.com/youknowone/pyre/blob/318269abb2478aa5ce0c14bb2a56f252714025b8/pyre-jit-trace/src/jitcode_dispatch/specialize.rs#L7041-L7042
When a frame-owned locals dict contains a non-string key whose hash collides with a bound local name, this branch calls jit_locals_dict_setitem_local, which uses unchecked w_dict_setitem_str and always returns the dict even if the stored key's __eq__ raises. The interpreter's fast2locals instead uses fallible setitem_str_object and propagates that exception, so a compiled locals()/vars()/dir() can silently continue with a different mapping. Route bound stores through a checked helper and guard its failure just as the new delete branch does.
AGENTS.md reference: AGENTS.md:L14-L19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/module/signal/interp_signal.rs`:
- Around line 626-638: Update check_signum_in_range and its call sites,
including strsignal and the other signal-number handlers, to use the
libc/host-derived maximum signal value rather than the fixed signalstate::NSIG
value of 64. Preserve the existing lower bound and error behavior while allowing
all signals reported as valid by the host platform.
In `@pyre/pyre-object/src/dictmultiobject.rs`:
- Around line 3465-3470: Update w_dict_getitem_wtf8_checked to root obj before
constructing w_key, then reload the rooted object before passing it to
w_dict_lookup_checked. Ensure the key allocation and lookup occur while obj
remains rooted, following the existing lock_dict_refs!/rooting pattern.
🪄 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: c4d02102-5c51-4e30-94c9-3f81ec7b869a
📒 Files selected for processing (10)
majit/majit-backend-wasm/src/codegen.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/virtualizable_spec.rspyre/pyre-object/src/dictmultiobject.rspyre/pyre-object/src/listobject.rs
Comment on lines
+3465
to
+3470
| pub unsafe fn w_dict_getitem_wtf8_checked( | ||
| obj: PyObjectRef, | ||
| key: &rustpython_wtf8::Wtf8, | ||
| ) -> Result<Option<PyObjectRef>, DictKeyError> { | ||
| let w_key = crate::w_str_from_wtf8(key.to_wtf8_buf()); | ||
| w_dict_lookup_checked(obj, w_key) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 --glob '*.rs' '\bfn\s+w_str_from_wtf8\b' pyre
rg -n -C 10 --glob '*.rs' 'macro_rules!\s+lock_dict_refs|lock_dict_refs!' pyre
rg -n -C 8 --glob '*.rs' 'ShadowStack|Root(?:ed)?|root.*PyObjectRef|PyObjectRef.*root' pyreRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused inspection around the reported helper, definition sites, and GC root APIs.
wc -l pyre/pyre-object/src/dictmultiobject.rs
sed -n '3430,3485p' pyre/pyre-object/src/dictmultiobject.rs
# Locate targeted definitions without flooding output.
python3 - <<'PY'
import pathlib,re
root=pathlib.Path("pyre")
for label in ["fn w_str_from_wtf8","macro_rules! lock_dict_refs","lock_dict_refs!","pub fn push_roots","pin_root","shadow_stack"]:
print(f"\n=== {label} ===")
count=0
for p in root.rglob("*.rs"):
text=p.read_text(errors="ignore")
idx=text.find(label)
while idx!=-1 and count<8:
line=text[:idx].count('\n')+1
lo=max(1,line-12); hi=min(text.count('\n')+1,line+18)
lines=text.splitlines()
print(f"{p}:{line}")
for i in range(lo,hi+1):
if i<=len(lines):
print(f"{i}: {lines[i-1]}")
print()
idx=text.find(label,idx+1)
count+=1
PY
# Locate w_str_from_wtf8 and lock macro only.
python3 - <<'PY'
import pathlib,re
for glob,pattern in [
("**/*.rs", r"\bfn\s+w_str_from_wtf8\b"),
("**/*.rs", r"macro_rules!\s+lock_dict_refs|lock_dict_refs!"),
]:
p=rule=pathlib.Path("./pyre")
import fnmatch
print(f"\n--- searches for {pattern} under pyre ---")
for f in p.rglob("*.rs"):
try:
text=f.read_text(errors="ignore")
except UnicodeDecodeError:
continue
if re.search(pattern, text):
print(f)
PYRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read exact definitions once locations are known.
python3 - <<'PY'
import pathlib,re
root=pathlib.Path("pyre")
targets={}
for pat in [r"\bfn\s+w_str_from_wtf8\b",r"macro_rules!\s+lock_dict_refs|lock_dict_refs!",r"pub(?: fn| struct)\s+\b(push_roots|RootScope|pin_root|shadow_stack_get)\b",r"pub\s+(?:mod|fn)\s+gc_roots"]:
targets.setdefault(pat, []).append(0)
print(f"\n=== {pat} ===")
for f in root.rglob("*.rs"):
try:
text=f.read_text(errors="ignore")
except UnicodeDecodeError:
continue
for m in re.finditer(pat, text):
line=text.count("\n",0,m.start())+1
print(f"{f}:{line}")
PY
# Read the helper and one nearby allocation example.
sed -n '3450,3475p' pyre/pyre-object/src/dictmultiobject.rs
git ls-files 'pyre/pyre-object/src/*.rs' | head -50Repository: youknowone/pyre
Length of output: 6610
Root obj before constructing w_key.
w_str_from_wtf8 allocates before w_dict_lookup_checked creates DictOperationGuard and reloads obj via lock_dict_refs!. Pin obj before constructing the key and reload it before w_dict_lookup_checked(obj, w_key), or move the allocation inside the root guard with w_key also rooted.
🤖 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/dictmultiobject.rs` around lines 3465 - 3470, Update
w_dict_getitem_wtf8_checked to root obj before constructing w_key, then reload
the rooted object before passing it to w_dict_lookup_checked. Ensure the key
allocation and lookup occur while obj remains rooted, following the existing
lock_dict_refs!/rooting pattern.
Source: Learnings
The zero-argument locals()/vars()/dir() walker fold always built its mapping from a fresh `jit_locals_dict_new()` and bound one key per bound fastlocal. A key the frame's own mapping already carried — one an `f_locals` write put there (PEP 667) — was therefore absent for as long as the loop stayed compiled, and `fast2locals`' `delitem` arm for an unbound slot was never emitted. Read `getorcreatedebug().w_locals` (pyframe.py:555-556) and rewrite that when the frame already carries a mapping. `debugdata` is a virtualizable field, so the read answers from `virtualizable_boxes` and the frame never becomes a residual operand. A `FrameDebugData` descr group carries `w_locals` for the emitted `getfield_gc_r`; the mapping is guarded non-null and exact-dict; an unbound slot emits the new `jit_locals_dict_delitem_local`; and `locals()` / `vars()` publish `jit_locals_dict_snapshot`, `frame_locals_snapshot`'s independent PEP 667 copy, while `dir()` keeps reading the mapping itself through `jit_dir_names_from_locals`. A frame that carries no mapping yet keeps the fresh `newdict` the residual would have materialised (pyframe.py:557), now under a `guard_isnull` on whichever of `debugdata` / `w_locals` is absent, so a mapping that appears mid-loop side-exits instead of being written past. Nothing else references that dict, so it is already the independent copy and its `delitem` arm is a no-op. The mapping check compares the shadow's `w_locals` against the frame's rather than the two `FrameDebugData` holders: a root portal seed expands the shadow from the `snapshot_for_tracing` copy, whose `clone_debugdata_ptr` hands out a fresh holder around the same mapping, so a holder comparison declines every portal trace. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 536a6198e0
ℹ️ 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
+7038
to
+7041
| let (helper, args, arg_types): (_, Vec<OpRef>, Vec<majit_ir::Type>) = if bound { | ||
| ( | ||
| pyre_interpreter::pyframe::jit_locals_dict_setitem_local as *const (), | ||
| vec![dict_op, code_const, index_const, slot_op], |
There was a problem hiding this comment.
When an optimized frame already has an exact locals dict containing a non-string key whose hash collides with a fast-local name—for example, it was inserted through f_locals before that local was bound—this new frame-owned arm passes the dict to jit_locals_dict_setitem_local. That helper uses unchecked w_dict_setitem_str, so an ObjectDictStrategy comparison whose __eq__ raises has its DictKeyError dropped and the helper still returns a non-null dict; the emitted call is also marked CannotRaise, and only the deletion arm receives a null guard. Consequently compiled locals(), vars(), or dir() suppresses an exception that residual PyFrame::fast2locals propagates, so bound stores need a fallible propagation path too.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
Comment on lines
+7073
to
+7075
| if !bound { | ||
| // The delete reports a raising comparison as PY_NULL instead of | ||
| // publishing it; side-exit so the residual re-runs and raises. |
There was a problem hiding this comment.
When deleting an unbound local encounters a colliding key whose __eq__ raises, jit_locals_dict_delitem_local has already executed that user callback, drained its pending error, and returned null. This guard then side-exits at the original call position so the eval loop re-runs residual locals()/vars()/dir(), invoking the same deletion and callback a second time before raising; callback mutations or counters therefore occur twice only under the JIT. Preserve and propagate the original failure instead of replaying the operation after the guard exit.
AGENTS.md reference: AGENTS.md:L14-L18
Useful? React with 👍 / 👎.
This was referenced Aug 6, 2026
Merged
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.
Six defect fixes plus one wasm codegen change, rebased onto
origin/main.locals()/vars()/dir()drop the frame's extra locals (task #62)The zero-argument fold added in 5518b96f211 always built its mapping from a
fresh dict and bound one key per bound fastlocal, so a key the frame's own
mapping already carried — one an
f_localswrite put there (PEP 667) —disappeared for as long as the loop stayed compiled, and
fast2locals'delitemarm was never emitted.The fold now models
pyframe.py:555-574line by line:getorcreatedebug()comes off the
debugdatavirtualizable field (so the frame never becomes aresidual operand and nothing arms the vable protocol), a new
FrameDebugDatadescr group carries
w_localsfor the emittedgetfield_gc_r, unbound slotsemit a
delitem, andlocals()/vars()publishframe_locals_snapshot'sindependent PEP 667 copy while
dir()keeps reading the mapping itself.A frame that has no mapping yet keeps the fresh
newdictthe residual wouldhave materialised, under a
guard_isnullso a mapping appearing mid-loopside-exits rather than being written past. Two details worth calling out:
FrameDebugDatais not the frame's — a root portal seedexpands the shadow from the
snapshot_for_tracingcopy, whoseclone_debugdata_ptrhands out a fresh holder around the same mapping.Comparing holders declines every portal trace; the check compares the
mapping.
recursive_forced_frame_kept_stack(
def f(n): d = locals()— every recursive call is a fresh frame whosew_localsis still null). Declining there brings the vable escape back.Other fixes
__eq__and reported a miss. The read chain now has a fallible sibling and three
call sites (
getdictvalue,getattr_str_impl,object_getattribute)route through it.
w_dict_getitem_wtf8_checkeddrained the pending-error flag afterthe unchecked spelling, and the strategy leaf
(
w_dict_lookup_object_strategy) is itself..._checked(..).unwrap_or(None)— so it had already taken the flag and the check always read
false. It nowroutes through
w_dict_lookup_checked.w_list_init_itemsreleased and reacquired the lock aroundthe storage replacement, leaving the Object items block unpinned.
siginterrupt,strsignalandpthread_sigmask's set walknarrowed before range-checking.
operand stack playing the role x86 condition flags play in
next_op_can_accept_cc), and drop identityJUMPself-moves. −1.77% emittedmodule bytes over 40 trace modules on four benches, same module counts.
Verification
pyre/check.py: dynasm 388 passed, cranelift 388 passed, wasm 384 passed.cargo test --all --no-default-features --features dynasm: rc=0, 101 testbinaries ok, zero failures.
One row fails and it is not this branch's:
synth/pickle_terminal_raise_resume loops_compiled 35 -> 30(wasm 72 -> 67).Controlled by path-checkout on this host:
origin/maincontent, 3 runsorigin/mainmisses a baseline it recorded itself. Deliberately notre-recorded.
bdea82761d4ande2625032895net to zero — their content reached mainthrough #1075 — and are kept only to avoid rewriting the branch.
Summary by CodeRabbit
New Features
locals(),vars(), anddir()behavior in optimized code, including preservation of existing local variables.Bug Fixes