cpyext: unrealized bytes, the refcount header, and a type mirror that is not immortal - #1278
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. 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 (26)
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: c31b72b45c
ℹ️ 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".
| super::modsupport::forget_module_fields(address); | ||
| super::unicodeobject::forget_block(address); | ||
| super::bytesobject::forget_pending(address); | ||
| super::typeobject::forget_type_name(address); |
There was a problem hiding this comment.
When an interpreter-defined heap class uses a C-defined metaclass with tp_dealloc, deallocating its synthesized type mirror reaches this line. Removing TYPE_NAMES drops the CString backing tp_name before the callback is invoked below, so a metaclass deallocator that inspects ((PyTypeObject *)obj)->tp_name dereferences freed memory. Keep the name in the upstream heap-type ownership shape, or at minimum defer its removal until after tp_dealloc returns.
AGENTS.md reference: AGENTS.md:L115-L118
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit c88e34d). 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
|
`PyBytes_FromStringAndSize(NULL, size)` returns a mirror with no `bytes` behind it and a `size + 1` buffer, as `bytesobject.py:64 new_empty_str` does. The buffer is the mirror's `cached_bytes` entry -- the side table that stands in for the `ob_sval` field upstream's mirror carries -- so `PyBytes_AS_STRING` hands out one address before and after the object exists. `pyobject::realize` builds the `bytes` from what the buffer holds and links the two (`bytesobject.py:100 bytes_realize`), which is the same point the `str` `PyUnicode_New` allocates is realized at. `PyBytes_Check`, `CheckExact`, `Size`, `AsString` and `AsStringAndSize` answer from the buffer while the mirror is pending, so reading one mid-fill does not build the object early; the deallocator drops the pending record, so a mirror released without ever being read is freed with no `bytes` behind it. The refcount macros move from `object.h` to a new `refcount.h`, which `Python.h` includes after `pyre_decl.h`, and `Py_NewRef` / `Py_XNewRef` join them. They are `static inline` functions calling `Py_IncRef`, which is declared in the generated header, so they cannot sit beside `Py_INCREF`. `cpyext_methods` covers both: `bytes_fill`, `bytes_pairs`, `bytes_empty` and `new_ref`. Assisted-by: Claude
…L class Reading a mirror allocates now: `from_ref` realizes the `str` `PyUnicode_New` and the `bytes` `PyBytes_FromStringAndSize(NULL, size)` hand out unlinked, so `argument` / `arguments` are collection points. Seven entry points still had the older order -- mint the interpreter object, then read the receiver -- which leaves the freshly minted object unpinned across that realization: `PyMapping_GetItemString`, `PyMapping_DelItemString`, `PyMapping_HasKeyString`, `PyList_GetSlice`, `PySequence_GetSlice`, `PySequence_SetSlice` and `PySequence_DelSlice`. Each gets the `realize_all` prologue the two list entry points already had, and `key_of`'s comment states the order that now holds. A census of every mint-before-convert site in `cpyext/` finds no others; `read_member` mints and converts in exclusive `match` arms. `import_module` pinned `__import__` after building the name, so the slot was seeded with a pre-move address; the two pins swap. `PySequence_Index` and `PySequence_Count` held the unpacked element `Vec` and the searched-for value in Rust locals across `eq_w`, which runs the element's own `__eq__`. Both take the shadow-stack discipline `PyDict_Merge` uses. `PyErr_Restore` left the previous exception standing when `ptype` was NULL and `pvalue` was not an exception. `errors.c:62-67` clears on a NULL class whatever the value is. On the same path `errors.c:77-86` builds an instance by calling the class when the value is NULL, which `normalized` already did, so that case no longer clears instead. `cpyext_methods.restore()` covers both. Assisted-by: Claude
`PY_VERSION_HEX` was a hard-coded 0x030E06F0 and the release level and serial were not defined at all. Define `PY_RELEASE_LEVEL_ALPHA` / `BETA` / `GAMMA` / `FINAL`, `PY_RELEASE_LEVEL`, `PY_RELEASE_SERIAL` and `PY_VERSION`, and compute `PY_VERSION_HEX` from the five parts. Add `PYRE_VERSION` and `PYRE_VERSION_NUM` beside them. The computed value equals the constant it replaces. `cpyext_methods` gains a fixture reading the macros back and comparing them against `sys.version_info`, `sys.hexversion` and `sys.pyre_version_info`. Also drop a stale count from the same doc section: the number of exports CPython does not declare is 52, not the 35 it named. Assisted-by: Claude
`cpyext-abi.py check` read only the `#[unsafe(no_mangle)] extern "C"` exports. The header also ships `static inline` functions -- the variadic ones and `PyArg_UnpackTuple` -- which an extension calls the same way, and which are hand-written rather than generated, so nothing compared them against CPython's declaration. Parse them out of `include/pyre3.14t/*.h` and run them through the same comparison. The gate now reports 332 exports and 12 header inlines; all 12 match as they stand. Assisted-by: Claude
`ensure_mirror` stamped `REFCNT_IMMORTAL` on every type mirror it allocated, and `describe_interpreter_type` re-stamped it. A type mirror is minted for the type of every object handed to C, so any class an extension merely observed became permanently uncollectable. Nothing upstream ever mints an immortal mirror: `_Py_IMMORTAL_REFCNT` is only read there, and `create_ref` leaves every mirror at exactly the link share. Allocate a synthesized type mirror at `REFCNT_FROM_PYRE`, and set `Py_TPFLAGS_HEAPTYPE` on it from the interpreter type's `flag_heaptype`. A block now holds a reference to its own type's mirror when that type is a heap type (`pyobject.py:91-93`), released in `dealloc` (`object.py:72-73`); `set_ob_type` moves that reference when a block is re-pointed. An extension's own `PyTypeObject` static stays immortal, since that storage is the library's. `tp_name` was a leaked `CString::into_raw`, justified by the mirror being immortal. It is now held in a table keyed by the mirror's address and dropped with the mirror. `allocate_raw` clears the `CPyObject` header when the caller did not ask for zeroed memory: `PyObject_Init` reads `ob_pyre_link` to tell an initialized block from raw bytes, and `PyObject_Malloc` handed out uninitialized ones. `cpyext_methods` reads `Py_TYPE(x)->tp_name` and the heap-type flag back for a built-in and for a Python class, and asserts a class the fixture passed to C is collected after two collections -- two because the instance mirror's reference to its type is given back by the first drain. Assisted-by: Claude
Threading the red `frame` operand through `load_super_attr` moved this pattern's `guard_failures` from 1010 to 1008. `loops_compiled` and `bridges_compiled` are unchanged at 6 and 5. The value is the one all three CI hosts observed -- ubuntu-24.04, macos-latest and windows-latest each reported 1008 -- so the shared baseline is re-recorded rather than given a per-platform overlay. Assisted-by: Claude
c31b72b to
c88e34d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c88e34d636
ℹ️ 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".
| global_super, | ||
| self_value, | ||
| cls_value, | ||
| frame_var.into(), |
There was a problem hiding this comment.
When LOAD_SUPER_ATTR runs in an inlined non-portal callee, this passes frame_var, but the same codewriter explicitly documents that this variable aliases the outermost caller frame for such callees. bh_load_super_attr_fn then calls builtin_super_from_frame on that caller, so zero-argument super() reads the caller's slot 0 and __class__ cell; for the added register -> parse_hookimpl_opts shape, the caller has no __class__ cell and the hot inlined path raises instead of constructing the callee's super proxy. The operand must be the callee's own per-frame red frame rather than the shared portal anchor.
AGENTS.md reference: AGENTS.md:L32-L42
Useful? React with 👍 / 👎.
1 participant
Six commits on the extension slice, on top of
main.PyBytes_FromStringAndSize(NULL, size)andPy_NewRefbytesobject.py:64 new_empty_str/:100 bytes_realize— the second ofthe two forms that hand out a mirror before its interpreter object
exists. The caller fills the buffer and
pyobject::realizebuilds thebytesat the first read;PyBytes_AsString/AsStringAndSize/Sizeand the type tests answer from the buffer, so asking themmid-fill does not decide the contents early.
Py_NewRef/Py_XNewRefneeded arefcount.h: they are inlinefunctions calling
Py_INCREF, which expands to the declaredPy_IncRef, so the six refcount macros moved out ofobject.hinto aheader
Python.hincludes afterpyre_decl.h.Review findings from #1264
Six of them were one family, caused by making
from_refrealize: anargument()can now allocate, so every entry point that mints aninterpreter object after converting one had to realize first. A census
of the entry points found the same seven sites the reviewers named and
no others.
PyErr_Restore's three degenerate inputs were reshaped toerrors.c:62-67, 77-86.patchlevel.hin partsPY_VERSION_HEXwas a hard-coded0x030E06F0and the release level andserial were not defined at all. It is now computed from the five parts;
the computed value equals the constant it replaces. A fixture reads the
macros back and compares them against
sys.version_info,sys.hexversionandsys.pyre_version_info.The ABI gate covers the header's inline functions
cpyext-abi.py checkread only the exports. The header also shipsstatic inlinefunctions — the variadic ones andPyArg_UnpackTuple—which an extension calls the same way and which are hand-written rather
than generated, so nothing compared them against CPython's declaration.
The gate now reports 332 exports and 12 header inlines; all 12 match
as they stand. Verified the gate can fail: giving
PyArg_UnpackTuplealongwhere CPython saysconst char *exits 1 and names the argument.A synthesized type mirror is no longer immortal
ensure_mirrorstampedREFCNT_IMMORTALon every type mirror. Atype mirror is minted for the type of every object handed to C, so any
class an extension merely observed became permanently uncollectable.
Nothing upstream ever mints an immortal mirror —
_Py_IMMORTAL_REFCNTis only read there, and
create_refleaves every mirror at exactly thelink share. What
Py_TPFLAGS_HEAPTYPEdecides is (a) whose storage theblock is (
typeobject.py:716-722) and (b) whether a block holds areference to its own type (
pyobject.py:91-93, released inobject.py:72-73). Both are implemented; an extension's ownPyTypeObjectstatic stays immortal, since that storage is thelibrary's.
tp_namewas a leakedCString::into_rawjustified by themirror being immortal, and is now dropped with the mirror.
One adjacent defect fell out of this:
PyObject_Initreadsob_pyre_linkto tell an initialized block from raw bytes, andPyObject_Mallocwas handing out uninitialized ones.allocate_rawnowdefines the header either way.
Verification
1/2/3
gc.collect()calls →False/True/True; two are neededbecause the instance mirror's reference to its heap type is given back
by the first drain, and the test says so.
assertion fail.
dynamically created classes routed through the C accelerator via
__html__, which is what exercises the new heap-type mirror lifecycle.— opened by Claude