jit: widen a catch target's liveness from an orphaned catch_exception too - #1136
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe JIT code writer now uses reusable sparse PC ownership helpers and post-merge instruction positions for catch ownership. Orphaned ChangesCatch liveness and ownership
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
Sequence Diagram(s)sequenceDiagram
participant JITCodeWriter
participant SparseOwnership
participant CatchAnalysis
participant LiveMarker
participant Census
JITCodeWriter->>SparseOwnership: build ownership from post-merge positions
JITCodeWriter->>CatchAnalysis: analyze catch targets
CatchAnalysis->>SparseOwnership: resolve orphan site owner
SparseOwnership-->>CatchAnalysis: return owning live marker
CatchAnalysis->>LiveMarker: widen marker with handler reference colors
CatchAnalysis->>Census: record catch and ownership counters
🚥 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 |
![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: d203e545ff
ℹ️ 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 mut anchorless_orphan = 0usize; | ||
| let mut orphan_sites_missing = 0usize; | ||
| let mut orphan_colors_missing = 0usize; | ||
| let pc_pos = sparse_pc_owner_table(ssarepr); |
There was a problem hiding this comment.
When remove_repeated_live collapses markers before an orphaned catch_exception, q indexes the rewritten post-merge ssarepr.insns, but sparse_pc_owner_table still reads the pre-merge positions in ssarepr.pc_first_insn_pos. filter_liveness_in_place already computes remapped first-instruction positions, but they are not supplied here, so the catch can be attributed to an earlier Python PC and its landing Ref colors are added to the wrong resume marker; the actual exception bridge can therefore still fail with RegisterReadUnbound. Build the owner table from positions translated through the liveness remap.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 81cda2d). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
… too
`catch_target_extra_ref_colors` adds the Ref colors a catch edge's landing
label holds live into the raising PC's `-live-` marker, so the resume
coordinate names what the handler reads. It only did that for a PC carrying
an entry in `after_call_markers`.
Those entries come from `derive_after_call_indices_from_sparse`, which records
one anchor per `catch_exception`, keyed to the py_pc owning the `-live-`
immediately before it, and skips the site when that predecessor is not a
`-live-`. A skipped site whose owning py_pc gets no anchor from any other site
was therefore never widened -- while still being a real exception edge.
The label was reachable the whole time: the `catch_exception` instruction
carries the landing `TLabel` itself. The preceding `-live-` is how the marker
*index* is located, not how the label is known. Orphaned sites now widen from
their own instruction.
Measured with the census this commit also adds (`PYRE_CATCH_LIVE_CENSUS=1`),
over the exception-bearing synth fixtures, before the change:
anchorless catch sites 37 (redundant 9, orphaned 28)
orphaned sites whose landing Refs were NOT already in the marker: 7
break_except_live_local run q=490 pc=90 missing {9}
check_exc_match_invalid_class f q=526 pc=104 missing {13}
except_tuple_clause_hot tuple_clause q=316 pc=75 missing {7}
except_tuple_clause_hot single_clause q=292 pc=69 missing {7}
exc_caught_in_callee_return_loop lookup q=103 pc=24 missing {5}
exception_as_cell_cleanup run q=675 pc=126 missing {7}
exception_bare_reraise_nested_outer main q=722 pc=134 missing {2}
After the change the same sweep reports 0 sites and 0 colors missing. The
counts themselves drift run to run with which functions get traced; the
population does not.
`catch_target_extra_ref_colors_widens_an_orphaned_catch_site` pins it. With the
orphan widening removed it fails on its `assert_eq`; with it in place it passes.
Two notes on what this does and does not do:
- `derive_after_call_indices_from_sparse` is unchanged. Structurally orphaned
sites still exist -- roughly 30 across the sweep -- they are simply no longer
a reason to skip widening.
- The two paths use different evidence that a PC is protected. The anchored
path consults `lookup_exceptiontable` per PC; the orphan path relies on the
`catch_exception` instruction existing, which is the stronger fact but not
the same test.
Widening a `-live-` is the conservative direction, as this function's own doc
comment states: the snapshot carries boxes the no-exception arm does not
consume, which costs a slot and never mis-restores.
The census is kept because it is what made this measurable, and it costs
nothing when the variable is unset.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c0f4a26e1
ℹ️ 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 owner_pc | ||
| .and_then(|pc| after_call_markers.get(pc)) | ||
| .and_then(|entry| *entry) | ||
| .is_some() |
There was a problem hiding this comment.
When one Python PC owns both an anchored catch and an anchorless catch_exception, this branch treats the anchorless site as redundant solely because after_call_markers[pc] is populated. That marker represents just one catch site—derive_after_call_indices_from_sparse overwrites the per-PC entry—and a second catch can have a different TLabel with additional label2alive Ref colors. Those colors are therefore never added to the PC's resume marker, so an exception bridge taking that site can still reach the landing with an unbound register; union every anchorless site's own colors rather than skipping it based on another anchor at the same PC.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 4530-4617: Replace the six-counter tuple around the scan block
with a named CatchCensus struct, defining the counters as fields in the shown
order with Default support. Update the scan block to initialize and mutate the
struct fields, return the struct, and access its named fields at the existing
consumers instead of positional destructuring.
- Around line 4445-4448: Update catch_live_census_enabled so
PYRE_CATCH_LIVE_CENSUS enables the feature only when its value is exactly "1";
values such as "0", an empty string, or an unset variable must return false
while preserving the existing OnceLock caching.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9624ab2-4597-4084-93a9-98d96fc9904c
📒 Files selected for processing (1)
pyre/pyre-jit/src/jit/codewriter.rs
| fn catch_live_census_enabled() -> bool { | ||
| static ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); | ||
| *ENABLED.get_or_init(|| std::env::var_os("PYRE_CATCH_LIVE_CENSUS").is_some()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the flag check with the documented =1 contract.
The doc comment states PYRE_CATCH_LIVE_CENSUS=1. var_os(...).is_some() also enables the census for PYRE_CATCH_LIVE_CENSUS=0 and for an empty value. A user who sets =0 then gets per-code-object stderr output.
🔧 Proposed fix
- *ENABLED.get_or_init(|| std::env::var_os("PYRE_CATCH_LIVE_CENSUS").is_some())
+ *ENABLED.get_or_init(|| {
+ std::env::var("PYRE_CATCH_LIVE_CENSUS")
+ .map(|value| !matches!(value.as_str(), "" | "0"))
+ .unwrap_or(false)
+ })🤖 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/jit/codewriter.rs` around lines 4445 - 4448, Update
catch_live_census_enabled so PYRE_CATCH_LIVE_CENSUS enables the feature only
when its value is exactly "1"; values such as "0", an empty string, or an unset
variable must return false while preserving the existing OnceLock caching.
`catch_target_extra_ref_colors` runs inside `filter_liveness_in_place`, after `compute_liveness_with_pc_anchors` has rewritten the SSARepr. Its orphan scan indexed the post-merge `ssarepr.insns` with `q` but read owner positions out of `ssarepr.pc_first_insn_pos`, which is never remapped -- `liveness.rs` states that callers must translate stream positions such as that field through the remap themselves. Comparing a post-merge index against pre-merge positions resolves the "greatest first-insn position at or before `q`" lookup to an earlier Python PC than the one owning the site, so the landing Ref colors were added to that earlier PC's resume marker and the PC that needed them was left unwidened. The orphan path was the only consumer of `sparse_pc_owner_table` in post-merge coordinates. `derive_after_call_indices_from_sparse` runs before `filter_liveness_in_place`, where its index and the table are both pre-merge; the anchored path indexes `after_call_markers` and `live_markers` by py_pc and reads no position table. Both are unchanged. `filter_liveness_in_place` already builds `first_insn_post_merge` by putting each pre-merge first-insn position through the remap. That table is now passed in and the orphan owner is resolved from it. `orphan_owner_remap_differs` joins the census: orphan sites whose owner computed from the pre-merge table differs from the post-merge one. It is computed only when `PYRE_CATCH_LIVE_CENSUS` is set. `catch_target_extra_ref_colors_uses_post_merge_owner_positions` pins the repair. It builds a stream where the two tables disagree -- the catch at index 3 resolves to PC 0 pre-merge and PC 1 post-merge -- asserts that disagreement directly, then asserts the landing Ref color lands on PC 1's marker and not on PC 0's. Also runs rustfmt over the file, which reflows three constructs the previous commit left unformatted. Measured with the census over the 159 exception-bearing synth fixtures, running the fixed binary so the counter reports what the old resolution would have done: catch_sites 3565 anchorless_orphan 545 orphan_owner_remap_differs 518 518 of the 545 orphan sites resolved to a different owning PC under the pre-merge table, so the misattribution was the rule rather than an edge case, and the gap is not small: `break_except_live_local` q=490 read PC 90 where the site is owned by PC 173. That also retracts the coordinates in the previous commit's message. They were read off the pre-merge table, so every py_pc column there names the wrong opcode. Assisted-by: Claude
Three findings from the review on PR #1136. An anchorless `catch_exception` whose owning py_pc already had an entry in `after_call_markers` was counted "redundant" and skipped. That anchor does not cover it: `derive_after_call_indices_from_sparse` writes `out[pc] = Some(live_pos)` once per `catch_exception`, so the per-PC entry is overwritten and records one arbitrary site. The anchored path then widens from that one landing only, and a second site owned by the same PC can carry a different `TLabel` whose `label2alive` set holds colors the recorded landing does not. Anchorless sites now contribute their own landing colors whether or not the owner carries an anchor; the census keeps the redundant/orphan split, it just no longer decides whether widening happens. `PYRE_CATCH_LIVE_CENSUS` had no row in `pyre/gate-triage.md`, which `every_live_pyre_gate_has_a_gate_triage_entry` enforces -- the `cargo test` leg was red on that alone. Listed in the default-OFF diagnostics section. The census doc comment documented the knob as `PYRE_CATCH_LIVE_CENSUS=1` while the reader is `var_os(..).is_some()`, which also fires on `=0` and on an empty value. The comment is what changed: `is_some()` is the idiom every default-OFF diagnostic in this repo uses, and `gate-triage.md` documents it as the default-OFF polarity rule. The six positional census counters became a named struct. The printed `[catch-live-census]` fields and their order are unchanged. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5ce6e1c4a
ℹ️ 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 mut pc_pos: Vec<(usize, usize)> = first_insn_post_merge | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(pc, entry)| entry.map(|pos| (pos, pc))) | ||
| .collect(); |
There was a problem hiding this comment.
When supersede re-walks produce multiple graph blocks for the same Python PC, this table cannot identify the later block: flatten.rs:1368-1377 deliberately stores only the first instruction position for each PC, while codewriter.rs:8157-8160 explicitly documents that duplicate-PC blocks occur. Consequently, an anchorless catch_exception emitted after a later occurrence is assigned to whichever unrelated PC has the greatest recorded first position, so its landing colors widen the wrong marker and the intended exception bridge can still hit RegisterReadUnbound. Fresh evidence beyond the earlier pre/post-remap issue is that the source ownership table loses later occurrences before any remapping; preserve an owner entry per emitted occurrence/site instead of reconstructing ownership from this first-wins table.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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/jit/codewriter.rs (1)
4824-4831: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRun the required dynasm checks with an updated LLBC fingerprint.
Run
cargo check --features dynasmandcargo test --features dynasmafter resolving the stale LLBC fingerprint. As the JIT changes require, run all eight benchmarks and include regression explanations where applicable.🤖 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/jit/codewriter.rs` around lines 4824 - 4831, Update the stale LLBC fingerprint associated with the JIT changes near catch_extra_refs and catch_target_extra_ref_colors, then run cargo check --features dynasm and cargo test --features dynasm. Execute all eight required benchmarks and document regression explanations where applicable.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@pyre/pyre-jit/src/jit/codewriter.rs`:
- Around line 4824-4831: Update the stale LLBC fingerprint associated with the
JIT changes near catch_extra_refs and catch_target_extra_ref_colors, then run
cargo check --features dynasm and cargo test --features dynasm. Execute all
eight required benchmarks and document regression explanations where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10a830dc-8304-46c5-9006-8ef6fa750bdd
📒 Files selected for processing (2)
pyre/gate-triage.mdpyre/pyre-jit/src/jit/codewriter.rs
1 participant
Two commits, both in
pyre/pyre-jit/src/jit/codewriter.rs.1. Widen a catch target's liveness from an orphaned
catch_exceptiontoocatch_target_extra_ref_colorsadds the Ref colors a catch edge's landinglabel holds live into the raising PC's
-live-marker, so the resumecoordinate names what the handler reads. It only did that for a PC carrying an
entry in
after_call_markers.Those entries come from
derive_after_call_indices_from_sparse, which recordsone anchor per
catch_exception, keyed to the py_pc owning the-live-immediately before it, and skips the site when that predecessor is not a
-live-. A skipped site whose owning py_pc got no anchor from any other sitewas therefore never widened -- while still being a real exception edge.
The label was reachable the whole time: the
catch_exceptioninstructioncarries the landing
TLabelitself. The preceding-live-is how the markerindex is located, not how the label is known. Orphaned sites now widen from
their own instruction.
Measured with the census this commit also adds (
PYRE_CATCH_LIVE_CENSUS=1),over the exception-bearing synth fixtures, before the change:
Widening a
-live-is the conservative direction, as the function's own doccomment states: the snapshot carries boxes the no-exception arm does not
consume, which costs a slot and never mis-restores.
derive_after_call_indices_from_sparseis unchanged -- structurally orphanedsites still exist, they are simply no longer a reason to skip widening.
2. Resolve the orphan's owning PC in post-merge coordinates
Found by the Codex review on this PR, and confirmed: commit 1 mixed two
coordinate systems.
catch_target_extra_ref_colorsruns insidefilter_liveness_in_place, aftercompute_liveness_with_pc_anchorshas rewritten the SSARepr. The orphan scanindexed the post-merge
ssarepr.insnswithqbut read owner positions out ofssarepr.pc_first_insn_pos, which is never remapped --liveness.rsstatesthat callers must translate stream positions such as that field through the
remap themselves.
Comparing a post-merge index against pre-merge positions resolves the
"greatest first-insn position at or before
q" lookup to an earlier Python PCthan the one owning the site, so the landing Ref colors were added to that
earlier PC's resume marker and the PC that needed them was left unwidened --
which is exactly the failure commit 1 set out to remove.
The orphan path was the only consumer of
sparse_pc_owner_tablein post-mergecoordinates.
derive_after_call_indices_from_sparseruns beforefilter_liveness_in_place, where its index and the table are both pre-merge;the anchored path indexes
after_call_markersandlive_markersby py_pc andreads no position table. Both are unchanged.
filter_liveness_in_placealready buildsfirst_insn_post_mergeby puttingeach pre-merge first-insn position through the remap. That table is now passed
in and the orphan owner resolved from it.
orphan_owner_remap_differsjoins the census: orphan sites whose ownercomputed from the pre-merge table differs from the post-merge one, so the size
of the misattribution is measurable rather than argued.
Tests
catch_target_extra_ref_colors_widens_an_orphaned_catch_site-- the plainorphan shape. Fails on commit 1's parent.
catch_target_extra_ref_colors_uses_post_merge_owner_positions-- builds astream where the two tables disagree (the catch at index 3 resolves to PC 0
pre-merge, PC 1 post-merge), asserts that disagreement directly so the
fixture cannot silently stop discriminating, then asserts the landing Ref
color lands on PC 1's marker and not on PC 0's.
Verification status
check.py --backend dynasmwas 415/415, zero failures with commit 1 -- butthat measured base
3fcae7f0ea3, two rebases ago, and does not cover commit 2.Commit 2 is not yet compiled or tested locally: the LLBC fingerprint went
stale twice mid-session (once from the majit-translate change in #1133, once
from the test removal now on main), and each re-extract takes ~20 minutes on a
loaded machine. CI is the first signal for it.
Summary by CodeRabbit
Bug Fixes
Diagnostics
Documentation