jit-trace: stop indexing the Python-keyed depth table with a JitCode offset - #1145
Conversation
`bridge_semantic_maps_at_with_jitcode_pc` indexes the Python-PC-keyed `LiveVars::depth_at_py_pc` with its `pc` parameter on two legs: when the carried jitcode coordinate decodes but the twin tables hold no entry, and when no coordinate is carried at all. `bridge_semantic_maps_from_pc` passes one word into both the `pc` and `jitcode_pc` slots, and all five of its callers supply a JitCode byte offset -- three named `jitcode_pc` / `op_pc`, and two `RebuiltFrame::pc`, which `majit-ir/src/resumedata.rs` documents as the byte offset with the Python coordinate held in the separate `py_pc` field. Adds `PYRE_M73_EMPTYTWIN_CENSUS`, which sorts each fallback read into null_code / out_of_range / in_range_zero / in_range_nonzero and prints the first witness of every bucket per call site. Only in_range_nonzero reaches `setup_bridge_sym`'s `semantic_prefix_len`. Over the 397-file synth corpus the leg runs in 20 files, every one of them `site=non_decodable bucket=out_of_range`; in_range_nonzero is 0 and site=twin_miss does not execute. With the variable unset every counter path is skipped. `check.py --backend dynasm,cranelift` is 415/415 and 414/414. Assisted-by: Claude
…inate `bridge_semantic_maps_at_with_jitcode_pc` indexes the Python-PC-keyed `LiveVars::depth_at_py_pc` with its second parameter. Six of its seven call sites reach it holding a JitCode byte offset: the five through `bridge_semantic_maps_from_pc`, which passed one word into both the Python and the JitCode slot, and `resume_snapshot.rs`'s `marker`, which is `inline_call_return_marker`'s `decode_op_at(..).next_pc`. `py_coord.rs` states the invariant they violate: "runtime consumers never project a JitCode PC through a Python-keyed map". The second parameter becomes `Option<i32>`, and the fallback answers 0 without reading the table when it is `None`. `bridge_semantic_maps_from_pc` becomes `bridge_semantic_maps_from_jitcode_pc` and passes `None`; only `walker_capture_snapshot_for_last_guard_impl`'s `liveness_py_pc` stays `Some`. The sibling `try_frame_liveness_reg_indices_by_bank_at_with_jitcode_pc` already declines on an absent coordinate rather than reconstructing one. This does not derive a depth from the JitCode offset. The fallback reads the static dense liveness, while the jitcode-keyed twin carries the walk-visited `depth_at_pc`; `codewriter.rs:14808` records these as deliberately different tables, and `depth_at_pc` is not published into `PyJitCodeMetadata`. Census over the synth corpus, before and after: the same 20 files reach the leg, moving from `bucket=out_of_range` (rp=231..1214 against table lengths 28..257) to `bucket=no_py_pc`. `in_range_nonzero` — the only bucket reaching `setup_bridge_sym`'s `semantic_prefix_len` — is 0 in both. The census now shares `py_coord::emptytwin_census_enabled` instead of re-reading `PYRE_M73_EMPTYTWIN_CENSUS`, which that module already owns, and prints `[m73-bridge-maps]` so its lines stay distinct from `note_empty_twin_fallback`'s. `check.py --backend dynasm,cranelift`: cranelift 416/416; dynasm 416 passed with `test.test_format` failing at `test_locale`, which reproduces under `PYRE_NO_JIT=1`. Assisted-by: Claude
…nsus The comment justifying the `None` fallback's 0 asserted that "there is no jitcode-keyed spelling of this read to fall back to either", citing `codewriter.rs:14808`. Three such spellings exist: `depth_containing_for_jitcode_pc`, built in `finalize_jitcode`'s `depth_containing_by_jit_pc` block to reproduce `depth_at_py_pc[containing_py]` for every offset, plus the trivia and block-head twins. `collect_outer_active_boxes`, the encode half this function mirrors, already reads its own `stack_depth_at_pc` from those twins with no Python PC in hand. The cited line is the closing brace of an unrelated `[pcmap-residual]` block; the passage it meant is `finalize_jitcode`'s `static_depth` construction, and it records the converse — the trivia twins deliberately carry the static `depth_at_py_pc`, and `depth_pred_by_jit_pc` alone carries the walk-visited `depth_at_pc`. The replacement cites by symbol rather than by line. `empty_twin_census` gains `jit_pc` and `twin`: the coordinate the leg declined on, and what `depth_containing_for_jitcode_pc` would answer there. Over the 399-file synth corpus all 20 executions carry `jit_pc >= 0` with `twin = Some(3..=5)`, so routing the fallback through the twin would change the reconstructed frame's width rather than nothing. The leg still answers 0; what it declines is now recorded instead of asserted away. The counters remain per leg, so those 20 are still unattributed to a call site — the comment says so, because `setup_bridge_sym` is the only caller whose `stack_depth_at_pc` read is unconditional. The census doc claimed `in_range_nonzero` is the only bucket reaching `setup_bridge_sym`'s `semantic_prefix_len`. The preceding commit made that site pass `None`, so the bucket is unreachable from there; the doc now names `no_py_pc` as that site's only bucket, and `semantic_limit` in `walker_capture_snapshot_for_last_guard_impl` as the consumer of the one `Some` caller's depth. `EMPTY_TWIN_NON_DECODABLE`'s doc said no coordinate was carried, while its branch also covers a carried offset that does not decode. `bridge_semantic_maps_from_jitcode_pc`'s doc said its callers hold only a JitCode coordinate; the two `RebuiltFrame` callers also hold `py_pc`, which `py_coord.rs` names as a sanctioned Python-coordinate source, so the doc now records that and why it is declined. `resume_marker_jit_pc`'s binding in `compute_nested_inline_caller_frame` no longer names a value and becomes `is_some()`. The kept-stack comment in `setup_bridge_sym`'s caller attributed `stack_depth_at_pc` to `depth_at_py_pc`; it reads the `depth_pred_by_jit_pc` twin. Retracts two claims from 397320f's message: the `codewriter.rs:14808` citation together with its reading of which table the jitcode-keyed twins carry, and "`in_range_nonzero` — the only bucket reaching `setup_bridge_sym`'s `semantic_prefix_len`". 6b30254's `415/415 and 414/414` was measured on a base that has since moved twice and does not describe this tree. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe change separates JitCode PCs from optional Python PCs during bridge semantic-map lookup. State fallback handling now uses explicit coordinates and telemetry. Bridge dispatch and resume reconstruction call the JitCode-aware lookup paths. ChangesJitCode-PC bridge reconstruction
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a2bb94d). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
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: a2bb94ddb1
ℹ️ 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 maps = | ||
| crate::state::bridge_semantic_maps_from_jitcode_pc(frame0.jitcode_index, frame0.pc); |
There was a problem hiding this comment.
When the carried JitCode position is non-decodable or lacks a depth/pcdep twin, this helper now returns stack_depth_at_pc = 0, even though frame0.py_pc is the forward-carried Python coordinate available for the fallback. For a kept-stack guard whose virtualizable depth reflects the shallower merge target, line 10097 consequently fails to widen semantic_prefix_len; live guard-time stack slots are then excluded from the reconstructed frame, potentially aborting or mis-seeding the bridge. Pass Some(frame0.py_pc) to bridge_semantic_maps_at_with_jitcode_pc here rather than deliberately discarding the valid coordinate.
Useful? React with 👍 / 👎.
1 participant
Summary
bridge_semantic_maps_at_with_jitcode_pcindexes the Python-PC-keyedLiveVars::depth_at_py_pcwith its second parameter. Six of its seven callsites reach it holding a JitCode byte offset — the thing
py_coord.rs's moduleinvariant forbids:
Five arrived through
bridge_semantic_maps_from_pc(idx, pc), which fed one wordinto both the Python slot and the JitCode slot. The sixth passed
inline_call_return_marker'sdecode_op_at(..).next_pc.The parameter becomes
Option<i32>, and the fallback answers0withouttouching the table when it is
None.bridge_semantic_maps_from_pcbecomesbridge_semantic_maps_from_jitcode_pc, named for the coordinate it carries sothe word cannot be re-routed into the Python slot. Only
walker_capture_snapshot_for_last_guard_impl'sliveness_py_pcstaysSome.The sibling
try_frame_liveness_reg_indices_by_bank_at_with_jitcode_pcalreadydeclines on an absent coordinate rather than reconstructing one; this matches it.
Each of the seven coordinates was classified independently from its producer
(not from the comments under test), and all seven agreed: six JitCode byte
offsets, one genuine Python PC.
Measurement
399-file synth corpus, census before and after: the same 20 files reach the leg,
moving
bucket=out_of_range(rp 231..1214 against table lengths 28..257) →bucket=no_py_pc. Both answer 0, so this is inert on the corpus. The defectit removes is the unmeasured case — a short function whose JitCode offset lands
inside the table and returns a foreign instruction's depth.
What this deliberately does not do, and the number that says it matters
Answering
0is a decline, not the best available answer. This PR measures howlossy the decline is, rather than asserting it away:
jit_pc >= 0depth_containing_for_jitcode_pcwould answer thereSome(3)×14,Some(4)×4,Some(5)×2NoneorSome(0)depth_containing_for_jitcode_pcis built to reproducedepth_at_py_pc[containing_py]for every offset, andcollect_outer_active_boxes— the encode half this function mirrors — alreadysources its own
stack_depth_at_pcfrom those twins with no Python PC in hand.So routing this leg through them is invariant-conforming and not a no-op: it
would widen
setup_bridge_sym'ssemantic_prefix_lenby 3–5 slots wherever theleg runs. That is a change in reconstructed frame width, not a coordinate repair,
so it is out of scope here and left as follow-up.
Stated as open: the census counters are per leg, not per call site, so
those 20 non-zero counterfactuals are not yet attributed to
setup_bridge_sym—the only caller whose
stack_depth_at_pcread is unconditional(
stack_only.max(..)). At the other five an emptypcdep_entriesmakessemantic_ref_slot_for_reg_colorreturnNoneregardless of the depth, so thevalue is inert there. Per-call-site attribution is step 1 of that follow-up.
Retractions
The third commit retracts two claims from the second commit's message: the
codewriter.rs:14808citation (it is the closing brace of an unrelated[pcmap-residual]block — the passage meant isfinalize_jitcode'sstatic_depthconstruction, which records the converse), and"
in_range_nonzero— the only bucket reachingsetup_bridge_sym'ssemantic_prefix_len", which that same commit made unreachable from there. Thefirst commit's
415/415 and 414/414was measured on a base that has since movedand does not describe this tree.
Verification
(
pyre-jit's fingerprint covers its dependency closure, so a comment-only editin
pyre-jit-tracestaled it — and a plaincargo buildreturned 0 withoutre-running the check.)
LC_ALL=C python3 pyre/check.py --backend dynasm,cranelift: ALL PASSED —dynasm 417/417, cranelift 416/416, exit 0.
LC_ALL=Cand dynasm reportstest.test_format PASS -> FAILattest_locale. That is environmental, not this patch: it reproducesunder
PYRE_NO_JIT=1, reproduces on cranelift too (which simply has notest.test_formatentry inpyre/cpython_tests/baseline.json— only{"dynasm": "PASS"}is recorded), and the assertion isassertIn(',', '123456789')—format(n, 'n')does not apply the locale'sthousands_sep. The recorded PASS was taken wheresepwas empty, soassertIn('', text)was vacuously true. No workflow in.github/workflows/pins a locale, so which way this gate reads depends on the runner's
LANG.Flagging it; not fixing it here.
Self-review
Reviewed in a separate session from the one that generated the code, two ways:
the repo's Codex parity prompt (
.github/codex-review-prompt.md), and aseven-way independent classification of the call-site coordinates plus four
adversarial refutation passes over value-safety, census completeness,
instrumentation, and comment accuracy.
Those reviews found real defects in this patch, all of them in what the patch
asserted rather than what it computed, and all are fixed in the third commit: a
line citation that a rebase had turned into a closing brace; a negative existence
claim ("no jitcode-keyed spelling of this read exists") refuted by three live
ones; a causal claim about the census that this patch's own change inverted; a
from_jitcode_pcdoc premise false at two of five callers; and a binding leftunused by the change.
Section 2 carried one finding: that the
(0, empty pcdep)fallback "silentlyfabricates an empty stack map" and these paths "should decline/abort
reconstruction when their required JitCode metadata is absent". It is not
resolved here, deliberately, and it is not introduced by this patch — the same
leg produced
(0, empty)before, by an accident of the offset falling out ofrange rather than by decision. Turning it into a genuine decline changes
reconstruction outcomes on the 20 benches above and needs its own before/after
measurement and gate. What this patch does instead is make the decline
representable and measured, which is the precondition for that work. The
counterfactual table above is the evidence for the follow-up.
Assisted-by: Claude.🤖 Generated with Claude Code
Summary by CodeRabbit