Skip to content

jit(wasm): fix #420 wasm-only failures + inline_helper strict-MF regression#440

Merged
youknowone merged 7 commits into
mainfrom
wasm-jit
Jul 9, 2026
Merged

jit(wasm): fix #420 wasm-only failures + inline_helper strict-MF regression#440
youknowone merged 7 commits into
mainfrom
wasm-jit

Conversation

@youknowone

@youknowone youknowone commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Closes #420 — the four wasm-only synthetic failures pass, plus a fix for an inline_helper regression the #420 work itself introduced (caught by CI on all three backends). Rebased onto current main; the tagged-int groundwork this branch previously carried is now upstream via #417, so the branch is purely the #420 fixes.

#420: four wasm-only failures

The four benches failed only on wasm (dynasm + cranelift were green). Three distinct roots:

Inline-callee guard snapshot (inlined_mutation_before_abort, inline_callee_constructs_object)

A guard emitted inside an inlined callee sub-walk built its resume snapshot at the outer CALL boundary (single-frame collapse), corrupting the wasm resume two ways:

  • jit: fill inline sub-walk guard snapshot from live register banks — the full-body walk initializes outer_active_boxes empty and recomputes per-guard on the FBW path, so a callee guard read an empty set (zero frame boxes while the decoder expected the full liveness set). Recompute it fresh via collect_outer_active_boxes at the CALL-site py_pc when the parent walk carries an empty set.
  • jit: route strict straight-line inline-callee guards through the multi-frame snapshot — a strict straight-line callee pushed no paused caller frame, so its in-callee guards resumed at the caller CALL boundary, re-executing the call and mis-sizing the resumed frame (LOAD_FAST push past the frame array / rd_numb decode overrun → inline_callee_constructs_object pyframe.rs index-out-of-bounds panic). Under PYRE_FBW_STRICT_MULTIFRAME (default on) the strict path computes a paused caller frame via compute_inline_caller_frame, and declines to a residual call when the callee frame is not snapshot-able.

W_ObjectObject.map field width on wasm32 (inlined_helper_mutation, sre_pattern_methods)

jit(wasm): size W_ObjectObject.map at word width, not fixed 8 bytes — the map field descr hardcoded field_size: 8. map is a *const pointer erased to an opaque Type::Int word; on wasm32 a pointer is 4 bytes, and map's neighbor at offset+4 is the storage pointer. The guard_value(map) loaded 8 bytes — folding storage into the high half — so the garbage-high value never matched the clean 4-byte map constant and the guard failed every iteration, deopting after the loop body had already committed its side effect and then re-executing it. Fix: field_sizecore::mem::size_of::<usize>() (4 on wasm32, 8 on 64-bit; native descr byte-identical, so dynasm/cranelift unaffected).

Fresh-collect liveness coordinate

jit: derive inline sub-walk fresh-collect liveness coordinate from the CALL op — the fresh collect_outer_active_boxes path passed the ctx sentinels (outer_jitcode_index 0, walk-entry py_pc) instead of the CALL op's own coordinate, so a CALL in a non-root jitcode selected the wrong liveness window. Derive (jitcode index, CALL-site py_pc) from the snapshot sym's jitcode at op.pc, matching orthodox_list_append_commit.

inline_helper strict-MF regression (all three backends)

The strict-multiframe fix above pushed a paused caller frame for every strict straight-line inlined callee. A strict callee seeds no callee frame red, so its in-callee guard snapshot always returns Unsupported and the inline declines. For a pure value-returning leaf (inline_helper's a + b / a * b) whose declining guard is an arithmetic-overflow guard, that Unsupported propagates out of the sub-walk and aborts the whole loop trace — so inline_helper, which compiled fine on main via the single-frame collapse, stopped compiling (loops_compiled=0 loops_aborted=25) and timed out in the interpreter on all three backends.

jit: gate strict-MF caller-frame push on a side-effecting callee — a new strict_callee_collapse_unsound predicate gates the push: route through the MF decline only when the callee body commits a heap side effect the collapse re-execute would double (a general ref-arg residual call / constructor residual_call_r_*, a void side-effect residual call residual_call_*_v, or a setfield / setarrayitem write). inline_callee_constructs_object (P(n) construct) and inlined_mutation_before_abort (c.n = c.n + 1 store) still take the decline; a pure leaf keeps its sound single-frame collapse.

Results (full pyre/check.py, all three backends)

backend result inline_helper
dynasm 159/159 0.16s (1.1x)
cranelift 159/159 0.16s (1.1x)
wasm 159/159 0.34s (2.3x)

All four #420 benches (inlined_mutation_before_abort, inline_callee_constructs_object, inlined_helper_mutation, sre_pattern_methods) pass on wasm; inline_helper compiles again on all three; no synth regression.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Improved JIT trace handling for strict inlining, making guard and snapshot behavior more reliable in deeper call paths.
    • Added support for an environment toggle to control stricter multi-frame handling.
  • Bug Fixes

    • Fixed a target-specific field size issue so object layout works correctly on 32-bit platforms.
    • Prevented incorrect guard collapse that could re-execute side effects or resume from the wrong location during deoptimization.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adjusts a hardcoded field size in descr.rs for 32-bit target correctness, and adds strict multiframe inline callee guard handling in jitcode_dispatch.rs: a config flag, a bytecode-based unsound-collapse detector, updated inline decision logic, and corrected outer active-box/snapshot coordinate computation for callee WalkContext.

Changes

Field size correctness fix

Layer / File(s) Summary
Fix hardcoded map field size
pyre/pyre-jit-trace/src/descr.rs
field_size for W_ObjectObject.map changed from hardcoded 8 to core::mem::size_of::<usize>(), with a comment explaining 32-bit/wasm32 storage-pointer corruption risk.

Estimated code review effort: 2 (Simple) | ~10 minutes

Strict multiframe inline callee guard resume (gh#420)

Layer / File(s) Summary
Strict multiframe config flag
pyre/pyre-jit-trace/src/jitcode_dispatch.rs
Adds fbw_strict_multiframe_enabled() gated by PYRE_FBW_STRICT_MULTIFRAME (default enabled), and extends dispatch comments describing the residual-call decline path for strict callees.
Unsound collapse bytecode scan
pyre/pyre-jit-trace/src/jitcode_dispatch.rs
Adds strict_callee_collapse_unsound(body_code), decoding bytecode to flag residual calls or mutation-like ops that would double-apply side effects on collapse.
Inline decision and callee frame resume
pyre/pyre-jit-trace/src/jitcode_dispatch.rs
Extends inlining decision logic to require strict multiframe enablement, depth bounding, and unsound-collapse detection, computing a paused caller frame via compute_inline_caller_frame for callee-coordinate resume.
Outer active boxes and snapshot coordinates
pyre/pyre-jit-trace/src/jitcode_dispatch.rs
Computes fresh outer active boxes and a CALL-site-derived liveness coordinate when empty, then applies these to the callee WalkContext so guards resume at the correct CALL boundary.

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

Possibly related PRs

  • youknowone/pyre#99: Both modify jitcode_dispatch.rs around collecting outer active boxes and snapshot liveness coordinates.
  • youknowone/pyre#320: Both modify the multiframe/guard-resume inline dispatch path including FBW_MAX_MULTIFRAME_DEPTH-related resumption coordinate handling.
  • youknowone/pyre#365: Both change how guard/resume coordinates are computed for gh#420-style "resume at correct PC" behavior.

Poem

A hop, a byte, a guard reborn,
No more collapse where callees mourn 🐇
Boxes filled where once were bare,
Resuming right, with proper care.
Thump thump — the JIT trace runs so true,
Strict and sound, both old and new!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address the linked #420 wasm-only failures by fixing map sizing, empty snapshots, and CALL-site resume behavior.
Out of Scope Changes check ✅ Passed No obvious unrelated changes appear beyond the wasm JIT snapshot and resume fixes needed for the linked issue.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main changes: wasm-only JIT fixes and the strict-MF inline_helper regression.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch wasm-jit

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 8, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit a51cbf0).

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:11684 ↔ rpython/jit/metainterp/pyjitpl.py:2178: our new strict-callee classifier uses opcode spelling, "op.starts_with(\"residual_call_r\")", to decide that a callee must leave the strict collapse path; PyPy resolves the actual callable target, "jitcode = sd.bytecode_for_address(key)", and if present follows it with perform_call. This can decline/ref-route ref-returning residual calls that PyPy would still inline through a real MIFrame.

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:12841 ↔ rpython/jit/metainterp/opencoder.py:819: strict multi-frame capture is conditional on "strict_callee_collapse_unsound(body.code)", while PyPy’s "capture_resumedata(self, framestack, ...)" snapshots the active framestack for guards regardless of whether the inlined callee is side-effecting. This is a partial restoration of frame parity, not full PyPy equivalence.

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:12869 ↔ rpython/jit/metainterp/pyjitpl.py:2447: when compute_inline_caller_frame(ctx, op.pc) returns None, the new strict path keeps the old collapse; PyPy’s "f = self.newframe(jitcode, greenkey)" creates a frame for the inlined call unconditionally once perform_call is chosen.

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

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:12414 ↔ rpython/jit/metainterp/pyjitpl.py:2445: strict pure-leaf callees still use a single-frame caller-boundary snapshot, quoted in our code as "Guards inside a pure-leaf callee resume to the caller's CALL boundary", whereas PyPy’s perform_call enters the callee by appending a separate frame.

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:10003 ↔ rpython/jit/metainterp/opencoder.py:823: our multi-frame path is only used when "n_parents > 0 && n_parents == n_callees"; PyPy always calls create_top_snapshot(top, ...) and then _ensure_parent_resumedata(...) for the whole framestack.

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:11877 ↔ rpython/jit/metainterp/pyjitpl.py:177: our callee frame snapshot can abort when a live register is missing, "holds OpRef::NONE", while PyPy reads active boxes directly from the live MIFrame registers via get_list_of_active_boxes.

4. Structural adaptations

  • pyre/pyre-jit-trace/src/descr.rs:1882 ↔ pypy/objspace/std/mapdict.py:906: W_ObjectObject.map now uses "core::mem::size_of::<usize>()"; PyPy stores/promotes self.map as an object pointer with platform pointer width. This is a Rust layout adaptation and improves parity versus the previous hardcoded 8.

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:12904 ↔ rpython/jit/metainterp/pyjitpl.py:177: our inline sub-walk recomputes "inline_outer_active_boxes" from full-body walker registers when inherited boxes are empty; PyPy asks the current frame for get_list_of_active_boxes(...). This is an implementation adaptation to Pyre’s full-body walker state shape.

  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs:7241 ↔ rpython/jit/metainterp/pyjitpl.py:2610: PYRE_FBW_STRICT_MULTIFRAME is an environment-gated rollout switch; PyPy has no corresponding feature flag around capture_resumedata. This is a deployment adaptation, not a source-level PyPy construct.

@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: b1a12814b9

ℹ️ 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 +12677 to +12678
ctx.outer_jitcode_index,
ctx.entry_py_pc,

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 Use the caller's call-site pc for inline snapshots

When this branch is taken from production dispatch_via_miframe, ctx.outer_jitcode_index is still the sentinel 0 and ctx.entry_py_pc is the full-body walk entry, because normal full-body guard capture derives those values from FULL_BODY_SNAPSHOT_SYM plus the current op pc. For an inlined call in any function whose jitcode index is not 0, or whose CALL is not at the walk-entry Python pc, this asks collect_outer_active_boxes for the wrong liveness window, so the callee guard snapshot can still encode the wrong number/order of frame boxes and deopt/resume with the decoder mismatch this fix is trying to avoid. Derive the jitcode index from sym.jitcode and the CALL-site py pc from op.pc here, as try_walker_list_append_inline does.

Useful? React with 👍 / 👎.

@youknowone youknowone changed the title jit(wasm): fix all four #420 wasm-only failures (+ inert tagged-int groundwork) jit(wasm): fix #420 wasm-only failures + inline_helper strict-MF regression Jul 9, 2026

@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: 61a09ec78b

ℹ️ 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 +12791 to +12793
entry_py_pc: ctx.entry_py_pc,
outer_jitcode_index: ctx.outer_jitcode_index,
outer_active_boxes: ctx.outer_active_boxes.clone(),
outer_active_boxes: inline_outer_active_boxes,

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 Stamp inline snapshots with the call-site coordinate

Fresh evidence in this revision: the new call_site_py_pc/call_site_jc_index are used only to build inline_outer_active_boxes, but the sub-walk still stores ctx.entry_py_pc and ctx.outer_jitcode_index here. In full-body dispatch_via_miframe those fields are the walk-entry pc and sentinel index 0, so a guard inside an inlined callee whose CALL is later in the function or in a nonzero jitcode still captures CALL-site boxes under the wrong snapshot frame header, leaving the encoder/decoder liveness windows inconsistent and preserving the resume mismatch this patch is trying to fix.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in c0d8cf2a6ad: the fresh-collect path now carries the CALL-site coordinate to the snapshot header too — sub_wc.entry_py_pc/outer_jitcode_index are stamped with call_site_py_pc/call_site_jc_index alongside the boxes, so the decoder's liveness window at the header matches the collected boxes. Non-fresh-collect paths keep the inherited ctx coordinate (unchanged). Full check.py is 159/159 on all three backends. — commented by Claude

Comment on lines +12674 to +12676
} else if strict_inlinable
&& fbw_strict_multiframe_enabled()
&& strict_callee_collapse_unsound(body.code)

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 Keep strict multiframes within the supported inline depth

This strict branch can run while the walker is already inside an inlined callee, unlike the try_multiframe path above which is gated by inline_depth < FBW_MAX_MULTIFRAME_DEPTH. For a side-effecting straight-line callee that inlines another side-effecting straight-line callee, both calls can push InlineParentFrames, so the inner guard sees n_parents == n_callees == 2 and emits a three-frame snapshot even though this file documents the multiframe resume path as only sound for one paused caller frame; that can hit the existing middle-frame virtual PyFrame resume corruption/crash instead of declining the nested inline.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in c0d8cf2a6ad: added && inline_depth < FBW_MAX_MULTIFRAME_DEPTH to the strict-MF branch, mirroring try_multiframe. A side-effecting strict callee nested inside another now falls back to the single-frame collapse instead of pushing a second paused frame, so n_parents stays ≤ 1. Full check.py is 159/159 on dynasm/cranelift/wasm. — commented 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: c0d8cf2a6a

ℹ️ 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 +11681 to +11684
if op.starts_with("residual_call_r")
|| (op.starts_with("residual_call") && op.ends_with("_v"))
|| op.contains("setfield")
|| op.contains("setarrayitem")

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 Treat interior-field writes as side effects

When a strict inlined callee contains a straight-line setinteriorfield_gc_* (emitted for OpKind::InteriorFieldWrite in the codewriter) followed by a guard, this predicate still returns false because setinteriorfield does not contain the substring setfield. That leaves the callee on the single-frame collapse path, so a deopt after mutating array-of-struct/dict-style storage re-executes the whole callee and applies the write a second time, the same unsoundness this gate is trying to avoid for setfield/setarrayitem.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 0a2ba5d2d5e: added the setinteriorfield substring to strict_callee_collapse_unsound's side-effect set (the plain setfield substring does not span setinteriorfield_gc_*). A strict callee with an interior-field write now routes through the decline instead of the collapse. check.py 159/159 on all three backends. — commented by Claude

Comment on lines +3535 to +3536
self.lookup_typeid_from_classptr(int_classptr)
.map(|int_typeid| {

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 Emit the tag check without relying on int typeid table hits

With gcremovetypeptr, the runner only materializes classptr→typeid entries for the constant classes that appear as guard operands, so a trace containing only GuardClass(obj, float) usually has no INT_TYPE entry. In that case this .map(...) emits no tagged-immediate branch at all, and a tagged int still falls through to emit_load_gc_typeid_into_x16(obj), dereferencing obj - GcHeader::SIZE; the guard crashes unless some unrelated int-class guard happened to seed the table.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Resolved by removing the dynasm tag-test groundwork entirely: the majit-backend-dynasmpyre-object dependency was an architectural layering violation, so that commit (the GuardClass/GuardGcType tag-test using INT_TYPE/CAN_BE_TAGGED) was dropped from the branch. The backend no longer emits any tagged-immediate branch or depends on pyre-object. The tagged-int enablement epic will re-introduce the tag-test sourcing its config and the int classptr from the appropriate lower layer (e.g. the GC hook) rather than hardcoding INT_TYPE and relying on typeid-table hits. — commented by Claude

`try_walker_inline_user_call` cloned `ctx.outer_active_boxes` for the
callee sub-walk. Under the main full-body walk `dispatch_via_miframe`
initializes that set empty (`Vec::new()`), computing it per-guard on the
FBW path instead. A callee guard falls through to the per-opcode arm,
which reads the inherited empty set, so its resume snapshot records zero
frame boxes while the decoder expects the full liveness-derived set.

When the parent walk carries an empty set, recompute it fresh via
`collect_outer_active_boxes` from `FULL_BODY_SNAPSHOT_SYM` at the
CALL-site py_pc, mirroring `try_walker_list_append_inline`.

Fixes the wasm `inlined_mutation_before_abort` failure and removes two
decode-tag/OOB crash variants in the gh#420 inline-callee benches.

Assisted-by: Claude
…i-frame snapshot

`try_walker_inline_user_call` computed a paused caller frame only on the
branch multiframe path (`try_multiframe`); a STRICT straight-line callee
pushed no parent frame, so its in-callee guards took the single-frame
collapse (resume at the caller CALL boundary via
`INLINE_SUBWALK_CAPTURE_BOUNDARY`).

Under `PYRE_FBW_STRICT_MULTIFRAME` (default on) the strict path now also
computes the paused caller frame via `compute_inline_caller_frame`, so an
in-callee guard captures through `walker_capture_multi_frame_inline_snapshot`
and resumes at the callee's own coordinate. When the callee frame is not
snapshot-able (a kept operand-stack temp the callee sub-walk does not
mirror) the multi-frame capture returns `Unsupported`, and the inline
declines to a residual call instead of the single-frame collapse — whose
caller-boundary re-execute mis-sizes the resumed frame (a `LOAD_FAST` push
past the frame array / `rd_numb` decode overrun) on the wasm resume path.

wasm synth/inline_callee_constructs_object no longer panics
(`pyframe.rs` index-out-of-bounds). dynasm 146/146, cranelift 147/147,
wasm 144/146 (2 pre-existing residual-side-effect / closure-cell resume
failures unaffected).

Assisted-by: Claude
The `map` field descr hardcoded field_size 8. `map` is a `*const`
pointer erased to an opaque Int word; on wasm32 it is 4 bytes. Reading
it as 8 bytes folded the adjacent `storage` pointer into the high half,
so the LOAD_ATTR/STORE_ATTR `guard_value(map)` compared a garbage-high
value against the clean map constant and failed every iteration. The
deopt re-executed the already-committed body, so the wasm backend
over-counted `inlined_helper_mutation`'s `c.n` by `compiles - 1`.

Use `size_of::<usize>()` so the field is one machine word: 4 on wasm32,
8 on 64-bit (native descr unchanged, so dynasm/cranelift are unaffected).

Assisted-by: Claude
…e CALL op

try_walker_inline_user_call's fresh outer_active_boxes path passed
ctx.outer_jitcode_index and ctx.entry_py_pc to collect_outer_active_boxes.
Under dispatch_via_miframe those are the sentinel 0 and the walk-entry
py_pc, not the CALL site, so a CALL in a non-root jitcode or not at the
walk-entry pc selected the wrong liveness window and the callee guard
snapshot could encode the wrong frame boxes.

Derive (jitcode index, CALL-site py_pc) from the snapshot sym's jitcode at
op.pc, matching orthodox_list_append_commit (python_pc_for_jitcode_pc plus
skip_python_trivia_forward normalization), guarded on non-null sym.jitcode.

Assisted-by: Claude
The gh#420 strict-multiframe path pushed a paused caller frame for every
STRICT straight-line inlined callee, routing its in-callee guards through
walker_capture_multi_frame_inline_snapshot. A strict callee seeds no
callee frame red, so that snapshot always returns Unsupported and the
inline declines. For a callee whose declining guard is an arithmetic
overflow guard (after_residual=false, captured via `?`) the Unsupported
propagates out of the sub-walk and aborts the whole loop trace, so a
pure-leaf callee (inline_helper's `a + b` / `a * b`) that compiled fine
via the single-frame collapse on main now aborts (loops_compiled=0
loops_aborted=25) and the interpreter falls back over the bench timeout.

Gate the caller-frame push on strict_callee_collapse_unsound: push (and
thus decline to a residual) only when the callee body commits a heap side
effect the collapse re-execute would double — a general ref-arg residual
call / constructor (residual_call_r_*), a void side-effect residual call
(residual_call_*_v), or a setfield / setarrayitem write. A pure
value-returning leaf has an idempotent collapse and is left on it.

inline_callee_constructs_object (P(n) construct) and
inlined_mutation_before_abort (c.n = c.n + 1 store) still take the decline;
inline_helper compiles again. dynasm/cranelift/wasm each 145/145 synthetic;
inline_helper 985375007 in ~0.2s on all three.

Assisted-by: Claude
…inate

Two follow-ups on the strict straight-line inline multiframe path.

Depth bound: the strict-MF caller-frame push had no `inline_depth <
FBW_MAX_MULTIFRAME_DEPTH` gate, unlike `try_multiframe`. A side-effecting
strict callee nested inside another pushed a second `InlineParentFrame`
(n_parents == n_callees == 2), producing a three-frame snapshot on a
resume path sound for only one paused caller frame. Gate it the same as
`try_multiframe`; a nested side-effecting strict callee then falls back to
the single-frame collapse.

Snapshot coordinate: the fresh-collect path computed `outer_active_boxes`
at the CALL-site coordinate but the sub-walk still stamped
`entry_py_pc`/`outer_jitcode_index` from the `ctx` sentinels
(`dispatch_via_miframe` walk-entry pc / index 0). The decoder sizes and
places the stored boxes from the liveness window at the snapshot header
coordinate, so a CALL later in the function or in a nonzero jitcode
desynced the header window from the collected boxes. Carry the CALL-site
coordinate to the header alongside the boxes.

check.py: dynasm/cranelift/wasm each 159/159.

Assisted-by: Claude
`strict_callee_collapse_unsound` matched `setfield`/`setarrayitem` but not
`setinteriorfield_gc_*` (emitted for an array-of-struct / dict-storage
interior write) — `"setinteriorfield"` does not contain the `setfield`
substring. A strict straight-line callee with an interior-field write
followed by a guard therefore stayed on the single-frame collapse, so a
deopt re-executed the callee and applied the interior write twice. Add the
`setinteriorfield` substring to the side-effect set.

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: 0a2ba5d2d5

ℹ️ 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 +11684 to +11685
if op.starts_with("residual_call_r")
|| (op.starts_with("residual_call") && op.ends_with("_v"))

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 Treat mixed-arg residual calls as side effects

When a strict callee contains a side-effecting mixed int/ref residual such as residual_call_ir_r (for example a BINARY_SUBSCR/attribute helper with an int tag that can run arbitrary Python code), this predicate returns false because it only treats residual_call_r* and void residuals as unsafe. That leaves the callee on the single-frame collapse path, so a later guard failure resumes by re-executing the whole call after the helper's side effect already ran, duplicating the mutation this gate is meant to avoid.

Useful? React with 👍 / 👎.

// n_callees == 2), a 3-frame snapshot the resume path is sound for only
// one paused caller frame. A nested side-effecting strict callee then
// falls back to the single-frame collapse (its pre-strict-MF behavior).
compute_inline_caller_frame(ctx, op.pc)

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 Decline strict side-effect callees without a parent frame

When compute_inline_caller_frame returns None for a side-effecting strict callee (e.g. the CALL is inside a try block, which this helper explicitly declines because of the catch-marker resume pc), this branch still inlines with no InlineParentFrame. Guards inside the callee then fall back to the old single-frame collapse and re-execute the whole call on deopt, so the committed store/residual effect is applied twice; for callees that strict_callee_collapse_unsound identifies as unsafe, a missing caller frame should decline/abort rather than keep the collapse.

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: 1

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

Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch.rs`:
- Around line 12904-12962: Nested inline calls can re-enter the full-body
recompute path with a callee pc while FULL_BODY_SNAPSHOT_SYM still refers to the
outer jitcode, causing the wrong liveness window and boxes to be collected.
Update the branch in jitcode_dispatch.rs that computes inline_outer_active_boxes
so it only recomputes for the outer full-body walk, using a guard tied to op.pc
and sym.jitcode (or an equivalent sub-walk check) before calling
python_pc_for_jitcode_pc and collect_outer_active_boxes; otherwise fall back to
the existing ctx.outer_active_boxes/ctx.entry_py_pc/ctx.outer_jitcode_index
values.
🪄 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

Run ID: a8631879-a574-4e65-b399-096abf0b5cce

📥 Commits

Reviewing files that changed from the base of the PR and between 650d378 and 0a2ba5d.

📒 Files selected for processing (2)
  • pyre/pyre-jit-trace/src/descr.rs
  • pyre/pyre-jit-trace/src/jitcode_dispatch.rs

Comment on lines +12904 to +12962
let (inline_outer_active_boxes, inline_outer_py_pc, inline_outer_jc_index) =
if ctx.is_full_body_walk && ctx.outer_active_boxes.is_empty() {
let sym_ptr = FULL_BODY_SNAPSHOT_SYM.with(|c| c.get());
if sym_ptr.is_null() {
(
ctx.outer_active_boxes.clone(),
ctx.entry_py_pc,
ctx.outer_jitcode_index,
)
} else {
let sym = unsafe { &*sym_ptr };
if sym.jitcode.is_null() {
(
ctx.outer_active_boxes.clone(),
ctx.entry_py_pc,
ctx.outer_jitcode_index,
)
} else {
// Liveness coordinate is the CALL op's own (jitcode index,
// py_pc) — NOT the `ctx` sentinels. `dispatch_via_miframe`
// initializes `ctx.outer_jitcode_index` to 0 and
// `ctx.entry_py_pc` to the walk-entry py_pc, so for a CALL in
// a non-root jitcode, or a CALL not at the walk-entry pc,
// those select the wrong liveness window and the callee guard
// snapshot encodes the wrong frame boxes. Derive the
// coordinate from the snapshot sym's jitcode at the CALL op's
// pc, matching `orthodox_list_append_commit`.
let (call_site_py_pc, call_site_jc_index) = unsafe {
let jc = &*sym.jitcode;
let jc_index = jc.index as u32;
let mut py = python_pc_for_jitcode_pc(&jc.payload.metadata, op.pc);
if !jc.payload.code_ptr.is_null() {
let codeobj = &*jc.payload.code_ptr;
py = skip_python_trivia_forward(codeobj, py as usize) as u32;
}
(py, jc_index)
};
let boxes = collect_outer_active_boxes(
sym,
ctx.trace_ctx,
ctx.registers_i,
ctx.registers_r,
ctx.registers_f,
call_site_jc_index,
call_site_py_pc,
None,
majit_ir::resumedata::NO_JITCODE_PC,
None,
);
(boxes, call_site_py_pc, call_site_jc_index)
}
}
} else {
(
ctx.outer_active_boxes.clone(),
ctx.entry_py_pc,
ctx.outer_jitcode_index,
)
};

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 | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Trace how outer_active_boxes flows into nested inline sub-walks and whether
# empty results are reachable, plus how op.pc relates to FULL_BODY_SNAPSHOT_SYM.
rg -nP --type=rust -C3 '\bfn collect_outer_active_boxes\b' pyre/pyre-jit-trace/src
rg -nP --type=rust -C3 'is_full_body_walk\s*:' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
rg -nP --type=rust -C3 'FULL_BODY_SNAPSHOT_SYM' pyre/pyre-jit-trace/src/jitcode_dispatch.rs

Repository: youknowone/pyre

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Small, targeted reads around the relevant logic and contracts.
sed -n '6488,6625p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
printf '\n---\n'
sed -n '12900,13020p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
printf '\n---\n'
sed -n '6866,6888p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
printf '\n---\n'
sed -n '6932,6942p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
printf '\n---\n'
sed -n '8866,8880p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs

Repository: youknowone/pyre

Length of output: 16522


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check whether empty outer_active_boxes is a supported outcome and how the
# branch is guarded at its call sites.
sed -n '6625,6765p' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
printf '\n---\n'
rg -n -C3 'outer_active_boxes\.is_empty\(\)|outer_active_boxes: inline_outer_active_boxes|collect_outer_active_boxes\(' pyre/pyre-jit-trace/src/jitcode_dispatch.rs
printf '\n---\n'
rg -n -C3 'INLINE_SUBWALK_CAPTURE_BOUNDARY|FBW_ABORT_IN_SUBWALK|nested self-call|sub-walk' pyre/pyre-jit-trace/src/jitcode_dispatch.rs

Repository: youknowone/pyre

Length of output: 50371


Gate the recompute to the outer full-body walk

Nested inline calls inherit ctx.is_full_body_walk, so a callee can re-enter this branch after the caller’s collect_outer_active_boxes() returned empty. At that point op.pc is a callee coordinate, but FULL_BODY_SNAPSHOT_SYM still points at the outer jitcode, so python_pc_for_jitcode_pc(...) can pick the wrong liveness window and seed the callee with bad boxes. Add a guard so this path only runs when op.pc belongs to sym.jitcode, or skip the recompute for nested sub-walks.

🤖 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/jitcode_dispatch.rs` around lines 12904 - 12962,
Nested inline calls can re-enter the full-body recompute path with a callee pc
while FULL_BODY_SNAPSHOT_SYM still refers to the outer jitcode, causing the
wrong liveness window and boxes to be collected. Update the branch in
jitcode_dispatch.rs that computes inline_outer_active_boxes so it only
recomputes for the outer full-body walk, using a guard tied to op.pc and
sym.jitcode (or an equivalent sub-walk check) before calling
python_pc_for_jitcode_pc and collect_outer_active_boxes; otherwise fall back to
the existing ctx.outer_active_boxes/ctx.entry_py_pc/ctx.outer_jitcode_index
values.

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.

wasm JIT: inline callee guard snapshot empty + resume side-effect duplication (4 wasm-only failures)

1 participant