gc_roots: walk the root stack by value; name the entry-bridge InvalidLoop reason - #987
Conversation
WalkthroughThe PR adds specific speculative-field validation errors, improves field descriptor resolution through the GC cache, and updates ChangesJIT validation and diagnostics
Root-stack safety
Estimated code review effort: 3 (Moderate) | ~20 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 dedff68). 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-object/src/gc_roots.rs`:
- Around line 706-714: Extend the existing growth test for the stack-root walk
to grow the buffer inside the visitor, assign a different root through slot,
then assert shadow_stack_get(index) after the walk. Keep the assertion focused
on confirming the mutation is written to the re-read buffer slot exercised by
the write-back in the root-walking function.
🪄 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 Plus
Run ID: 1dac4c78-1157-473e-8058-9c4ecefef44a
📒 Files selected for processing (3)
majit/majit-metainterp/src/pyjitpl.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-object/src/gc_roots.rs
![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: a249b7fefb
ℹ️ 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 struct_key = majit_ir::descr::LLType::Struct(p.type_id); | ||
| let mut gc = majit_ir::descr::gc_cache().lock().unwrap(); | ||
| gc.get_size_descr(struct_key.clone(), p.size, p.vtable as usize, false); |
There was a problem hiding this comment.
When an empty parent spec has p.type_id == 0, this publishes it under the shared LLType::Struct(0) key, even though zero is explicitly the no-identity sentinel (simple_descr_group_from_bh_size therefore fresh-mints such descriptors). Two unrelated type-id-less parents will consequently reuse the first size descriptor and field namespace, so speculative field validation can reject a valid bridge against the wrong size/vtable or attach a field to the wrong parent. Handle zero via the existing non-keyed fresh-mint path rather than caching it.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
…ngle the visitor's slot `walk_shadow_stack_cell` re-reads `base` for every slot so a re-entrant push that calls `grow` cannot strand the walk in the freed buffer, but it still handed the visitor a `&mut PyObjectRef` pointing into that buffer. A visitor that pins a root and then writes through its reference would write into the allocation `grow` had already freed. Read the slot instead, hand the visitor `&mut` to the local, and store it back through a re-read `base`. `PyObjectRef` is `*mut PyObject`, so this is a register copy either way, and `grow` copies the live prefix, so the index still names the same slot. Both walkers (`walk_shadow_stack` and `walk_shadow_stack_area`) go through this one loop. stack_check: record why `MAX_RECURSION_LIMIT` is the bound on the eager root stack allocation. `shadowstack.py:351-364` resizes on the spot, and `vm.py:83-88` adds its 10**6 ceiling for exactly that reason; a tighter bound would size the root stack below the recursion limit it exists to serve. Assisted-by: Claude
`compile_entry_bridge` discarded the `InvalidLoop` payload, so its log line reported only that the bridge was abandoned. The reason string is what distinguishes a speculative-field rejection from a quasi-immutable invalidation, and both reach this arm. Assisted-by: Claude
`protect_speculative_field` fails closed on every path where the type-validity verdict cannot be produced — a null gcptr, a missing `parent_descr`, an unresolvable typeid or subclass range, or a genuine type mismatch. It reported all of them as a bare `Err(())`, so the `InvalidLoop` the caller signals said only that a speculative read was refused. Return `&'static str` naming the branch instead, and log the field's identity next to it under `MAJIT_LOG`. On `exception_args_virtual` this resolves all six refusals to `field descr has no parent_descr` for an unnamed `FieldDescr(offset=32, size=8, Int)`, which is a parentless descr from the `field_descr_ref_from_bh` placeholder path rather than an ill-typed pointer. No counter moves: the four jit-stats fixtures measure identically before and after. Assisted-by: Claude
…tened field list
`field_descr_ref_from_bh` only reached `get_field_descr` when the attached
parent spec carried a non-empty `all_fielddescrs`; with an empty list it fell
through to the parentless `make_field_descr` placeholder even though the
producer had supplied the struct's size, type id and vtable.
`descr.py:238 parent_descr = get_size_descr(gccache, STRUCT, vtable)` derives
the parent from the STRUCT, not from the flattened field list — that list only
supplies `index_in_parent` (`descr.py:228`). Mint the SizeDescr from what the
producer did attach and route the field through `get_field_descr`, so the
descr carries a parent in this case too. The remaining parentless path (no
parent spec at all) is unchanged and now logs the field it fell back on.
A parentless descr made `protect_speculative_field` (`llmodel.py:560`, which
asserts the parent exists) fail closed with no type to validate against,
deferring an `InvalidLoop` that abandoned the bridge and left `compile_loop`
to abort on `has_compiled_targets`.
Both fixtures that had drifted return to their committed baselines:
exception_args_virtual loops_aborted 3 -> 0, guard_failures 1002 -> 401,
loops_compiled 2 -> 1
list_length_hint_validate loops_aborted 34 -> 14, guard_failures 4923 -> 828,
bridges_compiled 3 -> 4
check.py: 370/370 on dynasm and 370/370 on cranelift. cargo test on
majit-metainterp, majit-backend and pyre-object: 1840 passed, 0 failed.
Assisted-by: Claude
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)
majit/majit-metainterp/src/pyjitpl/dispatch.rs (1)
8511-8524: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate typed call arguments before dispatch.
arg_types.get(i)treats missing metadata as non-float, so extra argument values can be sent with too few type entries. Any extra trailing metadata is discarded because the loop followsargs. Addargs.len() == arg_types.len()validation before both host and native dispatch, matchType::IntandType::Refexplicitly, and reject invalid metadata such asType::Void.🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs` around lines 8511 - 8524, Update call_float_function to validate that args.len() equals arg_types.len() before either residual_host_call or native dispatch. Validate each metadata entry explicitly, accepting only Type::Float, Type::Int, and Type::Ref, while rejecting Type::Void or any unsupported type; then dispatch using the validated argument metadata without silently treating missing entries or trailing metadata as valid.
🤖 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 `@majit/majit-metainterp/src/pyjitpl/dispatch.rs`:
- Around line 8511-8524: Update call_float_function to validate that args.len()
equals arg_types.len() before either residual_host_call or native dispatch.
Validate each metadata entry explicitly, accepting only Type::Float, Type::Int,
and Type::Ref, while rejecting Type::Void or any unsupported type; then dispatch
using the validated argument metadata without silently treating missing entries
or trailing metadata as valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0721b8c-f528-4b00-bdcd-a00b2eee16af
📒 Files selected for processing (6)
majit/majit-backend/src/model.rsmajit/majit-metainterp/src/optimizeopt/mod.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rspyre/pyre-interpreter/src/stack_check.rspyre/pyre-object/src/gc_roots.rs
1 participant
Two review responses from PR #966, plus the diagnostic that found them.
gc_roots: walk the root stack by valuewalk_shadow_stack_cellalready re-readsbasefor every slot so a re-entrantpush that calls
growcannot strand the walk in the freed buffer — but it stillhanded the visitor a
&mut PyObjectRefpointing into that buffer. A visitorthat pins a root and then writes through its reference writes into the
allocation
growhad already freed.The slot is now read into a local, the visitor gets
&mutto the local, and thevalue is stored back through a re-read
base.PyObjectRefis*mut PyObject, so it is a register copy either way, andgrowcopies the liveprefix, so the index still names the same slot.
walk_shadow_stackandwalk_shadow_stack_areaboth go through this one loop.cargo test --release -p pyre-object gc_roots: 10 passed, 0 failed.stack_check: record whyMAX_RECURSION_LIMITis the boundThe review also asked for a bound on
root_stack_depthbefore the eagerallocation. That bound already exists and is upstream's:
shadowstack.py:351-364resizes on the spot, and
vm.py:83-88adds its 106 ceiling for exactly thatreason — "because huge values cause huge shadowstacks to be allocated (or
MemoryErrors)".
MAX_RECURSION_LIMITis that same 106. A tighter bound wouldsize the root stack below the recursion limit it exists to serve, so the change
here is the citation, not a new clamp.
jit: name theInvalidLoopreason in the entry-bridge logcompile_entry_bridgediscarded theInvalidLooppayload, so the log said onlythat the bridge was abandoned. The reason string is what separates a
speculative-field rejection from a quasi-immutable invalidation, and both reach
that arm. With it,
exception_reraise_tb_depth_jitstress's 1198 aborts resolved1:1 to
InvalidLoop(quasi immutable field changed during tracing)— the failure#977 has since fixed.
Summary by CodeRabbit
Bug Fixes
Documentation