gateway: publish the argument set at the two builtin dispatches that did not - #1229
Conversation
…did not `builtin_code_call` roots nothing of its own — `args` is a native slice no root walker updates, and the implementation it dispatches to runs Python. Nine of its eleven call sites publish `[code, args...]` on the shadow stack and read the slice back from it; two did not: * `funccall_valuestack`'s `PASSTHROUGHARGS1` arm built `args_w` from `peekvalue`/`make_arguments` and then called `dropvalues()`, which retires the frame slots that rooted those values. Its fixed-arity sibling publishes before dropping for that reason. * `call_builtin_with_args`, behind the `jit_call_known_builtin_N` helpers, passed the compiled call's own argument array straight through. Neither is reached today: `make_builtin_function_passthrough_args1` has no call site, and no trace emits `known_builtin_call_helper` — its addresses are published in `jit_fnaddr` but unselected. The contract is now stated on `builtin_code_call` itself. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 29 minutes 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?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 Plus Run ID: 📒 Files selected for processing (3)
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 ac80f25). 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
|
Follow-up from the #1209 / #1225 review thread. A complete census of
builtin_code_call's call sites, and the two that did not honour its rooting contract.The contract
builtin_code_callroots nothing of its own —argsis a native slice no root walker updates, and the implementation it dispatches to runs Python. Two callers already spell out why that matters:funccall_valuestack— "The Python frame slots cease to be roots whendropvalues()clears them, so publish the same live-variable set on pyre's shadow stack before doing so."call_builtin_code_positional— "A collection between the outerspace.call_functionreload and this indirect Rust function-pointer call updates the outer shadow slots but not the copied native slice."It was nowhere stated on
builtin_code_callitself, so each reader re-derives it. It is now.Census: 11 direct sites, 9 complied
Complied:
function.rs×5 (fixed arity 0..=4),call.rs:946,call.rs:949,call.rs:2573,call_jit.rs:5134.Did not:
funccall_valuestack'sPASSTHROUGHARGS1arm builtargs_wfrompeekvalue/make_arguments, then calleddropvalues()— which retires exactly the frame slots that rooted those values. Its fixed-arity sibling, 90 lines above, publishes before dropping for that stated reason.call_builtin_with_args, behind thejit_call_known_builtin_Nhelpers, passed the compiled call's own argument array straight through.Reachability — please read this before treating it as a bug fix
Neither site is reached today.
make_builtin_function_passthrough_args1andbuiltin_code_new_passthrough_args1have no call sites outside their own definitions, so no builtin carriesPASSTHROUGHARGS1; and no trace emitsknown_builtin_call_helper— its addresses are published injit_fnaddrbut unselected. That is the unsurprising part: a live violation of this invariant would already have crashed.Consequences: no test covers either path, and neither can be covered without first making it reachable. What this buys is that the first registration or first trace emission does not silently arm a GC bug.
Still unjudged
Whether
jit_may_forcekeeps the arguments in the walked jitframe is what separates "stale native copy" from "not rooted at all" for site 2. This change makes the site comply either way; it does not settle that question.Checked and refuted while doing this
call.rs:2544-2551(the C-profiler arm) holds an unrootedbound: Vecacross aw_str_from_wtf8loop. Not a defect:w_str_from_wtf8→malloc_typed→alloc_with_gc_header→std::alloc::allocis a plain system allocation, not a collection point.cargo check --all --tests --no-default-features --features dynasm→ exit 0.