jit: constant-depth sys._getframe fold, fget_f_back force removal, and find_biggest_function on portal_trace_positions - #1096
Merged
Merged
Conversation
`TraceCtx::find_biggest_function` read `inline_trace_positions`, a stack of the *active* inlined callees. Its three writers -- `push_inline_trace_position`, `pop_inline_trace_position`, `truncate_inline_trace_positions` -- had no caller in the tree, so the stack was always empty, the function returned `None` unconditionally, and `blackhole_trace_too_long_slow` always took the `prepare_trace_segmenting` else-arm. `inline_trace_depth`, its other reader, had no caller either. The shape was also wrong for the question being asked. pyjitpl.py:3538-3575 walks `portal_trace_positions`, a flat log where `newframe` appends `(jd_no, Some(greenkey), pos)` and `popframe` appends `(jd_no, None, pos)`, so a callee that already returned keeps both of its entries and can still be sized -- and that is usually the culprit, since the function that grew the trace tends to have finished before the limit was crossed. A stack of active frames pops on return and cannot see it. `MetaInterp::find_biggest_function` walks the log with a side stack, sizing each closed frame by the distance between its two `TracePosition::_pos` cursors (the `pos[0]` upstream subtracts), then measures the outermost frame still open at the overflow against the current position (pyjitpl.py:3560-3570). The old field and its five functions are deleted. `portal_trace_positions` was armed only in `MetaInterp::new`, while `blackhole_trace_too_long_slow` sets it to `None` (pyjitpl.py:2795). Upstream builds one MetaInterp per tracing attempt, so there `[]` is per-trace; here the `None` retired the log for the rest of the process after the first overflow, and between traces the list kept entries whose `_pos` cursors index a recorder the next trace does not use. `arm_portal_trace_positions` re-arms it at the three trace-start sites. No behaviour change: production reaches `newframe` only through `do_residual_or_indirect_call` -> `perform_call(jitcode, argboxes, None)`, and pyjitpl.py:2184 passes no greenkey there either, so the log admits nothing and the segmenting arm still runs. Feeding it from the FBW walker's inline path, which inlines Python callees without reaching `newframe`, is the remaining half. Four tests: a returned callee is still sized, an open frame is measured against the current position, an un-inlined trace answers `None`, and a log retired by a previous overflow is empty rather than `None` at the next trace start. cargo test --all --features dynasm --no-run: no errors. cargo test -p majit-metainterp: 1452 + 84 passed, 0 failed. Assisted-by: Claude
pyframe.py:767-768 fget_f_back is `return self.get_f_back()`, with no
force of either end: `f_backref` is a jit.virtual_ref
(executioncontext.py:88-89), so the `frame.f_backref()` read at :80 is
itself the force, and executioncontext.py:323-331 names that read as the
mechanism ("We get this effect simply by reading the f_back field of all
frames").
pyre forced both `self` and the resulting caller concretely instead,
which escapes the virtualizable while tracing.
Recorded jitstats for synth/getframe_inlined_callee_own_frame, identical
on dynasm, cranelift and wasm:
loops_compiled 0 -> 1, loops_aborted 10 -> 6,
fbw_blackhole_adopted_single_frame 9 -> 5, guard_failures 0 -> 1.
Assisted-by: Claude
vm.py:41 marks `getframe` `@jit.look_inside_iff(jit.isconstant(depth))`, so a constant depth is traced through: `ec.gettopframe_nohidden()` is a vref read that pyjitpl.py:2153-2172 `_do_jit_force_virtual` answers with `virtualizable_boxes[-1]` under a ptr_eq + guard_value, the `depth == 0` test folds, and `mark_as_escaped` is one setfield_gc. pyre residualized the whole walk instead, and `getframe`'s two force_frame calls cleared TOKEN_TRACING_RESCALL from inside that residual, which `tracing_after_residual_call` reads as an escape. `try_walker_specialize_sys_getframe` takes the one level the walk can resolve: depth 0 at the top walk level, where the answer is the portal virtualizable. It emits guard_value on the callable; guard_class + exact-class + getfield_gc_i on the depth box with the UNBOXED value required constant (the wrapped W_IntObject the residual receives is built in-trace and is never constant); getfield_gc_r(frame, execution_context) + getfield_gc_r(ec, topframeref) + ptr_eq + guard_true; and a void call for mark_as_escaped. The result is `standard_virtualizable_box()` itself. Inline sub-walks, any other depth, a rebound name, a non-int depth and a topframeref that is not the portal all decline to the existing residual. getframe_* corpus, identical direction on dynasm, cranelift and wasm: loops_aborted 155 -> 71, loops_compiled 6 -> 22, guard_failures 4114 -> 201 on both bridge fixtures. 11 of the 12 fixtures that never compiled now compile a loop. No output changed anywhere in the synthetic suite. Seven fixtures lost their vable escape to the fold, including the corpus's only bridge=true escape and its only ContinueRunningNormally-at-a-merge-point drive. Each gets a `*_declined` sibling carrying the same shape at a force the arm refuses -- sys._getframe(1) where the frame identity does not matter, an added .f_locals read where it does. Every sibling reproduces its original's pre-fold counters exactly. Assisted-by: Claude
WalkthroughThe PR adds specialized Changessys._getframe specialization and frame traversal
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant residual_call
participant specialize
participant sys_vm
participant MetaInterp
residual_call->>specialize: Match top-level sys._getframe call
specialize->>sys_vm: Mark portal frame escaped
specialize-->>residual_call: Return virtualizable frame
MetaInterp->>MetaInterp: Rearm portal trace positions
MetaInterp->>MetaInterp: Select largest portal frame on overflow
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 |
![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: a874e1d70b
ℹ️ 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".
| crate::executioncontext::force_frame(back); | ||
| } | ||
| back | ||
| crate::executioncontext::ExecutionContext::getnextframe_nohidden(this) |
There was a problem hiding this comment.
When f_back names a caller that is currently running in a nested compiled activation, get_f_back() only forces a JitVirtualRef; a raw caller-frame pointer remains unmaterialized. The later f_lasti and f_lineno getters read last_instr directly, unlike f_locals (typedef.rs:7230), so expressions such as sys._getframe().f_back.f_lasti can observe the caller's stale heap value instead of its live JIT register state. Keep the result force until those virtualizable-field getters perform the missing force_frame injection.
AGENTS.md reference: AGENTS.md:L14-L20
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: 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 `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 14176-14209: Update find_biggest_function so the final open-frame
measurement only runs when self.tracing is available; if tracing is None, skip
that measurement and continue returning the max_key computed from closed frames.
Preserve the existing current-position comparison when tracing is Some and
retain None only when no closed or open frame produces a result.
- Around line 20384-20499: Add a regression test alongside the existing
find_biggest_function tests that uses meta_with_recursive_portal, starts
tracing, records and closes a larger portal frame, then opens another frame
without closing it and clears self.tracing via the established
trace-finalization path. Assert find_biggest_function() still returns the
max_key from the previously closed frame rather than None when
portal_trace_positions contains the unmatched entry.
In `@pyre/bench/synth/blackhole_inlined_callee_local_after_escape_declined.py`:
- Around line 19-21: Update the fixture description near the comment after the
inlined-callee escape to state that sys._getframe() folds and the subsequent
f.f_locals attribute read is the forcing residual, matching the exercised path
and the description on lines 9-11.
🪄 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: 98febcc7-d25b-415d-801e-30c4169fac17
📒 Files selected for processing (77)
majit/majit-metainterp/src/compile.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/trace_ctx.rspyre/bench/synth/blackhole_inlined_callee_local_after_escape.cranelift.jitstatspyre/bench/synth/blackhole_inlined_callee_local_after_escape.dynasm.jitstatspyre/bench/synth/blackhole_inlined_callee_local_after_escape.wasm.jitstatspyre/bench/synth/blackhole_inlined_callee_local_after_escape_declined.cranelift.jitstatspyre/bench/synth/blackhole_inlined_callee_local_after_escape_declined.dynasm.jitstatspyre/bench/synth/blackhole_inlined_callee_local_after_escape_declined.pypyre/bench/synth/blackhole_inlined_callee_local_after_escape_declined.wasm.jitstatspyre/bench/synth/getframe_bridge_force_after_store.cranelift.jitstatspyre/bench/synth/getframe_bridge_force_after_store.dynasm.jitstatspyre/bench/synth/getframe_bridge_force_after_store.wasm.jitstatspyre/bench/synth/getframe_bridge_force_after_store_declined.cranelift.jitstatspyre/bench/synth/getframe_bridge_force_after_store_declined.dynasm.jitstatspyre/bench/synth/getframe_bridge_force_after_store_declined.pypyre/bench/synth/getframe_bridge_force_after_store_declined.wasm.jitstatspyre/bench/synth/getframe_bridge_force_plain.cranelift.jitstatspyre/bench/synth/getframe_bridge_force_plain.dynasm.jitstatspyre/bench/synth/getframe_bridge_force_plain.wasm.jitstatspyre/bench/synth/getframe_bridge_force_plain_declined.cranelift.jitstatspyre/bench/synth/getframe_bridge_force_plain_declined.dynasm.jitstatspyre/bench/synth/getframe_bridge_force_plain_declined.pypyre/bench/synth/getframe_bridge_force_plain_declined.wasm.jitstatspyre/bench/synth/getframe_inlined_callee_own_frame.cranelift.jitstatspyre/bench/synth/getframe_inlined_callee_own_frame.dynasm.jitstatspyre/bench/synth/getframe_inlined_callee_own_frame.wasm.jitstatspyre/bench/synth/getframe_residual_callee_own_frame.cranelift.jitstatspyre/bench/synth/getframe_residual_callee_own_frame.dynasm.jitstatspyre/bench/synth/getframe_residual_callee_own_frame.wasm.jitstatspyre/bench/synth/getframe_residual_callee_own_frame_declined.cranelift.jitstatspyre/bench/synth/getframe_residual_callee_own_frame_declined.dynasm.jitstatspyre/bench/synth/getframe_residual_callee_own_frame_declined.pypyre/bench/synth/getframe_residual_callee_own_frame_declined.wasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn.cranelift.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn.dynasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn.wasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_declined.cranelift.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_declined.dynasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_declined.pypyre/bench/synth/getframe_root_loop_force_blackhole_crn_declined.wasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent.cranelift.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent.dynasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent.wasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent_declined.cranelift.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent_declined.dynasm.jitstatspyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent_declined.pypyre/bench/synth/getframe_root_loop_force_blackhole_crn_nonidempotent_declined.wasm.jitstatspyre/bench/synth/getframe_root_loop_force_while_merge.cranelift.jitstatspyre/bench/synth/getframe_root_loop_force_while_merge.dynasm.jitstatspyre/bench/synth/getframe_root_loop_force_while_merge.wasm.jitstatspyre/bench/synth/getframe_root_loop_force_while_merge_declined.cranelift.jitstatspyre/bench/synth/getframe_root_loop_force_while_merge_declined.dynasm.jitstatspyre/bench/synth/getframe_root_loop_force_while_merge_declined.pypyre/bench/synth/getframe_root_loop_force_while_merge_declined.wasm.jitstatspyre/bench/synth/getframe_stored_fback_walk.cranelift.jitstatspyre/bench/synth/getframe_stored_fback_walk.dynasm.jitstatspyre/bench/synth/getframe_stored_fback_walk.wasm.jitstatspyre/bench/synth/getframe_while_caller_locals_across_subwalk.cranelift.jitstatspyre/bench/synth/getframe_while_caller_locals_across_subwalk.dynasm.jitstatspyre/bench/synth/getframe_while_caller_locals_across_subwalk.wasm.jitstatspyre/bench/synth/getframe_while_captured_frame_outlives_call.cranelift.jitstatspyre/bench/synth/getframe_while_captured_frame_outlives_call.dynasm.jitstatspyre/bench/synth/getframe_while_captured_frame_outlives_call.wasm.jitstatspyre/bench/synth/getframe_while_escaping_read_frame_identity.cranelift.jitstatspyre/bench/synth/getframe_while_escaping_read_frame_identity.dynasm.jitstatspyre/bench/synth/getframe_while_escaping_read_frame_identity.wasm.jitstatspyre/bench/synth/getframe_while_inlined_callee_subwalk.cranelift.jitstatspyre/bench/synth/getframe_while_inlined_callee_subwalk.dynasm.jitstatspyre/bench/synth/getframe_while_inlined_callee_subwalk.wasm.jitstatspyre/bench/synth/getframe_while_subwalk_decline_shapes.cranelift.jitstatspyre/bench/synth/getframe_while_subwalk_decline_shapes.dynasm.jitstatspyre/bench/synth/getframe_while_subwalk_decline_shapes.wasm.jitstatspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rs
💤 Files with no reviewable changes (2)
- majit/majit-metainterp/src/compile.rs
- majit/majit-metainterp/src/trace_ctx.rs
Comment on lines
+14176
to
+14209
| pub fn find_biggest_function(&self) -> Option<u64> { | ||
| let positions = self.portal_trace_positions.as_ref()?; | ||
| let mut start_stack: Vec<(u64, usize)> = Vec::new(); | ||
| let mut max_size = 0usize; | ||
| let mut max_key = None; | ||
| for &(_jd_no, key, pos) in positions { | ||
| match key { | ||
| // pyjitpl.py:3547-3548 `if key is not None: start_stack.append`. | ||
| Some(key) => start_stack.push((key, pos._pos)), | ||
| // pyjitpl.py:3549-3559 the closing entry sizes the frame it | ||
| // closes. An unmatched close cannot happen while `newframe` / | ||
| // `popframe` are the only writers, so it is left to `pop`'s | ||
| // `None` rather than given a recovery path. | ||
| None => { | ||
| if let Some((green_key, start_pos)) = start_stack.pop() { | ||
| let size = pos._pos.saturating_sub(start_pos); | ||
| if size > max_size { | ||
| max_size = size; | ||
| max_key = Some(green_key); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| // pyjitpl.py:3560-3570 `if start_stack:` — one frame, the outermost, | ||
| // measured against where the trace stopped. | ||
| if let Some(&(green_key, start_pos)) = start_stack.first() { | ||
| let current = self.tracing.as_ref()?.get_trace_position()._pos; | ||
| if current.saturating_sub(start_pos) > max_size { | ||
| max_key = Some(green_key); | ||
| } | ||
| } | ||
| max_key | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the early return that discards an already-computed result.
In find_biggest_function, the final block reads:
if let Some(&(green_key, start_pos)) = start_stack.first() {
let current = self.tracing.as_ref()?.get_trace_position()._pos;
...
}The ? on self.tracing.as_ref() returns None from the whole function when self.tracing is None. This discards max_key, even when the loop above already found a valid answer from properly closed frame pairs. The function's own doc says it returns None only "when the log holds no closed or open portal frame", but this code returns None whenever tracing has ended and a stale open frame remains in the log, regardless of any closed frames already sized.
The only current call site (blackhole_trace_too_long_slow) guards self.tracing to always be Some, so this is not reachable in production today. But find_biggest_function is pub, so a future caller invoking it after tracing ends (with a stale unmatched entry left in portal_trace_positions) gets a silently wrong None instead of the valid max_key from closed frames.
Skip the "measure the open frame" step instead of aborting the whole function when tracing is not active.
🐛 Proposed fix
- if let Some(&(green_key, start_pos)) = start_stack.first() {
- let current = self.tracing.as_ref()?.get_trace_position()._pos;
- if current.saturating_sub(start_pos) > max_size {
- max_key = Some(green_key);
- }
- }
+ if let Some(&(green_key, start_pos)) = start_stack.first() {
+ if let Some(ctx) = self.tracing.as_ref() {
+ let current = ctx.get_trace_position()._pos;
+ if current.saturating_sub(start_pos) > max_size {
+ max_key = Some(green_key);
+ }
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 14176 - 14209, Update
find_biggest_function so the final open-frame measurement only runs when
self.tracing is available; if tracing is None, skip that measurement and
continue returning the max_key computed from closed frames. Preserve the
existing current-position comparison when tracing is Some and retain None only
when no closed or open frame produces a result.
Comment on lines
+20384
to
+20499
| /// A MetaInterp with one recursive portal registered — the shape whose | ||
| /// frames `is_main_jitcode` admits to `portal_trace_positions` — and the | ||
| /// jitcode `perform_call` takes. Not yet tracing. | ||
| fn meta_with_recursive_portal() -> (MetaInterp<()>, std::sync::Arc<crate::jitcode::JitCode>) { | ||
| use crate::jitcode::JitCodeBuilder; | ||
|
|
||
| let mut meta = MetaInterp::<()>::new(0); | ||
| meta.finish_setup_descrs_for_jitdrivers(); | ||
| let mut jd = crate::jitdriver::JitDriverStaticData::new(vec![], vec![]); | ||
| jd.is_recursive = true; | ||
| let idx = { | ||
| let MetaInterp { | ||
| staticdata, | ||
| backend, | ||
| .. | ||
| } = &mut meta; | ||
| std::sync::Arc::get_mut(staticdata) | ||
| .unwrap() | ||
| .register_jitdriver_sd(jd, backend) | ||
| }; | ||
| let mut jc = JitCodeBuilder::new().finish(); | ||
| jc.replace_jitdriver_sd(Some(idx)); | ||
| (meta, std::sync::Arc::new(jc)) | ||
| } | ||
|
|
||
| fn start_tracing(meta: &mut MetaInterp<()>) { | ||
| let action = meta.force_start_tracing(0, (0, 0), None, &[]); | ||
| assert!(matches!(action, crate::BackEdgeAction::StartedTracing)); | ||
| } | ||
|
|
||
| fn record_ops(meta: &mut MetaInterp<()>, n: usize) { | ||
| let ctx = meta.tracing.as_mut().expect("tracing is Some"); | ||
| for _ in 0..n { | ||
| ctx.record_op(majit_ir::OpCode::PtrEq, &[]); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn find_biggest_function_sizes_a_callee_that_already_returned() { | ||
| // pyjitpl.py:3538-3559. The frame that grew the trace is usually one | ||
| // that returned before the limit was crossed; `portal_trace_positions` | ||
| // keeps both of its entries, so the walk can still size it. The | ||
| // `inline_trace_positions` stack this replaced popped on return and | ||
| // could only ever see the frames still open at the overflow. | ||
| let (mut meta, jc) = meta_with_recursive_portal(); | ||
| start_tracing(&mut meta); | ||
|
|
||
| meta.perform_call(jc.clone(), &[], Some(0xa11)).unwrap_err(); | ||
| record_ops(&mut meta, 5); | ||
| meta.popframe(true); | ||
|
|
||
| meta.perform_call(jc, &[], Some(0xb22)).unwrap_err(); | ||
| record_ops(&mut meta, 1); | ||
| meta.popframe(true); | ||
|
|
||
| assert_eq!( | ||
| meta.find_biggest_function(), | ||
| Some(0xa11), | ||
| "the larger frame wins even though both have returned" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn find_biggest_function_measures_an_open_frame_against_the_current_position() { | ||
| // pyjitpl.py:3560-3570 `if start_stack:` — a frame the overflow | ||
| // interrupted has no closing entry, so its size is measured against | ||
| // where the trace stopped. | ||
| let (mut meta, jc) = meta_with_recursive_portal(); | ||
| start_tracing(&mut meta); | ||
|
|
||
| meta.perform_call(jc.clone(), &[], Some(0xa11)).unwrap_err(); | ||
| record_ops(&mut meta, 1); | ||
| meta.popframe(true); | ||
|
|
||
| meta.perform_call(jc, &[], Some(0xb22)).unwrap_err(); | ||
| record_ops(&mut meta, 5); | ||
|
|
||
| assert_eq!(meta.find_biggest_function(), Some(0xb22)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn find_biggest_function_is_none_without_an_inlined_portal_frame() { | ||
| // The root frame carries no greenkey, so a trace that inlined nothing | ||
| // leaves the log empty and the caller takes the segmenting arm. | ||
| let (mut meta, _jc) = meta_with_recursive_portal(); | ||
| start_tracing(&mut meta); | ||
| record_ops(&mut meta, 5); | ||
| assert_eq!(meta.find_biggest_function(), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn portal_trace_positions_are_rearmed_for_each_trace() { | ||
| // pyjitpl.py:2391. Upstream builds a MetaInterp per tracing attempt; | ||
| // pyre re-arms the log instead. Without it the `= None` that | ||
| // `blackhole_trace_too_long_slow` writes would retire the log for the | ||
| // rest of the process, and a surviving list would mix `_pos` cursors | ||
| // from a recorder the next trace does not use. | ||
| let (mut meta, jc) = meta_with_recursive_portal(); | ||
| // The state `blackhole_trace_too_long_slow` leaves behind: this | ||
| // MetaInterp already overflowed one trace and retired its log. | ||
| meta.portal_trace_positions = None; | ||
|
|
||
| start_tracing(&mut meta); | ||
| assert_eq!( | ||
| meta.portal_trace_positions.as_ref().map(Vec::len), | ||
| Some(0), | ||
| "the next trace starts from an empty log, not from None" | ||
| ); | ||
| meta.perform_call(jc, &[], Some(0xa11)).unwrap_err(); | ||
| assert_eq!( | ||
| meta.portal_trace_positions.as_ref().expect("Some").len(), | ||
| 1, | ||
| "and newframe records into it again" | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Tests do not cover the self.tracing == None edge case.
These new tests (find_biggest_function_sizes_a_callee_that_already_returned, find_biggest_function_measures_an_open_frame_already_returned, find_biggest_function_is_none_without_an_inlined_portal_frame, portal_trace_positions_are_rearmed_for_each_trace) always call find_biggest_function() while self.tracing is Some. None of them exercise the case where portal_trace_positions holds a stale unmatched open-frame entry while self.tracing is None, so they do not catch the ?-discards-valid-result issue flagged at Lines 14176-14209.
Consider adding a test that: starts tracing, opens a portal frame without closing it, then takes self.tracing (e.g., via finish_trace_for_parity or a direct self.tracing = None), and asserts find_biggest_function() still returns the max_key from any previously closed frames instead of None.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 20384 - 20499, Add a
regression test alongside the existing find_biggest_function tests that uses
meta_with_recursive_portal, starts tracing, records and closes a larger portal
frame, then opens another frame without closing it and clears self.tracing via
the established trace-finalization path. Assert find_biggest_function() still
returns the max_key from the previously closed frame rather than None when
portal_trace_positions contains the unmatched entry.
Comment on lines
+19
to
+21
| # An inlined callee assigns a local, the frame then escapes through a residual | ||
| # `sys._getframe()`, and an attribute read POSITIONED AFTER that escape reads the | ||
| # local back. The read is executed by the blackhole, not by the walk, so the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the fixture description.
Lines 9-11 state that sys._getframe() folds and f.f_locals is the forcing residual. Lines 19-21 instead describe sys._getframe() as residual. Keep the description consistent with the exercised path.
Proposed fix
-# An inlined callee assigns a local, the frame then escapes through a residual
-# `sys._getframe()`, and an attribute read POSITIONED AFTER that escape reads the
-# local back.
+# An inlined callee assigns a local. `sys._getframe()` folds. The subsequent
+# `.f_locals` read is the forcing residual and reads the local after escape.🤖 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/bench/synth/blackhole_inlined_callee_local_after_escape_declined.py`
around lines 19 - 21, Update the fixture description near the comment after the
inlined-callee escape to state that sys._getframe() folds and the subsequent
f.f_locals attribute read is the forcing residual, matching the exercised path
and the description on lines 9-11.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a874e1d). 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
|
youknowone
added a commit
that referenced
this pull request
Aug 7, 2026
…es that have main red on every OS (#1099) * jit: keep find_biggest_function's closed-frame result when the recorder is gone pyjitpl.py:3562 reads `self.history.get_trace_position()` unconditionally, so the `max_key` the closed-frame loop above produced always survives to the return. pyre's recorder is an `Option` and the port spelled that read as `self.tracing.as_ref()?`, which returns `None` for the whole function whenever tracing has ended with an unmatched open entry still in `portal_trace_positions`. Only the open frame is unmeasurable without a recorder, so only its measurement is skipped now. Not reachable from `blackhole_trace_too_long_slow`, which holds `self.tracing` as `Some`; `find_biggest_function` is `pub`. `find_biggest_function_keeps_a_closed_frame_when_the_recorder_is_gone` covers it and fails with the `?` put back. Also corrects blackhole_inlined_callee_local_after_escape_declined.py's second description block, which still called `sys._getframe()` the residual after the file's own header states it folds and the added `.f_locals` read is the force. Assisted-by: Claude * jit: emit sys._getframe's mark_as_escaped as a setfield, and carry the sized frame's jitdriver out of find_biggest_function vm.py:54 `f.mark_as_escaped()` is traced as an ordinary `setfield_gc` on the flag. The constant-depth fold emitted it as a void CallN into a Rust helper instead, which hides the update from the optimizer and its heap cache. Replaced with the read/or/store the `tb_frame` fold in the same file already uses (specialize.rs:2299-2313): getfield_gc_i(flags) + int_or(FLAG_ESCAPED) + setfield_gc + heapcache_setfield_cached. `jit_frame_mark_as_escaped` is deleted. pyjitpl.py:3575 returns `max_jdsd, max_key`, and pyjitpl.py:2821-2824 uses both -- the disable goes through the OWNING driver's warmstate and that driver is what `aborted_tracing_jitdriver` stores. The port dropped the jd_no its own log entries already carry and hardcoded driver 0. It now returns `Option<(usize, u64)>` and the caller stores the index it was given. pyre keeps one WarmEnterState on the MetaInterp rather than one per JitDriverStaticData, so `disable_noninlinable_function` still lands on that single state; the comment names it. No recorded counter moves on dynasm, cranelift or wasm. Assisted-by: Claude * bench: restore the three jit-stats baselines #1063 replaced with values no host produces `pyre/check.py` has been red on main for binary_int_overflow_local_resume, exc_bridge_entry_guard_not_removed and list_append_write_barrier_gc since 9d2fff9, on ubuntu-24.04, macos-latest AND windows-latest (run 31140634730), and locally on macOS across all three backends. Every one of those hosts observes bridges_compiled/guard_failures 5/647, 4/809 and 5/1345. Those are exactly the values that stood on main before 9d2fff9 (last written by #947 and #1059); 9d2fff9 recorded 6/686, 5/1009 and 6/1562, which reproduce nowhere. The re-record was taken against a base whose behaviour these fixtures no longer had, and the merge replayed it. Re-recorded on dynasm, cranelift and wasm. The counters land back on the pre-9d2fff92649 values; the `field_pos_*` fields 9d2fff9 added are kept. Assisted-by: Claude * jit: arm the deferred escape-flush undo when only the locals region flushed `flush_active_frame_escape`'s force arm has three outcomes. A committed full flush publishes a resume pc into `COMMITTED_FRAME_ESCAPE_PC`; an all-or-nothing decline discards the undo capture; the third -- the full flush declines and `flush_locals_region_to_frame` writes slots `0..nlocals` on their own -- did neither. That leg claims no resume pc, so `take_committed_frame_escape_pc` yields nothing and the walk-end block gated on it is skipped in its entirety, including the `restore_escape_flush_undo()` in its `else`. The capture stays armed, `LiveLastInstrGuard::drop` reads an armed capture as a flush owning the frame and declines to put `last_instr` back, and the legacy replay re-enters one opcode past the call on an operand stack no flush wrote: `value-stack underflow: depth=N base=N`, a JIT-only panic with no program output. `mark_escape_flush_undo_pending()` routes the leg to the walk-end deferred restore, which is already conditioned on no continuation having claimed the flushed frame -- so where the walk goes on to adopt a blackhole image the request is consumed without restoring and the adoption keeps the frame it claimed. Restoring earlier is not equivalent: making `LiveLastInstrGuard::drop` test the commit instead removes the crash and returns a stale caller line, because the walk goes on after the residual and nothing else advances `last_instr`. `bench/synth/handler_tb_frame_locals_after_declined_flush.py` reaches the leg: `'i' in tb.tb_frame.f_locals` forces the frame mid-expression, with the `seen.add` receiver and its bound method live below the value being computed. A/B on the cranelift binary that reproduced it: 10/10 panics without the change, 0/10 with it, output `[True]` matching `PYRE_NO_JIT=1`. Assisted-by: Claude * bench: survey a caller's f_lineno and f_lasti from two call sites A callee reading its caller's frame through `sys._getframe(1)` had no coverage of the resume coordinate: `bench/synth` holds ten `_getframe(1)` fixtures, one `f_lineno` fixture (a traceback frame) and no `f_lasti` fixture at all. Both fields resolve off `last_instr`, which compiled code does not store per opcode, so the value only reaches the frame if the force publishes it. Two call sites are what make that observable. One holds the caller's coordinate constant by construction, so a frozen read is indistinguishable from a live one. Surveying every iteration into a set rather than sampling the last one is the other half: the pre-compile iterations are correct, so a miss appears as a changed row count. `f_lasti` is a bytecode offset and so is not comparable against the pypy oracle; only its discrimination is printed. `f_lineno` is compared directly, relative to `co_firstlineno`. Measured by putting a defect back in: with the `flushed` test dropped from `LiveLastInstrGuard::drop`, so the guard restores at the residual's return instead of at walk end, the fixture reports ([(0, 3), (0, 8), (1, 3), (1, 6)], [0, 0, 1, 1], 3) against its ([(0, 8), (1, 6)], [0, 1], 2) -- the pre-call coordinate appears alongside the call-site one on both legs. cpython, pypy, `PYRE_NO_JIT=1`, dynasm, cranelift and wasm all print the latter. The walk-end epilogue gains the negative result measured while looking for a counter to gate the same defect: every walk reaching that point on this fixture reports `armed=false fb=true`, so a leak counter conditioned on the three adoption flags being false reads 0 whether or not the force arm arms its deferred restore. Assisted-by: Claude * check.py: fail the build on a stale LLBC instead of measuring through it `pyre-jit-trace/build.rs` compares each `build/llbc/*.ullbc` against what its crate's sources hash to now and reports a mismatch as `cargo::warning`, which cargo replays only when it re-runs the build script -- so a run whose crates were cached prints nothing at all. Every number check.py produces is read out of a binary whose field offsets come from those artefacts. Measured on this tree: four measurement runs -- a three-backend gate, two A/B arms and a base control -- carried the mismatch, and the string `LLBC STALE` appears in none of their logs, while `cargo check -p pyrex` on the same tree printed it for all three crates. check.py only ever tested for the artefacts being missing. It now exports `PYRE_LLBC_STRICT=1` before every backend build, the promotion build.rs documents for callers that want a gate, and names staleness in the build-failure diagnostics beside the missing-artefact branch. The cost is that a rebase which moves the LLBC crates stops the next check.py until a re-extraction; `PYRE_LLBC_SKIP_FINGERPRINT_CHECK=1` still opts out for an A/B whose only changed crate contributes no field offsets. First use found one: the wasm jit-stats fall on `exception_reused_object_tb_not_doubled` that four arms reproduced was an artefact of the stale artefacts, and the bench passes on all three backends after a re-extraction with nothing re-recorded. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
gate-triage.md claimed `opimpl_virtual_ref` / `_finish` have no caller outside a `#[test]`. They do: `walker_ec_enter` / `walker_ec_leave` call them on the live inline-push path in `pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`, which the same file already says 100 lines later. The parenthetical now states the narrower residue instead -- `vrefs_before_residual_call` / `vrefs_after_residual_call` iterate zero times over a level the walker inlines without seeding a frame -- and points at `jitcode_dispatch/mod.rs` item (a), where that residue is recorded. It also notes that `mod.rs`'s upstream citation for the residue is wrong: `perform_call` (pyjitpl.py:2445-2449) is `newframe` + `setup_call` and never touches `virtualref_boxes`; upstream's vref comes from `ExecutionContext.enter` (pypy/interpreter/executioncontext.py:88-89), traced through on an inlined call. getframe_root_loop_force_blackhole_crn.py said "this file adopts it five times". Its committed baselines record fbw_blackhole_adopted_single_frame=0, loops_aborted=0, loops_compiled=1 on all three backends; ca9edf7 (#1096) moved them from 5 / 5 / 0. Header now states the recorded numbers and points at the `_declined` sibling, which records 5 / 5 / 0. blackhole_inlined_callee_local_after_escape.py opened "Guard for what an adopted multi-frame blackhole chain owes its inner levels". fbw_blackhole_adopted_multi_frame is 0 in all three of its baselines and was 0 before #1096 as well; the five adopts it used to take were single-frame, and today it takes none (0 / 0 / 2). Header now says so, names the eight fixtures' worth of corpus that does pin the multi-frame arm (getframe_inline_subwalk_multiframe, getframe_while_inlined_callee_subwalk et al., all nonzero on three backends), and points at the `_declined` sibling. No baseline was re-recorded; no Rust and no executable Python changed. Assisted-by: Claude
youknowone
added a commit
that referenced
this pull request
Aug 20, 2026
…d deny through the warm state (#1364) * jit: name the fixture the inline-chain depth cap was measured on `FBW_INLINE_CHAIN_DEPTH`'s doc cited `depthN_inline_chain`, which no longer names any file: #829 deleted `depth2_`/`depth3_`/`depth7_inline_chain_typeflip.py` and added `inline_chain_depth_typeflip.py` in the same commit. Cite the surviving file and record that the ~2.0-2.3x number was taken before the consolidation. Assisted-by: Claude * majit: publish the preview short-preamble export as one PreviewShortState Replace the four OptContext fields `exported_short_boxes`, `exported_short_inputargs`, `exported_short_inputarg_refs` and `exported_short_args_state` with a single `Option<PreviewShortState>` holding the three vectors plus an `Option` args_state. `preamble_end_args` stays a separate `Option<Vec<OpRef>>`. optimizer.rs binds `create_short_inputargs`, `create_short_inputarg_refs`, the args-state tuple and the filtered short boxes to locals and assigns the struct once, after the majit_log dump. `force_box_for_end_of_preamble` and its `preamble_end_args` write are unmoved. unroll.rs::export_state_with_bounds reads the args-state through `preview_short_state.and_then(|p| p.args_state)`, and takes the short inputargs / inputarg refs / short boxes from one match on the `Option` instead of an `is_empty()` test on a defaulted vector. The `debug_assert_eq!` cross-checking `exported_short_inputarg_refs` against `exported_short_inputargs` is dropped; the two are now built and published together. The length check against the export-site `label_args + virtuals` recompute is kept. unroll.rs tests gain `publish_preview_short_state` and `mint_short_inputargs` helpers; the four fixtures that wrote the ctx fields directly now construct the struct. Assisted-by: Claude * docs: correct the jit.virtual_ref emit note and two fbw fixture headers gate-triage.md claimed `opimpl_virtual_ref` / `_finish` have no caller outside a `#[test]`. They do: `walker_ec_enter` / `walker_ec_leave` call them on the live inline-push path in `pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`, which the same file already says 100 lines later. The parenthetical now states the narrower residue instead -- `vrefs_before_residual_call` / `vrefs_after_residual_call` iterate zero times over a level the walker inlines without seeding a frame -- and points at `jitcode_dispatch/mod.rs` item (a), where that residue is recorded. It also notes that `mod.rs`'s upstream citation for the residue is wrong: `perform_call` (pyjitpl.py:2445-2449) is `newframe` + `setup_call` and never touches `virtualref_boxes`; upstream's vref comes from `ExecutionContext.enter` (pypy/interpreter/executioncontext.py:88-89), traced through on an inlined call. getframe_root_loop_force_blackhole_crn.py said "this file adopts it five times". Its committed baselines record fbw_blackhole_adopted_single_frame=0, loops_aborted=0, loops_compiled=1 on all three backends; ca9edf7 (#1096) moved them from 5 / 5 / 0. Header now states the recorded numbers and points at the `_declined` sibling, which records 5 / 5 / 0. blackhole_inlined_callee_local_after_escape.py opened "Guard for what an adopted multi-frame blackhole chain owes its inner levels". fbw_blackhole_adopted_multi_frame is 0 in all three of its baselines and was 0 before #1096 as well; the five adopts it used to take were single-frame, and today it takes none (0 / 0 / 2). Header now says so, names the eight fixtures' worth of corpus that does pin the multi-frame arm (getframe_inline_subwalk_multiframe, getframe_while_inlined_callee_subwalk et al., all nonzero on three backends), and points at the `_declined` sibling. No baseline was re-recorded; no Rust and no executable Python changed. Assisted-by: Claude * docs: separate the MIFrame perform_call builds from the frame a vref is taken of The previous commit's gate-triage note called `pyjitpl.py:2445-2476` a wrong citation and said `mod.rs` held the last copy. Both overreach. The range spans `newframe` (:2455-2476), which does build a fresh frame per inlined call -- an `MIFrame`, the tracer's register frame. `pyre-jit-trace/src/helpers.rs` cites it for exactly that and is correct; stripping it there would remove a right citation. The same sentence also lives in `inline_call.rs`, so `mod.rs` was never the only copy. What is actually wrong at the two vref sites is conflating that `MIFrame` with the app-level frame `ExecutionContext.enter` takes `jit.virtual_ref` of. State that at both, and say in gate-triage which of the two claims the range supports. Also: the emit note said `virtualref_boxes` is populated for every seeded level, but `entered_ec` adds a non-null concrete frame and a non-null `execution_context` on top of seeding -- necessary, not sufficient. And the crn fixture header's inserted paragraph left "Its effects are idempotent" pointing at the `_declined` sibling instead of the drive. Assisted-by: Claude * majit: lower the int/float list capacity read as arraylen_gc `list.int_capacity` / `list.float_capacity` emitted `getfield_gc_r(<strategy>_items.block)` followed by a struct `getfield_gc_i(ItemsBlock.capacity)`. Upstream's capacity read is `len(l.items)` on `l.items: Ptr(GcArray(ITEM))` (rpython/rtyper/lltypesystem/rlist.py:251 in `_ll_list_resize_hint`, rlist.py:286 in `_ll_list_resize_ge`), which the rtyper lowers to `getarraysize` and `jtransform.py:808 rewrite_op_getarraysize` rewrites to `arraylen_gc`. `list.obj_capacity` already emitted `ArrayLen`. Both arms now emit `OpKind::ArrayLen` on the backing block, matching the object-strategy arm. The two unit tests are renamed and assert the new op shape. Assisted-by: Claude * majit: narrow the PreviewShortState alignment claim to the two vectors that share an index space The struct doc and the publication comment both said the three published vectors are "index-aligned by construction". Only two of them are: `short_inputargs` and `short_inputarg_refs` get one entry per `add_short_input_arg`. `short_boxes` is a different population -- the surviving produced short ops, after `short_boxes_exported`'s `filter_map` drops every one whose `canonical_result` is constant -- so its length is unrelated to the other two and `short_boxes[i]` pairs with nothing. What one publication site does buy is that a reader cannot see one vector from this evaluation of the preview beside another that was never written; both comments now say that instead. The unroll.rs test helper restates the refs/inputargs length invariant as a `debug_assert_eq!`. The production publisher gets it from `create_short_inputarg_refs`, which asserts internally; a fixture builds the two vectors by hand and had no check between a short refs vector and the failed `Weak` upgrade it causes past the peel boundary. Assisted-by: Claude * docs: correct eight comments that assert a capability the code already has Each of these states an absolute negative -- "never emits", "never calls it", "cannot be added and go unnamed" -- that its own call site contradicts. - `residual_call.rs`: `vrefs_after_residual_call` is called by the walker, under the `is_may_force` gate mirroring `pyjitpl.py:2007`. Its loops are empty because no `jit.virtual_ref` producers exist, which is a fact about the vref list, not about the call site. - `branch.rs` `decode_side_other_target`: the fused `goto_if_not_<cmp>` forms do reach the walk dispatch, minted by `majit-translate`'s jtransform as `ExitSwitch::Fused` for the LLBC-lowered graphs. What is true is narrower: the sole caller passes a `PyJitCode`, built per Python CodeObject by pyre's own codewriter, and `ExitSwitch::Tuple` -- the only path to a fused goto there -- has no producer outside `flatten.rs`'s own unit test. A fused form would be declined, not mis-decoded. - `inline_call.rs` / `fbw_state.rs`: `callee_body_contains_raise` caps a raising callee at TWO multiframe levels, not at the top inline level, and the cross-frame unwind bridge is built. The measurement beside `effective_multiframe_depth` is what bounds it: two levels green, a third taking `selfrec_tail_exception_unwind` from 937 to 7408 guard failures. - `state.rs` (two sites) / `helpers.rs`: `perform_call` (`pyjitpl.py:2445-2449`) is `newframe` + `setup_call` + `raise ChangeFrame`, and `newframe` (`:2455-2476`) builds an `MIFrame` and nothing else. Upstream has no recording-time app-level frame to hand out at that point; it gets one from tracing the interpreter's own frame construction, which pyre does not do. - `state.rs` / `trace.rs`: the pointer to `perform_call (trace_opcode.rs)` is dead -- no such function there. Callee sym state is set by `inline_call.rs`'s `setup_call` port. - `diag.rs` `SPEC_FOLD_ROWS`: the table cannot become a complete census by adding rows. It names a fold by its function, and two shapes have no name to give: a fold whose emit is inlined into a `match` arm has no function, and a registry-dispatched fold grows by one entry with no new call site. Also: `specialize.rs` drops line numbers from an in-repo file reference, and `tupleobject.rs` records that the `w_tuple_new` interception is the sole reason the `_ff` layout has no producer -- so restoring the upstream shape also makes the walker's `ff` specialisation arm live. Assisted-by: Claude * fbw: print all fourteen fbw_diag slots from both readers and namespace the escape/force keys The two readers of the same counter array printed disjoint index sets: the native reader (pyre/pyrex) printed {1, 6..13} and the wasm host (pyre-wasm-runner) printed {0..5, 11..13}, so slots 0 and 2..5 were bumped on the native backends and readable only through the wasm export, and slots 6..10 the other way round. One key per tally slot is now declared beside the counters as `pyre_jit_trace::trace::fbw_diag::LABELS` (length `RING_BASE`, so rustc rejects an unnamed slot), re-exported as `pyre_jit::FBW_DIAG_LABELS`, and joined against `get(i)` by both readers into a single `[jit-stats] fbw_diag` line carrying the same keys in the same order. The runner mirrors the array positionally, as it already does for `MC_DIAG_LABELS`, since it links no pyre crate. The MIDBODY_LATCH doc said the native corpus reaches neither leg "so these say whether the wasm target does"; it now says that a nonzero native value is itself the news, which is why both readers print it. On wasm the tally line moves out of the PYRE_WASM_JIT_STATS block — check.py never sets that variable — into the MAJIT_STATS block, the gate the native reader prints under. The `[fbw-census]` ring stays where it was. The four gated keys (fbw_rolled_back_with_effects, fbw_store_journal_rollback_failed, fbw_blackhole_adopted_single_frame, fbw_blackhole_adopted_multi_frame) keep their spelling and move from the counter line onto that fbw_diag line; the single `pyre_fbw_diag` lookup still feeds the missing-export refusal. The two `subset/total` fractions become named keys, the hazardous subset spelled `fbw_midbody_latch_new_unjournaled` and `fbw_escape_plain_fallback_unclean`. The five bare keys portal_only, published_callee_only, portal_and_published_callee, by_portal and by_callee_only are renamed fbw_escape_portal_only, fbw_escape_published_callee_only, fbw_escape_portal_and_published_callee, fbw_force_by_portal and fbw_force_by_callee_only: check.py's `_jit_stats_merged` folds every `[jit-stats]` line into one flat map, in which an un-namespaced key is a collision hazard. No committed .jitstats baseline carries any of the five under either spelling, so nothing is orphaned by the rename. They are left out of JITSTATS_SNAPSHOT_FIELDS, i.e. deliberately ungated, and check.py now records why: they are workload counts with no healthy value and no measured polarity (the reason `bridges_compiled` sits in neither regression list), and listing one would make every baseline that lacks it compare 0 -> N and fail until re-recorded. That re-record is a decision to take deliberately, with a polarity in hand. Checked with `cargo check -p pyrex`, `cargo check -p pyre-wasm-runner` and `cargo fmt --check`. No pyre binary was built or run, and no .jitstats snapshot was re-recorded. Assisted-by: Claude * fbw: make the decline census process-wide, as its own doc already claimed `FBW_DECLINE_CENSUS` was a `thread_local!` while the comment above it called it a "Per-process census". pyre installs `_thread` (`pyre-interpreter/src/importing.rs`), so Python threads are real OS threads and each traces on its own; the dump therefore reported only whichever thread happened to print it and silently dropped every decline the others took. Now a `static Mutex<BTreeMap>` behind a `census_map()` accessor that recovers from poisoning -- a map of counters has no invariant a panicking writer can leave broken, and a diagnostic that goes silent after an unrelated panic is worse than one that keeps counting. The lock costs nothing at this rate: the map is touched only on the cold decline path, never on the hot trace path. Pinned by `the_decline_census_counts_a_record_from_another_thread`, shown to fail on the `thread_local!` storage first: assertion `left == right` failed: a decline recorded off-thread never reached the census left: 0 right: 1 No gate exposure: `fbw_census` appears in no check.py field, no pyrex path and no committed baseline, so this changes a diagnostic only. Assisted-by: Claude * fbw: pin the wasm runner's fbw_diag label mirror against the slot constants `pyre-wasm-runner` links no pyre crate, so it restates `pyre_jit_trace::trace::fbw_diag::LABELS` as a positional array. rustc length-checks each side against its own constant (`RING_BASE` / `FBW_SLOTS`), but neither compiler sees the spellings, so a rename drifts silently and every tally from the divergence onward is printed under the wrong key -- and check.py folds every `[jit-stats]` line into one flat map, so a wrong name is compared against the wrong baseline rather than reported as missing. Four checks, following `majit-metainterp/tests/mc_diag_mirror.rs`: the parser is validated against the compiler-enforced count before being used to diagnose drift, the two declared counts must agree, and the two arrays must agree entry by entry. A positive control injects both drift shapes into the real runner source in memory -- a rename, caught by the entry comparison, and a dropped last slot, which leaves every surviving entry correctly named and so can only be caught by the length check. Perturbing the real text rather than a fixture is what makes the control cover the anchors. A fifth check closes what a two-array diff structurally cannot see: `LABELS` shifting against the slot CONSTANTS moves both arrays together, renaming every tally on both backends at once. Each label is bound to its own constant, and the bound slots are required to be exactly `0..RING_BASE` so a new slot cannot go unbound. The bindings are written out rather than derived from the constant names because two of them break the mechanical reading: `ESCAPE_FORCE_BY_PORTAL` is `fbw_force_by_portal`, not `fbw_escape_force_by_portal`. Assisted-by: Claude * docs: name the third fold shape SPEC_FOLD_ROWS structurally cannot hold The table names a fold by its function. Two shapes with no name to give were already recorded; a sweep of `vable_ops.rs` found a third. An ELISION fold recognises a shape and emits nothing, so a census keyed on "what IR did this fold emit instead" has nothing to key on. Three arms are this, all guarded by `fbw_strict_fold_frame_reg`: a store to the current inline level's own unseeded portal frame is a virtual-field write, folded away with no SETFIELD_GC recorded. Their recognisers (`fbw_strict_fold_frame_reg`, `folded_store_is_observable_local`) are predicates -- they cannot emit, because the eliding is the arm. This is distinct from the functionless-replace shape already listed: `bool_box_truth_lookup`'s arm has no function but does write a result. Assisted-by: Claude * majit-translate: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` citations in codewriter/ and annotator/ comments with the symbol that owns the cited code, verified by opening each target. Upstream `.py` line citations are untouched. String-literal occurrences (assertion and panic messages) are untouched. Also drop the internal tracking labels `Z2.5 Path C`, `Phase I3` and `F2 followup`, the filename-less `(line ~3273)` pointer, and replace insns.rs's "documented at the const-table site above" with the fact that byte 18 now houses `BC_GOTO_IF_NOT`. Four citations are left as-is because their target no longer exists: `build_flow.rs:215` (file deleted with the syn-AST front-end) in call.rs and codewriter.rs, and `parse.rs:314-318` (parse.rs shrank from ~2000 to 98 lines) twice in call.rs. Assisted-by: Claude * pyre-jit: re-anchor comment references from line numbers to symbols Replace every `file.rs:NNN` citation in pyre/pyre-jit comments with the symbol that lives at the cited location, or with the bare filename where the surrounding text already names the symbol. Upstream `.py` citations (rpython/pypy/lib-python) are untouched. Also replace directional cross-references ("see below", "see comment above", "see the deferral below") with the named symbol they point at, drop the internal "Slice α-2" and "Phase L2" markers, and drop stale self-file line refs ("line 1891", "line 2120-2122", "at line 1495"). Comment-only; no code, string literal or test data changed. Assisted-by: Claude * optimizeopt: re-anchor pyre-side comment refs to symbols Replace `file.rs:NNN` / `symbol:NNN` citations in optimizeopt comments with the symbol that lives there. Upstream `.py:NNN` citations are unchanged. - `propagate_from_pass_range:3336-3339` and `Optimizer::emit_operation:3524-3528` / `:3527` drop their line ranges; both symbols had moved (4582 / 4816). - `dispatch_emit:2631/2766` in heap.rs and virtualize.rs names no existing symbol; replaced with `emit_residual_call` / `handle_side_effects`. Strip internal tracking labels from comments: `Cat-2.2`, `Path A`, `Post-S0`, `S11`, `S7`, `S8`, `E5b`. GitHub references (`#9`, `#115`, `#160`, `#175`) and the `PYRE_S9_PROBE` env-knob name are kept. Make cross-reference pointers self-contained: "see comment above", "see doc comment above", "see the closure above", "see field doc", "same evidence as the args loop above", "for the reason given in the field loop above", "same rationale as raw fields above", "see the Virtual arm above", "the arms below", "the guard below" now state the load-bearing fact or name the owning symbol. Repair two sentences left dangling by previously stripped refs (`optimizeopt/mod.rs` setinfo_from_preamble, `virtualstate.rs` visit count). Comment-only: no code, string literal, or test data changed. Assisted-by: Claude * pyre-jit-trace: re-anchor pyre-side comment refs to symbol names Replace `<file>.rs:NNN` line citations in pyre-jit-trace comments with the symbol that lives at the cited location, after opening each target. Upstream `rpython/`, `pypy/` and `lib-python/` `.py:NNN` parity citations are left unchanged. Where the cited pyre-side location holds no nameable symbol, or the file/function no longer exists, only the `:NNN` is dropped and the filename kept. Strip internal session tracking labels (`B3`, `C3 S1`, `E1`, `G0`/`G1`/`G2`, `Epic G`, `gap 10 slice 2b`, `P2 drain`, `P3`, `Route C`, `Task 8`, `increment 2b`, `STEP 5`) from the comments that carried them; GitHub issue references (`#32`, `#73`, `#171`, `#203`, `#215`, `#62`/`#23`) are kept. Also make four cross-reference comments self-contained by stating the fact instead of pointing at another comment ("see the module preamble", "see above", "the `current`-frame pattern", "see `history.rs`"). Comment-only: no code, string literal or test data is modified. Assisted-by: Claude * docs: correct three comments refuted by their own call sites Each claimed a capability was missing; each is contradicted by the code it sits next to. `descr.rs`'s tag block says the Field tag is load-bearing for a synthetic `FieldIndexDescr` that unpacks offset/size/type/signed out of the index bits. That descriptor and its helpers were deleted — `majit-ir`'s descr module records the removal — and `VirtualizableFieldState.fields` is keyed by `FieldDescr::index_in_parent()` now (`info.py:203-206`). Nothing decodes the tag; what it still buys is disjoint index ranges so two descr kinds cannot collide on one `HeapCache` key. The `ptr_eq/rr>i` opcode-table row says the `b1 is b2` fast path is omitted, "same rationale as int comparisons". Both handlers implement it: `binop_ref_to_int_record` answers an identical operand pair out of `fastpath_same_boxes` without recording, and so does `binop_int_record`. The `raise` arm says resume-data capture is omitted, pointing at the `goto_if_not/iL` arm, which carries no such comment. The guard this arm emits calls `walker_capture_snapshot_for_last_guard(ctx, op.pc)` twelve lines below, which is `generate_guard`'s `resumepc=orgpc`. Assisted-by: Claude * descr: mark PyCode.co_firstlineno immutable, per _immutable_fields_ `pycode.py:95-106` lists `co_firstlineno` in `_immutable_fields_`; the PyCode descr group marked every field mutable because its spec builder hard-coded the flag. Give the builder the flag as a parameter and set it from the upstream list: only `co_firstlineno` changes. `co_name` and `hidden_applevel` are absent from that list and stay mutable — `w_name` is realized lazily by `w_code_name_obj` and does go null -> non-null after construction — and `code_ptr` is the raw body pointer with no upstream slot. The slot really is write-once: `box_code_constant_with_firstlineno` writes it onto an object `box_code_constant` has just boxed out of a fresh `Box`, so no caching lets a reader see it first, and `code.replace` reads it and builds a new code object rather than writing this one. No behaviour change is expected or observed. The only trace-side reader of a field descr's `is_immutable` is the replay-cleanliness rule in `fbw_state.rs`, which fires on a `setfield_gc` into a freshly allocated object, and traced Python never constructs a PyCode. `check.py --no-build --backend dynasm`: 441/441, no jitstats delta. Assisted-by: Claude * majit-metainterp: replace pyre-side line refs in comments with symbol names Comment-only change across `majit/majit-metainterp/src/` (excluding `src/optimizeopt/`) and `majit/majit-metainterp/tests/`. - Rewrite `<file>.rs:NNN` citations of pyre's own Rust sources to name the symbol that lives there, or drop the line number when the surrounding prose already names it. Line refs inside string literals and inside ```text panic transcripts are left untouched. - Upstream `.py:NNN` citations (rpython/, pypy/, lib-python/, lib_pypy/) are unchanged. - Strip session-local tracking tags (Slice X-D/X-G/X3-E/QQ-n/P3/T-final, Sub-slice B/C.x, F.n-orthodox, M2 Step n, Box Identity Phase E Step n, #19 Step n, Step 2e.2b, P1.5) from comment prose. - Replace "see above"/"see the header"/"same rationale as" pointers with the fact plus the symbol that holds the rest. Assisted-by: Claude * majit-translate: replace pyre-side comment line refs with symbol names Strip `file.rs:NNN` / `file.rs:NNN-MMM` line numbers from comments in majit/majit-translate/src (excluding codewriter/ and annotator/) and majit/majit-translate/tests, keeping the symbol name the comment already cited or naming the enclosing item where the citation had none. Upstream `.py` citations are untouched. Also: - qualify ambiguous bare `model.rs` references to `flowspace/model.rs` or `annotator/model.rs` where the named symbol resolves there - drop self-referential filename parentheticals in rclass.rs, rpbc.rs, rtyper.rs, mir.rs, flowspace_adapter.rs, cutover.rs and rbuiltin.rs - point flowspace_adapter.rs's exc_from_raise cross-reference at the "TODO: `Constant` SSA carrier shape" section that exists in that module preamble - remove the "slice A" / "Slice C" tracking labels from flowspace_adapter.rs and llinterp.rs Comment-only: no code, string literal or test data changed. Assisted-by: Claude * pyre-interpreter: replace pyre-side line-number comment refs with symbol names Rewrite `<file>.rs:NNN` citations in comments under pyre/pyre-interpreter to name the symbol at the cited location instead of a line number, or drop the line number where the symbol was already named. Citations against the pinned rustpython-compiler-core and rustpython-sre_engine snapshots (`oparg.rs`, `bytecode/instruction.rs`, `string.rs`, `engine.rs`) are left as they are. Also replace directional cross-references ("see above", "see below", "the note above", "as noted above") with the fact or the owning symbol, and drop the "B1" tracking prefix from a jit_fnaddr comment. Comment-only; no code, string literal or test data changed. Assisted-by: Claude * Re-anchor pyre-side comment references to symbols Replace `<file>.rs:NNN` line citations in comments across majit-backend-{dynasm,cranelift,wasm}, majit-macros, majit-ir, majit-gc and pyre-object with the file name alone, or with the symbol that the cited line's enclosing item defines where the cited location still matches the comment's claim. Upstream `rpython/`, `pypy/`, `lib-python/` and `lib_pypy/` `.py:NNN` citations are unchanged, as is the `compiler.rs:12884` reference inside the `bridge_cache_addrs` expect string. Symbols named where verified: `bh_call_r` / `bh_call_f` / `bh_call_v` default trait impls, `gc_rewriter`, `do_compile`, `emit_guard_exit`, `cranelift_realloc_frame`, `dynasm_typeid_subclass_range`, `generate_state_fields_jit_state`, `generate_trace_fn`, `handle_new`, `gen_malloc_nursery`, `gen_write_barrier`, `handle_write_barrier_setfield`, `do_collect_nursery`, `rescan_major_nonstack_roots_and_drain`, `register_active_hooks`, `CompiledLoopToken`, `next_op_can_accept_cc`, `AbstractVirtualPtrInfo`. Replace three cross-reference pointers with the fact they pointed at: the `write_float_at_mem` "see read sibling above", the `reg_write_audit` "see the module doc", and the wasm `stamp_and_publish_label_targets` "the comment below". Drop the internal tracking ids `S-11`, `Phase E.3+`, `Slice 80-G.7` and `Pre-A.2.3` from the comments they appeared in. Assisted-by: Claude * fbw: record the measured mechanism behind the loop-bearing blackhole handoff decline The `walk_abort_adopted` deny-list arm for `LoopBearingCalleeInlineUnsupported` carried a comment saying its second blocker was open and that dropping the arm produces wrong code, without naming a cause. Measured it on both failing fixtures and replaced that paragraph with the mechanism. `bhimpl_jit_merge_point` treats a frame that has a `nextblackholeinterp` as the recursive portal level: it takes `bhimpl_recursive_call_*`, parks the result in `tmpreg_*` and raises `LeaveFrame`. A multi-frame image stacks the callee above its caller, and this decline reports that the callee bears a loop, so the callee reaches its own loop-header merge point before any `*_return` and the caller below it receives `tmpreg_*` as the callee's return value. Both fixtures leave at that opcode with `ret_type=Ref`: `inline_subwalk_user_iterator` on `[run@1054, step@260]`, and `list_append_write_barrier_gc` on `[big_live_len_regrow@1191, churn@162]`. Comment only; no behaviour change. Assisted-by: Claude * fbw: name the caller-image refusal, the vref bracket's size, and the two qmut decline causes Three diagnostics, all gated on fbw_debug_abort_enabled, no behaviour change. capture_inline_parent_blackhole answered None from its three liveness-pass early returns without printing anything, so the downstream "parent.blackhole None (capture missing)" could not say which bank, which color, or whether the walk's shadow was merely too short. Added report_caller_image_decline and wired it into the int, ref and float sites. Each site changed from `...get(color).copied()?` to an explicit `let Some(..) = got else`, which separates an out-of-range color from one whose shadow holds a different concrete kind; the `?` conflated them. The vref bracket's two halves iterate virtualref_boxes and nothing reported its length, so its size was only ever restated from which call sites populate it. Print the pair count above vrefs_before_residual_call. The qmut flush leg printed one decline message for both WalkEndResume variants that can be unprovable. Split it: RewindUnproven means no opcode-entry sample was taken, a still-unprovable Rewind means the opcode had already applied an effect. Assisted-by: Claude * fbw: correct two docs that call the vref bracket's loops empty Both said the bracket's loops are empty because no jit.virtual_ref producers exist. A producer does exist and runs: walker_ec_enter takes a vref of every seeded callee frame through TraceCtx::opimpl_virtual_ref, paired with opimpl_virtual_ref_finish when the frame leaves. Measured with the [vref-bracket] report over 431 synth + 93 parity fixtures: 5487 bracket entries, 686 of them (12.5%) with at least one pair, 66 of the 316 emitting fixtures reaching a nonzero count, maximum 7 pairs. Assisted-by: Claude * docs: name the produced-view source PreviewShortState::short_boxes `produced_short_boxes_from_exported_boxes`'s header still cited `ctx.exported_short_boxes`, a field removed when the preview export was collapsed into `PreviewShortState`. Point it at the surviving field. Assisted-by: Claude * descr: state PyCode field purity per field in its spec test `pycode_field_descrs_share_parent_and_preserve_specs` asserted `!descr.is_always_pure()` for every PyCode field. Marking `co_firstlineno` immutable made that field answer true — `is_always_pure()` returns the `immutable` flag — so the test has been failing since that change; its commit verified with `check.py` only, which does not run crate unit tests. Add the expected purity to the per-field tuple and compare it, so each field states its own answer and a move in either direction fails. Also re-point one comment in optimizer.rs at `exported_short_boxes`, the name the local kept after the preview-export collapse. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: answer a caller image's unstamped ref color instead of refusing the image `capture_inline_parent_blackhole`'s liveness pass demanded a `ConcreteValue::Ref` for every ref color live at the resume pc and returned `None` for the whole image otherwise. The innermost-frame fill `build_single_frame_miframe` (residual_call.rs) runs the same pass and answers the two ways that demand fails: * a live color whose register holds no box is skipped — a `-live-` set is the union over the paths INTO its coordinate, so a color can be live there and undefined on the path walked, and `_copy_data_from_miframe` (`blackhole.py:1711-1730`) likewise leaves a `None` box unset; * a color whose shadow is `ConcreteValue::Null`, the walker's untracked sentinel that `write_ref_reg` stamps for every recorded-but-unobserved result, is recovered through `TraceCtx::recover_ref_value`. Port both. The image is still refused when neither applies. Measured with a report added at the refusal site, over 431 synth + 93 parity fixtures (dynasm, darwin): 21 refusals across 10 fixtures, all bank `r` with a `Null` shadow, partitioning as 11 no-box and 10 recoverable and 0 neither. After the change the corpus reports none. The report itself stays, extended with the box and its recoverability, and the header's decline count is corrected: it cited the downstream `[s2-build-decline]` symbol, which prints only when a multi-frame build was attempted and so undercounted tenfold. `check.py --no-build`: dynasm 441/441, cranelift 441/441, wasm 434/434, no jitstats delta. `cargo test --release -p pyre-jit-trace -p majit-metainterp --features dynasm`: rc=0. Assisted-by: Claude * fbw: route the hazardous-inline deny through disable_noninlinable_function `fbw_abort_nested_unjournaled_residual` names the callee an abort is attributable to and denies it, but the deny wrote only the walker-local `FBW_HAZARDOUS_INLINE_DENY` thread-local, so the callee's JitCell never carried `JC_DONT_TRACE_HERE` and no warm-state reader saw it. It now also calls `disable_noninlinable_function` on `make_green_key(callee_code, 0)`, the function-entry key `inline_call.rs` already uses for that callee — the same answer `pyjitpl.py:2818-2828` gives for the callee `find_biggest_function` names. The consuming half — `warmstate.py:485-496`, where a `JC_DONT_TRACE_HERE` cell that has never seen a procedure token retraces at once instead of waiting out the counter — is already carried by `WarmEnterState::maybe_compile_decision`. Measured (dynasm): the three fbw witness fixtures now mint the denied callee's cell, cells 3 -> 4 on each. `get_stats` counts the `BaseJitCellState` enum rather than the flag, so its `dont_trace_here` reads 1 only on `wasm_ca_trampoline_decline`; on the two `foriter_exempt_*` fixtures the new cell has already moved on to tracing (tracing 0 -> 1) and the state no longer names the deny its flag still records. `list_append_write_barrier_gc` gains a compiled loop (loops_compiled 12 -> 13), re-recorded on all three backends; the wasm baseline is from a measured wasm run. Assisted-by: Claude * fbw: name which hazard clause denied the nested-residual inline `fbw_inline_callee_hazardous` fires on three clauses and returned only the callee's code key, so the `PYRE_LB_SITE=1` `[lb-arm]` line could say `hazard=true` and nothing more. It now returns the clause name alongside the key and the report prints it: `hazard=repeat`, `hazard=for-iter`, `hazard=self-recursive`, or `hazard=false`. The clauses are not equally tight. `repeat` and `self-recursive` name the frame that is actually recursing; `for-iter` is `code_has_for_iter`, which fires on any code object whose bytecode contains a `FOR_ITER` anywhere, whether or not an iterator is in flight at the decline point. Census over 441 synth + 83 parity fixtures: for-iter 8 fires / 6 fixtures, self-recursive 2 / 2, repeat 2 / 2. Only two of the six `for-iter` fixtures are the witnesses that clause documents. Same denial set as before — the three clauses are checked in the same order and return the same key. check.py --no-build --backend dynasm: 441/441. cargo test --release -p pyre-jit-trace: rc=0. Assisted-by: Claude * warmstate: drop the DontTraceHere state and count the denial off its flag `JC_DONT_TRACE_HERE` had two representations: the flag, and a `BaseJitCellState` variant. Every real decision already read the flag — `can_inline_callable`, `counter_tick_checked`, `should_start_dont_trace_here_trace`, and `should_remove_jitcell` — while `is_compiled` and `is_tracing` read the token and `JC_TRACING`. The state variant reached only `get_stats`, and the two answers disagreed: `disable_noninlinable_function` set the state only when `JC_TRACING` was clear, so a cell denied on its way into a trace carried the flag but never took the state, and the census counted zero denials on every fixture that reaches the fbw hazard arm. The flag is now the only representation. `get_stats` counts it directly and independently of the lifecycle state, which is what makes a denied-then-tracing cell visible; `is_dont_trace_here` reads it; the two `state == DontTraceHere` tests in `counter_would_fire` and `counter_tick` were unreachable behind the flag test on the line above and are gone. warmstate.py has no such state either: `JC_DONT_TRACE_HERE` is orthogonal to the lifecycle — a denied cell still traces, compiles, and is invalidated, and `warmstate.py:485-496` retraces it once its procedure token dies. So the abort paths now leave `BaseJitCellState::NotHot` and set the flag alone, which also collapses `abort_tracing`'s three branches into the single condition `abort_tracing_for_key` already used. cargo test --release -p majit-metainterp --features dynasm: rc=0. Assisted-by: Claude * fbw: record that narrowing the for-iter hazard clause is wrong code `fbw_inline_callee_hazardous`'s `for-iter` clause is deliberately loose — it fires on any callee whose bytecode contains a `FOR_ITER`, in flight or not — and the census this branch added shows it carrying 8 of the 32 declines across 441 synth + 83 parity fixtures for 2 witnesses. Narrowing it to "a consume already ran in this frame" is measurable and wrong. `FBW_FORITER_INFLIGHT` answers that question without the per-frame Python pc `InlineFrame` lacks, since its `Jit` entries carry the `jitcode_index` each consume ran in, and it does cut the clause to 3 fires with both witnesses still declining. But `foriter_exempt_shared_generator` then produces wrong output on all three backends, `inline_subwalk_user_iterator` regresses (loops_aborted 1 -> 5, fbw_rolled_back_with_effects 0 -> 5, loops_compiled 3 -> 2) and `list_append_write_barrier_gc` loses its compiled loop again (13 -> 12). The witness still declined, just at pc 533 instead of 261: inlining the residual is what carries the walk to the consume, so a test conditioned on the consume having happened is always one step late. The clause has to stay forward-looking, and a real narrowing needs FOR_ITER reachability from the frame's current position — which is where the missing per-frame pc actually bites. 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.
Three independent JIT/interpreter parity fixes on the
sys._getframe/virtualizable-escape axis, plus one dead-code port. Each commit stands alone.
jit: port find_biggest_function onto portal_trace_positionsTraceCtx::find_biggest_functionreadinline_trace_positions, a stack of theactive inlined callees whose three writers had no caller in the tree — so it
returned
Noneunconditionally andblackhole_trace_too_long_slowalways tookthe
prepare_trace_segmentingelse-arm.The shape was also wrong for the question.
pyjitpl.py:3538-3575walksportal_trace_positions, a flat log wherenewframeappends(jd_no, Some(greenkey), pos)andpopframeappends(jd_no, None, pos), so acallee that already returned keeps both entries and can still be sized — and
that is usually the culprit, since the function that grew the trace tends to
have finished before the limit was crossed. A stack of active frames pops on
return and cannot see it. The old field and its five functions are deleted.
interp: drop fget_f_back's two frame forcespyframe.py:767-768 fget_f_backisreturn self.get_f_back()with no force ofeither end:
f_backrefis ajit.virtual_ref(
executioncontext.py:88-89), so theframe.f_backref()read at:80is theforce, and
executioncontext.py:323-331names that read as the mechanism("We get this effect simply by reading the f_back field of all frames"). pyre
forced both
selfand the resulting caller concretely, which escapes thevirtualizable while tracing.
synth/getframe_inlined_callee_own_frame, identical on all three backends:loops_compiled 0 -> 1,loops_aborted 10 -> 6,fbw_blackhole_adopted_single_frame 9 -> 5,guard_failures 0 -> 1.jit: fold constant-depth sys._getframe(0) onto the portal virtualizablevm.py:41marksgetframe@jit.look_inside_iff(jit.isconstant(depth)), so aconstant depth is traced through:
ec.gettopframe_nohidden()is a vref readthat
pyjitpl.py:2153-2172 _do_jit_force_virtualanswers withvirtualizable_boxes[-1]under a ptr_eq + guard_value, thedepth == 0testfolds, and
mark_as_escapedis onesetfield_gc. pyre residualized the wholewalk, and
getframe's twoforce_framecalls clearedTOKEN_TRACING_RESCALLfrom inside that residual — which
tracing_after_residual_callreads as anescape.
try_walker_specialize_sys_getframetakes the one level the walk can resolve:depth 0 at the top walk level, where the answer is the portal virtualizable.
It emits
guard_valueon the callable;guard_class+ exact-class +getfield_gc_ion the depth box with the unboxed value required constant(the wrapped
W_IntObjectthe residual receives is built in-trace and is neverconstant, which is why
is_constant()on the box is the wrong test — upstream's@unwrap_spec(depth=int)has already unwrapped beforelook_inside_iffruns);getfield_gc_r(frame, execution_context)+getfield_gc_r(ec, topframeref)+ptr_eq+guard_true; and a void call formark_as_escaped. The result isstandard_virtualizable_box()itself.Inline sub-walks, any other depth, a rebound name, a non-int depth and a
topframerefthat is not the portal all decline to the existing residual.getframe_*corpus, identical direction on dynasm, cranelift and wasm:loops_abortedloops_compiledguard_failures(each bridge fixture)11 of the 12 fixtures that had never compiled now compile a loop. No output
changed anywhere in the synthetic suite.
Coverage the fold deletes, and how it is preserved
Seven fixtures lose their vable escape to the fold, including the corpus's only
bridge=trueescape and its only ContinueRunningNormally-at-a-merge-pointdrive. Each gets a
*_declinedsibling carrying the same shape at a force thearm refuses —
sys._getframe(1)where the frame identity does not matter, anadded
.f_localsread where it does. Every sibling was checked to reproduce itsoriginal's pre-fold counters exactly (
fbw_blackhole_adopted_single_frame,..._multi_frame,loops_compiled,loops_aborted,bridges_compiled,guard_failures), so the machinery those fixtures document keeps its coverage.Verification
pyre/check.py --backend dynasm,cranelift,wasm— ALL PASSED, 3/3 backends(dynasm 398/398, cranelift 398/398, wasm 394/394), run at base
1391a9656dd.The branch was subsequently rebased onto
aaefe84f9a3, which brings in #1063'sJIT changes; a re-run at the new base is in flight and CI covers it.
Not in scope
A pre-existing JIT-only crash was found while probing and is not addressed
here:
tb.tb_frame.f_localsread inside anexcepthandler in a hot looppanics
value-stack underflow: depth=3 base=3atpyframe.rs:1301, on bothdynasm and cranelift, at a clean
main;PYRE_NO_JIT=1is green. It reproduceson binaries built before any commit in this PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
sys._getframe()behavior for current and caller frames, including inlined calls, loops, and escaped frames.Tests