jit: stop recover_ref_value answering the NO_CONCRETE sentinel, and record why filtering it downstream is not the follow-up - #1380
Conversation
… value `recover_ref_value` returned whatever `concrete_of_opref` held, including `Value::Ref(GcRef::NO_CONCRETE)` -- the stamp `heapcache_ops` writes over a load the walk could not replay. The two blackhole-frame ref fills that reach this function directly match on the `Value::Ref` variant alone, so `usize::MAX - 1` was written into a resumed frame's ref bank as if it were an object address: `capture_inline_parent_blackhole` (jitcode_dispatch/resume_snapshot.rs) and `build_single_frame_miframe` (jitcode_dispatch/residual_call.rs). It now answers unresolved, which is what those sites already do for a color whose register holds no box: the image is declined and the outer flush falls back to the legacy replay. The receiver guard in the same function checked NULL and the all-ones tombstone by hand and missed the sentinel; it now calls `live_gc_ptr`, the one place that names all three. Sites that read the recorder's stamp table first and only then fall back to this function are NOT covered, because `Some(_).or_else(f)` never calls `f`: `fill_trace_too_long_register_banks` and the two `vable_ops` resolvers still see the sentinel. The test fails on the unfixed function (returns `Some(Ref(NO_CONCRETE))`) and carries a positive control so the assertion cannot pass by the stamp failing to land. Reported by Codex review on #1364. Assisted-by: Claude
…stamp is not the fix The recorder's stamp table reports `Value::Ref(GcRef::NO_CONCRETE)` verbatim -- `heapcache_ops`' materialized-array walk writes it over a load it could not replay. `recover_ref_value` rejects the sentinel; this method does not, because "never stamped" and "stamped unresolved" are different states and only the caller knows which one it can act on. Three sites pair the two stamp-first, so `Some(_).or_else(f)` short-circuits ahead of that guard: `vable_value_concrete`, `current_inline_vable_target` and `fill_trace_too_long_register_banks`. Two more accept the sentinel through a `value.0 != 0` arm, which excludes NULL but not `usize::MAX - 1`. Filtering it at those sites was proposed and does not hold: each one ends in a fallback tail, so rejecting the sentinel does not decline the image, it promotes `from_register` / `from_shadow` / `live_vable_frame_addr` -- a different concrete address landing in a blackhole frame's ref bank or, through `store_live_frame_array_slot`, in a live frame's locals array behind a write barrier. `vable_ops.rs` already records that re-reading the shadow it would promote can return a stale concrete value from a prior loop iteration. Comment only; no behaviour change. Assisted-by: Claude
`exception_bridge_traceback_head` reports `loops_compiled=2` with a single `[jit] compiled loop at key=` line, which reads as a double count. It is not. Five sites bump the counter, one per `Backend::compile_loop` caller, and only `compile_loop_body` logs that line. The second increment on that fixture is this site, reached only from the `bridge.is_none()` arm of `compile_finish_from_active_session`, and upstream counts a root FINISH trace with no LABEL as a loop as well: `ResumeFromInterpDescr.compile_and_attach` (compile.py:1006-1017) mints its own JitCellToken and calls `send_loop_to_backend(.., "entry bridge", ..)`. The backend-side `total_compiled_loops`, bumped at the `model.py:297` point, also reads 2 there -- two CompiledLoopTokens, two green keys. Comment only; no behaviour change. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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: 79837e2845
ℹ️ 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".
| // `bridge.is_some()` to `compile_trace_finish`), and a root | ||
| // trace that ends in FINISH with no LABEL attaches upstream | ||
| // through `ResumeFromInterpDescr.compile_and_attach` | ||
| // (compile.py:1006-1017), which mints its own JitCellToken and |
There was a problem hiding this comment.
This comment cites compile.py:1006-1017 even though the preceding line already identifies ResumeFromInterpDescr.compile_and_attach. Repository guidance requires upstream citations by symbol and permits line numbers only where no symbol pins the claim, because numeric references silently rot as upstream changes; keep the symbol reference and remove the range.
AGENTS.md reference: AGENTS.md:L188-L191
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 79837e2). 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
|
1 participant
Three commits on top of
origin/main. One behaviour change — the Codex review finding on #1364, which merged before the fix was written — and two comments recording measured refutations.The fix
recover_ref_valueno longer answers theNO_CONCRETEsentinel as if it were an address. It returned whateverconcrete_of_oprefheld, including theValue::Ref(GcRef::NO_CONCRETE)stampheapcache_ops' materialized-array walk writes over a load it could not replay. The two blackhole-frame ref fills that reach this function directly match on theValue::Refvariant alone, sousize::MAX - 1was written into a resumed frame's ref bank as an object address:capture_inline_parent_blackhole(the site Codex flagged) andbuild_single_frame_miframe. It now answers unresolved, which is what those sites already do for a color whose register holds no box — the image is declined and the outer flush falls back to the legacy replay. The receiver guard inside the same function checked NULL and the all-ones tombstone by hand and missed the sentinel; it now callslive_gc_ptr, the one predicate that names all three. Pinned by a test shown to fail on the unfixed function, with a positive control.The deliberately conservative variant was chosen: return
Nonerather than falling through to the producer-chain replay. Falling through could produce a different concrete value; answering unresolved can only turn a poisoned answer into a decline.Two comments, and why they are worth their diff
Both record work whose result was "do not change the code" — the kind of finding that gets rediscovered and re-attempted if it is not written where the next reader hits it.
lookup_opref_concrete: filtering the sentinel is not the follow-up fix it looks like. Three more sites pair the stamp table withrecover_ref_valuein the orderlookup_opref_concrete(..).or_else(|| recover_ref_value(..)), andSome(_).or_else(f)never callsf, so the stamp short-circuits ahead of the new guard. Two others accept the sentinel through avalue.0 != 0arm that excludes NULL but notusize::MAX - 1. The obvious patch — filter it at those sites — was written and then refuted: every one of them ends in a fallback tail (.or(from_register),.or(from_shadow),_ => sym.live_vable_frame_addr()), so rejecting the sentinel does not decline the image, it promotes the next fallback — a different concrete address into a blackhole ref bank or, throughstore_live_frame_array_slot, into a live frame's locals array behind a write barrier.vable_ops.rsalready records that re-reading the shadow it would promote "can return a stale concrete value from a prior loop iteration". Wrong data, not a refusal. The real question is per-site: what should each answer for an unresolved box? Filed as a follow-up; reachability there is inferred, never observed, and the first step is a measurement, not a patch.Related, and not changed here: the literal worry that started this — that the sentinel could reach
vable_setfield— is inert. That function doeslet _ = concrete;on one leg andconcrete.unwrap_or(Value::Ref(GcRef::NO_CONCRETE))on the other, soNoneandSome(sentinel)are indistinguishable inputs there.finish_and_compile: the secondloops_compiledis not a double count.exception_bridge_traceback_headreadsloops_compiled=2with a single[jit] compiled loop at key=line, which invites exactly that conclusion. Five sites bump the counter, one perBackend::compile_loopcaller, and onlycompile_loop_bodylogs that line. The second increment is this site, reached only from thebridge.is_none()arm, and upstream counts a root FINISH trace with no LABEL as a loop too —ResumeFromInterpDescr.compile_and_attach(compile.py:1006-1017) mints its own JitCellToken and callssend_loop_to_backend(.., "entry bridge", ..). The backend-sidetotal_compiled_loops, bumped at themodel.py:297point, independently reads 2 on that fixture: two CompiledLoopTokens, two green keys, two code buffers.Verification
cargo fmt --allclean;cargo check --all --all-targets --features dynasmrc=0 (the target set a rebase can break withoutcheck.pyever seeing it); full LLBC re-extract; thenpyre/check.py— dynasm 443/443, cranelift 443/443, wasm 436/436, 3/3 backend runs.🤖 Generated with Claude Code
https://claude.ai/code/session_01TBNBmnBveAQFbqRtrvQVyQ