Fix blackhole failure handling and update documentation references#15
Fix blackhole failure handling and update documentation references#15lifthrasiir wants to merge 2 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR implements two independent changes: a blackhole failure abort-limit mechanism in the JIT meta-interpreter that prevents infinite retrace cycles, and updates to documentation and workflow skills to reference the new ChangesBlackhole Failure Abort Limiting
Check Tool Migration to Python Script
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-jit/src/eval.rs (1)
2974-2996:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuarantining the cell still leaves the broken loop executable.
record_blackhole_failure_and_maybe_invalidate()only helps if the failing loop stops being entered. Here the compiled token remains live, while Line 2610 still takeshas_compiled_loop(green_key)before consultingDONT_TRACE_HERE, and Line 3282 has the same compiled-entry fast path. Once the abort limit flipsDONT_TRACE_HERE, the next hit can still re-enter the same machine code and reproduceBlackholeResult::Failedforever. Please either clear/invalidate the compiled token when the limit is reached, or gate compiled entry on that flag too.Also applies to: 3204-3206, 3369-3385
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit/src/eval.rs` around lines 2974 - 2996, The current fix only records a blackhole failure via driver.record_blackhole_failure_and_maybe_invalidate(green_key) but leaves the compiled token live so the compiled entry fast paths (the has_compiled_loop(green_key) checks and the compiled-entry code path) can still re-enter and reproduce BlackholeResult::Failed; update the logic so that when record_blackhole_failure_and_maybe_invalidate flips the DONT_TRACE_HERE flag you also clear or invalidate the compiled loop/token (remove the compiled entry or drop its token) OR add an explicit gate that checks DONT_TRACE_HERE before any has_compiled_loop(green_key) fast-path and before the compiled-entry dispatch (the same paths referenced around record_blackhole_failure_and_maybe_invalidate and the other locations noted) so that once the abort limit is reached the JIT cannot re-enter the broken machine code.
🤖 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 @.claude/skills/merge/SKILL.md:
- Around line 10-13: The frontmatter description in
.claude/skills/merge/SKILL.md still references pyre/check.sh even though the
policy text was moved to pyre/check.py (lines that moved are already updated);
update the frontmatter line that mentions "pyre/check.sh" to "pyre/check.py" so
the file header and body are consistent and do not provide conflicting guidance.
In `@majit/majit-metainterp/src/warmstate.rs`:
- Around line 788-789: Replace the plain increment of the monotonic counter with
a saturating add to avoid overflow: change the `cell.abort_count += 1;` update
to use `cell.abort_count = cell.abort_count.saturating_add(1);` (keeping the
existing comparison to `MAX_TRACE_ABORT_COUNT` intact) so `abort_count` never
wraps or panics when it reaches its max value.
- Around line 786-794: record_blackhole_failure is incrementing abort_count only
on the bucket head via self.cells.get_mut(&green_key_hash), which misattributes
failures when multiple keys collide; change it to walk the collision chain
stored in the bucket to find the cell whose key equals the provided green key
(rather than assuming the hash maps to the exact key), then increment that
cell.abort_count and apply the MAX_TRACE_ABORT_COUNT logic (set flags |=
jc_flags::DONT_TRACE_HERE and state = BaseJitCellState::DontTraceHere) only for
the matched cell; ensure you compare the stored key/type identity when
traversing the chain and return false if no matching cell is found.
In `@pyre/README.md`:
- Line 34: Replace the ambiguous invocation "pyre/check.py" in the README with
an executable command form so users can copy-paste safely: update the line
referencing pyre/check.py to use either "python ./pyre/check.py" (explicit
interpreter) or "./pyre/check.py" after making pyre/check.py executable, and
mention the shebang/execute permission if the latter is chosen; this change
targets the README entry that currently shows "pyre/check.py".
---
Outside diff comments:
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 2974-2996: The current fix only records a blackhole failure via
driver.record_blackhole_failure_and_maybe_invalidate(green_key) but leaves the
compiled token live so the compiled entry fast paths (the
has_compiled_loop(green_key) checks and the compiled-entry code path) can still
re-enter and reproduce BlackholeResult::Failed; update the logic so that when
record_blackhole_failure_and_maybe_invalidate flips the DONT_TRACE_HERE flag you
also clear or invalidate the compiled loop/token (remove the compiled entry or
drop its token) OR add an explicit gate that checks DONT_TRACE_HERE before any
has_compiled_loop(green_key) fast-path and before the compiled-entry dispatch
(the same paths referenced around record_blackhole_failure_and_maybe_invalidate
and the other locations noted) so that once the abort limit is reached the JIT
cannot re-enter the broken machine code.
🪄 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: 7533ee9d-11c3-4bd8-a7d5-44de993a10fb
📒 Files selected for processing (8)
.claude/skills/merge/SKILL.md.claude/skills/parity/SKILL.md.claude/skills/rebase/SKILL.mdmajit/majit-metainterp/src/jitdriver.rsmajit/majit-metainterp/src/pyjitpl/mod.rsmajit/majit-metainterp/src/warmstate.rspyre/README.mdpyre/pyre-jit/src/eval.rs
|
generated review:
|
lifthrasiir
force-pushed
the
bound-bh-retrace-thrashing
branch
from
b588241 to
cafaab5
Compare
|
generated review • 1. Regressed PyPy Parity Compared To main
These are not 1:1 PyPy parity issues because they come from Pyre’s Python 3.13/3.14 and CPython-compatible bytecode shape:
Bottom line: the main parity regression is the new blackhole-failure warmstate path. The stack-effect additions are mostly reasonable |
BlackholeResult::Failed → invalidate_loop never incremented the cell's abort_count, so the compile → guard-fail → bh-fail → invalidate → retrace cycle could repeat indefinitely. Add record_blackhole_failure() which increments abort_count on each failure; after MAX_TRACE_ABORT_COUNT (5) the key is marked DONT_TRACE_HERE and the loop is NOT invalidated, stopping the cycle. Applied to all three BlackholeResult::Failed handlers in eval.rs (execute_assembler, bound_reached, try_function_entry_jit). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
At the abort limit, the previous code set DONT_TRACE_HERE but skipped invalidation, leaving the broken compiled loop in place. Since has_compiled_loop is checked before is_dont_trace_here, the same loop would be re-entered every iteration, failing and falling back to the interpreter in an infinite cycle. Now we always invalidate (removing the compiled code) and at the limit additionally set DONT_TRACE_HERE to prevent retracing. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
lifthrasiir
force-pushed
the
bound-bh-retrace-thrashing
branch
from
cafaab5 to
71efe05
Compare
|
diagnosis suggests to revert the changes to make it pypy-orthodox To fix sections 1 and 2 in a PyPy-orthodox way, keep Concretely:
The orthodox patch shape is: remove the warmstate extension, keep Failed |
2 participants
Summary
This pull request primarily updates documentation and code to reflect the migration of the main test script from
pyre/check.sh(a shell script) topyre/check.py(a Python script). In addition, it introduces a new mechanism in the JIT system to record and limit repeated blackhole resume failures, preventing infinite retracing cycles and marking problematic code paths as non-traceable after a threshold. The changes improve robustness in JIT error handling and ensure all documentation and workflows reference the correct test entry point.Documentation and Workflow Updates:
.claude/skills/merge/SKILL.md,.claude/skills/parity/SKILL.md,.claude/skills/rebase/SKILL.md, andpyre/README.md) frompyre/check.shtopyre/check.py, ensuring consistency across merge, rebase, and parity workflows.JIT Blackhole Failure Handling Improvements:
record_blackhole_failure_and_maybe_invalidateinJitDriver,MetaInterp, andWarmEnterStateto track blackhole resume failures per loop ("green key"), incrementing an abort counter and marking the loop as non-traceable after a set limit, thus preventing infinite retrace cycles.Self-review
Prompt & Model
Model: opus-4.6[1m]
Prompt:
/parityAnswer
Summary by CodeRabbit
Documentation
Bug Fixes