jit(wasm): fix latent CALL_ASSEMBLER terminal-decline pointer miscast - #575
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (2)
WalkthroughThe wasm backend replaces fixed CALL_ASSEMBLER metadata with per-target dispatch state, supports forced terminal-decline regression testing, and adds runtime validation. New benchmarks cover loop and exception-resume behavior, while benchmark timing subtracts measured backend startup overhead. ChangesCALL_ASSEMBLER dispatch flow
Benchmark workloads
Benchmark startup timing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WasmRunner
participant GuestExport
participant WasmBackend
participant DispatchTable
participant CAFrame
WasmRunner->>GuestExport: pass terminal-decline selector
GuestExport->>WasmBackend: set selector
WasmBackend->>DispatchTable: publish or redirect target state
WasmBackend->>CAFrame: marshal recursive CALL_ASSEMBLER live-ins
CAFrame->>DispatchTable: load function, finish index, and compiled pointer
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![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: c9d50d7f1e
ℹ️ 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".
| for artifact in [&dynasm, &wasm_runner, &wasm_module] { | ||
| assert!( | ||
| artifact.exists(), | ||
| "runtime CA regression needs {}; build the requested dynasm and wasm-host artifacts first", | ||
| artifact.display() | ||
| ); |
There was a problem hiding this comment.
In a clean workspace, or any CI job that runs cargo test -p majit-backend-wasm without first building target/release/pyre-dynasm, target/release/pyre-wasm-runner, and the wasm32 module, this unconditional artifact check makes the crate's normal test suite fail before exercising the regression. Please mark this as an ignored/manual runtime test or build the prerequisites in a dedicated harness instead of requiring unrelated release/wasm artifacts to already exist.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit e270c5f). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchUnable to assess: the command runner fails before executing read-only commands ( 2. Other mismatches introduced by this patchUnable to assess for the same environment failure. 3. Pre-existing mismatches (already present before this patch)Unable to assess for the same environment failure. 4. Structural adaptationsUnable to assess for the same environment failure. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/tests/codegen_test.rs`:
- Around line 51-70: Gate the runtime regression tests in
global_reassign_retraces_non_last_label_backedge_at_runtime and the test at the
sibling site in majit/majit-backend-wasm/tests/codegen_test.rs:51-70 and 110-130
from normal cargo test by marking them ignored or requiring an explicit
integration-test flag. For the sibling test, require the wasm-host module
specifically instead of falling back to the plain module, since it lacks the
test-hook export.
In `@pyre/bench/synth/exception_const_operand_resume.py`:
- Line 54: Update the expression assigned to v so the constant operand is
evaluated first: use 7.25 + DATA[j] instead of DATA[j] + 7.25, preserving the
intended pre-exception operand stack state.
- Around line 21-22: Update the relevant DATA subscript expression in
exception_const_operand_resume.py from DATA[j] + 7.25 to 7.25 + DATA[j],
ensuring the constant remains on the stack when the subscript raises IndexError.
🪄 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: 871e9b26-3026-4dd3-8c35-525ed8a4bfb6
📒 Files selected for processing (10)
majit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/failguard.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-backend-wasm/tests/codegen_test.rspyre/bench/ca_terminal_decline.pypyre/bench/synth/exception_const_operand_resume.pypyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
💤 Files with no reviewable changes (2)
- pyre/pyre-jit/src/eval.rs
- pyre/pyre-jit/src/call_jit.rs
| #[test] | ||
| fn global_reassign_retraces_non_last_label_backedge_at_runtime() { | ||
| let root = workspace_root(); | ||
| let dynasm = root.join("target/release/pyre-dynasm"); | ||
| let wasm_runner = root.join("target/release/pyre-wasm-runner"); | ||
| let host_module = root.join("target/wasm32-unknown-unknown/release/pyre_wasm.wasm-host.wasm"); | ||
| let plain_module = root.join("target/wasm32-unknown-unknown/release/pyre_wasm.wasm"); | ||
| let wasm_module = if host_module.exists() { | ||
| host_module | ||
| } else { | ||
| plain_module | ||
| }; | ||
|
|
||
| for artifact in [&dynasm, &wasm_runner, &wasm_module] { | ||
| assert!( | ||
| artifact.exists(), | ||
| "runtime global-reassign regression needs {}; build the requested dynasm and wasm-host artifacts first", | ||
| artifact.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Gate prepared-artifact runtime regressions from normal cargo test.
Both tests require binaries and wasm artifacts that Cargo does not build for this test target.
majit/majit-backend-wasm/tests/codegen_test.rs#L51-L70: mark the runtime test ignored or guard it with an explicit integration-test flag.majit/majit-backend-wasm/tests/codegen_test.rs#L110-L130: apply the same gate and require the wasm-host module; the plain module lacks the test hook export.
📍 Affects 1 file
majit/majit-backend-wasm/tests/codegen_test.rs#L51-L70(this comment)majit/majit-backend-wasm/tests/codegen_test.rs#L110-L130
🤖 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/tests/codegen_test.rs` around lines 51 - 70, Gate
the runtime regression tests in
global_reassign_retraces_non_last_label_backedge_at_runtime and the test at the
sibling site in majit/majit-backend-wasm/tests/codegen_test.rs:51-70 and 110-130
from normal cargo test by marking them ignored or requiring an explicit
integration-test flag. For the sibling test, require the wasm-host module
specifically instead of falling back to the plain module, since it lacks the
test-hook export.
| q = 3.5 / d # constant float numerator on pre-call stack | ||
| r = 10.0 / d # second constant, deeper stack |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify bytecode differences for statement boundaries vs combined expressions.
# Notice that in the first output, the stack is empty after STORE_FAST before LOAD_CONST 10.0 begins.
echo "--- Original (Separate statements) ---"
python -c 'import dis; dis.dis("q = 3.5 / d; r = 10.0 / d")'
echo -e "\n--- Proposed (Combined expression) ---"
python -c 'import dis; dis.dis("q = 10.0 + (3.5 / d)")'Repository: youknowone/pyre
Length of output: 1146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('pyre/bench/synth/exception_const_operand_resume.py')
for start, end in [(1, 90)]:
print(f'--- {p} lines {start}-{end} ---')
for i, line in enumerate(p.read_text().splitlines(), 1):
if start <= i <= end:
print(f"{i:>4}: {line}")
PYRepository: youknowone/pyre
Length of output: 2560
Swap the operands in pyre/bench/synth/exception_const_operand_resume.py:54. DATA[j] + 7.25 evaluates the subscript first, so 7.25 is never on the stack when IndexError is raised. Use 7.25 + DATA[j] to keep the constant at the raising bytecode.
🤖 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/bench/synth/exception_const_operand_resume.py` around lines 21 - 22,
Update the relevant DATA subscript expression in
exception_const_operand_resume.py from DATA[j] + 7.25 to 7.25 + DATA[j],
ensuring the constant remains on the stack when the subscript raises IndexError.
| while i < n: | ||
| j = 0 if i < 2000 else (i % 9) # > 4 -> IndexError after warm-up | ||
| try: | ||
| v = DATA[j] + 7.25 # constant 7.25 on pre-call stack |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify bytecode evaluation order for left-to-right operations.
echo "--- Original (Right operand) ---"
python -c 'import dis; dis.dis("v = DATA[j] + 7.25")'
echo -e "\n--- Proposed (Left operand) ---"
python -c 'import dis; dis.dis("v = 7.25 + DATA[j]")'Repository: youknowone/pyre
Length of output: 1007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby benchmark context.
python3 - <<'PY'
from pathlib import Path
path = Path("pyre/bench/synth/exception_const_operand_resume.py")
text = path.read_text().splitlines()
for i, line in enumerate(text, 1):
if 1 <= i <= len(text):
if 1 <= i <= 120:
print(f"{i:4}: {line}")
PY
echo
echo "--- related files ---"
git ls-files 'pyre/bench/synth/*exception*' | sed -n '1,200p'Repository: youknowone/pyre
Length of output: 3270
Swap the operands here DATA[j] + 7.25 evaluates DATA[j] before loading the constant, so the IndexError can be raised before 7.25 reaches the stack. Use 7.25 + DATA[j] to keep the constant on the pre-exception operand stack.
Proposed fix
- v = DATA[j] + 7.25 # constant 7.25 on pre-call stack
+ v = 7.25 + DATA[j] # constant 7.25 on pre-call stack📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| v = DATA[j] + 7.25 # constant 7.25 on pre-call stack | |
| v = 7.25 + DATA[j] # constant 7.25 on pre-call stack |
🤖 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/bench/synth/exception_const_operand_resume.py` at line 54, Update the
expression assigned to v so the constant operand is evaluated first: use 7.25 +
DATA[j] instead of DATA[j] + 7.25, preserving the intended pre-exception operand
stack state.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fadde675e
ℹ️ 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".
| for artifact in [&dynasm, &wasm_runner, &wasm_module] { | ||
| assert!( | ||
| artifact.exists(), | ||
| "runtime global-reassign regression needs {}; build the requested dynasm and wasm-host artifacts first", |
There was a problem hiding this comment.
This new test runs under the crate's default cargo test -p majit-backend-wasm, but in a clean checkout those release binaries and the wasm-host module are not produced by the test harness, so the suite fails at the artifact assertion before reaching the codegen-only tests. Please mark this as ignored/manual or build these prerequisites in a dedicated integration harness instead of requiring unrelated prebuilt artifacts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b28d0bb4
ℹ️ 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".
| // callee correctly. This is temporarily more expensive in | ||
| // the bounded caller-invalidation window; deopting the outer | ||
| // trace at the Python CALL needs resume metadata that a |
There was a problem hiding this comment.
This relies on the caller invalidation flag to bound the slow path, but a caller already executing inside a wasm trace will not observe that flag: this backend lowers GuardNotInvalidated as an unconditional pass in codegen.rs, and the removed terminal_declined_ptr load was the only per-CALL_ASSEMBLER check. If a callee bridge terminally declines while a long-running outer trace is active, each later iteration keeps entering the CA frame path and blackhole-deopting the callee until the outer trace finally exits, which is the hot scenario this change is meant to handle. Please keep a live terminal-decline escape or force the outer trace to exit when the decline is marked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4822f99d4
ℹ️ 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 wasm_module = if host_module.exists() { | ||
| host_module | ||
| } else { | ||
| plain_module |
There was a problem hiding this comment.
When pyre_wasm.wasm-host.wasm is absent but pyre_wasm.wasm exists, this fallback lets the artifact check pass and then runs with PYRE_WASM_FORCE_CA_TERMINAL_DECLINE=1; the runner responds to that env var by requiring the pyre_set_force_ca_terminal_decline export, which pyre/pyre-wasm/src/lib.rs only exposes under the wasm-host feature. In that artifact state the regression fails with “wasm module lacks terminal-decline regression hook” before exercising the CA path, so this test should require/skip on the host module instead of accepting the plain module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc3379e2de
ℹ️ 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 old_target.input_types != new_target.input_types | ||
| || old_target.callee_frame_bytes != new_target.callee_frame_bytes | ||
| || old_target.callee_gcmap_ptr != new_target.callee_gcmap_ptr |
There was a problem hiding this comment.
When a CALL_ASSEMBLER target is reattached to a replacement loop with the same ABI/frame shape, build_callee_gcmap still leaks a fresh Box for the new compile, so this pointer comparison is false even though the baked frame geometry is compatible. redirect_call_assembler then returns Unsupported (and attach_procedure_with_redirect ignores that result), leaving existing wasm dispatch entries for old.number pointing at the previous loop instead of the replacement after retracing/invalidation. Compare the layout or gcmap contents, or otherwise reuse a stable gcmap for identical geometry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
pyre/bench/synth/exception_const_operand_resume.py (1)
54-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSwap the operands to preserve the intended pre-exception stack state.
DATA[j] + 7.25evaluatesDATA[j]before loading the constant, so theIndexErroris raised before7.25reaches the stack. To ensure the constant is on the stack when the exception is raised (as the inline comment intends), use7.25 + DATA[j].🐛 Proposed fix
- v = DATA[j] + 7.25 # constant 7.25 on pre-call stack + v = 7.25 + DATA[j] # constant 7.25 on pre-call stack🤖 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/bench/synth/exception_const_operand_resume.py` at line 54, Update the operand order in the expression assigned to v so the constant 7.25 is loaded before DATA[j] is evaluated, using 7.25 + DATA[j] while preserving the existing exception behavior and pre-call stack intent.
🤖 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/lib.rs`:
- Around line 2806-2809: Update the redirection compatibility condition around
old_target and new_target to compare only input_types and callee_frame_bytes;
remove the callee_gcmap_ptr identity requirement while preserving the existing
redirect behavior for matching layouts.
- Around line 1808-1811: Refresh or reacquire the pending self-target metadata
before iterating in the ca_targets handling near
mark_call_assembler_target_active, ensuring each target reflects its current
compiled_ptr and is eligible for activation. Preserve the existing activation
loop, and ensure the refreshed self target is marked ca_active and registered
for invalidation before any later trampoline bridge can use its movable CA
frame.
In `@pyre/bench/synth/nested_loop_correctness.py`:
- Line 7: Replace the semicolon-separated initializations in
pyre/bench/synth/nested_loop_correctness.py at lines 7, 22, and 34 with chained
assignments: use s = sq = cu = 0 at line 7 and s = sq = 0 at lines 22 and 34.
In `@pyre/check.py`:
- Around line 1265-1270: Replace the Unicode multiplication sign `×` in the
comment above `vs_pypy` with a plain ASCII `x`; leave the backend logic
unchanged.
- Around line 560-573: Update the startup measurement loop in the
target-processing flow to surface non-zero results from run_timed instead of
silently recording 0.0. When a startup sample fails, emit a warning or error
that identifies the affected key/backend and failure details, while preserving
the existing successful-sample median calculation and ratio behavior.
- Around line 578-582: Replace the manual try/except OSError cleanup around
os.unlink(empty_path) in the finally block with contextlib.suppress(OSError),
adding the contextlib import required by the new form.
- Around line 923-953: Update _performance_gate_passed and both callers of its
result so the slowdown failure report uses the same _exec_time values used by
the gate, while preserving raw elapsed and baseline times for reporting
elsewhere. Propagate the execution-time values for both the initial comparison
and median retry paths, and use them in the SLOWER message.
---
Duplicate comments:
In `@pyre/bench/synth/exception_const_operand_resume.py`:
- Line 54: Update the operand order in the expression assigned to v so the
constant 7.25 is loaded before DATA[j] is evaluated, using 7.25 + DATA[j] while
preserving the existing exception behavior and pre-call stack intent.
🪄 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: 288bf70f-42aa-4ff3-8789-d58ec024ef51
📒 Files selected for processing (13)
.github/workflows/pyre-ci.ymlmajit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/failguard.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-backend-wasm/tests/codegen_test.rspyre/bench/ca_terminal_decline.pypyre/bench/synth/exception_const_operand_resume.pypyre/bench/synth/nested_loop_correctness.pypyre/check.pypyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
💤 Files with no reviewable changes (2)
- pyre/pyre-jit/src/eval.rs
- pyre/pyre-jit/src/call_jit.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
♻️ Duplicate comments (1)
pyre/bench/synth/exception_const_operand_resume.py (1)
54-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSwap the operands to preserve the intended pre-exception stack state.
DATA[j] + 7.25evaluatesDATA[j]before loading the constant, so theIndexErroris raised before7.25reaches the stack. To ensure the constant is on the stack when the exception is raised (as the inline comment intends), use7.25 + DATA[j].🐛 Proposed fix
- v = DATA[j] + 7.25 # constant 7.25 on pre-call stack + v = 7.25 + DATA[j] # constant 7.25 on pre-call stack🤖 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/bench/synth/exception_const_operand_resume.py` at line 54, Update the operand order in the expression assigned to v so the constant 7.25 is loaded before DATA[j] is evaluated, using 7.25 + DATA[j] while preserving the existing exception behavior and pre-call stack intent.
🤖 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/lib.rs`:
- Around line 2806-2809: Update the redirection compatibility condition around
old_target and new_target to compare only input_types and callee_frame_bytes;
remove the callee_gcmap_ptr identity requirement while preserving the existing
redirect behavior for matching layouts.
- Around line 1808-1811: Refresh or reacquire the pending self-target metadata
before iterating in the ca_targets handling near
mark_call_assembler_target_active, ensuring each target reflects its current
compiled_ptr and is eligible for activation. Preserve the existing activation
loop, and ensure the refreshed self target is marked ca_active and registered
for invalidation before any later trampoline bridge can use its movable CA
frame.
In `@pyre/bench/synth/nested_loop_correctness.py`:
- Line 7: Replace the semicolon-separated initializations in
pyre/bench/synth/nested_loop_correctness.py at lines 7, 22, and 34 with chained
assignments: use s = sq = cu = 0 at line 7 and s = sq = 0 at lines 22 and 34.
In `@pyre/check.py`:
- Around line 1265-1270: Replace the Unicode multiplication sign `×` in the
comment above `vs_pypy` with a plain ASCII `x`; leave the backend logic
unchanged.
- Around line 560-573: Update the startup measurement loop in the
target-processing flow to surface non-zero results from run_timed instead of
silently recording 0.0. When a startup sample fails, emit a warning or error
that identifies the affected key/backend and failure details, while preserving
the existing successful-sample median calculation and ratio behavior.
- Around line 578-582: Replace the manual try/except OSError cleanup around
os.unlink(empty_path) in the finally block with contextlib.suppress(OSError),
adding the contextlib import required by the new form.
- Around line 923-953: Update _performance_gate_passed and both callers of its
result so the slowdown failure report uses the same _exec_time values used by
the gate, while preserving raw elapsed and baseline times for reporting
elsewhere. Propagate the execution-time values for both the initial comparison
and median retry paths, and use them in the SLOWER message.
---
Duplicate comments:
In `@pyre/bench/synth/exception_const_operand_resume.py`:
- Line 54: Update the operand order in the expression assigned to v so the
constant 7.25 is loaded before DATA[j] is evaluated, using 7.25 + DATA[j] while
preserving the existing exception behavior and pre-call stack intent.
🪄 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: 288bf70f-42aa-4ff3-8789-d58ec024ef51
📒 Files selected for processing (13)
.github/workflows/pyre-ci.ymlmajit/majit-backend-wasm/src/codegen.rsmajit/majit-backend-wasm/src/failguard.rsmajit/majit-backend-wasm/src/lib.rsmajit/majit-backend-wasm/tests/codegen_test.rspyre/bench/ca_terminal_decline.pypyre/bench/synth/exception_const_operand_resume.pypyre/bench/synth/nested_loop_correctness.pypyre/check.pypyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-wasm-runner/src/main.rspyre/pyre-wasm/src/lib.rs
💤 Files with no reviewable changes (2)
- pyre/pyre-jit/src/eval.rs
- pyre/pyre-jit/src/call_jit.rs
🛑 Comments failed to post (7)
majit/majit-backend-wasm/src/lib.rs (2)
1808-1811: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Refresh pending self-target metadata before marking it active.
ca_targetswas captured while the self target hadcompiled_ptr == 0, somark_call_assembler_target_activesilently skips it. The loop is consequently not markedca_active, its caller is not registered for invalidation, and a later trampoline bridge can run on its movable CA frame.Proposed fix
if let Some(targets) = ca_targets.as_ref() { - for (_, target) in targets { - mark_call_assembler_target_active(target, token); + for (target_token, _) in targets { + let target = call_assembler_target(*target_token) + .expect("admitted CALL_ASSEMBLER target disappeared after publication"); + mark_call_assembler_target_active(&target, token); } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if let Some(targets) = ca_targets.as_ref() { for (target_token, _) in targets { let target = call_assembler_target(*target_token) .expect("admitted CALL_ASSEMBLER target disappeared after publication"); mark_call_assembler_target_active(&target, token); }🤖 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/lib.rs` around lines 1808 - 1811, Refresh or reacquire the pending self-target metadata before iterating in the ca_targets handling near mark_call_assembler_target_active, ensuring each target reflects its current compiled_ptr and is eligible for activation. Preserve the existing activation loop, and ensure the refreshed self target is marked ca_active and registered for invalidation before any later trampoline bridge can use its movable CA frame.
2806-2809: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not require GC-map pointer identity for redirection.
Each compiled loop receives a freshly allocated GC map, so equivalent old/new targets normally have different addresses. This check rejects valid redirects even when their input types and frame layout match; existing callers can safely retain their already-baked equivalent map.
Proposed fix
if old_target.input_types != new_target.input_types - || old_target.callee_frame_bytes != new_target.callee_frame_bytes - || old_target.callee_gcmap_ptr != new_target.callee_gcmap_ptr + || old_target.callee_frame_bytes != new_target.callee_frame_bytes {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if old_target.input_types != new_target.input_types || old_target.callee_frame_bytes != new_target.callee_frame_bytes {🤖 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/lib.rs` around lines 2806 - 2809, Update the redirection compatibility condition around old_target and new_target to compare only input_types and callee_frame_bytes; remove the callee_gcmap_ptr identity requirement while preserving the existing redirect behavior for matching layouts.pyre/bench/synth/nested_loop_correctness.py (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Avoid multiple statements on a single line.
Using semicolons to separate statements on the same line violates PEP 8 and triggers static analysis warnings. Consider using chained assignments (e.g.,
s = sq = cu = 0) or placing each statement on its own line for better readability.
pyre/bench/synth/nested_loop_correctness.py#L7-L7: Replaces = 0; sq = 0; cu = 0withs = sq = cu = 0.pyre/bench/synth/nested_loop_correctness.py#L22-L22: Replaces = 0; sq = 0withs = sq = 0.pyre/bench/synth/nested_loop_correctness.py#L34-L34: Replaces = 0; sq = 0withs = sq = 0.🧰 Tools
🪛 Ruff (0.15.21)
[error] 7-7: Multiple statements on one line (semicolon)
(E702)
[error] 7-7: Multiple statements on one line (semicolon)
(E702)
📍 Affects 1 file
pyre/bench/synth/nested_loop_correctness.py#L7-L7(this comment)pyre/bench/synth/nested_loop_correctness.py#L22-L22pyre/bench/synth/nested_loop_correctness.py#L34-L34🤖 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/bench/synth/nested_loop_correctness.py` at line 7, Replace the semicolon-separated initializations in pyre/bench/synth/nested_loop_correctness.py at lines 7, 22, and 34 with chained assignments: use s = sq = cu = 0 at line 7 and s = sq = 0 at lines 22 and 34.Source: Linters/SAST tools
pyre/check.py (4)
560-573: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Startup-measurement failures fall back to
0.0silently.If an empty-program run for an interpreter/backend exits non-zero,
samplesis reset to[]andself.startup[key]becomes0.0(no subtraction) with no warning printed — only the successful entries show up in thedim(...)summary line. A broken backend binary (e.g. a wasm runner crash on a trivial script) would silently degrade to unadjusted ratios instead of surfacing the failure.🛡️ Proposed fix: surface the failure
startup = statistics.median(samples) if samples else 0.0 self.startup[key] = startup - parts.append(f"{key}={startup:.3f}s") + parts.append(f"{key}={startup:.3f}s" if samples else f"{key}=FAILED")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.parts = [] for key, cmd, env in targets: samples = [] for _ in range(STARTUP_SAMPLES): _out, elapsed, code, _err = run_timed( cmd, timeout_s=60, env=env, ) if code != 0: samples = [] break samples.append(elapsed) startup = statistics.median(samples) if samples else 0.0 self.startup[key] = startup parts.append(f"{key}={startup:.3f}s" if samples else f"{key}=FAILED")🤖 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/check.py` around lines 560 - 573, Update the startup measurement loop in the target-processing flow to surface non-zero results from run_timed instead of silently recording 0.0. When a startup sample fails, emit a warning or error that identifies the affected key/backend and failure details, while preserving the existing successful-sample median calculation and ratio behavior.
578-582: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use
contextlib.suppress(OSError).Ruff SIM105 flags this
try/except/passcleanup block on the newly added temp-file removal.♻️ Proposed fix
finally: - try: - os.unlink(empty_path) - except OSError: - pass + with contextlib.suppress(OSError): + os.unlink(empty_path)(requires
import contextlib)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.finally: with contextlib.suppress(OSError): os.unlink(empty_path)🧰 Tools
🪛 Ruff (0.15.21)
[warning] 579-582: Use
contextlib.suppress(OSError)instead oftry-except-passReplace
try-except-passwithwith contextlib.suppress(OSError): ...(SIM105)
🤖 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/check.py` around lines 578 - 582, Replace the manual try/except OSError cleanup around os.unlink(empty_path) in the finally block with contextlib.suppress(OSError), adding the contextlib import required by the new form.Source: Linters/SAST tools
923-953: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '900,1070p' pyre/check.py | cat -nRepository: youknowone/pyre
Length of output: 8819
🏁 Script executed:
python3 - <<'PY' from pathlib import Path text = Path('pyre/check.py').read_text() for start, end in [(900, 1070)]: lines = text.splitlines() for i in range(start-1, min(end, len(lines))): print(f"{i+1}: {lines[i]}") PYRepository: youknowone/pyre
Length of output: 8548
Report the exec-time values used for the slowdown gate
_performance_gate_passedcompares startup-subtracted times, but the slowdown failure path logs the raw elapsed/baseline values. That makes theSLOWERmessage inconsistent with the gate decision when startup overhead is nontrivial. Return or log the exec-time values alongside the raw times at both call sites.🧰 Tools
🪛 Ruff (0.15.21)
[warning] 923-923: Missing return type annotation for private function
_performance_gate_passed(ANN202)
🤖 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/check.py` around lines 923 - 953, Update _performance_gate_passed and both callers of its result so the slowdown failure report uses the same _exec_time values used by the gate, while preserving raw elapsed and baseline times for reporting elsewhere. Propagate the execution-time values for both the initial comparison and median retry paths, and use them in the SLOWER message.
1265-1270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Ambiguous multiplication sign in comment.
Ruff RUF003 flags the
×(U+00D7) here; a plainxavoids linter/encoding surprises.✏️ Proposed fix
- # `wasm_vs_*` parameter at all): it legitimately runs a few× slower + # `wasm_vs_*` parameter at all): it legitimately runs a few x slower📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# wasm carries no perf-ratio gate, matching `run_bench` (which has no # `wasm_vs_*` parameter at all): it legitimately runs a few x slower # than the native backends a fixture's ratio is tuned for, so one # ratio cannot gate both — see WASM_TIMEOUT_SCALE. Its per-bench # timeout stays the hang guard. vs_pypy = None if backend == "wasm" else max_pypy_ratio🧰 Tools
🪛 Ruff (0.15.21)
[warning] 1266-1266: Comment contains ambiguous
×(MULTIPLICATION SIGN). Did you meanx(LATIN SMALL LETTER X)?(RUF003)
🤖 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/check.py` around lines 1265 - 1270, Replace the Unicode multiplication sign `×` in the comment above `vs_pypy` with a plain ASCII `x`; leave the backend logic unchanged.Source: Linters/SAST tools
The general CALL_ASSEMBLER arm's terminal-declined branch passed the op's first red input (op.arg(0)) to wasm_ca_baseline_call, which cast it to *mut PyFrame and ran eval_with_jit on it. A CALL_ASSEMBLER operand is a callee loop-header live-in value (Int or Ref), not a PyFrame, so the cast was undefined behavior. The branch is reached only when a CA target's callee bridge terminally declines on wasm, which no bench triggers, so the bug was latent. - Delete the terminal-declined fast-path; a terminally-declined target now takes the ordinary CA frame path, which marshals the live-ins into a callee JitFrame and blackhole-resumes on a non-finish exit. The outer loop pays the compiled-entry + deopt per call during the bounded caller-invalidation window. Deopting the outer trace at the Python CALL would need resume metadata a CALL_ASSEMBLER op does not carry. - Remove the now-dead baseline_helper_slot / terminal_declined_ptr fields, CA_BASELINE_HELPER_SLOT, set/get_ca_baseline_helper_slot, and wasm_ca_baseline_call. - Add a dormant PYRE_WASM_FORCE_CA_TERMINAL_DECLINE hook (guest export pyre_set_force_ca_terminal_decline, BRIDGE_DIAG[16]) and a runtime regression test that forces terminal-decline and asserts byte-exact output vs dynasm. Assisted-by: Claude
…ining The loop lowering resolves the loop-back label through the terminal JUMP's loop-target descr identity (find_loop_label_index / find_label_args), so a JUMP that targets an earlier (non-last) label already lowers correctly: the `loop` opens at the descr-selected label, the parallel move maps the jump args onto that label's params, and is_resumable_peeled keeps the key-dispatch wrapper restricted to a last-label target so the ordinary local `loop`/`br 0` handles the shape. Remove the conservative wasm_unsupported_trace_reason decline that rejected a loop whose closing JUMP descr differs from the last label's descr. That shape arises when a loop is re-traced after a quasi-immutable module-global invalidation (wide preamble entry label, narrower peeled header last). The decline forced these re-traced loops to the interpreter, which reallocated a boxed int every iteration; global_reassign / global_reassign_obj now recompile (compiles 1->4, gc_majors 42->4) matching dynasm, wasm 0.8s -> 0.21s, output byte-exact. Add a runtime regression asserting both benches match dynasm with compiles>1 and no GC storm, and a synthetic test that the non-last-label back-edge shape validates as wasm. Assisted-by: Claude
…dException Remove the compile_bridge decline that rejected any bridge containing a GuardException op. It was added when a compiled exception-resume bridge could not rematerialise a constant pre-call operand: the bridge reconstructed state from spilled input slots alone, so a constant that lived only in the guard's rd_consts (e.g. a float dividend) surfaced as NULL and the re-entered interpreter ran op(NULL, ...), raising a spurious TypeError. The bridge path now runs collect_constants_from_ops over its own ops, so a constant operand of the raising op is materialised from the bridge constant pool (rd_consts is also captured into the guard-exit metadata). The decline is obsolete: an except-handler bridge compiles and stays in JIT instead of declining, which registered the guard in declined_bridge_guards and short-circuited every subsequent raise to the blackhole interpreter resume. On exception_multi_handler_warmup the post-warmup raises now bridge (BRIDGE_OK 0->3, decl_shortcircuit 21797->0, jit_calls 306767->6381, gc_majors 0), wasm 0.46s -> 0.22s, output byte-exact (11000, 4889). Add synth/exception_const_operand_resume.py: a warm-up-then-raise loop whose raising bytecode has a constant numerator/addend on the pre-call operand stack (float div, int floordiv, list index), exercising 6 GuardException bridges and locking in byte-exact resume (a lost constant would raise a spurious TypeError). Assisted-by: Claude
Add synth/nested_loop_correctness.py: rectangular, triangular (inner trip depends on the outer index), for-in-for, and triple-nested loops, each printing sum / sum-of-squares / sum-of-cubes checksums so a single dropped or duplicated inner iteration cannot be masked by a plain count. Covers the trip-count alignments and triangular shapes the nested-loop merge-point and close guards (trace.rs loop_header_merge_point_regs, jitcode_dispatch.rs root-only close) protect against, at sizes that exercise the JIT. Byte-exact across dynasm, wasm, and the interpreter. Assisted-by: Claude
…it from perf ratios and gates Times are user-CPU. The wasm backend pays a fixed per-process startup (~0.14s here: wasmtime instantiating the module) that is added to every run regardless of workload, inflating the elapsed/pypy ratio in the comparison table and the synthetic max-pypy-ratio gate of short benches. measure_startups() runs an empty program STARTUP_SAMPLES times per interpreter/backend after the build and records the median user-CPU; _exec_time() subtracts each interpreter's own measured startup (floored at EXEC_TIME_FLOOR_S) before ratios and gates are computed, so the reported numbers reflect execution only. The startup is measured every run, not hardcoded, so it self-corrects for runner speed. --no-startup-subtract restores raw times. Assisted-by: Claude
Implement register_pending_target and redirect_call_assembler on WasmBackend, previously the Backend-trait no-op defaults. Each CALL_ASSEMBLER token gets a stable guest-memory WasmCaDispatchEntry (boxed) holding func_handle, loop_finish_fi, and compiled_ptr as atomics. The CA codegen arm loads these three install-dependent values from the baked entry address at runtime instead of baking them as immediates; a zero func_handle traps rather than dispatching table slot 0. compile_loop freezes its frame before CALL_ASSEMBLER admission and publishes the real values into the entry after glue::compile_module installs the module and its finish index is known. general_int_call_assembler_target admits the currently-compiling token as a self-target through its fresh pending dispatch entry, using the frozen frame's ca_frame_bytes and build_callee_gcmap. A self-recursive loop now compiles with CALL_ASSEMBLER instead of being declined (wasm cannot execute a non-lifted CALL_ASSEMBLER), removing the per-call host round-trip a self-recursive trace previously took. Distinct cross-token targets keep the live-loop census. The CA payload carries per-token geometry in a CaTarget map so a trace may enter multiple sibling targets. invalidate_loop and CompiledWasmLoop::drop retract dispatch entries by compiled_ptr so a redirect source survives an old loop's drop. Assisted-by: Claude Assisted-by: Codex
…ts exist The two runtime integration tests in majit-backend-wasm/tests/codegen_test.rs (added on this branch by the terminal-decline and non-last-label-backedge fixes) run a bench on the dynasm and wasm backends and compare their output, so they require the release pyre-dynasm, pyre-wasm-runner, and the wasm-host module. The `cargo test --all` job never builds those, so both tests hit their assert!(artifact.exists()) and fail on every OS. Mark both tests #[ignore] so `cargo test --all` skips them, and run them via `cargo test -- --ignored` in the Linux pyre/check.py job, which has just built all three artifacts (WASM_MODULE_PATH is the .wasm-host.wasm the test loads). Assisted-by: Claude
Assisted-by: Claude Assisted-by: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e270c5f4fd
ℹ️ 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".
| for (_, target) in targets { | ||
| mark_call_assembler_target_active(target, token); |
There was a problem hiding this comment.
When ca_targets contains the pending self target admitted during compile_loop, the target iterated here is still the placeholder built with compiled_ptr: 0, so mark_call_assembler_target_active returns without setting the newly published loop's ca_active or registering its caller invalidation flag. For self-recursive CALL_ASSEMBLER loops, a later bridge with trampoline-lowered calls can then pass the source_ca_active && bridge_has_trampoline_calls safety floor because source_ca_active remains false, allowing host-trampoline code to run on movable CA frames. Refresh/use the just-published self target before marking activity.
Useful? React with 👍 / 👎.
1 participant
Fixes a latent undefined-behavior bug in the wasm backend's general
CALL_ASSEMBLERlowering (introduced with the CA generalization merged in #564).The bug
The
CALL_ASSEMBLERarm's terminal-declined fast-path passed the op's first red input (op.arg(0)) towasm_ca_baseline_call, which cast it to*mut PyFrameand raneval_with_jiton it. ACALL_ASSEMBLERoperand is a callee loop-header live-in value (anIntorRefPython value) — not aPyFramepointer. Casting a red value to*mut PyFrameand interpreting it is undefined behavior / heap corruption.It is latent: the branch is reached only when a CA target's callee exit-bridge terminally declines on wasm, which no current bench triggers (nbody's CA bridge compiles —
BRIDGE_OK). A long-running outer loop over a terminally-declining callee would hit it every iteration.The fix (Option C — fall through to the normal CA path)
Delete the broken fast-path. A terminally-declined target now takes the ordinary CA frame path, which marshals the live-ins into a callee
JitFrameand blackhole-resumes viawasm_ca_resume_deopton a non-finish exit — already correct for a declining callee. The outer loop pays the compiled-entry + deopt per call only during the bounded caller-invalidation window (general_int_call_assembler_targetrefuses the target onceca_terminal_declinedis set, so re-entry recompiles without the CA).Deopting the outer trace directly at the Python
CALL(Option A) would be cheaper but is not feasible here: aCALL_ASSEMBLERop carries no caller-side resume snapshot / per-op fail descriptor, only callee-run metadata — a follow-up if terminal-decline ever becomes hot.Also removes the now-dead
wasm_ca_baseline_call,CA_BASELINE_HELPER_SLOT,set/get_ca_baseline_helper_slot, and thebaseline_helper_slot/terminal_declined_ptrfields.A reconstruction approach (rebuilding a function-entry callee frame from the reds) is not correct: the reds are loop-header live-ins, not the function's call parameters when the loop header sits past the function's local setup.
Regression test (with teeth)
Because no organic workload terminal-declines, a dormant test hook (
PYRE_WASM_FORCE_CA_TERMINAL_DECLINE, guest exportpyre_set_force_ca_terminal_decline,BRIDGE_DIAG[16]) forcesmark_call_assembler_terminal_declineon the first admitted CA target after it compiles. The new runtime testterminal_declined_call_assembler_matches_dynasm_at_runtimerunsbench/ca_terminal_decline.py(an outer loop calling an inner-loop-bearing callee) with the hook on and asserts byte-exact output vs dynasm,accepted_ca≠0, andforced_ca_terminal_decline=1. It crashes on the pre-fix code (verified via a historical worktree reproduction) and passes after.Verification
cargo test -p majit-backend-wasm— the new runtime test passes.python3 pyre/check.py --backend dynasm,wasm— green except the pre-existingsynth/list_insert_pop_indexBASEFAIL: dynasm 187/1, wasm 186/1.accepted_ca=1,BRIDGE_OK=5, byte-exact within the known cross-ISApowULP tolerance (CA win from jit(wasm): general CALL_ASSEMBLER + float residual/local perf gaps (nbody/fannkuch/float_loop/spectral) #564 unchanged).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--no-startup-subtract).Bug Fixes
Tests