Skip to content

bpo-38206: Clarify tp_dealloc requirements for heap allocated types. #16248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 27, 2019
Merged

bpo-38206: Clarify tp_dealloc requirements for heap allocated types. #16248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92a9087
bpo-38206: Clarify tp_dealloc requirements for heap allocated types.
ammaraskar Sep 18, 2019
291258b
Incorporate suggestions from review
ammaraskar Sep 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Doc/c-api/type.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ The following functions and structs are used to create

.. c:function:: PyObject* PyType_FromSpecWithBases(PyType_Spec *spec, PyObject *bases)

Creates and returns a heap type object from the *spec*.
Creates and returns a heap type object from the *spec*
(:const:`Py_TPFLAGS_HEAPTYPE`).

If *bases* is a tuple, the created heap type contains all types contained
in it as base types.
Expand Down
24 changes: 20 additions & 4 deletions Doc/c-api/typeobj.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,16 +654,31 @@ and :c:type:`PyType_Type` effectively act as defaults.)
the instance is still in existence, but there are no references to it. The
destructor function should free all references which the instance owns, free all
memory buffers owned by the instance (using the freeing function corresponding
to the allocation function used to allocate the buffer), and finally (as its
last action) call the type's :c:member:`~PyTypeObject.tp_free` function. If the type is not
subtypable (doesn't have the :const:`Py_TPFLAGS_BASETYPE` flag bit set), it is
to the allocation function used to allocate the buffer), and call the type's
:c:member:`~PyTypeObject.tp_free` function. If the type is not subtypable
(doesn't have the :const:`Py_TPFLAGS_BASETYPE` flag bit set), it is
permissible to call the object deallocator directly instead of via
:c:member:`~PyTypeObject.tp_free`. The object deallocator should be the one used to allocate the
instance; this is normally :c:func:`PyObject_Del` if the instance was allocated
using :c:func:`PyObject_New` or :c:func:`PyObject_VarNew`, or
:c:func:`PyObject_GC_Del` if the instance was allocated using
:c:func:`PyObject_GC_New` or :c:func:`PyObject_GC_NewVar`.

Finally, if the type is heap allocated (:const:`Py_TPFLAGS_HEAPTYPE`), the
deallocator should decrement the reference count for its type object after
calling the type deallocator. In order to avoid dangling pointers, the
recommended way to achieve this is:

.. code-block:: c

static void foo_dealloc(foo_object *self) {
PyTypeObject *tp = Py_TYPE(self);
// free references and buffers here
tp->tp_free(self);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a heap type, change this to use the slot API to be compliant with PEP384.

freefunc free_func = PyType_GetSlot(tp, Py_tp_free);
free_func(self);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP 384 defines a stable ABI – an API subset you can use when you can't recompile extensions for each minor version of CPython.

How is it relevant here?

Copy link
Contributor

@eduardo-elizondo eduardo-elizondo Sep 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@encukou it is very relevant! What you said is correct, but this PEP has the added benefit of making PyTypeObject opaque. This gives the type system control over the implementation of the type object. For instance, in our system, the type object has a very different implementation. Yet, we can correctly interface with C Extensions. Using tp_free directly ties the code to CPython's implementation.

By the way, this is only relevant for C extensions, that is, cpython/Modules + third-party libs. Internal code CPython code should be free to use all the implementation details it wants! Given that the updated doc has the context of heap types, it's a fair assumption to make :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that using the stable ABI is good, and we all migrate to it eventually. However, it's orthogonal to this change, which is about making the documentation correct. Pushing a stable ABI agenda in a fix for "Clarify that tp_dealloc must decref for heap allocated type" seems too sneaky for me – even though I count myself as a proponent of the stable ABI :)

In CPython, PyType_GetSlot is likely* much slower than using the slot directly, so it is not a clear win.
I'm for avoiding direct slot access (both in stdlib and extension modules), but only after a wider discussion and perhaps after also adding a fast (and unstable) API.

And again: the text above talks about PyTypeObject.tp_free. The example is confusingly out of sync with that. I think we should change both occurences, but please, do it in a different PR.

  • (I haven't measured, but there's a bunch of extra checks in CPython's PyType_GetSlot)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted back to tp->tp_free(self);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, didn't mean to try any sneaky stuff! 😛

I agree with your point, let's get to anything related to PEP384 independently.

Py_DECREF(tp);
}


**Inheritance:**

This field is inherited by subtypes.
Expand Down Expand Up @@ -1021,7 +1036,8 @@ and :c:type:`PyType_Type` effectively act as defaults.)

.. data:: Py_TPFLAGS_HEAPTYPE

This bit is set when the type object itself is allocated on the heap. In this
This bit is set when the type object itself is allocated on the heap, for
example, types created dynamically using :c:func:`PyType_FromSpec`. In this
case, the :attr:`ob_type` field of its instances is considered a reference to
the type, and the type object is INCREF'ed when a new instance is created, and
DECREF'ed when an instance is destroyed (this does not apply to instances of
Expand Down