Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions majit/majit-backend-wasm/src/codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3068,8 +3068,46 @@ fn build_function(
}

// ── Exception handling ──
OpCode::SaveException | OpCode::SaveExcClass | OpCode::RestoreException => {
// No-op in wasm MVP — exception state is managed by the host.
OpCode::SaveException => {
// x86/assembler.py:1820-1821 genop_save_exception:
// _store_and_reset_exception → resloc = [pos_exc_value];
// [pos_exception] = 0; [pos_exc_value] = 0.
// The result is the caught exception the resumed handler reads,
// so it must be written even though the slots themselves are
// shared with the host: skipping the op leaves the local null.
let vi = op.pos.get().raw();
if !OpRef::raw_is_constant(vi) {
sink.i32_const(crate::jit_exc_value_addr() as i32);
sink.i64_load(mem64(0));
sink.local_set(1 + vi);
}
sink.i32_const(crate::jit_exc_type_addr() as i32);
sink.i64_const(0);
sink.i64_store(mem64(0));
sink.i32_const(crate::jit_exc_value_addr() as i32);
sink.i64_const(0);
sink.i64_store(mem64(0));
}
OpCode::SaveExcClass => {
// x86/assembler.py:1817-1818 genop_save_exc_class:
// MOV resloc, [pos_exception]
let vi = op.pos.get().raw();
if !OpRef::raw_is_constant(vi) {
sink.i32_const(crate::jit_exc_type_addr() as i32);
sink.i64_load(mem64(0));
sink.local_set(1 + vi);
}
}
OpCode::RestoreException => {
// x86/assembler.py:1845-1850 _restore_exception:
// MOV [pos_exc_value], excvalloc
// MOV [pos_exception], exctploc
sink.i32_const(crate::jit_exc_value_addr() as i32);
emit_resolve(&mut sink, constants, value_types, op.arg(1).to_opref());
sink.i64_store(mem64(0));
sink.i32_const(crate::jit_exc_type_addr() as i32);
emit_resolve(&mut sink, constants, value_types, op.arg(0).to_opref());
sink.i64_store(mem64(0));
}

// ── Conditional calls ──
Expand Down
29 changes: 27 additions & 2 deletions majit/majit-backend-wasm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,25 @@ use std::sync::atomic::{AtomicI64, AtomicU64, Ordering};
/// `wasm_unsupported_trace_reason` (the #62 loop-callee CALL_ASSEMBLER gap),
/// 26 = the wasm host rejected the emitted module (`func_handle == 0`).
/// Indices 2 and 4 double as compile_loop's other two declines.
pub static BRIDGE_DIAG: [AtomicU64; 27] = {
///
/// 27-28 ask 12-13's question of EVERY accepted bridge rather than only the
/// CALL_ASSEMBLER ones: 27 = the source guard's dispatch cell was written, so
/// the loop epilogue now tail-calls this bridge in-module; 28 = it was not,
/// because the owning trace reserved no cell array, so the guard keeps
/// round-tripping to the host and the bridge is compiled but unreachable.
/// `BRIDGE_OK` (5) only says the backend accepted a bridge, which is strictly
/// weaker than "the guard can reach it"; without this split the two are
/// indistinguishable from outside the guest.
///
/// 29 = the cell written at 27 was ALREADY non-zero, i.e. this guard had a
/// reachable bridge and kept failing anyway. One of those is ordinary (a guard
/// re-bridged after its first bridge was outgrown); a count that tracks
/// `BRIDGE_OK` says the epilogue dispatch is not taking the cell at all and
/// every bridge after the first is dead weight.
pub static BRIDGE_DIAG: [AtomicU64; 30] = {
const Z: AtomicU64 = AtomicU64::new(0);
[
Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z,
Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z, Z,
]
};

Expand Down Expand Up @@ -2842,11 +2857,21 @@ impl majit_backend::Backend for WasmBackend {
diag_bump(13);
}
}
// The same question for every accepted bridge (slots 27/28): a bridge
// whose source guard has no cell is compiled and then unreachable.
if source_cells_base != 0 && bridge_slot != 0 {
diag_bump(27);
} else {
diag_bump(28);
}
#[cfg(all(target_arch = "wasm32", not(target_os = "wasi")))]
if source_cells_base != 0 && bridge_slot != 0 {
// cells[source_fail_index] = bridge_slot — the loop epilogue now
// tails into this bridge instead of returning to the host.
let cell = (source_cells_base as usize + source_fail_index as usize * 4) as *mut u32;
if unsafe { core::ptr::read(cell) } != 0 {
diag_bump(29); // this guard already had a reachable bridge
}
unsafe {
core::ptr::write(cell, bridge_slot);
}
Expand Down
13 changes: 10 additions & 3 deletions majit/majit-metainterp/src/pyjitpl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9468,7 +9468,12 @@ impl<M: Clone> MetaInterp<M> {
let exc_class = if result.exception_value.is_null() {
0
} else {
unsafe { *(result.exception_value.0 as *const i64) }
// `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 }
Comment on lines +9471 to +9476

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

};
let exception = ExceptionState {
exc_class,
Expand Down Expand Up @@ -9638,7 +9643,8 @@ impl<M: Clone> MetaInterp<M> {
let exc_class = if exc_value_ref.is_null() {
0
} else {
unsafe { *(exc_value_ref.0 as *const i64) }
// Pointer-width read — see the `result.exception_value` site.
unsafe { *(exc_value_ref.0 as *const usize) as i64 }
};
let exception = ExceptionState {
exc_class,
Expand Down Expand Up @@ -9816,7 +9822,8 @@ impl<M: Clone> MetaInterp<M> {
let exc_class = if exc_value_ref.is_null() {
0
} else {
unsafe { *(exc_value_ref.0 as *const i64) }
// Pointer-width read — see the `result.exception_value` site.
unsafe { *(exc_value_ref.0 as *const usize) as i64 }
};
let exception = ExceptionState {
exc_class,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=2
bridges_compiled=3
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=23748
guard_failures=613
internal_compile_panics=0
loops_aborted=1
loops_aborted=0
loops_compiled=2
6 changes: 3 additions & 3 deletions pyre/bench/synth/handler_reraise_second_exc.wasm.jitstats
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=2
bridges_compiled=4
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=1285
guard_failures=804
internal_compile_panics=0
loops_aborted=1
loops_aborted=0
loops_compiled=2
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=3
bridges_compiled=4
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=23950
guard_failures=815
internal_compile_panics=0
loops_aborted=1
loops_aborted=0
loops_compiled=4
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=2
bridges_compiled=3
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=23748
guard_failures=613
internal_compile_panics=0
loops_aborted=1
loops_aborted=0
loops_compiled=2
6 changes: 3 additions & 3 deletions pyre/bench/synth/named_reraise_sibling_hot.wasm.jitstats
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=4
bridges_compiled=7
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=2649
guard_failures=1712
internal_compile_panics=0
loops_aborted=3
loops_aborted=0
loops_compiled=4
6 changes: 3 additions & 3 deletions pyre/bench/synth/sre_pattern_methods.wasm.jitstats
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=7
bridges_compiled=8
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=2536
guard_failures=1671
internal_compile_panics=0
loops_aborted=1
loops_aborted=0
loops_compiled=7
6 changes: 3 additions & 3 deletions pyre/bench/synth/type_name_surrogate_reject.wasm.jitstats
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
bridges_compiled=0
bridges_compiled=1
descr_set_absent=0
descr_set_ambiguous=0
descr_set_stale_absent=0
guard_failures=9464
guard_failures=202
internal_compile_panics=0
loops_aborted=1
loops_aborted=0
loops_compiled=2
6 changes: 5 additions & 1 deletion pyre/pyre-jit-trace/src/jitcode_dispatch/bridge_subwalk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,11 @@ pub fn dispatch_via_miframe<Sym: WalkSym>(
// typeptr at offset 0 (`_store_exception` invariant): the expected class the
// bridge-entry GUARD_EXCEPTION checks the restored pending exception against.
let exc_edge_class = if exc_edge_catch_target.is_some() && !exc_edge_concrete.is_null() {
unsafe { *(exc_edge_concrete as *const i64) }
// One machine word, so read it at pointer width: an i64 read on a
// 32-bit target pulls the adjacent header word into the high half, and
// the guard then compares against a class value the pending-exception
// cell (`jit_exc_raise`, pointer-width) can never hold.
unsafe { *(exc_edge_concrete as *const usize) as i64 }
} else {
0
};
Expand Down
37 changes: 28 additions & 9 deletions pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3107,8 +3107,11 @@ fn finishframe_lookahead_at(code: &[u8], position: usize) -> FinishframeLookahea
// assert arg1 == 1
// cintf.jit_rvmprof_code(arg1, arg2)
// Walker surfaces the operand byte indices for the caller to
// decide whether to symbolically record (today: drop, mirroring
// RPython's non-record direct cintf call).
// decide whether to symbolically record. The `cintf` call itself
// belongs to the frame-popping loop, which is
// `MetaInterp::finishframe_exception` and does perform it; firing
// it from here would mark an enter/leave on a lookahead that pops
// nothing.
let arg1_reg = code[next.pc + 1];
let arg2_reg = code[next.pc + 2];
return FinishframeLookahead::RvmprofCode { arg1_reg, arg2_reg };
Expand All @@ -3120,9 +3123,10 @@ fn finishframe_lookahead_at(code: &[u8], position: usize) -> FinishframeLookahea
/// `try_catch_exception_at(...) -> Option<target>` shape used by
/// existing callers. Returns `Some(target)` only on the
/// `CatchTarget` arm; `RvmprofCode` and `NoMatch` collapse to `None`
/// (both cases continue unwinding from the caller's POV — the
/// instrumentation side effect is dropped today, matching RPython's
/// non-trace-recorded `cintf` call).
/// (both cases continue unwinding from the caller's POV). This is a
/// lookahead predicate, not a frame-popping loop, so it carries no
/// `jit_rvmprof_code` side effect — `MetaInterp::finishframe_exception`
/// is the loop that decodes `rvmprof_code` and calls it.
pub(crate) fn try_catch_exception_at(code: &[u8], position: usize) -> Option<usize> {
match finishframe_lookahead_at(code, position) {
FinishframeLookahead::CatchTarget(target) => Some(target),
Expand All @@ -3133,11 +3137,26 @@ pub(crate) fn try_catch_exception_at(code: &[u8], position: usize) -> Option<usi
/// Exception-edge bridge: route an exception-guard bridge
/// (GUARD_NO_EXCEPTION / GUARD_EXCEPTION) resume to the in-frame `except`
/// handler instead of declining to the blackhole (`call_jit.rs` pending-exc
/// decline). Native backends run the exception-edge bridge unconditionally.
/// The wasm guest's abort-replay exception class (#727) is still open, so it
/// stays off there.
/// decline). Every backend runs it.
///
/// It was off on wasm because the bridge it produced deopted again on its own
/// entry GUARD_EXCEPTION, so the guard failure just moved one chain link deeper
/// on every raising iteration and the chain grew one link per `trace_eagerness`
/// cycle without bound (47 / 97 / 197 bridges at 10k / 20k / 40k iterations of
/// `type_name_surrogate_reject`, with `guard_failures` byte-identical to the
/// declining arm). Two pointer-width reads were the cause: the expected class
/// was read as an i64 out of the exception's one-word `typeptr`, so on a 32-bit
/// target it carried the adjacent header word in its high half and could never
/// equal the pending-exception cell, which `jit_exc_raise` publishes at pointer
/// width. With both reads narrowed and the wasm backend's SAVE_EXCEPTION /
/// SAVE_EXC_CLASS / RESTORE_EXCEPTION lowered instead of skipped, the wasm
/// counters land on the native ones: `type_name_surrogate_reject` 9464 -> 202
/// guard failures against dynasm's 201, `inline_subwalk_property_mutates`
/// 23748 -> 613 against 611, `handler_reraise_second_exc` and
/// `named_reraise_sibling_hot` exactly on dynasm's 804 and 1712, and
/// `loops_aborted` 1 -> 0 on all of them.
pub fn exc_edge_bridge_enabled() -> bool {
cfg!(not(target_arch = "wasm32"))
true
}

/// `PYRE_CARRIER_EXC_RESUME=1` enables the multi-frame (carrier) exception
Expand Down
3 changes: 3 additions & 0 deletions pyre/pyre-wasm-runner/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,9 @@ fn run(module_path: &PathBuf, source: &str, script: &Path) -> Result<i32> {
"cl_ok",
"cl_decl_unsupported",
"cl_decl_host_reject",
"cell_set",
"cell_missing",
"cell_rebridge",
];
let mut parts = Vec::new();
for (i, lbl) in labels.iter().enumerate() {
Expand Down
Loading