Convert _ctypes static types (and the "meta class") to heap types

[vstinner]

Feature 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:

• April 2023: PR gh-103092: Adapt _ctypes's PyCStgDict type to heap type #103932
• January 2024: PR gh-103092: Port some _ctypes data types to heap types #113630

Linked PRs

• gh-114314: Move types to ctypes_state #114316
• gh-114314: Avoid possibly invalid args when initializing a ctypes meta type #114740
• gh-114314: ctypes: remove stgdict and switch to heap types #116458

[vstinner]

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]

First step: merged PR #113857

[vstinner]

See merged PR #113620.

[encukou]

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).

[erlend-aasland]

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]

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]

@vstinner "PyCStgDict" words in the title and description are typos?

PyCStgDict is a dictionary subclass rather than a metaclass, I think.

[vstinner]

I changed the title to make it more generic.

[encukou]

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.

[encukou]

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.

[encukou]

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;

  This hack is removed as soon as the class becomes a heap type.
• 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?

[erlend-aasland]

This is good news, @encukou! Looking forward to review it :)

[neonene]

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]

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]

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]

@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:

• 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;
+   }

[encukou]

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.

[erlend-aasland]

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]

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