On 2019-12-09 18:22, Christian Tismer wrote:
On 08.12.19 09:49, Nick Coghlan wrote:
On Fri., 6 Dec. 2019, 3:31 am Christian Tismer, <tismer@stackless.com <mailto:tismer@stackless.com>> wrote:
Hi guys,
during the last few weeks I have been struggling quite much in order to make PySide run with Python 3.8 at all.
The expected problems were refcounting leaks due to changed handling of heaptypes. But in fact, the runtime behavior was much worse, because I always got negative refcounts!
After exhaustive searching through the different 3.8 commits, I could isolate the three problems with logarithmic search.
The hard problem was this: Whenever PySide creates a new type, it crashes in PyType_Ready. The reason is the existence of the Py_TPFLAGS_METHOD_DESCRIPTOR flag. During the PyType_Ready call, the function mro() is called. This mro() call results in a negative refcount, because something behaves differently since this flag is set by default in mro().
When I patched this flag away during the type_new call, everything worked ok. I don't understand why this problem affects PySide at all. Here is the code that would normally be only the newType line:
// PYSIDE-939: This is a temporary patch that circumvents the problem // with Py_TPFLAGS_METHOD_DESCRIPTOR until this is finally solved. PyObject *ob_PyType_Type = reinterpret_cast<PyObject *>(&PyType_Type); PyObject *mro = PyObject_GetAttr(ob_PyType_Type, Shiboken::PyName::mro()); auto hold = Py_TYPE(mro)->tp_flags; Py_TYPE(mro)->tp_flags &= ~Py_TPFLAGS_METHOD_DESCRIPTOR; auto *newType = reinterpret_cast<SbkObjectType *>(type_new(metatype, args, kwds)); Py_TYPE(mro)->tp_flags = hold;
Isn't this manipulating the flags in the tuple type, rather than anything on a custom object? Or is "mro" a custom object rather than an MRO tuple?
If anything, given the combination of factors required to reproduce the problem, I would guess that there might be a ref counting problem in the __set_owner__ invocations when called on a new type rather than a regular instance, and that was somehow affected by the change to increment the type refcount in PyObject_Init rather than PyType_GenericAlloc.
Hi Nick,
after staring long at the code, I fount something funny in typeobject.c #286 ff:
static void type_mro_modified(PyTypeObject *type, PyObject *bases) { /* Check that all base classes or elements of the MRO of type are able to be cached. This function is called after the base classes or mro of the type are altered.
Unset HAVE_VERSION_TAG and VALID_VERSION_TAG if the type has a custom MRO that includes a type which is not officially super type, or if the type implements its own mro() method.
Called from mro_internal, which will subsequently be called on each subclass when their mro is recursively updated. */ Py_ssize_t i, n; int custom = (Py_TYPE(type) != &PyType_Type); int unbound; PyObject *mro_meth = NULL; PyObject *type_mro_meth = NULL;
if (!PyType_HasFeature(type, Py_TPFLAGS_HAVE_VERSION_TAG)) return;
if (custom) { _Py_IDENTIFIER(mro); mro_meth = lookup_maybe_method( (PyObject *)type, &PyId_mro, &unbound); if (mro_meth == NULL) goto clear; type_mro_meth = lookup_maybe_method( (PyObject *)&PyType_Type, &PyId_mro, &unbound); if (type_mro_meth == NULL) goto clear; if (mro_meth != type_mro_meth) goto clear; Py_XDECREF(mro_meth); Py_XDECREF(type_mro_meth); }
Look at the "if (custom)" clause. "mro_meth = lookup_maybe_method(" uses lookup_maybe_method which gives a borrowed reference. The same holds for "type_mro_meth".
But then both are decreffed, which IMHO is not correct.
Look at what happens at the label "clear": it DECREFs them. If mro_meth != NULL or mro_meth != type_mro_meth, they'll get DECREFed at "clear".