jit(wasm): panic on missing pool-indexed const at emit time instead of emitting 0 - #691
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughWASM codegen now strictly resolves constants and layout descriptors, rejects unsupported lowering paths, corrects guard addressing and finish metadata, and implements overflow tracking for integer operations. A synthetic benchmark exercises overflow-sensitive multiplication and big-integer promotion. ChangesWASM codegen correctness
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit ae979ee). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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-backend-wasm/src/codegen.rs`:
- Line 4434: Re-extract the corresponding Charon .ullbc files with
scripts/extract-llbc.py after the JIT changes, rebuild the rtyper prepass, and
run all eight benchmarks before committing to verify there are no regressions.
🪄 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: f7f0159a-9fb8-4318-8043-f48a171070ea
📒 Files selected for processing (1)
majit/majit-backend-wasm/src/codegen.rs
| Vec::new() | ||
| } | ||
|
|
||
| /// A legacy pool-indexed const that is absent from the constants pool at emit |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial
Re-extract LLBC and run benchmarks after JIT changes.
As per coding guidelines, since this modifies JIT source, please ensure you re-extract the corresponding Charon .ullbc files using scripts/extract-llbc.py before rebuilding the rtyper prepass, and run all eight benchmarks to verify there are no regressions prior to committing.
🤖 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-backend-wasm/src/codegen.rs` at line 4434, Re-extract the
corresponding Charon .ullbc files with scripts/extract-llbc.py after the JIT
changes, rebuild the rtyper prepass, and run all eight benchmarks before
committing to verify there are no regressions.
Source: Coding guidelines
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edb3625c95
ℹ️ 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".
| OpCode::Strlen | ||
| | OpCode::Unicodelen | ||
| | OpCode::Strgetitem | ||
| | OpCode::Unicodegetitem => { |
There was a problem hiding this comment.
When optimized traces still contain residual string operations, this arm now aborts wasm compilation instead of emitting descriptor-driven loads. These opcodes are not impossible: OptString emits residual Strgetitem/Unicodegetitem when it cannot fold a character (majit/majit-metainterp/src/optimizeopt/vstring.rs:771-779), and the cranelift backend compiles the same string length/getitem opcodes from their ArrayDescr (majit/majit-backend-cranelift/src/compiler.rs:12892-12913, 12948-12975). In those cases hot string/unicode loops on wasm fall back/retry rather than compile; please lower them via the descriptors instead of unconditionally returning Unsupported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06554bf6e8
ℹ️ 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".
| | OpCode::GcLoadIndexedI | ||
| | OpCode::GcLoadIndexedR | ||
| | OpCode::GcLoadIndexedF | ||
| | OpCode::GcStore | ||
| | OpCode::GcStoreIndexed => { |
There was a problem hiding this comment.
These indexed GC ops are not GC-rewrite-only: the translated bytecode table wires gc_load_indexed_i/f and gc_store_indexed_i/f as real blackhole instructions (majit/majit-metainterp/src/blackhole.rs:7840-7843), and another backend lowers GcLoadIndexed* directly (majit/majit-backend-cranelift/src/compiler.rs:12452-12478). When a frontend llop/buffer trace containing one of these opcodes reaches wasm, this new arm now panics during compilation instead of declining the trace with BackendError::Unsupported, preventing the normal interpreter fallback path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cd137f80b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let num_bytes = const_operand_value(constants, arg1).unwrap_or_else(|| { | ||
| panic!( | ||
| "wasm int_signext: num_bytes operand (raw={}) is not a \ | ||
| resolvable compile-time constant", | ||
| arg1.raw() |
There was a problem hiding this comment.
When IntSignext reaches wasm with arg(1) produced by another op/input instead of a constant, this new panic! aborts compilation rather than lowering the dynamic shift or returning BackendError::Unsupported for interpreter fallback. The IR/blackhole path does not make the width a compile-time-only operand (int_signext/ii>i is wired in majit/majit-metainterp/src/blackhole.rs:7544-7545), and another backend resolves op.arg(1) as a normal operand in majit/majit-backend-cranelift/src/compiler.rs:14082-14085, so such traces are valid inputs to backend codegen.
Useful? React with 👍 / 👎.
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-backend-wasm/src/codegen.rs`:
- Around line 2106-2135: Centralize the overflow-flag local index used by the
GuardNoOverflow, GuardOverflow, and emit_ovf_binop paths. Add an
ovf_flag_local(num_vars: u32) helper near the existing codegen helpers and
replace each direct num_vars + UMULHI_SCRATCH + 1 calculation with calls to it,
preserving the current overflow guard behavior.
🪄 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: 2324459f-41c9-4217-825a-31817c632bf5
📒 Files selected for processing (3)
majit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/lib.rspyre/bench/synth/int_mul_ovf_bignum_promote.py
2d3ac4e to
f1f86fa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1f86fa382
ℹ️ 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".
| emit_sized_int_load( | ||
| &mut sink, | ||
| offset2 as u64, | ||
| std::mem::size_of::<usize>(), | ||
| true, |
There was a problem hiding this comment.
On wasm32 this emits a 4-byte load because size_of::<usize>() is 4, but the runtime layouts read by this guard are PyType::subclassrange_min / ClassTypeLayout::subclassrange_min, both 8-byte i64 fields. If a range value ever needs the upper 32 bits, GuardSubclass truncates/sign-extends the object's min and can accept or reject the guard incorrectly; use the actual field width for both this vtable path and the gcremovetypeptr path below.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
…no clean finish loop_finish_fi is the clean-DoneWithThisFrame fail_index published into the CALL_ASSEMBLER dispatch entry; the CA arm treats a callee whose returned fail_index equals it as a clean finish and reads the result Ref from the output slot, else deopts. When a compiled loop has no clean finish descr the fallback published 0, but 0 is a valid fail_index (the first descr registered) and the declared "none" sentinel is WASM_CA_FINISH_FI_UNKNOWN (u32::MAX, the value the pending dispatch entry is initialized to). Publishing 0 could make a callee's fail_index-0 guard exit be misread as a clean finish and read a wrong result. Use the sentinel; no real fail_index equals it, so the case deopts correctly. check.py --backend wasm: 240/240. Assisted-by: Claude
…g hardcoded read strlen/unicodelen/strgetitem/unicodegetitem were lowered with a hardcoded layout: the length as an 8-byte i64_load of a 4-byte word field (folding garbage into the high bits), and the item as a 1-byte stride-1 read at a fixed offset — wrong for UNICODE, whose code units are 4 bytes at stride 4. On wasm this is a silent wrong value (offset is valid linear memory, no trap). pyre models strings/unicode as Array(Char) and routes these through the descr-driven GETARRAYITEM/ARRAYLEN paths, so no producer emits these ops; decline them (interpreter fallback) like the interior-field / GC-load arms. check.py --backend wasm: 240/240. Assisted-by: Claude
…bytes Both GuardSubclass arms read the word-sized (lltype.Signed) subclassrange_min field with i64_load (8 bytes), folding the adjacent subclassrange_max into the high 32 bits. On wasm32 the field is 4 bytes, so the guard's unsigned range check (loc_tmp - check_min <u check_max - check_min) sees a huge value and reliably fails, forcing a deopt (correct but unaccelerated). Load it at word width via emit_sized_int_load (4-byte signed on wasm32), matching the width-correct ArraylenGc sibling. The vtable-pointer and GC-header i64_loads in the same arm are unchanged (the former is immediately i32-wrapped; the latter is a genuine 8-byte header). check.py --backend wasm: 240/240 (GuardSubclass is latent, not in the corpus). Assisted-by: Claude
… clean finish bridge_finish_fi is the CA-emitting trace's own clean DoneWithThisFrame global fail_index; the CALL_ASSEMBLER arm treats a callee whose returned fail_index equals it (or the callee's published loop_finish_fi) as a clean finish and reads the result slot, else routes to wasm_ca_resume_deopt. g.fail_index is already base-offset into the global fail-index space, so the .unwrap_or(0) fallback for a trace with no non-exception finish is global index 0 — a valid first-registered descr. A callee exiting through global fail_index 0 would then be misclassified as a clean finish, dropping a guard deopt or an exception. Use WASM_CA_FINISH_FI_UNKNOWN (u32::MAX, bakes as i32 -1, never matches a real index), mirroring the loop_finish_fi fix. check.py --backend wasm: 240/240. Assisted-by: Claude
GUARD_VALUE checks whether a runtime value equals its promoted constant. The Value/Const equality contract (value.rs Value::eq, history.py same_constant) compares floats by to_bits() — so 0.0 != -0.0 and NaN == same-bit NaN — and the dynasm/cranelift backends implement it as an integer bit-compare. The wasm arm's float branch used IEEE f64.ne, which passes -0.0 == +0.0 (a guard the reference fails): a runtime -0.0 promoted against a recorded +0.0 keeps running the trace and const-folds to +0.0, corrupting any sign-of-zero-observable result. It also fails NaN == same-bit NaN, spuriously deopting. emit_resolve reinterprets an F64 local to its i64 bits, so the int path's i64_ne is the correct compare for both; remove the float special-case. check.py --backend wasm: 240/240. Assisted-by: Claude
…pile diagnostics On a successful run the wasmtime Store/Module/Engine are dropped at process exit, munmapping ~40MB of compiled code and guest linear memory the OS reclaims anyway; on a short run that teardown is ~0.2s, larger than the cwasm load and far larger than trace compilation. Exit via process::exit on the success path after stdout/profiler/stats are flushed; PYRE_WASM_FULL_TEARDOWN=1 restores the drops for leak checks. Add PYRE_WASM_STARTUP_TRACE (engine_new/load_module/instantiate/ run_python/total laps) and compile_ms (total Module::new time) in PYRE_WASM_JIT_STATS to attribute the fixed startup cost. Assisted-by: Claude
Exercises an overflow-crossing int multiply on a JIT-hot path: the loop is traced while the product fits an i64 (GUARD_NO_OVERFLOW passes), then a large operand overflows and must promote to a big int. A backend that drops the overflow check silently wraps instead of promoting. Assisted-by: Claude
…cording A bridge/retrace recording walk runs the range FOR_ITER as an authoritative eager-executor: it advances the real iterator and captures the consumed item in-flight. The advance is left unjournaled because the main walk recovers the item via in-flight forward-delivery on abort. The bridge/retrace abort path has no such delivery, so a recording that did not commit left the iterator advanced while the item was never appended, and the interpreter resume continued past it (dropping one comprehension element, or corrupting a returned list). Journal the pre-advance cursor while is_bridge_trace and restore it via w_range_iter_set_cursor when the walk does not commit, so the aborted recording is side-effect neutral and the resume re-consumes the item exactly once. A committed bridge keeps the advance (the authoritative continuation). Gated to is_bridge_trace, so the main walk's forward-delivery path is untouched. Assisted-by: Claude
…ack slot collect_call_stack_overrides rebuilds the caller operand stack for the gh#467 nested-inline-decline forward-flush. The live vstack/color sources emit genuine null-or-self sentinels as explicit null Refs; the shadow fallback then pushed every remaining slot as an override, including ones the walk could not resolve (a LOAD_CONST operand whose concrete Ref was never mirrored) which resolved to a null Ref. The flush override validation only checked that an override existed, not that it was non-null, so it committed the null into the caller frame operand stack. Re-executing the flushed CALL then bound a NULL argument, raising UnboundLocalError on the callee parameter. Push only non-null values from the shadow fallback. An unresolved slot is left absent, so the outer-call flush validation declines and the legacy replay rebuilds the frame from its start state. Add pyre/bench/synth/comprehension_param_range_call_flush.py covering the module-scope hot-loop shape that triggers the flush. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae979eec0b
ℹ️ 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".
| // A bridge/retrace recording walk has no in-flight forward-delivery on | ||
| // abort, so journal the pre-advance cursor for restore if the walk does | ||
| // not commit (keeps the aborted recording side-effect neutral). | ||
| fbw_bridge_iter_journal_push(iter_obj, concrete_current, concrete_remaining); |
There was a problem hiding this comment.
When this specialization runs inside drive_bridge_carrier_subwalk, ctx.trace_ctx.is_bridge_trace is true, so it records the pre-advance range cursor here, but that subwalk does not return through run_perfn_walk's new epilogue; its failure path only calls fbw_store_journal_rollback() at pyre/pyre-jit-trace/src/trace.rs:1184 and other safe-abort paths reset at trace.rs:1455. For a bridge-carrier subwalk that consumes a range item and then declines, the journal is never rolled back, leaving the live iterator advanced while the interpreter/blackhole replays from the guard and drops that iteration.
Useful? React with 👍 / 👎.
| static FBW_BRIDGE_ITER_JOURNAL: std::cell::RefCell<Vec<(pyre_object::PyObjectRef, i64, i64)>> = | ||
| const { std::cell::RefCell::new(Vec::new()) }; |
There was a problem hiding this comment.
This new TLS stores raw PyObjectRef iterators across the rest of an authoritative bridge walk, but capture_fbw_store_journal_root_area / fbw_store_journal_root_walker_area still visit the existing store/append/sys_exc/foriter journals only, not this one. If a minor collection runs after w_range_iter_next before rollback, the live frame's iterator pointer is forwarded while this journal slot is not, so w_range_iter_set_cursor can write through a stale moved pointer instead of restoring the iterator; add this journal to the root area/walker lifecycle like the other FBW journals.
AGENTS.md reference: AGENTS.md:L153-L155
Useful? React with 👍 / 👎.
…ext/indexed-GC decline, bridge-iter journal root) (#737) * wasm: correct GuardSubclass field width; decline runtime IntSignext and indexed GC ops - GuardSubclass read subclassrange_min at size_of::<usize>() (4 bytes on wasm32); PyType::subclassrange_min is AtomicI64, so read 8 bytes on both the vtable and gcremovetypeptr paths. - IntSignext with a non-constant num_bytes operand aborted via panic; return BackendError::Unsupported so the trace declines to interpreter fallback. - gc_load_indexed_*/gc_store_indexed_* are frontend blackhole ops that can reach the backend; return Unsupported for the indexed forms instead of panicking. The bare GcLoad*/GcStore GC-rewrite forms keep the panic. Assisted-by: Claude * jit: root the FBW bridge iterator cursor journal FBW_BRIDGE_ITER_JOURNAL stores range-iterator refs across an authoritative bridge walk but was visited by no root walker, unlike the five sibling FBW journals. Add it to FbwStoreJournalRootArea and fbw_store_journal_root_walker_ area so a minor collection forwards the iterator before the non-commit rollback restores its cursor via w_range_iter_set_cursor. Assisted-by: Claude * check.py: report exec times and true ratio in perf-gate FAIL lines The FAIL line printed raw run times and the gate threshold formatted as if it were the measured ratio, so the numbers were not self-consistent. Add _gate_fail_detail to print the startup-subtracted exec times the gate actually compared, their ratio, and the threshold. Assisted-by: Claude * wasm: add unicode str-subscript regression bench; document string-op decline as verified inert - bench/synth/str_getitem_len_hot.py: hot str/unicode subscript and len over ASCII/latin1/BMP/astral strings (item_size 1/2/4), routed through the GETARRAYITEM/ARRAYLEN paths; output asserted cpython==pypy. - codegen.rs: a str-subscript / len / compare / find hot loop traces to GETARRAYITEM, never STRGETITEM (verified with PYRE_DUMP_PERFN_JITCODE), so the STRGETITEM/UNICODEGETITEM/STRLEN/UNICODELEN decline covers ops no trace emits; note this so the decline is not mistaken for a missing descr-driven lowering. Assisted-by: Claude * dynasm: materialize wide binop immediates through the scratch register emit_binop_reg_loc's Loc::Immed arm truncated the value with `as i32`, encoding an out-of-i32-range immediate as a sign-extended imm32 — `x & 0xFFFF_FFFF_FFFF` degenerated to `x & -1` in compiled code, the wrong output of synth/str_getitem_len_hot on dynasm. Follow regloc.py:456-464: mov the value into X86_64_SCRATCH_REG and retry the reg-reg form. The IntAdd LEA emitter gets the same fallback for its immediate arm, which the consider_binop_symm path reaches with an arbitrary 64-bit constant; its two symmetric arms are merged. Two backend tests compile and execute AND/ADD with wide immediates. * jit: expose pypyjit.set_param for runtime JIT-parameter control Register a pypyjit module whose set_param accepts the positional-string form ("name=value,…", "off", "default") and keyword arguments, routing both through the JIT's set_user_param parser. pyre-interpreter cannot import pyre-jit, so add a SET_JIT_PARAM_STRING_HOOK alongside the existing per-pair SET_JIT_PARAM_HOOK; pyre-jit registers set_jit_param_string_via_warmstate at boot and per-eval. The hook is an in-process function pointer, so a pypyjit.set_param call configures the warmstate on every backend including the wasm guest, which sees no environment. * bench: add threshold-1 JIT-stress twins of the exception recording benches exception_metadata_jitstress and exception_reraise_tb_depth_jitstress call pypyjit.set_param("threshold=1,function_threshold=1") so trace recording fires on the earliest iterations of every section rather than after the ~1600-iteration warmup. Recording then lands on the traceback/context/ exc_info/reraise shapes on every run and every backend, making coverage of the recording path deterministic instead of dependent on which iteration a warmup pass happens to hit. The import is guarded so the benches run unchanged under CPython, which has no pypyjit. Output matches the natural-threshold twins. * bench: add bare/named/finally re-raise traceback-depth regression bench A module-level hot loop executes bare re-raise (depth 2), named re-raise (depth 3), and finally-passthrough (depth 2) so the recording iteration itself runs the re-raise chain. Guards the instruction-keyed traceback recording against spurious nodes at re-raise / handler-cleanup coordinates.
1 participant
What
emit_resolve,emit_resolve_f64, andconst_operand_valuein the wasm backend resolved a legacy pool-indexed const that was absent from the constants pool to a silent0(a nullRef). This replaces thatunwrap_or(0)with a loud panic via a sharedresolve_const_bitshelper (missing_emit_const).Why
collect_constants_from_opsalready panics on exactly this condition (missing_legacy_const), with the comment "never register a placeholder0— that would emit the constant as zero." The emit path silently did the opposite. This is a wasm-specific hazard: on native a nullReftraps on first dereference, but wasm offset 0 is valid linear memory, so a silent0is read as garbage and miscompiles quietly instead of crashing. The emit path now matches the collection path — a missing pool const fails loud (naming the rawOpRef) rather than becoming a silent wrong answer.Verification
cargo check -p majit-backend-wasmclean.check.py --backend wasm --synthetic-only: wasm 230/230 ALL PASSED — the new panic never fires on any valid trace (consistent withcollect_constants_from_opsguaranteeing every referenced legacy const is seeded), so this is behavior-preserving for correct traces.Turns any future (or latent) missing-const regression into a loud, debuggable failure instead of a silent wasm miscompile.
Assisted-by: Claude
Summary by CodeRabbit
Bug Fixes
Benchmarks