jit: pointer-width exception-class reads, the wasm exception triple, and the exception-edge bridge on every backend - #1038
Conversation
…and the exception-edge bridge on every backend
`PyObject` is `{ ob_type, w_class }`, two machine words. Four sites read the
one-word `typeptr` at offset 0 through `*const i64`, so on a 32-bit target the
value carried the adjacent `w_class` in its high half and could never compare
equal to the pending-exception cell, which `jit_exc_raise` publishes at pointer
width. All four now read at pointer width; the reads are byte-identical on
64-bit targets.
The site that reaches generated code is `bridge_subwalk::dispatch_via_miframe`,
whose `exc_edge_class` becomes the bridge-entry GUARD_EXCEPTION constant. On
wasm that guard executed 218889 times and passed 0 times, measured by baking a
`BRIDGE_DIAG` slot address into the emitted guard and storing both compare
operands: runtime `JIT_EXC_TYPE` 0x0000_0000_0083_4658 against a baked
0x00E0_EA40_0083_4658. The three `pyjitpl.rs` sites are the same read; the
third fires once per raising iteration.
The wasm backend no-opped SAVE_EXCEPTION / SAVE_EXC_CLASS / RESTORE_EXCEPTION.
SAVE_EXCEPTION produces a value, and the explicit arm bypassed the
value-producing decline in the `_` fallback, so the local stayed null and the
resumed handler compared against it. Lowered all three against
`x86/assembler.py` genop_save_exc_class, genop_save_exception and
_restore_exception.
With both fixed, `exc_edge_bridge_enabled()` returns `true` for every backend
instead of `cfg!(not(target_arch = "wasm32"))`, and the wasm guard-failure
counts land on the native ones:
fixture wasm before -> after dynasm
type_name_surrogate_reject 9464 -> 202 201
exc_caught_in_callee_return_loop 23748 -> 613 611
inline_subwalk_property_mutates 23748 -> 613 611
inline_subwalk_mutating_residual 23950 -> 815 813
named_reraise_sibling_hot 2649 -> 1712 1712
sre_pattern_methods 2536 -> 1671 1670
handler_reraise_second_exc 1285 -> 804 804
`loops_aborted` goes to 0 on all seven; `type_name_surrogate_reject` also drops
`jit_calls` 103624 -> 2248 and `compile_ms` 76 -> 7.8. The seven `.wasm.jitstats`
baselines are re-recorded.
`BRIDGE_DIAG` grows to 30 slots: `cell_set` / `cell_missing` / `cell_rebridge`
separate "the backend accepted a bridge" from "the source guard's dispatch cell
can reach it", which `BRIDGE_OK` alone does not.
The `if !exc_edge_bridge_enabled()` legacy prologue branch in `call_jit.rs` is
now unreachable and is left in place.
Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change enables wasm exception-edge bridges, implements wasm exception save and restore operations, fixes pointer-width exception-class reads, adds bridge dispatch-cell diagnostics, and updates wasm benchmark statistics. ChangesWasm exception bridge support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 9471-9476: Extract a shared helper in the relevant module that
null-checks a GcRef and reads its typeptr header using pointer width before
widening to i64. Replace the duplicated unsafe reads at the sites around the
exception handling logic, including the code using result.exception_value, with
this helper, preserving each site’s existing null behavior. Ensure the helper is
reusable by the corresponding bridge_subwalk.rs call site.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e38d8738-0baf-4543-acb6-4b7350f961e0
📒 Files selected for processing (13)
majit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-metainterp/src/pyjitpl.rspyre/bench/synth/exc_caught_in_callee_return_loop.wasm.jitstatspyre/bench/synth/handler_reraise_second_exc.wasm.jitstatspyre/bench/synth/inline_subwalk_mutating_residual.wasm.jitstatspyre/bench/synth/inline_subwalk_property_mutates.wasm.jitstatspyre/bench/synth/named_reraise_sibling_hot.wasm.jitstatspyre/bench/synth/sre_pattern_methods.wasm.jitstatspyre/bench/synth/type_name_surrogate_reject.wasm.jitstatspyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-wasm-runner/src/main.rs
| // `typeptr` is one machine word at offset 0, so read it at | ||
| // pointer width: an i64 read on a 32-bit target would pull the | ||
| // adjacent header word into the high half and the class value | ||
| // would never compare equal to a pointer-width one | ||
| // (`Cpu::cls_of_gcref`, `jit_exc_raise`). | ||
| unsafe { *(result.exception_value.0 as *const usize) as i64 } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract a shared helper for the pointer-width exception-class read.
The same pattern repeats at three sites in this file: null-check a GcRef, then read its typeptr header word at pointer width and widen to i64. This is exactly the read that was wrong in four places before this fix (three here, one in bridge_subwalk.rs). A shared helper removes the duplication and prevents a future call site from reintroducing the fixed-width bug by copy-paste.
♻️ Proposed helper extraction
+/// Read a GC object's `typeptr` header word at pointer width (matching
+/// `Cpu::cls_of_gcref` / `jit_exc_raise`). Returns 0 for a null ref.
+fn read_exc_class(gcref: majit_ir::GcRef) -> i64 {
+ if gcref.is_null() {
+ 0
+ } else {
+ unsafe { *(gcref.0 as *const usize) as i64 }
+ }
+}Then each site becomes, e.g.:
- let exc_class = if result.exception_value.is_null() {
- 0
- } else {
- unsafe { *(result.exception_value.0 as *const usize) as i64 }
- };
+ let exc_class = read_exc_class(result.exception_value);Also applies to: 9646-9647, 9825-9826
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 9471 - 9476, Extract a
shared helper in the relevant module that null-checks a GcRef and reads its
typeptr header using pointer width before widening to i64. Replace the
duplicated unsafe reads at the sites around the exception handling logic,
including the code using result.exception_value, with this helper, preserving
each site’s existing null behavior. Ensure the helper is reusable by the
corresponding bridge_subwalk.rs call site.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 654a5e0). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
`try_catch_exception_at` is a lookahead predicate called from the inlining and resume-snapshot walkers; the frame-popping loop that decodes `rvmprof_code` and calls `cintf::jit_rvmprof_code` is `MetaInterp::finishframe_exception`. The comment claimed the dropped call matched a non-trace-recorded upstream `cintf` call, which reads as a divergence from `pyjitpl.py:2547`. Assisted-by: Claude
1 participant
The wasm exception-edge bridge was disabled because the bridge it produced deopted again on its own entry guard. The guard could never pass: the expected class was read out of a one-word
typeptrthrough*const i64.The over-wide read
PyObjectis{ ob_type: *const PyType, w_class: *mut PyObject }. On wasm32 those are 4 bytes each, so ani64read ofob_typereturns(w_class << 32) | ob_type. Measured, by baking aBRIDGE_DIAGslot address into the emitted guard the wayjit_exc_type_addr()is already baked and storing both compare operands:The guard executed 218889 times and passed 0 times.
jit_exc_raisehad already been narrowed to pointer width, and its comment says why ("…so the high bits stay clear andGuardException's type comparison matches the baked class pointer") — the producer side was never matched to it. 64-bit hosts are immune, which is why dynasm and cranelift never saw it.Four sites are fixed. The one that reaches generated code is
bridge_subwalk::dispatch_via_miframe'sexc_edge_class, which becomes the bridge-entryGUARD_EXCEPTIONconstant. The three inpyjitpl.rsare the same read (the third fires once per raising iteration). On 64-bit targets all four are byte-identical before and after.Second blocker, visible only after the first was fixed
SAVE_EXCEPTIONproduces a value — the caught exception the resumed handler reads. The explicit arm bypassed the value-producing decline in the_fallback, so the local stayed 0 and the fixture died withTypeError: comparison on null operandinside theexceptblock. All three are lowered againstx86/assembler.pygenop_save_exc_class,genop_save_exceptionand_restore_exception.Result
exc_edge_bridge_enabled()is nowtruefor every backend instead ofcfg!(not(target_arch = "wasm32")), and wasm converges onto the native counts:type_name_surrogate_rejectalso dropsjit_calls103624 → 2248 andcompile_ms76 → 7.8.Before the fix the cost was unbounded in the iteration count, not a fixed factor: every raising iteration after the guard was blacklisted took a full deopt → blackhole → resume round trip, so
guard_failuresscaled with N while dynasm stayed flat (611 at every N from 15k to 240k).BRIDGE_DIAGgrows to 30 slots —cell_set/cell_missing/cell_rebridge— which separate "the backend accepted a bridge" from "the source guard's dispatch cell can reach it".BRIDGE_OKalone cannot tell those apart from outside the guest, and that distinction is what started this diagnosis.Verification
check.py wasm 373/373, dynasm 377/377, cranelift 377/377;
cargo test --release -p majit-backend-wasm -p majit-metainterp --features dynasmgreen;cargo fmt --checkclean. Seven.wasm.jitstatsbaselines re-recorded.Left in place
exc_edge_bridge_enabled()now returns a constanttrue, so theif !exc_edge_bridge_enabled()legacy prologue branch incall_jit.rsis unreachable. Removing the flag and that branch is not folded in here — it sits inside a cranelift feature gate that this change cannot verify from the wasm side.— authored by Claude
Summary by CodeRabbit
Bug Fixes
Diagnostics