posix, _queue, _types: what the #1359 review threads left, and the frame the method-load opcodes pushed through - #1367
Conversation
…frame `PyFrame` overrides the anchored shared bodies for both opcodes and pushed through `&mut self` after `getattr_str` and `load_special_resolve`, which run app-level code and allocate. A minor collection there relocates a JIT-created frame, so the push landed on the abandoned copy -- the failure c0c99bd described for the shared helpers. `load_method` also reads the popped receiver back after that call to compute the bound value, and returns it for the `null_or_self` slot, so the receiver is pinned across the lookup and read out of its slot. Assisted-by: Claude
…t across `collect_iterable` returns a plain vector with its own root scope already dropped, so `setgroups` and `posix_spawn`'s `setsigmask` / `setsigdef` held every unconverted entry in stale locals: converting one entry reaches `__index__`, and the collection that runs there moves the entries behind it. `build_posix_spawn` likewise kept the three bound arguments and the keyword dictionary in plain locals across `__fspath__`, a mapping's `keys()` and `__index__`, and `parse_spawn_scheduler` read both tuple elements before converting the priority. Each is pinned and read back at its use. `sched_setaffinity` takes the same vector but converts it with `isinstance` against a fixed type and `int_w` on an already-checked int, neither of which reaches app-level code. Assisted-by: Claude
`_queue_SimpleQueue_get_impl` runs `_PyTime_FromSecondsObject` before the sign
check, so a value with no nanosecond timestamp is refused rather than turned
into a wait. Reading the seconds as a plain float instead made
`get(timeout=float('inf'))` block forever and `get(timeout=float('nan'))` poll
once. The bounds and the two messages are the ones `parse_acquire_args`
already applies for `_thread.lock.acquire`.
CPython 3.14.2 answers `OverflowError: timestamp out of range for platform
time_t` for an infinity and `ValueError: Invalid value NaN (not a number)` for
a NaN.
Assisted-by: Claude
`PyCapsule_Type` carries no `tp_new` and no `Py_TPFLAGS_BASETYPE`; both the cpyext type and the fallback `types.CapsuleType` inherited `object.__new__` and accepted subclasses, so either could produce an object that carries the capsule's name without the payload `is_capsule` reads. Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 22 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 (5)
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: 3c2e302801
ℹ️ 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".
| pyre_object::dictmultiobject::w_dict_setitem_str_no_proxy( | ||
| ns, | ||
| "__new__", | ||
| crate::typedef::make_new_descr(|_| { |
There was a problem hiding this comment.
__new__ out of the capsule type dictionary
When code introspects types.CapsuleType, this insertion makes "__new__" in vars(types.CapsuleType) true and exposes a capsule-specific descriptor, whereas CPython's null tp_new and PyPy's W_Capsule.typedef both leave __new__ absent while still rejecting construction. The fallback type in _types/mod.rs duplicates the same observable mismatch; construction should be disabled in the type machinery without adding a dictionary entry.
AGENTS.md reference: AGENTS.md:L165-L166
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3c2e302). 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
Four follow-ups to the review threads on #1359, each verified against the source it claims to follow.
PyFramepushed LOAD_METHOD and LOAD_SPECIAL results through a stale framePyFrameoverrides the anchored shared bodies for both opcodes and pushed through&mut selfaftergetattr_strandload_special_resolve, which run app-level code and allocate. A minor collection there relocates a JIT-created frame, so the push landed on the abandoned copy — the failure c0c99bd fixed for the shared helpers, still open in the two overrides that bypass them.load_methodalso reads the popped receiver back after that call to compute the bound value, andcompute_load_method_boundreturns it for thenull_or_selfslot, so a relocated receiver could be pushed as the call's receiver. It is pinned across the lookup and read out of its slot.The spawn calls converted sequences and options across their own collection points
collect_iterablereturns a plain vector with its own root scope already dropped, sosetgroupsandposix_spawn'ssetsigmask/setsigdefheld every unconverted entry in stale locals: converting one entry reaches__index__, and the collection that runs there moves the entries behind it.build_posix_spawnlikewise kept the three bound arguments and the keyword dictionary in plain locals across__fspath__, a mapping'skeys()and__index__, andparse_spawn_schedulerread both tuple elements before converting the priority.sched_setaffinitytakes the same vector but converts it withisinstanceagainst a fixed type andint_won an already-checked int, neither of which reaches app-level code, so it is left alone.SimpleQueue.get(timeout=float('inf'))blocked forever_queue_SimpleQueue_get_implruns_PyTime_FromSecondsObjectbefore the sign check, so a value with no nanosecond timestamp is refused rather than turned into a wait. Reading the seconds as a plain float instead made an infinity block forever and a NaN poll once. CPython 3.14.2 answersOverflowError: timestamp out of range for platform time_tandValueError: Invalid value NaN (not a number); the bounds and both messages are the onesparse_acquire_argsalready applies for_thread.lock.acquire.types.CapsuleTypewas constructible and subclassablePyCapsule_Typecarries notp_newand noPy_TPFLAGS_BASETYPE. Both the cpyext type and the fallback inheritedobject.__new__and accepted subclasses, so either could produce an object carrying the capsule's name without the payloadis_capsulereads.Verification
cargo check -p pyre-interpretergreen fordynasmanddynasm,cpyext. The fullpyre/check.pygate and the CPython suite are running locally against this tree; the same four changes passed dynasm 442/442, wasm 435/435 and the CPython suite with no regressions on the pre-rebase base, where the one cranelift red wasgenerator_tree_recursion, the documented boundary flake, attributed away by timing it directly against a control binary (0.74 s vs 0.79 s median).🤖 Generated with Claude Code
https://claude.ai/code/session_01PuYePQknDcMCUsy8omQ1fh