Skip to content
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.

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

bpo-35810: Incref heap-allocated types in PyObject_Init #11661

Merged
merged 10 commits into from
Mar 27, 2019
67 changes: 67 additions & 0 deletions Doc/whatsnew/3.8.rst
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,12 @@ Build and C API Changes
``1`` for objects implementing ``__index__()``.
(Contributed by Serhiy Storchaka in :issue:`36048`.)

* Heap-allocated type objects will now increase their reference count
in :c:func:`PyObject_Init` (and its parallel macro ``PyObject_INIT``)
instead of in :c:func:`PyType_GenericAlloc`. Types that modify instance
allocation or deallocation may need to be adjusted.
(Contributed by Eddie Elizondo in :issue:`35810`.)


Deprecated
==========
Expand Down Expand Up @@ -661,6 +667,67 @@ Changes in the Python API
(Contributed by Xiang Zhang in :issue:`33106`.)


Changes in the C API
--------------------------

* Instances of heap-allocated types (such as those created with
:c:func:`PyType_FromSpec`) hold a reference to their type object.
Increasing the reference count of these type objects has been moved from
:c:func:`PyType_GenericAlloc` to the more low-level functions,
:c:func:`PyObject_Init` and :c:func:`PyObject_INIT`.
This makes types created through :c:func:`PyType_FromSpec` behave like
other classes in managed code.

Statically allocated types are not affected.

For the vast majority of cases, there should be no side effect.
However, types that manually increase the reference count after allocating
an instance (perhaps to work around the bug) may now become immortal.
To avoid this, these classes need to call Py_DECREF on the type object
during instance deallocation.

To correctly port these types into 3.8, please apply the following
changes:

* Remove :c:macro:`Py_INCREF` on the type object after allocating an
instance - if any.
This may happen after calling :c:func:`PyObject_New`,
:c:func:`PyObject_NewVar`, :c:func:`PyObject_GC_New`,
:c:func:`PyObject_GC_NewVar`, or any other custom allocator that uses
:c:func:`PyObject_Init` or :c:func:`PyObject_INIT`.

Example::

static foo_struct *
foo_new(PyObject *type) {
foo_struct *foo = PyObject_GC_New(foo_struct, (PyTypeObject *) type);
if (foo == NULL)
return NULL;
#if PY_VERSION_HEX < 0x03080000
Copy link
Contributor

Choose a reason for hiding this comment

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

In Cython, we normally use the exact alpha/beta version where the change was introduced (to support testing during the CPython release cycle), but we obviously don't know that yet.

Copy link
Member

Choose a reason for hiding this comment

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

The example is not really written for projects with Cython's level of involvement. Most projects will only test against a nightly build or the latest alpha/beta (if even that). The few others should know to adjust the example. We don't explicitly tell you to rename foo_new, either :)

// Workaround for Python issue 35810; no longer necessary in Python 3.8
PY_INCREF(type)
#endif
return foo;
}

* Ensure that all custom ``tp_dealloc`` functions of heap-allocated types
decrease the type's reference count.

Example::

static void
foo_dealloc(foo_struct *instance) {
PyObject *type = Py_TYPE(instance);
PyObject_GC_Del(instance);
#if PY_VERSION_HEX >= 0x03080000
// This was not needed before Python 3.8 (Python issue 35810)
Py_DECREF(type);
#endif
}

(Contributed by Eddie Elizondo in :issue:`35810`.)


CPython bytecode changes
------------------------

Expand Down
3 changes: 3 additions & 0 deletions Include/objimpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ _PyObject_INIT(PyObject *op, PyTypeObject *typeobj)
{
assert(op != NULL);
Py_TYPE(op) = typeobj;
if (PyType_GetFlags(typeobj) & Py_TPFLAGS_HEAPTYPE) {
Py_INCREF(typeobj);
}
_Py_NewReference(op);
return op;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Modify ``PyObject_Init`` to correctly increase the refcount of heap-
allocated Type objects. Also fix the refcounts of the heap-allocated types
that were either doing this manually or not decreasing the type's refcount
in tp_dealloc
6 changes: 5 additions & 1 deletion Modules/_curses_panel.c
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,10 @@ PyCursesPanel_New(PANEL *pan, PyCursesWindowObject *wo)
static void
PyCursesPanel_Dealloc(PyCursesPanelObject *po)
{
PyObject *obj = (PyObject *) panel_userptr(po->pan);
PyObject *tp, *obj;

tp = (PyObject *) Py_TYPE(po);
obj = (PyObject *) panel_userptr(po->pan);
if (obj) {
(void)set_panel_userptr(po->pan, NULL);
Py_DECREF(obj);
Expand All @@ -261,6 +264,7 @@ PyCursesPanel_Dealloc(PyCursesPanelObject *po)
remove_lop(po);
}
PyObject_DEL(po);
Py_DECREF(tp);
}

/* panel_above(NULL) returns the bottom panel in the stack. To get
Expand Down
3 changes: 0 additions & 3 deletions Modules/_tkinter.c
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,6 @@ Tkapp_New(const char *screenName, const char *className,
v = PyObject_New(TkappObject, (PyTypeObject *) Tkapp_Type);
if (v == NULL)
return NULL;
Py_INCREF(Tkapp_Type);

v->interp = Tcl_CreateInterp();
v->wantobjects = wantobjects;
Expand Down Expand Up @@ -802,7 +801,6 @@ newPyTclObject(Tcl_Obj *arg)
self = PyObject_New(PyTclObject, (PyTypeObject *) PyTclObject_Type);
if (self == NULL)
return NULL;
Py_INCREF(PyTclObject_Type);
Tcl_IncrRefCount(arg);
self->value = arg;
self->string = NULL;
Expand Down Expand Up @@ -2722,7 +2720,6 @@ Tktt_New(PyObject *func)
v = PyObject_New(TkttObject, (PyTypeObject *) Tktt_Type);
if (v == NULL)
return NULL;
Py_INCREF(Tktt_Type);

Py_INCREF(func);
v->token = NULL;
Expand Down
8 changes: 5 additions & 3 deletions Objects/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,9 @@ PyObject_Init(PyObject *op, PyTypeObject *tp)
return PyErr_NoMemory();
/* Any changes should be reflected in PyObject_INIT (objimpl.h) */
Py_TYPE(op) = tp;
if (PyType_GetFlags(tp) & Py_TPFLAGS_HEAPTYPE) {
Py_INCREF(tp);
}
_Py_NewReference(op);
return op;
}
Expand All @@ -239,9 +242,8 @@ PyObject_InitVar(PyVarObject *op, PyTypeObject *tp, Py_ssize_t size)
if (op == NULL)
return (PyVarObject *) PyErr_NoMemory();
/* Any changes should be reflected in PyObject_INIT_VAR */
op->ob_size = size;
Py_TYPE(op) = tp;
_Py_NewReference((PyObject *)op);
Py_SIZE(op) = size;
PyObject_Init((PyObject *)op, tp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just call _PyObject_INIT() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, actually, shouldn't PyObject_Init() just call _PyObject_INIT() ? Duplicating that code across two files seems unnecessary, maybe even a bit error prone.

Copy link
Member

Choose a reason for hiding this comment

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

PyObject_Init is defined in the same file, so the compiler should have everything it needs to inline as it sees fit.

As for code duplication, that's a valid point, but I'd call that out of scope for this PR. Let's keep orthogonal issues revertable separately.

return op;
}

Expand Down
5 changes: 5 additions & 0 deletions Objects/structseq.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,17 @@ static void
structseq_dealloc(PyStructSequence *obj)
{
Py_ssize_t i, size;
PyTypeObject *tp;

tp = (PyTypeObject *) Py_TYPE(obj);
size = REAL_SIZE(obj);
for (i = 0; i < size; ++i) {
Py_XDECREF(obj->ob_item[i]);
}
PyObject_GC_Del(obj);
if (PyType_GetFlags(tp) & Py_TPFLAGS_HEAPTYPE) {
Py_DECREF(tp);
}
}

/*[clinic input]
Expand Down
3 changes: 0 additions & 3 deletions Objects/typeobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -987,9 +987,6 @@ PyType_GenericAlloc(PyTypeObject *type, Py_ssize_t nitems)

memset(obj, '\0', size);

if (type->tp_flags & Py_TPFLAGS_HEAPTYPE)
Py_INCREF(type);

if (type->tp_itemsize == 0)
(void)PyObject_INIT(obj, type);
else
Expand Down