Skip to content

jit: publish the frame's exit coordinate into last_instr - #823

Merged
youknowone merged 16 commits into
mainfrom
perf-exc
Jul 28, 2026
Merged

jit: publish the frame's exit coordinate into last_instr#823
youknowone merged 16 commits into
mainfrom
perf-exc

Conversation

@youknowone

@youknowone youknowone commented Jul 27, 2026

Copy link
Copy Markdown
Owner

A traceback that outlives its frame kept the frame answering for the wrong line
under the JIT: f.f_lineno reported the def line, or the raise inside a
loop body, where every other interpreter reports the line the frame stopped on.
Three routes reach a frame, and each one lost the coordinate differently. A
fourth loss shows up while the frame is still running, and the same replay that
loses it also read the frame's locals from before the walk — a SIGSEGV.

1. The function-entry portal returned without storing the virtualizable back

try_function_entry_jit compiles a loop-free function as a portal. Upstream has
no counterpart — can_enter_jit fires only from jump_absolute, so a loop-free
function is never a portal there, and its dispatch loop (hinted
access_directly=True) writes last_instr per opcode straight into the frame.
pyre's portal finished writing nothing, so the frame kept the -1 init sentinel
and offset2lineno answered with the code object's first line.

Measured, same callee, two drivers, MAJIT_LOG=1: the while driver runs the
callee as a function-entry portal 18339 times, all finished, and reports the
def line; the for driver inlines it into the loop trace, 0 portal runs, and
reports correctly. The compiled trace's only store into the frame was
ForceToken(); SetfieldGc(v0,v48) = vable_token — nothing at last_instr.

doc/jit/virtualizable.rst names the remedy for exactly this shape: "where the
virtualizable survives for longer, you want to force it before returning …
jit.hint(frame, force_virtualizable=True)"
. gen_store_back_in_vable was
already fully ported with a live hint_force_virtualizable FBW arm — just
unreachable, because nothing emitted it. It is mutually exclusive with the lazy
arming (it sets forced_virtualizable, on which store_token_in_vable early
-returns) and its final store zeroes vable_token.

pyre cannot take upstream's lazy route: the dynasm backend libc::frees the
whole jitframe chain before execute_token returns, so the force marker
store_token_in_vable leaves behind would name freed memory rather than a
retained deadframe.

The publish had to be generalized from the uncaught-raise exit to both exits,
and the recording iteration needs a concrete counterpart: the walker records ops
instead of executing them, so without a direct write to session.recording_frame_ptr
the recording iteration is the one iteration that still reports the stale
sentinel. It must be the LIVE frame — virtualizable_heap_ptr is the
trace-stepping snapshot, whose storage dies with tracing.

2. The frame that OWNS the compiled loop

It never goes through the portal. Its loop guard fails, and with no bridge the
blackhole replays the rest of the frame from the guard's resume image. That
replay syncs only valuestackdepth (emit_vsd!), never last_instr, so the
frame kept the last coordinate the trace published — for a loop whose body
raises and catches, the raise. Oracle 9/10/10 against pyre 5/5/5.

Instruction::ReturnValue now emits setfield_vable_i(frame, py_pc, last_instr) before the return edge — the blackhole-side twin of the walker's
exit publish. py_pc, not the py_pc - 1 the resume-at sites
(emit_abort_permanent!, DELETE_FAST) use, because the opcode is dispatched
there rather than resumed at.

3. GUARD_NOT_FORCED_2 was silently dropped

optimize_FINISH moved the stashed guard into a context field that no code read
repo-wide. Upstream's postprocess_FINISH re-inserts it at
len(_newoperations) - 1; the pass runs before the FINISH reaches the terminal
emit here, so emit_extra queues it for the passes after virtualize and the
drain flushes it in ahead of the FINISH — the same final layout. Inert today
(after the portal-return force nothing arms the token), correct if anything
does.

4. A frame read WHILE the blackhole is replaying it

Both exits publishing is not enough on its own. dispatch_bytecode
(pyopcode.py) stamps last_instr before every opcode, so a running frame
answers f_lineno, f_lasti and any traceback taken off it for the instruction
it is on. That store is a source-level one upstream and rides in the jitcode, so
blackhole replay reproduces it for free; this codewriter unrolls the bytecode
per PC, where the same store needs one distinct int pool constant per
instruction. Measured against check_result's 256-entry cap: a 240-statement
body already reports consts i=129 and would need ~1800–2200. Upstream's
inline-immediate 'c' argcode is a signed byte and its USE_C_FORM whitelist
deliberately excludes setfield_vable_i, so the direct route is closed.

handler_live gains a process-global hook — bhimpl_live is a no-op upstream
for exactly the reason above — and pyre registers a publisher that maps the
marker's jitcode pc back to its Python pc and stores it into the frame the
portal red names. The two blackhole builders (the guard-failure resume and the
force-adoption lease) both go through it. It runs once per replayed instruction,
so it resolves under one METAINTERP_SD borrow and takes no reference count:
the Arc-cloning accessors each re-run ensure_finish_setup, whose opname-map
clone alone costs more than the instruction being replayed (measured: 10.3s vs
3.4s user on the depth3_inline_chain_typeflip bench, against a 3.6s baseline
binary from an unrelated HEAD).

5. The blackhole read pre-walk locals — a SIGSEGV

Pre-existing, and reproducible on binaries built from unrelated HEADs. The
escape flush that runs ahead of a forcing residual is all-or-nothing, and its
decline is what the single-frame blackhole latch is gated on
(committed_frame_escape_pc().is_none()) — so the crash path is that latch's
normal path, not a corner. It declines on the operand-stack half, because the
vable shadow's stack region reads NULL away from a merge point, and the register
image supplies that half anyway. The locals half is not optional: every
LOAD_FAST lowers to getarrayitem_vable_r on the frame the register image
names, so the replay read whatever that frame held before the walk began, and a
local the walk assigned (tb = e.__traceback__) came back null. Reading an
attribute off one faulted in object_getattr_miss.

try_adopt_single_frame_blackhole now writes that half with
write_back_outer_locals before driving and withdraws it when it cannot
complete, so a decline still hands the legacy replay pristine pre-walk state.
write_back_outer_locals validates the whole local range before its first
store, so a withdrawal has one of two states to go back to rather than a mix.

The withdrawal covers the post-drive declines too, not just a failed publish: a
terminal the adopt arms reject falls back to the same legacy replay, so both
arms route every decline through one restore. The undo image stays registered
as a resume root across the drive — the publish overwrote the slots it came
from, so it holds the only remaining reference to the pre-walk locals, and a
collection inside the drive would free them and leave the restore writing
pre-move addresses.

try_adopt_multi_frame_blackhole had the same hole for frame 0 — the walked
frame — and now takes the same publish. Its INNER levels still have none: the
walk's shadow covers the walked frame only, so an inlined callee's frame array
keeps its pre-sub-walk contents. Measured with PYRE_FBW_MULTIFRAME=1 (default
off), the repro above still faults there where the single-frame arm is now
correct; the comment carries that measurement and names what publishing them
would need. With the gate on, the synthetic corpus is 313/313 and
getframe_while_inlined_callee_subwalk still reports its 5 adopts.

Two hypotheses were built, measured and refuted on the way here, and are
recorded so they are not re-attempted: an unresolvable resume coordinate
(measured resume_pc=659 live_r=[1,5,6,7,8], resolvable) and an unseeded portal
frame register (measured frame_reg=1 frame_seeded=Some(..)).

Test

pyre/bench/synth/exception_traceback_frame_lineno.py surveys EVERY iteration
into a set rather than sampling the last traceback, and crosses while against
for drivers so the two compilation routes must agree without the oracle having
to say anything. That design earned its keep immediately: it caught exactly one
wrong answer per compiled function (k=1702, right after the 1619 threshold =
the recording walk) that a last-traceback-only probe passed. A loop_owner_*
group covers route 2 with four different amounts of work between the last
iteration and the return; a route 4 — the frame read WHILE it is still running — lives in
pyre/bench/frame_lineno_mid_replay_regression.py instead, a self-checking
guard registered with skip_backends=("wasm",). It covers the two places that
can read a running frame, split across calls so the set holds the interpreted
answer and the replayed one together, plus a recursive case pinning direct
recursion (every level with its own hot loop, every level sharing ONE code
object with its caller — the shape where a per-level frame mix-up survives a
code-object check). It is scoped that way because wasm does not satisfy the
invariant yet; see below.

Verification

check result
check.py --backend dynasm 330/330
check.py --backend cranelift 330/330
check.py --backend wasm 326/326
new bench vs pypy3 / CPython / dynasm / cranelift / wasm byte-identical
cargo test pyre-interpreter / pyre-jit / pyre-jit-trace / majit-metainterp 401 + 322 + 298 + 1403 pass
cargo fmt --check clean

Rebased onto origin/main; every number above is from the rebased base.

The wasm gap this exposed

The mid-execution survey fails on wasm and only on wasm. A plain hot loop read
through sys._getframe(1) reports offset 0 — the -1 init sentinel, i.e. the
def line — from the first COMPILED call onward, against 4 on pypy3, CPython
and both native backends. It is not exception-specific and it survives both the
fixes in this PR and the current base.

Instrumenting both ends (guest output routed through print_output, since
wasm32-unknown-unknown has no stderr) shows the marker hook firing exactly as
often as on dynasm, writing the correct coordinate into the correct frame at the
correct offset, and an immediate read-back returning it — then the interpreter
reading 0 from that same address and offset. So the publish lands and a
wasm-side writer clears it before the residual call.

Ruled out by measurement: a cross-crate offset mismatch (frame_layout enforces
equality in a const block); restore_resume_state_from and
set_last_instr_from_next_instr (probed — on the failing interval they only
target the callee frame); a blackhole setfield_vable_i (its handler does
bh.cpu.expect(...) and the wasm builder sets no cpu, so it would panic); and
gating the three remaining codewriter last_instr emit sites on is_true_portal
(built and measured — no change, reverted).

Filed with the repro and the ruled-out list. The invariant is asserted for the
native backends meanwhile; the post-return coordinate, which wasm does satisfy,
stays in the synthetic bench.

Codex parity review, run against the branch's merge-base (origin/main is the
wrong base here — it is ahead of the branch point, and the diff then reports
other PRs' code as this branch reversing them). Findings across the rounds:

  • The live-marker hook fell back to virtualizable_ptr. A nested level carries
    no virtualizable, so an inlined callee could stamp its coordinate into the
    caller's frame. Fixed: the frame comes from the replaying level's OWN portal
    red, with no fallback, plus a code_ptr match.

  • The locals publish keyed on code-object equality, which two invocations of one
    function share. Fixed: it requires frame IDENTITY against
    live_vable_frame_addr.

  • Speculative writes were retained on post-drive declines. Fixed as described in
    section 5.

  • Direct recursion could pass the code-object check and corrupt the caller's
    coordinate — REFUTED by measurement. recursive_mid_replay reports
    ((17, 17), (17, 17), (17, 17), (17, 17)) on pypy3, CPython and both pyre
    backends alike; the shape is now pinned in the bench.

  • The portal-return force fires on an exit upstream leaves alone — REAL, and
    the point of section 1. interp_jit.py's PyFrame.dispatch applies
    force_virtualizable=True under except Yield against a bare
    except Return, so an ordinary return gives up the
    FORCE_TOKEN/GUARD_NOT_FORCED_2 protocol. That exact decision point is now
    cited at the force site along with why narrowing it back down needs the
    deadframe retention the backend does not provide.

  • The stashed GUARD_NOT_FORCED_2 reaches new_operations in upstream's final
    order but not with upstream's resume data: postprocess_FINISH finalizes it
    after emit(op) forced the FINISH args, the emit_extra route before. REAL,
    and the earlier comment wrongly called the two equivalent — corrected. The
    faithful order needs an Optimizer-side FINISH postprocess, because
    finalization needs collect_optimizer_knowledge_for_resume, which a pass
    cannot reach; finalizing without it would drop the bridgeopt sections every
    other guard carries. Deferred with that blocker and the convergence path
    recorded in-code. Inert today: nothing arms the token, so neither image is
    observable.

  • The live-marker hook dereferenced w_code after a null test only, so the
    GcRef(usize::MAX) sentinel a bridge sub-walk carries would reach
    w_code_get_ptr — REAL. Fixed: sentinel + is_code first, the order the two
    other readers of the field use.

  • The exit coordinate the walk publishes was never undone on a declined walk —
    REAL, and the sharpest of the round. The publish fires at the exit, the commit
    is decided afterwards, and a declined walk resumes the frame from its pre-walk
    state by reading that very field (next_instr = last_instr + 1), so it
    restarted past its own return or raise. Journaled and restored beside the store
    journal.

  • The ReturnValue coordinate store stamped the CALLER for an inlined callee —
    REAL; the same file already documents that frame_var aliases the outermost
    frame in a non-portal callee. Now portal-only.

  • The multi-frame f_backref chain stayed rewired on a decline — REAL. Recorded
    as it is overwritten and restored on every post-link decline.

  • Many return sites would PANIC at jitcode assembly — REFUTED. try_finish
    returns None and declines the jitcode; the interpreter keeps running it,
    which is the documented behaviour for every other register/const ceiling.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@youknowone, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fb060c73-e4d7-42dd-9e47-cef3a2395ad7

📥 Commits

Reviewing files that changed from the base of the PR and between 0e8f0de and 614b34c.

📒 Files selected for processing (12)
  • majit/majit-metainterp/src/blackhole.rs
  • majit/majit-metainterp/src/optimizeopt/mod.rs
  • majit/majit-metainterp/src/optimizeopt/virtualize.rs
  • pyre/bench/frame_lineno_mid_replay_regression.py
  • pyre/bench/synth/exception_traceback_frame_lineno.py
  • pyre/check.py
  • pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
  • pyre/pyre-jit-trace/src/state.rs
  • pyre/pyre-jit-trace/src/trace.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyre-jit/src/jit/codewriter.rs

Walkthrough

Adds live-marker instruction publication, synchronizes last_instr during JIT exits and returns, validates blackhole adoption locals, changes FINISH guard ordering, and introduces traceback line-number benchmark scenarios.

Changes

Traceback replay coordination

Layer / File(s) Summary
Live-marker coordinate publication
majit/majit-metainterp/src/blackhole.rs, pyre/pyre-jit-trace/src/state.rs, pyre/pyre-jit/src/eval.rs, pyre/pyre-jit/src/jit/codewriter.rs
Registers a one-time live-marker hook, publishes last_instr during blackhole replay, wires the hook during JIT setup, and records the coordinate on return.
Exit and finish bookkeeping
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs, pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
Publishes exit coordinates for normal and exception paths, updates the recording frame directly, forces virtualizable state back before finish, and stores finish payloads afterward.
Blackhole adoption state validation
pyre/pyre-jit-trace/src/state.rs, pyre/pyre-jit-trace/src/trace.rs
Adds frame-local snapshot and restoration helpers and gates single- and multi-frame blackhole adoption on frame identity and locals-publication checks.
Finish guard emission ordering
majit/majit-metainterp/src/optimizeopt/mod.rs, majit/majit-metainterp/src/optimizeopt/virtualize.rs
Replaces deferred FINISH guard postprocessing with immediate guard emission and updates optimizer context fields and comments.
Traceback coordinate benchmark scenarios
pyre/bench/synth/exception_traceback_frame_lineno.py
Adds synthetic scenarios for traceback shapes across loop, callee, replay, recursion, frame, and concurrent-traceback cases.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant JITDriver
  participant BlackholeInterpreter
  participant ReplayState
  participant PyFrame
  JITDriver->>BlackholeInterpreter: register live-marker hook
  BlackholeInterpreter->>ReplayState: invoke hook at live marker
  ReplayState->>PyFrame: publish last_instr
  JITDriver->>ReplayState: terminate with finish
  ReplayState->>PyFrame: publish exit coordinate and store back virtualizable state
Loading

Possibly related issues

  • youknowone/pyre#811 — Covers the same multi-frame blackhole adoption and frame/locals publication path updated here.

Possibly related PRs

  • youknowone/pyre#387 — Changes -live- marker insertion and handling used by the new hook.
  • youknowone/pyre#455 — Adds -live--anchored liveness coordinates related to this hook’s publication path.
  • youknowone/pyre#569 — Adjusts marker-derived coordinates used during guard resume and liveness handling.

Suggested reviewers: lifthrasiir

Poem

A rabbit watched the live marker glow,
And stamped the frame where traces flow.
Guards hop before FINISH’s gate,
Locals pause, then reinstate.
Tracebacks dance in numbered rows—
“Hop hooray!” the bunny knows.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: publishing frame exit coordinates into last_instr.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf-exc

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 614b34c).
Updated: 2026-07-28T08:09:09.591Z

Files in the reviewed diff
majit/majit-metainterp/src/blackhole.rs
majit/majit-metainterp/src/optimizeopt/mod.rs
majit/majit-metainterp/src/optimizeopt/virtualize.rs
pyre/bench/frame_lineno_mid_replay_regression.py
pyre/bench/synth/exception_traceback_frame_lineno.py
pyre/check.py
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
pyre/pyre-jit-trace/src/state.rs
pyre/pyre-jit-trace/src/trace.rs
pyre/pyre-jit/src/eval.rs
pyre/pyre-jit/src/jit/codewriter.rs

1. Regressions to PyPy parity introduced by this patch

  • majit/majit-metainterp/src/optimizeopt/virtualize.rs:2135 ↔ rpython/jit/metainterp/optimizeopt/virtualize.py:80 — “emit_extra(... guard_op)” finalizes and emits GUARD_NOT_FORCED_2 before FINISH; PyPy retains it, emits FINISH, then postprocess_FINISH calls store_final_boxes_in_guard() and inserts it immediately before FINISH (lines 84–90). This patch removed the prior deferred postprocess, so its resume data can encode virtual return boxes rather than PyPy’s forced/materialized image.

2. Other mismatches introduced by this patch

  • pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs:1359 ↔ pypy/module/pypyjit/interp_jit.py:94 — “fbw_force_virtualizable_before_return(ctx)” forces the standard virtualizable on every ordinary return. PyPy’s PyFrame.dispatch forces only the Yield exit (line 96); its ordinary Return path is simply return self.popvalue() (lines 98–99). This changes the FORCE_TOKEN/GUARD_NOT_FORCED_2 protocol and materialization timing for normal returns.

3. Pre-existing mismatches (already present before this patch)

  • pyre/pyre-jit/src/jit/codewriter.rs:7838 ↔ pypy/interpreter/pyopcode.py:197 — “pyre cannot mirror it here” confirms that compiled execution still lacks PyPy’s unconditional per-opcode self.last_instr = intmask(next_instr) (line 200). The new hook improves blackhole replay only; an executing compiled frame can still expose a stale f_lasti/f_lineno.
  • pyre/pyre-jit/src/jit/codewriter.rs:8430 ↔ pypy/interpreter/pyframe.py:78 — “frame_var aliases the OUTERMOST frame” leaves inlined callees without their own live frame identity, whereas PyPy has one PyFrame per invocation, each with its own last_instr, pycode, locals, and f_backref (lines 78–85). Consequently the patch intentionally declines inner-frame line publication rather than preserving PyPy’s per-frame behavior.

4. Structural adaptations

  • majit/majit-metainterp/src/blackhole.rs:5193 ↔ rpython/jit/metainterp/blackhole.py:403 — “LiveMarkerHook” adds a Rust global callback at live/ markers, while PyPy’s blackhole simply skips the marker. This is an adaptation for pyre’s bytecode-unrolled codewriter and its constant-pool limit; it is not a 1:1 upstream operation.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 05c406f9dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +8380 to +8384
"setfield_vable_i",
vable_setfield_int_graph_args(
frame_var.into(),
v_li.into(),
VABLE_LAST_INSTR_FIELD_IDX,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Thread each callee frame into the return store

When this ReturnValue belongs to an inlined non-portal callee, frame_var still aliases the portal/root frame, so the store writes the callee's py_pc into the caller's last_instr. The caller then interprets that coordinate using its own code object's line table; observing the caller after the callee returns, such as through sys._getframe(1) or a retained traceback, can therefore report an unrelated line. This needs the callee's per-frame red frame rather than the shared portal frame.

AGENTS.md reference: AGENTS.md:L32-L42

Useful? React with 👍 / 👎.

Comment on lines +7795 to +7800
// Until then the coordinate is published only where the frame
// stops being replayed — the frame exits (`ReturnValue`,
// `emit_abort_permanent!`) and the raises that resume in the
// interpreter — so a frame observed MID-replay (via a callee's
// `sys._getframe` or traceback) still reports the last published
// coordinate.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Publish last_instr at every replayed opcode

When code observes a frame before replay reaches a return, abort, or interpreter-resumed raise—for example, a callee calls sys._getframe(1) or a return expression reads a retained traceback—the exit-only strategy explicitly leaves last_instr at an earlier published coordinate. The interpreter updates this field before every opcode, so the generated JIT must preserve that behavior rather than leave a known mid-replay mismatch; the inline-immediate representation described immediately above is needed before treating this fix as complete.

AGENTS.md reference: AGENTS.md:L14-L20

Useful? React with 👍 / 👎.

Comment on lines +8376 to +8377
let v_li: super::flow::FlowValue =
super::flow::Constant::signed(py_pc as i64).into();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid allocating one int constant per return site

For a function with many distinct RETURN_VALUE bytecodes, each py_pc here becomes a distinct ConstInt in constants_i. Assembler::check_result requires num_regs_i + constants_i.len() <= 256, so roughly 256 return sites—or fewer when the jitcode already uses int constants—now panic during jitcode assembly even though these functions did not previously consume one int-pool slot per return. Encode this coordinate without the per-kind constant pool before emitting it at every return.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7786ddb15a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread pyre/pyre-jit-trace/src/trace.rs Outdated
unsafe {
majit_gc::shadow_stack::push_resume_ref_roots(locals_undo.as_mut_slice());
}
let locals_published = crate::state::write_back_outer_locals(ctx, vable_frame);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Mirror terminal replay state into the committed snapshot

When a virtualizable escape is adopted and this blackhole replay ends with DoneWithThisFrame* or ExitFrameWithExceptionRef, the locals are published only into vable_frame; unlike the ContinueRunningNormally arm, nothing updates cf_addr. The function then marks the VableEscape leg committed, causing the portal epilogue in eval.rs to call restore_resume_state_from(&executed_frame) and overwrite the live frame with that unchanged snapshot, discarding these locals and the replay's last_instr updates. A retained frame can therefore still expose pre-walk locals and the stale line after a terminal replay; mirror the terminal frame state into cf_addr before committing (as the other committed live-frame path does).

AGENTS.md reference: AGENTS.md:L32-L42

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2cbfef4d4f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread pyre/pyre-jit-trace/src/trace.rs Outdated
unsafe {
majit_gc::shadow_stack::push_resume_ref_roots(locals_undo.as_mut_slice());
}
let locals_published = crate::state::write_back_outer_locals(ctx, vable_frame);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore published locals when adoption declines

When the adopted blackhole reaches ContinueRunningNormally but has no first green or apply_blackhole_crn rejects the terminal state—for example, because neither terminal coordinate has a usable pcdep_trivia_at mapping—this function returns false after publishing the walk's locals here. The caller then takes the documented legacy replay path, but the original locals are restored only when write_back_outer_locals itself fails, so replay starts from partially advanced post-walk state and can double-apply effects or consume invalid locals. Keep the undo roots/state until adoption is guaranteed and restore them on every later decline.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 54f3597245

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +2122 to +2123
if let Some(guard_op) = self.finish_guard_op.clone() {
ctx.emit_extra(ctx.current_pass_idx, guard_op);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve FINISH reinsertion in postprocess_FINISH

When FINISH follows a stashed GUARD_NOT_FORCED_2, this queues the guard during optimize_FINISH, while the same change deletes OptVirtualize's postprocess callback. This is not structurally equivalent to upstream: the guard now runs through every pass after virtualize before FINISH is emitted, whereas postprocess_FINISH finalizes its resume boxes and inserts it directly at len(_newoperations) - 1 after FINISH emission. Restore the literal postprocess method rather than changing its timing; the repository rules expressly prohibit deleting an upstream method in favor of a shortcut rewrite.

AGENTS.md reference: AGENTS.md:L141-L146

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
majit/majit-metainterp/src/optimizeopt/mod.rs (2)

2670-2693: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

InputArgInt/Float/Ref probes in resop_const are likely always-miss lookups.

bind_input_resops and install_canonical_producer both explicitly skip OpRef::InputArgInt/Float/Ref positions ("InputArg slots are skipped ... only resop positions land here" / "InputArg positions have no producing op ... a rewrite never targets one"), so resop_refs should never contain those keys. The 3 InputArgInt/Float/Ref entries in the resop_const probe array therefore never hit and are redundant — inputarg_const below already covers the real InputArg check via self.inputarg_refs. Minor, but this method runs on every reserve_pos_typed call (a hot path this change is specifically optimizing).

🤖 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/optimizeopt/mod.rs` around lines 2670 - 2693,
Remove the OpRef::InputArgInt, OpRef::InputArgFloat, and OpRef::InputArgRef
entries from the resop_const probe array in the surrounding method. Keep the
four resop variants and the separate inputarg_const lookup through
self.inputarg_refs unchanged.

1887-1894: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

replace_new_operation can leave a stale new_operations_index entry if op.pos differs from the replaced op's position.

The new key is op.pos.get(); if the op at new_operations[idx] being overwritten had a different position, its old key keeps pointing at the now-discarded Rc<Op> in new_operations_index, while new_operations[idx] itself no longer corresponds to that key. find_producer_op would then resolve that stale position to an op no longer present in new_operations. The doc comment restricts usage to "guard-strengthening replacements" (same-position), which is presumably always true today, but a debug_assert! would catch a future misuse before it silently corrupts producer resolution.

🛡️ Suggested defensive assert
     pub(crate) fn replace_new_operation(&mut self, idx: usize, op: majit_ir::OpRc) {
+        debug_assert_eq!(
+            self.new_operations[idx].pos.get(),
+            op.pos.get(),
+            "replace_new_operation: position must match the replaced op, or the old \
+             new_operations_index entry is left dangling"
+        );
         self.new_operations_index.insert(op.pos.get(), op.clone());
         self.new_operations[idx] = op;
     }
🤖 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/optimizeopt/mod.rs` around lines 1887 - 1894,
Update replace_new_operation to capture the existing operation at
new_operations[idx] and add a debug_assert! that its position matches op.pos
before updating new_operations_index and replacing the entry. Preserve the
current replacement behavior while detecting future calls that would leave a
stale index key.
pyre/pyre-jit-trace/src/trace.rs (1)

1645-1693: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win

Blackhole adoption mutates frame state speculatively but only rolls it back on the first failure, not on later declines in the same code path. Both try_adopt_single_frame_blackhole and try_adopt_multi_frame_blackhole apply a frame-state write (locals publish / resume-state fold) before the adoption outcome is fully known, then have multiple later return false paths that leave that write in place — contradicting each function's own stated "pristine pre-walk state on decline" contract and this PR's goal of restoring state when replay cannot complete.

  • pyre/pyre-jit-trace/src/trace.rs#L1645-L1693: restore locals_undo via crate::state::restore_frame_locals(vable_frame, &locals_undo) before every return false in the ContinueRunningNormally arm (missing green_int.first() and a failing apply_blackhole_crn), not just when write_back_outer_locals itself fails.
  • pyre/pyre-jit-trace/src/trace.rs#L1946-L2017: capture cf_addr's pre-fold state before the unconditional restore_resume_state_from(root_addr -> cf_addr) fold, and restore it on every return false inside the ContinueRunningNormally arm (missing green int, cf_addr == 0, missing mf_terminal, bad jitcode_index cast, failing apply_blackhole_crn).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyre/pyre-jit-trace/src/trace.rs` around lines 1645 - 1693, Restore
speculative frame state on every failed adoption path: in
pyre/pyre-jit-trace/src/trace.rs lines 1645-1693, update
try_adopt_single_frame_blackhole to restore locals_undo before the missing
green_int.first() and failed apply_blackhole_crn returns; in lines 1946-2017,
update try_adopt_multi_frame_blackhole to capture cf_addr’s pre-fold state
before restore_resume_state_from and restore it before every listed
ContinueRunningNormally failure return, preserving each function’s
pristine-state-on-decline contract.
majit/majit-metainterp/src/optimizeopt/virtualize.rs (1)

2087-2126: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Bypass the optimization pipeline when re-inserting GUARD_NOT_FORCED_2 before FINISH.

ctx.emit_extra(ctx.current_pass_idx, guard_op) queues the guard, and emit_operation drains queued ops through propagate_from_pass_range(start, end_pass, ..) before emitting the current op. RPython’s postprocess_FINISH calls store_final_boxes_in_guard(...) directly, bypassing later passes; let the queued guard go through earlyforce:pure:heap:unroll, etc., and you can lose or reorder the guard. Apply the same bypass here, e.g. have the drain target only remaining emit_guard_operation emission/postpass handling or add the guard directly to new_operations so it stays before FINISH.

🤖 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/optimizeopt/virtualize.rs` around lines 2087 -
2126, Change the OpCode::Finish reinsertion path to bypass normal optimization
passes for the stashed GUARD_NOT_FORCED_2. Do not use ctx.emit_extra with the
current pass; instead insert the guard directly before FINISH in new_operations
or route it only through the remaining guard-emission/postprocessing logic,
ensuring store_final_boxes_in_guard runs and the guard cannot be removed or
reordered.
🤖 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.

Outside diff comments:
In `@majit/majit-metainterp/src/optimizeopt/mod.rs`:
- Around line 2670-2693: Remove the OpRef::InputArgInt, OpRef::InputArgFloat,
and OpRef::InputArgRef entries from the resop_const probe array in the
surrounding method. Keep the four resop variants and the separate inputarg_const
lookup through self.inputarg_refs unchanged.
- Around line 1887-1894: Update replace_new_operation to capture the existing
operation at new_operations[idx] and add a debug_assert! that its position
matches op.pos before updating new_operations_index and replacing the entry.
Preserve the current replacement behavior while detecting future calls that
would leave a stale index key.

In `@majit/majit-metainterp/src/optimizeopt/virtualize.rs`:
- Around line 2087-2126: Change the OpCode::Finish reinsertion path to bypass
normal optimization passes for the stashed GUARD_NOT_FORCED_2. Do not use
ctx.emit_extra with the current pass; instead insert the guard directly before
FINISH in new_operations or route it only through the remaining
guard-emission/postprocessing logic, ensuring store_final_boxes_in_guard runs
and the guard cannot be removed or reordered.

In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 1645-1693: Restore speculative frame state on every failed
adoption path: in pyre/pyre-jit-trace/src/trace.rs lines 1645-1693, update
try_adopt_single_frame_blackhole to restore locals_undo before the missing
green_int.first() and failed apply_blackhole_crn returns; in lines 1946-2017,
update try_adopt_multi_frame_blackhole to capture cf_addr’s pre-fold state
before restore_resume_state_from and restore it before every listed
ContinueRunningNormally failure return, preserving each function’s
pristine-state-on-decline contract.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6a371215-76a3-42d4-ab8b-65cd21bb1e7f

📥 Commits

Reviewing files that changed from the base of the PR and between 7786ddb and 54f3597.

📒 Files selected for processing (10)
  • majit/majit-metainterp/src/blackhole.rs
  • majit/majit-metainterp/src/optimizeopt/mod.rs
  • majit/majit-metainterp/src/optimizeopt/virtualize.rs
  • pyre/bench/synth/exception_traceback_frame_lineno.py
  • pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
  • pyre/pyre-jit-trace/src/state.rs
  • pyre/pyre-jit-trace/src/trace.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyre-jit/src/jit/codewriter.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee965f870f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// the state `restore_escape_flush_undo` puts back for the flush half.
// The publish above and the replay's own vable stores both landed here,
// so both have to come off.
crate::state::restore_frame_locals(vable_frame, &locals_undo);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore the complete frame state when adoption declines

When drive_single_frame_blackhole returns ContinueRunningNormally without a green value, or apply_blackhole_crn rejects its image, the blackhole has already updated scalar frame state such as last_instr and valuestackdepth, but this decline arm restores only the locals. The armed EscapeFlushUndo, which contains those scalars, is neither restored nor discarded, so the legacy eval_loop_jit re-entry derives its next opcode from post-blackhole last_instr while using pre-walk locals, potentially skipping or re-executing bytecode. Fresh evidence after the earlier rollback comment is that the newly added failure arm still calls only restore_frame_locals; the equivalent multi-frame decline has the same omission.

Useful? React with 👍 / 👎.

) -> Result<(), DispatchError> {
let finish_value = fbw_ensure_boxed_for_ca(ctx, op_pc, result)?;
fbw_publish_exit_last_instr(ctx, op_pc);
fbw_force_virtualizable_before_return(ctx);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep normal returns on the token-and-guard path

For every normal value or void portal return, this call runs gen_store_back_in_vable, marks the virtualizable forced, and therefore intentionally suppresses the subsequent store_token_in_vable plus GUARD_NOT_FORCED_2. That is not the upstream return shape: interp_jit.py forces only the Yield arm, while ordinary Return reaches compile_done_with_this_frame, which stores the token. The comment identifies premature backend jitframe-chain freeing as the reason for this substitution, so this masks that lifetime defect and adds a full frame/locals write-back to every compiled normal return; retain the deadframe correctly and preserve the standard token/guard sequence instead.

AGENTS.md reference: AGENTS.md:L194-L196

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 126ed43284

ℹ️ 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 w_code.is_null() {
return;
}
let raw_code = unsafe { pyre_interpreter::w_code_get_ptr(w_code as PyObjectRef) };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject synthetic pycode sentinels before dereferencing

When a blackhole replays a non-standard virtualizable frame from a bridge sub-walk, the frame can carry usize::MAX as its synthetic w_code value; jitcode_dispatch/mod.rs:581-589 already guards this exact representation because dereferencing it causes a SIGSEGV. This hook checks only for null and then passes the sentinel to w_code_get_ptr, whose safety contract requires a valid PyCode, so reaching the next live/ marker crashes instead of merely skipping the coordinate publication. Check the sentinel and validate that w_code is a code object before extracting its raw pointer.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e8f0ded2a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1452 to +1453
*((recording_frame_ptr + crate::frame_layout::PYFRAME_LAST_INSTR_OFFSET)
as *mut isize) = py_pc as isize;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore last_instr when terminal replay is declined

When a top-level walk reaches a return or uncaught raise after a residual executor declined, fbw_has_unjournaled_effect() disables terminate_no_replay; the epilogue resets the concrete result, rolls back its journals, and returns ContinueRunningNormally so the interpreter can replay from the pre-walk state. This direct write is not part of those journals and is never undone, however, so re-entry derives frame.next_instr() from the terminal coordinate and starts after the return/raise rather than replaying the declined residual, potentially skipping the remainder of the frame or falling off its bytecode. Preserve the previous scalar and restore it on every noncommitted exit.

AGENTS.md reference: AGENTS.md:L14-L20

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/trace.rs (2)

105-205: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Replace the runtime odometer deviation with upstream effect classification.

WalkEndResume::Rewind commits from a dynamic counter even though the surrounding documentation identifies upstream’s codewriter-time EffectInfo classification as the authoritative mechanism. This is a semantic fork, not a parity-preserving port; carry over the static classification and structural flow before relying on this gate.

As per coding guidelines, “When porting RPython/PyPy, maintain strict line-by-line structural parity; do not shortcut, reimplement from scratch, or declare a phase complete without the literal refactor.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyre/pyre-jit-trace/src/trace.rs` around lines 105 - 205, Replace the runtime
odometer-based WalkEndResume::Rewind proof with a static per-callee EffectInfo
classification matching upstream’s codewriter-time effect handling. Update
walk_end_resume_provable and the associated resume/commit flow so rewind
permission is derived structurally from the declared effect class, preserving
the upstream distinction between elidable/non-raising residuals and effectful
residuals; remove the dynamic counter dependency and related RewindUnproven
path.

Source: Coding guidelines


856-899: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

Do not fall back to replay after the rebuilt callee has run.

execute_frame can complete before carrier recovery or flush_walk_end_state_after_outer_call fails. That returns AfterRun, after which the caller takes no forward-resume path; the epilogue rolls back only the store journal and legacy replay invokes the already-executed Python callee again. Make every post-run step infallible from prevalidated data, or continue forward from the post-call state—never replay this CALL.

As per coding guidelines, “The generated JIT must preserve interpreter semantics.”

Also applies to: 3339-3358

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyre/pyre-jit-trace/src/trace.rs` around lines 856 - 899, Update the
post-`execute_frame` path around `below_now` and
`flush_walk_end_state_after_outer_call` so no `AfterRun` or other failure can
reach legacy replay after the callee has executed. Validate or retain all
required carrier and flush state before running the callee, make recovery/flush
infallible from that validated state, or provide a forward-resume path that
continues from the post-call state; apply the same guarantee to the
corresponding flow around the referenced additional occurrence.

Source: Coding guidelines

🤖 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/optimizeopt/virtualize.rs`:
- Around line 2102-2139: Replace the early guard dispatch in the Finish branch
of the pass method with Optimizer-side FINISH postprocessing. After FINISH
emission has forced its arguments, use Optimizer::store_final_boxes_in_guard
with collect_optimizer_knowledge_for_resume and insert the finalized
GUARD_NOT_FORCED_2 immediately before FINISH, preserving upstream
postprocess_FINISH ordering and bridgeopt knowledge.

In `@pyre/bench/synth/exception_traceback_frame_lineno.py`:
- Around line 11-14: Clarify the comment around catches_here() to distinguish
traceback tb_lineno from the escaped frame’s f_lineno: assert that tb_lineno
remains the raise-site line, while frame.f_lineno reports the later return line
reached before the function exits. Explicitly identify the return coordinate as
the expected frame.f_lineno value.

In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 2271-2284: In the multi-frame adoption flow, snapshot and root
each frame’s original f_backref before the temporary chain is rewired, then
restore the complete original link chain on every decline after linking,
including the capture/write_back_outer_locals failure paths and the
corresponding path around the other reported range. Keep restoring frame locals
and shadow-stack roots as currently required, but ensure all temporary frame
links are also restored before returning false.

---

Outside diff comments:
In `@pyre/pyre-jit-trace/src/trace.rs`:
- Around line 105-205: Replace the runtime odometer-based WalkEndResume::Rewind
proof with a static per-callee EffectInfo classification matching upstream’s
codewriter-time effect handling. Update walk_end_resume_provable and the
associated resume/commit flow so rewind permission is derived structurally from
the declared effect class, preserving the upstream distinction between
elidable/non-raising residuals and effectful residuals; remove the dynamic
counter dependency and related RewindUnproven path.
- Around line 856-899: Update the post-`execute_frame` path around `below_now`
and `flush_walk_end_state_after_outer_call` so no `AfterRun` or other failure
can reach legacy replay after the callee has executed. Validate or retain all
required carrier and flush state before running the callee, make recovery/flush
infallible from that validated state, or provide a forward-resume path that
continues from the post-call state; apply the same guarantee to the
corresponding flow around the referenced additional occurrence.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6788bea9-341d-4e74-9ba3-7f76bb09a7a7

📥 Commits

Reviewing files that changed from the base of the PR and between 54f3597 and 0e8f0de.

📒 Files selected for processing (10)
  • majit/majit-metainterp/src/blackhole.rs
  • majit/majit-metainterp/src/optimizeopt/mod.rs
  • majit/majit-metainterp/src/optimizeopt/virtualize.rs
  • pyre/bench/synth/exception_traceback_frame_lineno.py
  • pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
  • pyre/pyre-jit-trace/src/state.rs
  • pyre/pyre-jit-trace/src/trace.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyre-jit/src/jit/codewriter.rs
💤 Files with no reviewable changes (1)
  • majit/majit-metainterp/src/optimizeopt/mod.rs

Comment on lines +2102 to +2139
// majit ordering: upstream INSERTS because its postprocess runs
// after the FINISH is already appended. This pass runs BEFORE the
// FINISH reaches the terminal emit, so `emit_extra` queues the
// stashed guard for the passes after virtualize and
// `drain_extra_operations_from` (called right after this method
// returns) flushes it through the pipeline first. The guard lands
// in `new_operations` first, the FINISH second — the same final
// op order.
//
// RPython parity: optimize_FINISH does NOT call the generic
// escaping-op force path here. Forcing the FINISH args in the
// virtualize pass would happen before the stashed
// GUARD_NOT_FORCED_2 is reinserted, and store_final_boxes_in_guard
// would then see the already-forced return box in vable_array.
// The actual arg forcing belongs later in Optimizer._emit_operation,
// after the queued guard has been flushed ahead of FINISH.
// The RESUME DATA is where the two diverge. Upstream finalizes the
// guard in `postprocess_FINISH`, i.e. after `emit(op)` forced the
// FINISH args, so `store_final_boxes_in_guard` sees a return box
// that was virtual as already materialized. Here the guard is
// finalized on the way through the pipeline, before that forcing,
// and encodes the same box as still virtual. Both are consistent
// images, but they are not the same image.
//
// BLOCKER for the faithful order. `propagate_postprocess` (the
// port of optimizer.py's postprocess dispatch) is a method on a
// PASS, and the finalization a guard needs is
// `Optimizer::store_final_boxes_in_guard` with the knowledge
// `collect_optimizer_knowledge_for_resume(&self)` gathers — which
// needs the Optimizer, not a pass. Running it from here with no
// knowledge would drop the bridgeopt sections that
// `serialize_optimizer_knowledge` puts in every other guard, buying
// one ordering divergence with a worse one. Reaching upstream's
// shape needs an Optimizer-side FINISH postprocess that can insert
// at `new_operations.len() - 1` after its own emit.
//
// Nothing arms the token today — the portal-return
// `gen_store_back_in_vable` sets `forced_virtualizable`, so
// `store_token_in_vable` early-returns and no `GUARD_NOT_FORCED_2`
// reaches a FINISH — so neither image is currently observable.
OpCode::Finish => {
self.finish_guard_op = self.last_guard_not_forced_2.take();
if let Some(guard_op) = self.finish_guard_op.clone() {
ctx.emit_extra(ctx.current_pass_idx, guard_op);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Restore Optimizer-side FINISH postprocessing rather than finalizing the guard early.

ctx.emit_extra sends GUARD_NOT_FORCED_2 through the pipeline before terminal FINISH emission forces its arguments. As the comment notes, this captures a different resume-data image from upstream. Implement the Optimizer-side postprocess that finalizes the guard after FINISH argument forcing, then inserts it immediately before FINISH.

As per coding guidelines, “When porting RPython/PyPy, maintain strict line-by-line structural parity; do not shortcut, reimplement from scratch, or declare a phase complete without the literal refactor.”

🤖 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/optimizeopt/virtualize.rs` around lines 2102 -
2139, Replace the early guard dispatch in the Finish branch of the pass method
with Optimizer-side FINISH postprocessing. After FINISH emission has forced its
arguments, use Optimizer::store_final_boxes_in_guard with
collect_optimizer_knowledge_for_resume and insert the finalized
GUARD_NOT_FORCED_2 immediately before FINISH, preserving upstream
postprocess_FINISH ordering and bridgeopt knowledge.

Source: Coding guidelines

Comment thread pyre/bench/synth/exception_traceback_frame_lineno.py Outdated
Comment thread pyre/pyre-jit-trace/src/trace.rs
The top-level finish path (`fbw_terminate_with_finish` /
`fbw_terminate_void_with_finish`) now publishes the return coordinate into
`last_instr` and calls `gen_store_back_in_vable`, the route
`opimpl_hint_force_virtualizable` takes (pyjitpl.py); doc/jit/virtualizable.rst
names forcing before returning as the handling for a virtualizable that
survives its trace. `store_token_in_vable` runs after it and declines, because
`gen_store_back_in_vable` sets `forced_virtualizable` (pyjitpl.py), and its
final store leaves `vable_token` zero rather than naming a jitframe the backend
frees before `execute_token` returns.

`fbw_publish_raise_last_instr` becomes `fbw_publish_exit_last_instr` and is
called from both frame exits. Previously only the uncaught-raise exit published,
so a frame the function-entry portal compiled and left by RETURN kept the `-1`
initialization sentinel, which `offset2lineno` answers with the code object's
first line; a traceback the frame handed out reported the `def` line for
`tb_frame.f_lineno`.

The publish also writes `last_instr` into the live recording frame. The walker
records ops rather than executing them against the frame, so the iteration the
trace is recorded from carried the sentinel even with the store emitted — one
wrong answer per compiled function, at the iteration after the compile
threshold.

Adds pyre/bench/synth/exception_traceback_frame_lineno.py, which surveys every
iteration and crosses the `while` and `for` driver shapes; the two reach the
callee as a function-entry portal and as an inlined callee respectively.

Verified: check.py dynasm 330/330, cranelift 330/330; cargo test -p
pyre-interpreter -p pyre-jit -p pyre-jit-trace --lib --features dynasm.

Assisted-by: Claude
`dispatch_bytecode` stamps `self.last_instr` before running each opcode, so a
frame that has returned answers `offset2lineno` — `f_lineno`, and every
traceback that outlives the frame — for its `return`. The blackhole replays
codewriter jitcode instead of that loop and syncs only `valuestackdepth`, so a
frame finished by a guard-failure resume kept whichever coordinate the trace
last published: for a loop whose body raises and catches, the raise.

`Instruction::ReturnValue` now emits `setfield_vable_i(frame, py_pc,
last_instr)` before the return edge. It stores `py_pc`, not the `py_pc - 1` of
the resume-at sites, because the opcode is dispatched there rather than
resumed at.

The bench gains a `loop_owner_*` group for the frame that owns the compiled
loop, and the comment claiming RPython lowers no per-bytecode virtualizable
write is replaced with the actual blocker: one distinct int pool constant per
PC against `assembler.py check_result`'s 256-entry cap.

Assisted-by: Claude
`optimize_FINISH` moved `_last_guard_not_forced_2` into a context field that no
code read repo-wide, so the guard was dropped. Upstream's `postprocess_FINISH`
re-inserts it at `len(_newoperations) - 1`; this pass runs before the FINISH
reaches the terminal emit, so `emit_extra` queues the guard for the passes
after virtualize and `drain_extra_operations_from` flushes it into
`new_operations` ahead of the FINISH — the same final layout, with the resume
data finalized by the `store_final_boxes_in_guard` every emitted guard runs.

Deletes `pending_finish_guard_postprocess` and the `propagate_postprocess` /
`have_postprocess_op` pair that filled it.

Assisted-by: Claude
`dispatch_bytecode` (pyopcode.py) stamps `last_instr` before every
opcode, so a frame answers `f_lineno`, `f_lasti` and any traceback taken
off it for the instruction it is on.  That store is a source-level one
upstream and rides in the jitcode, so blackhole replay reproduces it;
this codewriter unrolls the bytecode per PC, where the same store needs
one distinct int pool constant per instruction against `check_result`'s
256-entry cap.

`handler_live` gains a process-global hook — `bhimpl_live` is a no-op
upstream for exactly the reason above — and pyre registers a publisher
that maps the marker's jitcode pc back to its Python pc and stores it
into the frame the portal red names.  The two blackhole builders (the
guard-failure resume and the force-adoption lease) both go through it.

The publisher resolves everything under one `METAINTERP_SD` borrow and
takes no reference count: it runs once per replayed instruction, and the
`Arc`-cloning accessors each re-run `ensure_finish_setup`, whose opname
-map clone alone costs more than the instruction being replayed.

`exception_traceback_frame_lineno` gains a `mid_replay` group reading the
frame from the two places that can while it is still running: a callee
walking up with `sys._getframe`, and a traceback taken inside a handler
the same frame is executing.  Split across calls so the set holds the
interpreted answer and the replayed one together.

Assisted-by: Claude
The escape flush that runs ahead of a forcing residual is
all-or-nothing, and its decline is what the single-frame blackhole latch
is gated on (`committed_frame_escape_pc().is_none()`).  It declines on
the operand-stack half — the vable shadow's stack region reads NULL away
from a merge point — which the register image supplies anyway.  The
locals half is not optional: every LOAD_FAST lowers to
`getarrayitem_vable_r` on the frame the register image names, so the
replay read whatever that frame held before the walk began, and a local
the walk assigned came back null.  Reading an attribute off one faulted
in `object_getattr_miss`.

`try_adopt_single_frame_blackhole` now writes that half with
`write_back_outer_locals` before driving, and withdraws it through
`capture_frame_locals` / `restore_frame_locals` when it cannot complete,
so a decline still hands the legacy replay pristine pre-walk state.  The
saved copy is registered as resume roots for the publish, which boxes and
can collect.

That frame is the live one while `cf_addr` is the walk's snapshot of it,
so the two addresses differ by design.  Identity decides which is
writable: two invocations of the same function share a code object, so
the publish runs only when the register names the frame this walk is
running (`live_vable_frame_addr`).

Assisted-by: Claude
`publish_last_instr_at_live_marker` fell back to `virtualizable_ptr`
when the level's own portal red held no frame.  A nested level carries
no virtualizable of its own, so that resolved to the level ABOVE, and
direct recursion — where caller and callee share a code object — passed
the code check and wrote the callee's coordinate into the caller's
frame.  Only this level's own register bank names a frame it may stamp.

Assisted-by: Claude
The write loop resolved each shadow entry as it stored, so a slot that
did not resolve left the frame carrying a mix of walk-current and
pre-walk locals — neither of the two states a caller can recover from.
Validate the whole range first, the way the merge-point flush validates
ahead of its own commit loop.

`capture_frame_locals` also records why it has no upstream counterpart:
`write_from_resume_data` (resume.py) runs on a per-call `MIFrame` whose
values RPython's GC sees as ordinary references, so publishing over a
live frame and taking it back does not arise there.

Assisted-by: Claude
Frame 0 of the recovered chain is the walked frame, and reaching the
latch means the same all-or-nothing escape flush declined as on the
single-frame arm, so its level read pre-walk locals through
`getarrayitem_vable_r` on `per_frame[0]`.  Same publish and same
withdrawal as that arm.

The inner levels get no counterpart and the comment now carries the
measurement: the walk's shadow covers the walked frame only, so an
inlined callee's frame array keeps its pre-sub-walk contents while the
sub-walk's values sit in that level's register image.  With
`PYRE_FBW_MULTIFRAME=1`, a callee that stores `e.__traceback__` and then
reads an attribute off it faults in `object_getattr_miss`, where the same
shape through the single-frame arm is correct.

The gate stays default-off.  With it on, the synthetic corpus is 313/313
and `getframe_while_inlined_callee_subwalk` still reports its 5 adopts.

Assisted-by: Claude
The locals publish that precedes the blackhole drive was only withdrawn
when the publish itself declined.  A terminal the adopt arms reject falls
back to legacy escape/replay, which resumes the frame from its pre-walk
state, so those speculative writes have to come off there too: both arms
now route every post-drive decline through a common restore.

The undo image stays registered as a resume root across the drive.  The
publish overwrites the slots it was taken from, so it holds the only
remaining reference to the pre-walk locals, and a collection inside the
drive would otherwise free them and leave the restore writing pre-move
addresses.

Assisted-by: Claude
Every level runs its own hot loop and every level shares one code object
with its caller, so a per-level frame mix-up would survive a code-object
check.  Each level reports its own coordinate through `caller_offset`;
a level answering for another one shows up as a shifted offset.

Assisted-by: Claude
Comments only.

The portal-return force fires on an exit upstream leaves alone:
`interp_jit.py` `PyFrame.dispatch` applies `force_virtualizable=True` under
`except Yield` against a bare `except Return`, so an ordinary return gives up
the FORCE_TOKEN/GUARD_NOT_FORCED_2 protocol for an unconditional store-back.
Narrowing it back down needs the deadframe retention the backend does not
provide, since whether the frame escapes is a runtime property.

The stashed GUARD_NOT_FORCED_2 reaches `new_operations` in upstream's final
order but not with upstream's resume data: `postprocess_FINISH` finalizes it
after `emit(op)` forced the FINISH args, while the `emit_extra` route
finalizes it before.  Reaching that order needs an Optimizer-side FINISH
postprocess, because faithful finalization needs
`collect_optimizer_knowledge_for_resume`, which a pass cannot reach.  The
earlier comment claimed the two layouts were equivalent.

Assisted-by: Claude
The header said the `return` is "the line its traceback has to report",
which reads as a claim about `tb_lineno`.  `tb_lineno` is frozen at the
raise site when the node is built; the coordinate that has to reach the
`return` is the frame's `f_lineno`, which is read off the frame on every
access.

Assisted-by: Claude
`frame_var` names the outermost frame in a non-portal callee, which the
`LoadGlobal` register-form decline in the same file already documents, so
the `ReturnValue` store stamped an inlined callee's `py_pc` into its
caller's `last_instr`.  The caller then resolved that coordinate against
its own line table, so a frame read after the callee returned — through
`sys._getframe(1)` or a retained traceback — reported an unrelated line.

The callee's own frame stays unpublished, the same level the `-live-`
marker hook declines to publish for.

Assisted-by: Claude
A non-standard virtualizable frame from a bridge sub-walk carries the
`GcRef(usize::MAX)` sentinel as `w_code`, and the hook tested only for
null before handing it to `w_code_get_ptr`, whose contract requires a
valid `PyCode`.  Test the sentinel and `is_code` first, the order the two
other readers of this field already use.

Assisted-by: Claude
Two writes a walk makes before its commit is decided outlived a decline.

`fbw_publish_exit_last_instr` writes the exit coordinate into the live
recording frame, and the walk-end epilogue only decides afterwards
whether the exit is kept.  A declined walk returns to a replay that
resumes the frame from its pre-walk state and derives the next
instruction from that very field (`next_instr` = `last_instr + 1`), so a
kept exit coordinate restarted the frame past its own return or raise.
Journal the pre-write value and restore it beside the store journal.

`try_adopt_multi_frame_blackhole` rewires each inner level's `f_backref`
into the chain the drive needs, ahead of the locals publish and the
terminal match.  Every decline there returns to a legacy escape/replay
that never entered those levels, so the abandoned chain stayed visible to
anything still reaching one of the frames.  Record the links as they are
overwritten and restore them on every post-link decline.

Assisted-by: Claude
The `mid_replay` and `recursive` groups read a frame while it is still
running, which the wasm backend does not answer correctly today, and the
synthetic suite has no per-backend scoping.  They move to a self-checking
guard registered with `skip_backends=("wasm",)`, the mechanism the three
existing wasm-skipped guards use.  The post-return groups, which wasm does
satisfy, stay in the synthetic bench.

Measured on wasm: a frame that owns a compiled loop reports offset 0 from
the first compiled call onward, against 4 on pypy3, CPython, dynasm and
cranelift.  Instrumenting both ends showed the marker hook writing the
right coordinate into the right frame at the right offset and reading it
back intact, and the interpreter then reading 0 from that same address, so
a wasm-side writer clears it before the residual `sys._getframe`.  The
guard's header records that measurement and what it rules out.

Assisted-by: Claude

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 614b34c3d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread pyre/check.py
"frame_lineno_mid_replay",
f"{B}/frame_lineno_mid_replay_regression.py",
20,
skip_backends=("wasm",),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fix wasm before skipping the replay-coordinate check

When wasm executes a compiled frame that is inspected mid-replay through sys._getframe, this skip hides a known semantic regression: the new test documents wasm producing [0, 4] where the interpreter and native backends produce [4], because last_instr is cleared after the live-marker publish. Fresh evidence beyond the earlier per-opcode finding is that this added regression explicitly reproduces the mismatch on compiled wasm calls; fix the wasm-side writer rather than exempting that production backend from the check.

AGENTS.md reference: AGENTS.md:L14-L20

Useful? React with 👍 / 👎.

@youknowone
youknowone merged commit 17484fd into main Jul 28, 2026
19 checks passed
@youknowone
youknowone deleted the perf-exc branch July 28, 2026 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant