Repository navigation
Convert _ctypes static types (and the "meta class") to heap types #114314
Description
Activity
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_Typecan be retrieved fromglobal_state.PyCStructType_Typeglobal 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 isPyCStructType_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 fromdict).
Example showing that tp_dict type is
PyCStgDict_Typeinstead ofdict:>>> 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'>-
- added a commit that references this issue
on Jan 19, 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 byPyObject_GetTypeData(o, _ctypes.PyCStructType).Reacted by Erlend E. AaslandIt 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.
- 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, 2024 I changed the title to make it more generic.
Reacted by neoneneCould 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 LongUnfortunately, while I did put in some time, I don't have a concrete PR just yet.
_ctypesis 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. AaslandSince 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-stgdictWhat this does:
- Instead of using
StgDict, a dict subclass with additional info that gets substituted astp_dictof the classes, the data is stored in the type instance itself, inStgInfo. The metaclass,PyCType_Type, is defined withbasicsize = -sizeof(StgInfo)(i.e. “reserve extra space forStgInfo), and the info is accessed by a helper that callsPyObject_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 asPyTypeObject, but as:This hack is removed as soon as the class becomes a heap type.typedef struct { PyHeapTypeObject ht; StgInfo info; } _HackyHeapType;
- Now that the metaclass
tp_newdoesn't overridetp_dictof its class, I feel comfortable moving the initialization logic totp_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
StgDictvariables (stgdict,dict,edictetc.) as I moved toStgInfo(so,stginfo,info,einfo, etc.). Where the dict is actually used for class attributes (as a regularPyDict), I named itattrdict.
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 neoneneReacted by Erlend E. Aasland- Instead of using
This is good news, @encukou! Looking forward to review it :)
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(andCreateSwappedType) 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.- Guard against calling
test_ctypes leaked [17495, 17467, 17489, 17479] references, sum=69930Not fully confirmed with a crash, but it seems like
CType_Type_dealloc()preferstype_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); }Reacted by Petr ViktorinGuard against calling
__init__twice or not at allThe 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_cinitslot. 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.Reacted by neonene@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
- callbacks.c
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_");- callproc.c
ffi_type *_ctypes_get_ffi_type(PyObject *obj) { + if (obj == NULL) { + return &ffi_type_sint; + }Reacted by Petr ViktorinThanks 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_ctypesmodule 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. AaslandI guess isolation of
_ctypesmodule 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.
- added 2 commits that reference this issue
on Apr 2, 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
Feature or enhancement
The
_ctypesextension implements types as static types. They should be converted to heap types: see issue gh-103092 for the rationale.Problem: the
PyCStgDicttype 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 thePyCStructType_new()function.Previous attempts to convert
_ctypestypes to heap types:_ctypesdata types to heap types #113630Linked PRs