jit: #369 S4 — retire the pyre-local resume-coordinate twins#550
Conversation
|
Warning Review limit reached
Next review available in: 47 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 (6)
✨ 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 347cac6). 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)
4. Structural adaptations
|
![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: 227b07d220
ℹ️ 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 pc_word < 0 { | ||
| return fallback; |
There was a problem hiding this comment.
When this helper is called with NO_JITCODE_PC or a branch-orgpc tag, decode_resume_pc leaves the word negative, so this new early return exposes that negative value as the “Python pc”. Several resume consumers immediately cast backxlat_py_pc(...) to usize/u32 (for example the guard-failure rd_numb_pc path in pyre/pyre-jit/src/eval.rs), so a no-snapshot or tagged frame can become usize::MAX/a huge pc and corrupt the resumed frame position instead of declining or expanding the tag. The guard needs to avoid the internal wrap without returning a negative coordinate to callers.
Useful? React with 👍 / 👎.
Remove ReconstructRecipe.pc and BridgeInlineCarrier.root_jitcode_pc, the two pyre-local resume-coordinate twins left after S5. Post-flip both twins already hold the JitCode offset, and recipe.pc is definitionally backxlat_py_pc(recipe.jitcode_index, recipe.jitcode_pc), so every reader re-derives its value from the surviving coordinate: recipe readers call backxlat_py_pc; carrier readers use root_pc directly and drop the carried offset parameters. The recipe and carrier resume frames now store a single JitCode-offset coordinate. Byte-identical: a gated census over the corpus on both backends certified 0 recipe divergences, 0 carrier divergences, and 0 sentinel-reaching readers before the fields were removed. Assisted-by: Claude Assisted-by: Codex
…pace After S4a the BridgeInlineCarrier carries a single JitCode-offset coordinate (root_pc). Several consumers fed that offset into Python-pc-keyed machinery (set_last_instr_from_next_instr, result_color_at_pc_at, make_green_key / set_green_key, resume_jitcode_pc_for, the frame-liveness pc argument, and the WalkContext entry_py_pc / parent resume_py_pc fields), i.e. they read a JitCode offset as if it were a Python bytecode pc. Back-translate the offset with backxlat_py_pc(jitcode_index, root_pc) at each of those py_pc-keyed sinks so the single coordinate is consumed in its correct space, and add BridgeInlineCarrier.root_jitcode_index (= frames[0].jitcode_index) as the back-translation key at the one sink (set_last_instr) that runs before a body index is otherwise in scope; every other sink uses its co-located body index. The raw offset is preserved wherever it is genuinely needed: the walker drivers' start_pc, the carried select_recipe_entry argument, root_word, and the frame-liveness carried argument. Byte-behaviour-neutral on the current corpus: the multi-frame carrier walk still declines (P2Framestack::OuterNonTerminate) for an unrelated reason, so no compile is newly enabled; calls_closures.py (the 299-carrier reproducer) stays stdout byte-identical to the interpreter and check.py holds 180/180 on both backends. Assisted-by: Claude Assisted-by: Codex
python_pc_for_jitcode_pc_public and skip_python_trivia_forward_public cast their i32 pc inputs to usize without a sign check, so a negative pc word (a sentinel or branch-orgpc tag) wraps to a huge offset before reaching the jitcode metadata lookup. Return the decode_resume_pc fallback directly when the word is negative, preserving the existing fallback behaviour without relying on every caller to pre-screen. Addresses the PR #536 review finding on backxlat_py_pc. Assisted-by: Claude
The rd_numb_pc computation in rebuild_typed_from_rd_numb passed the outer frame's pc to backxlat_py_pc and cast the result to usize without screening the `pc == -1` no-snapshot sentinel, so a sentinel frame produced `usize::MAX`. Its two siblings walking the same rebuilt-frame chain already screen it (build_resumed_frames `frame.pc >= 0`, the carrier construction `frames[0].pc >= 0`); mirror that here so the sentinel yields `rd_numb_pc = None`. Revert the backxlat_py_pc internal `pc < 0` guard added in the previous commit: it returned the negative word unchanged (decode_resume_pc passes negatives through), so it did not protect the usize-casting caller, and the internal metadata lookups are bounds-checked and never index with the word. The screen belongs at the caller, per the decode_resume_pc contract. Addresses the PR #550 review finding on backxlat_py_pc. Assisted-by: Claude Assisted-by: Codex
youknowone
force-pushed
the
rewrite-tracer
branch
from
227b07d to
f1d6783
Compare
1 participant
#369 S4 — retire the pyre-local resume-coordinate twins
Follows up #536 (which merged the P1–P5 pc-word flip: the resume frame now
holds RPython's single 2-word
(jitcode_index, pc)header wherepcis theJitCode byte offset). #536 left two pyre-local twin coordinates outside the
rd_numb wire — on
ReconstructRecipeandBridgeInlineCarrier. This branchretires them, so every resume coordinate in the tracer is the single JitCode
offset, back-translated to a Python pc only where a py_pc-keyed consumer needs
it.
Commits
S4a — delete the recipe/carrier jitcode_pc twins (
2996ef697be)Deletes
ReconstructRecipe.pcandBridgeInlineCarrier.root_jitcode_pc.recipe.pcwas definitionallybackxlat_py_pc(index, jitcode_pc), so recipereaders re-derive it; carrier readers repoint to
root_pc. Carried-offsetparameters drop out. Byte-identical: internal census gates
(
PYRE_S4_RECIPE_AUDIT/PYRE_S4_CARRIER_AUDIT) certified 0 divergences,0 sentinel-reach over the corpus on both backends (299 recipe + 1495 carrier
asserts/backend).
S4.5 — consume the carrier resume coordinate in Python-pc space
(
afd52b416db)After S4a the carrier holds one JitCode-offset coordinate. Several
py_pc-keyed consumers (
set_last_instr,result_color_at_pc_at,make/set_green_key,resume_jitcode_pc_for, the frame-liveness pc argument,the
WalkContext.entry_py_pc/ parentresume_py_pcfields) were reading thatoffset as if it were a Python bytecode pc. Back-translate at each of those
sinks with
backxlat_py_pc; addBridgeInlineCarrier.root_jitcode_indexasthe translation key at the one sink that runs before a body index is otherwise
in scope. The raw offset is preserved where genuinely needed (walker-driver
start_pc, the carriedselect_recipe_entryargument,root_word, theframe-liveness carried argument). This addresses the jit: #369 pc-word flip — resume frame collapses to RPython 2-word shape #536 codex review finding
"Translate root pc before building bridge carrier" via consume-side inversion.
Byte-behaviour-neutral on the current corpus: the multi-frame carrier walk
still declines (
P2Framestack::OuterNonTerminate) for an unrelated,non-coordinate reason, so no compile is newly enabled. The change makes the
un-exercised multi-frame path coordinate-correct; the remaining
OuterNonTerminateblocker is separate future work.guard backxlat_py_pc against negative pc words (
227b07d2205)Addresses the jit: #369 pc-word flip — resume frame collapses to RPython 2-word shape #536 coderabbit review finding: the
i32→usizecasts in thepc-translation helpers would wrap a negative word (sentinel / branch-orgpc tag)
to a huge offset. Return the decoded fallback directly for negative input.
Verification
check.py: 181/181 dynasm + 181/181 cranelift.correctness), both clean: all 12 inversions + the field + the debug_assert
present, every inverted/leave-raw sink coordinate-verified against re-derived
helper signatures, no over/under-inversion.
🤖 Generated with Claude Code