GeezerVison is showing github.com — Convert _ctypes static types (and the "meta class") to heap types · Issue #114314 · python/cpython
Convert _ctypes static types (and the "meta class") to heap types #114314DescriptionFeature or enhancement
The _ctypes extension implements types as static types. They should be converted to heap types: see issue gh-103092 for the rationale.
Problem: the PyCStgDict type is kind of special, it's a "metaclass" which creates a new class and then replaces the class namespace (type dict) with a custom "stgdict". See the PyCStructType_new() function.
Previous attempts to convert _ctypes types to heap types:
Linked PRs
Reacted by Erlend E. Aasland and An Long Activityvstinner commented on Jan 19, 2024 Example with _ctypes.Structure:
$ ./python
>>> import _ctypes
>>> type(_ctypes.Structure)
<class '_ctypes.PyCStructType'>
>>> _ctypes.Structure.__bases__
(<class '_ctypes._CData'>,)
>>> _ctypes.Structure.__base__
<class '_ctypes._CData'>
In Python, such type would be defined with a syntax like:
class Structure(_CData, metaclass=PyCStructType):
...
When a subclass is created, PyCStructType_new() is called. Example of subclass:
class MyStruct(_ctypes.Structure):
...
In C, PyCStructType_Type can be retrieved from global_state.PyCStructType_Type global variable.
(gdb) p global_state.PyCStructType_Type.tp_new
$12 = (newfunc) 0x7fffea18739f <PyCStructType_new>
(gdb) p global_state.PyCStructType_Type.tp_init
$13 = (initproc) 0x59321c <type_init>
(gdb) p global_state.PyCStructType_Type.tp_name
$14 = 0x7fffea57a230 "_ctypes.PyCStructType"
Main steps of PyCStructType_new():
-
Create a new type by calling PyType_Type.tp_new(type, args, kwds) where type is PyCStructType_Type.
-
Type members are initialized to:
- tp_name = "MyStruct"
- tp_new = GenericPyCData_new() -- inherited from
_ctypes.Structure.tp_new
- tp_init = Struct_init() -- inherited from
_ctypes.Structure.tp_init
-
Override tp_dict with an instance of ctypes PyCStgDict_Type (which inherits from dict).
Example showing that tp_dict type is PyCStgDict_Type instead of dict:
>>> import _ctypes
>>> import _testcapi
>>> class MyStruct(_ctypes.Structure): pass
...
>>> d=_testcapi.type_get_tp_dict(MyStruct) # I added this function locally just for this example
>>> type(d)
<class 'StgDict'>
vstinner commented on Jan 22, 2024 vstinner commented on Jan 22, 2024 encukou commented on Jan 23, 2024 It seems that stgdict is a workaround for the issue PEP-697 is designed to solve: have additional C data on a type.
If no one else is interested in this, I can try my hand at replacing the extra stgdict fields by PyObject_GetTypeData(o, _ctypes.PyCStructType). Reacted by Erlend E. Aasland erlend-aasland commented on Jan 23, 2024
It seems that stgdict is a workaround for the issue PEP-697 is designed to solve: have additional C data on a type. If no one else is interested in this, I can try my hand at replacing the extra stgdict fields by PyObject_GetTypeData(o, _ctypes.PyCStructType).
I agree; I already mentioned this in one of my (now closed) PRs. I think PEP-697 would be perfect for this. neonene commented on Jan 30, 2024
replacing the extra stgdict fields by PyObject_GetTypeData(o, _ctypes.PyCStructType).
@encukou Could you be more specific, please? I posted a PR #114740, which seems to have become a different attempt. neonene commented on Jan 30, 2024 @vstinner "PyCStgDict" words in the title and description are typos?
PyCStgDict is a dictionary subclass rather than a metaclass, I think.
changed the title [-]Convert _ctypes PyCStgDict meta static type to a meta heap type[/-] [+]Convert _ctypes static types (and the "meta class") to heap types[/+] on Jan 31, 2024vstinner commented on Jan 31, 2024 I changed the title to make it more generic. encukou commented on Jan 31, 2024
Could you be more specific, please?
I am now getting familiar with _ctypes (by implementing a proof of concept of my proposal).
I'll give you something more specific next week. Reacted by neonene, Erlend E. Aasland and An Long encukou commented on Feb 9, 2024 Unfortunately, while I did put in some time, I don't have a concrete PR just yet. _ctypes is big :(
The approach I'm trying is to put the extra (non-dict) stuff from StgDict in in the types, using the PEP-697 mechanism (relative basicsize & PyObject_GetTypeData). The tp_dict can then remain as a regular dict.
So far I haven't hit any unsurmountable obstacle, but I also haven't been able to align all the moving parts to make it work. I'll return to it next week. Reacted by Erlend E. Aasland encukou commented on Feb 23, 2024 Since the last comment, it took 2 weeks (not full-time of course), but I have a rough proof of concept. Bad news is that it's big (9 files changed, 1358 insertions(+), 1365 deletions(-)), and leaky as I ignored some details for the PoC (test_ctypes leaked [17495, 17467, 17489, 17479] references, sum=69930).
The PoC is here: main...encukou:cpython:ctypes-no-stgdict
What this does:
- Instead of using
StgDict, a dict subclass with additional info that gets substituted as tp_dict of the classes, the data is stored in the type instance itself, in StgInfo. The metaclass, PyCType_Type, is defined with basicsize = -sizeof(StgInfo) (i.e. “reserve extra space for StgInfo), and the info is accessed by a helper that calls PyObject_GetTypeData(type, state->PyCType_Type).
- To fit this info in while the types weren't yet converted (and so didn't honor the metaclass's
basicsize), I temporarily defined them not as PyTypeObject, but as:
typedef struct {
PyHeapTypeObject ht;
StgInfo info;
} _HackyHeapType;
- Now that the metaclass
tp_new doesn't override tp_dict of its class, I feel comfortable moving the initialization logic to tp_init. (There should be guards against calling __init__ twice or not at all -- my PoC doesn't do this yet.)
To keep everything straight during the transition, I renamed all the StgDict variables (stgdict, dict, edict etc.) as I moved to StgInfo (so, stginfo, info, einfo, etc.). Where the dict is actually used for class attributes (as a regular PyDict), I named it attrdict.
This made things much more straightforward, but it also results in a big diff. I wonder if it's worth minimizing the diff.
The next steps would be:
- Guard against calling
__init__ twice or not at all, and test this
- Remove the refleak :)
But I see the end of the tunnel; I'm rather confident it's possible!
Does this still look like a reasonable approach? Would anyone want to review the big PR, once it's done? Reacted by neonene Reacted by Erlend E. Aasland erlend-aasland commented on Feb 23, 2024 This is good news, @encukou! Looking forward to review it :) neonene commented on Feb 25, 2024 I agree with this dict type conversion.
- Guard against calling
__init__ twice or not at all,
I saw __init_subclass__ defined in typevarobject.c:
static PyMethodDef generic_methods[] = {
{"__init_subclass__", (PyCFunction)(void (*)(void))generic_init_subclass,
METH_VARARGS | METH_KEYWORDS | METH_CLASS, ...I like the tight sequence between __new__ and __init_subclass__. Then __init__ can follow if there is no error. If acceptable, PyCSimpleType_init (and CreateSwappedType) function might be safer to be invoked at __init_subclass__ phase.
The disadvantage is that __init_subclass__ receives no namespace argument, so the __build_class__ related names, including the __classcell__ and __classdictcell__ entries, need to be gathered from __dict__ and elsewhere. neonene commented on Feb 25, 2024
test_ctypes leaked [17495, 17467, 17489, 17479] references, sum=69930
Not fully confirmed with a crash, but it seems like CType_Type_dealloc() prefers type_dealloc(), which sub metatypes inherit:
CType_Type_dealloc(PyObject *self)
{
...
PyTypeObject *tp = Py_TYPE(self);
PyObject_GC_UnTrack(self);
(void)CType_Type_clear(self);
- tp->tp_free(self);
+ PyObject_GC_Track(self);
+ PyType_Type.tp_dealloc(self);
Py_DECREF(tp);
}encukou commented on Feb 26, 2024
Guard against calling __init__ twice or not at all
The main issue here is that __init__ can be called at any time by Python code. And so can __init_subclass__.
Another is that __init__ can be skipped by calling __new__ directly. (Most likely __init_subclass__ can too.)
So, there at least needs to be a C flag that prevents using unintialized data, and re-initializing data. So if you do the wrong thing in Python code, you get an exception, not a crash.
Possibly, we could instead initialize “lazily”, at first access, and skip initialization if things are already set up. But that's not always possible.
The ideal would be for the VM to do this for you -- e.g. we add a new tp_cinit slot. But I'm sticking to existing API here for now: I don't think I've seen enough use cases yet to generalize them into a good general API. neonene commented on Feb 26, 2024 @encukou I confirmed on Windows that the given PoC can pass ctypes tests with any heap type. The other refleaks came from PyType_GetDict(), which returns a new reference. And Windows-specific crashes could be fixed by a few changes like:
Details
TryAddRef(PyObject *cnv, CDataObject *obj)
{
IUnknown *punk;
+ PyObject *attrdict = _PyType_GetDict((PyTypeObject *)cnv);
+ if (!attrdict) {
+ return;
+ }
+ int r = PyDict_Contains(attrdict, &_Py_ID(_needs_com_addref_));
- int r = PyObject_GetAttrString((PyObject *)cnv, "_needs_com_addref_");ffi_type *_ctypes_get_ffi_type(PyObject *obj)
{
+ if (obj == NULL) {
+ return &ffi_type_sint;
+ }encukou commented on Mar 20, 2024 Thanks for all the encouragement and reviews!
I've merged the PR, and will watch the buildbots (and bug reports) for any unexpected issues.
I guess isolation of _ctypes module state should continue in #103092. It might take a few PRs, so maybe it could use a new issue. Reacted by neonene and Erlend E. Aasland erlend-aasland commented on Mar 20, 2024
I guess isolation of _ctypes module state should continue in #103092. It might take a few PRs, so maybe it could use a new issue.
I think it would be beneficial with a dedicated issue. It will not be straight-forward, I suspect. benmoran56 commented on Sep 1, 2024 It looks like this PR is causing a crash in pyglet - specifically our Windows com wrapper. I'm not familiar with the com code, so I haven't yet had time to determine if it's something we're doing wrong, or if we're hitting a edge case bug here.
pyglet/pyglet#1196
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|