jit-trace: anchor an inlined level's walk-time traceback on its own frame - #908
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (4)
WalkthroughThe JIT now declines multi-frame blackhole adoption when reconstructed chains exceed one level, preserving legacy escape/replay behavior. New and updated synthetic benchmarks, inline-subwalk comments, and triage documentation describe the depth limit, frame-identity cases, and local-publication constraints. ChangesMultiframe blackhole handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 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: d0f48a377e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if per_frame.len() > 1 { | ||
| mfdbg!("inner levels unpublishable: depth {}", per_frame.len()); | ||
| return false; |
There was a problem hiding this comment.
For a vable escape inside an inlined callee, every successfully built multi-frame image has at least two levels: build_multi_frame_miframe requires a nonempty parent stack and then appends the innermost frame. This condition therefore disables the adoption path completely and routes all such escapes through legacy replay. That replay is already known to be incorrect for a side-effecting inlined callee that returns from a while loop: pyre/gate-triage.md:450-453 records roughly 5.2k duplicate side effects, whereas adoption produces the exact count. Fix the per-frame identity and locals publication rather than blanket-declining every candidate.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8cf7471). 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
|
|
Pushed two more commits: both remaining blockers behind the multi-frame adopt
|
| fixture | builds / adopts / declines | output |
|---|---|---|
getframe_inline_subwalk_multiframe |
5 / 5 / 0 | matches reference |
getframe_while_escaping_read_frame_identity |
10 / 10 / 0 | matches reference |
getframe_while_inlined_callee_subwalk |
5 / 5 / 0 | matches reference |
blackhole_inlined_callee_local_after_escape |
— | [(True, 2)], matches |
dynasm: 1 failed, 345 passed — the failure is synth/ast_compile_roundtrip,
the cpython/pypy reference mismatch that predates this branch. cargo fmt --check clean.
Not fixed here
The runtime-emitted hook is unchanged. When record_prepend_application_ traceback declines but the level is seeded — mainly exc.is_constant(), which
holds for every raise after the first in a walk — compiled code still calls the
fabricating hook, so a traceback built inside a compiled inlined callee names a
frame nothing else can reach. Same defect class, one layer over.
— commented by Claude
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 857f48dccc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let owner = match semantic_slot_owner(&pcdep, slot) { | ||
| SemanticSlotOwner::None => None, | ||
| SemanticSlotOwner::One { bank, color } => Some((bank, color)), |
There was a problem hiding this comment.
When an inlined callee assigns a fast local that has no later LOAD_FAST but remains observable through its escaped frame, this treats the missing owner as an unmaterialized hole and leaves the pre-walk slot unchanged. build_pcdep_color_slots explicitly filters locals through is_local_live (pyre/pyre-jit/src/jit/codewriter.rs:4839-4843), so a shape such as x = value; f = sys._getframe(); return f.f_locals["x"] omits x from pcdep; after multi-frame adoption, f_locals therefore exposes the stale/null value. The adopt must retain a complete per-frame locals source or decline when a changed fast local cannot be published.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
youknowone
changed the title
|
Force-pushed after a rebase onto current Rebase changed the shape of this PR. The The runtime-emitted half is deliberately not ported, and the previous revision's claim that "compiled code still calls the fabricating hook" is retracted. An lldb breakpoint on that arm counts 0 hits across 43 corpus exception fixtures and 15 probes, because Verified on the new base after a fresh LLBC extraction: full corpus ALL PASSED 349/349 on dynasm, the two target fixtures matching the oracle on cranelift as well, latch fixtures 5/5/0, 10/10/0, 5/5/0, — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 138585d311
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let node_frame = crate::state::python_pc_for_jitcode_pc_public( | ||
| consts.jitcode_index, | ||
| opcode_position as i32, | ||
| ); | ||
| ) | ||
| .and_then(|py_pc| { | ||
| concrete_portal_frame(ctx, consts.jitcode_index).map(|frame| (frame, py_pc)) | ||
| }); |
There was a problem hiding this comment.
This selects the seeded callee frame only for execute_concrete; the runtime arm below still emits the frame-fabricating inline hook. That arm is reachable when an inlined callee explicitly raises a promoted constant exception (for example, a module-global exception instance), because record_prepend_application_traceback returns false for exc.is_constant(). The recording/adopted iteration will therefore attach the live frame here, while compiled iterations attach a different fabricated frame, so exc.__traceback__.tb_frame is sys._getframe() changes across iterations. The emitted path must preserve the same per-frame identity, or the affected shape must be declined until it can.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
…rame
`record_inline_application_traceback` recorded the walk-time concrete node
through `record_inline_traceback_for_recording`, which `createframe_obj`s a
traceback-only frame from the promoted code and globals. That hook predates the
inline seed. A seeded level owns a real frame -- the sub-walk runs the callee on
it, and the EMITTED node already names that same object, because
`traceback_node_site` resolves its frame operand from the level's portal frame
register and the seed stamps the concrete frame onto that operand's box. So the
walk and the compiled run named different frames for the same invocation, and
the walk's answer is the one a multi-frame blackhole adopt commits:
`tb.tb_frame is sys._getframe()` read False for exactly one iteration per adopt.
The concrete node is now recorded against that frame, falling back to the
fabricating hook only for a level inlined without one. The python pc is resolved
first, so the record-or-skip decision is unchanged: the fabricating hook drops
the node when the coordinate does not map, while the pointer-taking recorder
would substitute `frame.last_instr`.
Anchoring on the real frame moves the coordinate obligation with it. The
fabricated frame carried the raise coordinate because the hook stamped it; a
level's own frame carries the entry sentinel, since the recording walk does not
make `dispatch_bytecode`'s per-opcode `last_instr` store and a frame that leaves
by the exception reaches no exit that would publish one. `f_lineno` then answers
the code object's first line -- `synth/exception_traceback_frame_lineno` reports
a second `('raises_out', 1, 0)` shape beside `('raises_out', 1, 1)`. The anchor
makes the same store `publish_last_instr_at_live_marker` makes for the
blackhole's replay.
Assisted-by: Claude
…dopt `synth/blackhole_inlined_callee_local_after_escape` drives an inlined callee that assigns a local, escapes through a residual `sys._getframe()`, and reads the local back after the escape. The shape holds the adopt to two separate obligations and fails differently on each: a level whose locals are not resumable resumes the local as null and faults in `object_getattr_miss` (rc=139, no output), while a walk-time traceback anchored on any other object prints False and still exits 0. No corpus fixture covered it, which is why 342/342 green and a firing latch did not catch either. Corrects the three `getframe_*` headers, which described the chain-root gate as still declining their shapes, and gate-triage.md's §1d narrative, which recorded the 2026-07-30 flip as resolved when two blockers survived it. Assisted-by: Claude
…declined Only the walk-time record was moved onto the inlined level's own frame; the `emit_runtime` arm of `record_inline_application_traceback` still emits the frame-fabricating hook. Measuring that arm produced two results. It is unreachable. An lldb breakpoint on the arm's own call-descr construction counts zero hits across the 43 corpus exception fixtures and 15 hand-built probes, and a `MAJIT_LOG` scan finds no call carrying the hook's `[Ref, Ref, Ref, Int, Int]` signature in any dumped trace. `emit_runtime` is the negation of `record_prepend_application_traceback`, which never declines: the `exc.is_constant()` arm is suppressed because every raising residual assigns `class_of_last_exc_is_const = false` immediately before `walker_record_guard_exception` reads it. Porting it would break an allocation contract. `w_pytraceback_new` roots `w_next` and `w_code` but deliberately not `frame`, on the documented ground that executing frames are non-moving oldgen blocks; the top-level sibling, the walk and the fabricating hook all satisfy that. A compiled trace's inlined callee frame does not -- it is the trace's own `NewWithVtable`, which the GC rewriter lowers to a nursery allocation -- so passing it would hold a movable pointer across the parking allocation inside `w_pytraceback_new`. Assisted-by: Claude
1 participant
Rebased onto current
main. Two of the four original commits were dropped: #910 landed the same blocker onmainby a better mechanism, mirroring an inlined MIFrame's standard-vable writes onto that level's own concrete red frame as they are made (current_inline_concrete_frame,store_live_frame_static_int) instead of publishing register images at the adopt.#910also absorbed the per-level ref-bank rooting, sotrace.rs/state.rshere resolve entirely tomain's side.What is left is the third blocker and its guard.
The walk-time traceback named a fabricated frame
record_inline_application_tracebackrecorded the walk's concrete node throughrecord_inline_traceback_for_recording, whichcreateframe_objs a throwaway frame from the promoted code and globals. That hook predates the inline seed. A seeded level owns a real frame — the sub-walk runs the callee on it, and the emitted node already names that same object, sincetraceback_node_siteresolves its frame operand from the level's portal frame register and the seed stamps the concrete frame onto that operand's box.So the walk and the compiled run named different frames for the same invocation, and the walk's answer is the one a multi-frame blackhole adopt commits:
tb.tb_frame is sys._getframe()readFalsefor exactly one iteration per adopt.The obligation that rides with the anchor
The fabricating hook does two things —
frame.last_instr = last_instruction, then record. Porting only the second reintroduced a different wrong answer: a level's own frame carries the-1entry sentinel, because the recording walk does not makedispatch_bytecode's per-opcodelast_instrstore and a frame that leaves by the exception never reaches an exit that would publish one.f_linenothen answers thedefline.CI caught it and the local run did not:
synth/exception_traceback_frame_linenogrew a second('raises_out', 1, 0)shape beside the correct('raises_out', 1, 1), on all three backends and all three OSes. The anchor now makes the same store the blackhole already makes for its replay inpublish_last_instr_at_live_marker.The runtime half was investigated and declined
The
emit_runtimearm still emits the fabricating hook. Two measurements say to leave it:MAJIT_LOGscan finding no call with the hook's[Ref, Ref, Ref, Int, Int]signature in any dumped trace.emit_runtimeis the negation ofrecord_prepend_application_traceback, which never declines — theexc.is_constant()arm is suppressed because every raising residual assignsclass_of_last_exc_is_const = falseimmediately beforewalker_record_guard_exceptionreads it. (This also retracts the "compiled code still calls the fabricating hook" claim from the previous revision of this PR.)w_pytraceback_newrootsw_nextandw_codebut deliberately notframe, on the documented ground that executing frames are non-moving oldgen blocks. The top-level sibling, the walk and the fabricating hook all satisfy that; a compiled trace's inlined callee frame does not — it is the trace's ownNewWithVtable, lowered to a nursery allocation. Passing it would hold a movable pointer across the parking allocation insidew_pytraceback_new. A future port needs the root-and-reload shape on the recorder first, which also covers the same pre-existing exposure on itsw_nextargument.Both findings are recorded in
gate-triage.mdrather than left in a PR thread.Verified
Rebuilt from a fresh LLBC extraction on the new base.
synth/exception_traceback_frame_lineno(the CI failure)synth/blackhole_inlined_callee_local_after_escape[(True, 2)]= oracle, dynasm and craneliftgetframe_inline_subwalk_multiframegetframe_while_escaping_read_frame_identitygetframe_while_inlined_callee_subwalkcargo fmt --check🤖 Generated with Claude Code