Python 3.8 problem with PySide
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; I would really like to understand the reason for this unexpected effect. Does this ring a bell? I have no clue what is wrong with PySide, if it is wrong at all. Thanks -- Chris -- Christian Tismer :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Karl-Liebknecht-Str. 121 : https://github.com/PySide 14482 Potsdam : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
No idea why gmail landed such an important email in the spam folder (i grit my teeth if pyside freezes @ <3.7) Abdur-Rahmaan Janhangeer http://www.pythonmembers.club | https://github.com/Abdur-rahmaanJ Mauritius
On Fri., 6 Dec. 2019, 3:31 am Christian Tismer, <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. Cheers, Nick.
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?
no, "mro" is the default mro implementation which is a method descriptor of the standard PyTypeType object. The implementation of PyType_Ready just touches the mro in some helper function lookup_maybe_method. This is so funny: This side effect seems to be totally unrelated to PySide, but something we are doing wrong.
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.
Thanks a lot! I will try to use that to find finally what's wrong. Cheers -- Chris -- Christian Tismer :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Karl-Liebknecht-Str. 121 : https://github.com/PySide 14482 Potsdam : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
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. Cheers -- Chris -- Christian Tismer :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Karl-Liebknecht-Str. 121 : https://github.com/PySide 14482 Potsdam : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
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".
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com> wrote:
On 2019-12-09 18:22, Christian Tismer wrote:
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".
I believe Christian's point is that this entire "if (custom) {" branch looks suspicious, as it assumes "lookup_maybe_method" will increment the refcount on the returned object. If that assumption is incorrect, we're going to get DECREFs without a preceding INCREF. The specific code path is also obscure enough that it's plausible the test suite may not currently cover it (as it requires doing something that calls "type_mro_modified" on a type with a custom metaclass). Cheers, Nick.
On 09.12.19 23:26, Nick Coghlan wrote:
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com <mailto:python@mrabarnett.plus.com>> wrote:
On 2019-12-09 18:22, Christian Tismer wrote: > > > 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".
I believe Christian's point is that this entire "if (custom) {" branch looks suspicious, as it assumes "lookup_maybe_method" will increment the refcount on the returned object. If that assumption is incorrect, we're going to get DECREFs without a preceding INCREF.
The specific code path is also obscure enough that it's plausible the test suite may not currently cover it (as it requires doing something that calls "type_mro_modified" on a type with a custom metaclass).
Thanks Nick for this nice analysis. And it's exactly this codepath that is taken in the case of PySide: custom types all the time :-) what-a-relief - ly y'rs -- Chris -- Christian Tismer-Sperling :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Karl-Liebknecht-Str. 121 : https://www.qt.io/qt-for-python 14482 Potsdam : GPG key -> 0xE7301150FB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
Hi all, Sorry for the noise, I was wrong, and I retract. I was somehow mislead and hunted a phantom. Best - Chris On 10.12.19 00:29, Christian Tismer wrote:
On 09.12.19 23:26, Nick Coghlan wrote:
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com <mailto:python@mrabarnett.plus.com>> wrote:
On 2019-12-09 18:22, Christian Tismer wrote: > > > 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".
I believe Christian's point is that this entire "if (custom) {" branch looks suspicious, as it assumes "lookup_maybe_method" will increment the refcount on the returned object. If that assumption is incorrect, we're going to get DECREFs without a preceding INCREF.
The specific code path is also obscure enough that it's plausible the test suite may not currently cover it (as it requires doing something that calls "type_mro_modified" on a type with a custom metaclass).
Thanks Nick for this nice analysis. And it's exactly this codepath that is taken in the case of PySide: custom types all the time :-)
what-a-relief - ly y'rs -- Chris
_______________________________________________ Python-Dev mailing list -- python-dev@python.org To unsubscribe send an email to python-dev-leave@python.org https://mail.python.org/mailman3/lists/python-dev.python.org/ Message archived at https://mail.python.org/archives/list/python-dev@python.org/message/5MTJC47B... Code of Conduct: http://python.org/psf/codeofconduct/
-- Christian Tismer :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Karl-Liebknecht-Str. 121 : https://github.com/PySide 14482 Potsdam : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
I'm quite interested in the rest of the story here. PySide is probably the biggest open-source user of the limited API, so IMO it is relevant to this list. On 2019-12-11 23:48, Christian Tismer wrote:
Hi all,
Sorry for the noise, I was wrong, and I retract. I was somehow mislead and hunted a phantom.
Does that mean that there was never a problem? Or that a problem is still there, but we don't know if it's in CPython or PySide or their interaction? Or was the problem solved in PySide? Do we need to document something better, or just aware of some caveat?
Best - Chris
On 10.12.19 00:29, Christian Tismer wrote:
On 09.12.19 23:26, Nick Coghlan wrote:
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com <mailto:python@mrabarnett.plus.com>> wrote:
On 2019-12-09 18:22, Christian Tismer wrote: > > > 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".
I believe Christian's point is that this entire "if (custom) {" branch looks suspicious, as it assumes "lookup_maybe_method" will increment the refcount on the returned object. If that assumption is incorrect, we're going to get DECREFs without a preceding INCREF.
The specific code path is also obscure enough that it's plausible the test suite may not currently cover it (as it requires doing something that calls "type_mro_modified" on a type with a custom metaclass).
Thanks Nick for this nice analysis. And it's exactly this codepath that is taken in the case of PySide: custom types all the time :-)
what-a-relief - ly y'rs -- Chris
On 12.12.19 13:04, Petr Viktorin wrote:
I'm quite interested in the rest of the story here. PySide is probably the biggest open-source user of the limited API, so IMO it is relevant to this list.
I guess that PyQt5 is a similar huge user, and it may be that they have the same problem, since there is no 5.14 version yet ;-)
On 2019-12-11 23:48, Christian Tismer wrote:
Hi all,
Sorry for the noise, I was wrong, and I retract. I was somehow mislead and hunted a phantom.
Does that mean that there was never a problem? Or that a problem is still there, but we don't know if it's in CPython or PySide or their interaction? Or was the problem solved in PySide? Do we need to document something better, or just aware of some caveat?
The problem is still there. When PySide creates a new type, then PyType_Ready digs into the new type, fetches the "mro()" method and uses the Py_TPFLAGS_METHOD_DESCRIPTOR flag to decide how to handle it. When I leave the flag in, we crash. So this is the current fix, and I have no idea why this affects us at all. Here is the last change to SbkObjectTypeTpNew() #503: // The meta type creates a new type when the Python programmer extends a wrapped C++ class. auto type_new = reinterpret_cast<newfunc>(PyType_Type.tp_new); // PYSIDE-939: This is a temporary patch that circumvents the problem // with Py_TPFLAGS_METHOD_DESCRIPTOR until this is finally solved. // PyType_Ready uses mro(). We need to temporarily remove the flag from it's type. // We cannot use PyMethodDescr_Type since it is not exported by Python 2.7 . static PyTypeObject *PyMethodDescr_TypePtr = Py_TYPE( PyObject_GetAttr(reinterpret_cast<PyObject *>(&PyType_Type), Shiboken::PyName::mro())); auto hold = PyMethodDescr_TypePtr->tp_flags; PyMethodDescr_TypePtr->tp_flags &= ~Py_TPFLAGS_METHOD_DESCRIPTOR; auto *newType = reinterpret_cast<SbkObjectType *>(type_new(metatype, args, kwds)); PyMethodDescr_TypePtr->tp_flags = hold; I am still not sure where the bug is and who has it. My understanding is that this flag should have no impact on PySide at all, but it has. Thanks for taking care -- Chris
On 10.12.19 00:29, Christian Tismer wrote:
On 09.12.19 23:26, Nick Coghlan wrote:
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com <mailto:python@mrabarnett.plus.com>> wrote:
On 2019-12-09 18:22, Christian Tismer wrote: > > > 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".
I believe Christian's point is that this entire "if (custom) {" branch looks suspicious, as it assumes "lookup_maybe_method" will increment the refcount on the returned object. If that assumption is incorrect, we're going to get DECREFs without a preceding INCREF.
The specific code path is also obscure enough that it's plausible the test suite may not currently cover it (as it requires doing something that calls "type_mro_modified" on a type with a custom metaclass).
Thanks Nick for this nice analysis. And it's exactly this codepath that is taken in the case of PySide: custom types all the time :-)
what-a-relief - ly y'rs -- Chris
-- -- Christian Tismer-Sperling :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Strandstraße 37 : https://github.com/PySide 24217 Schönberg : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
On 12.12.19 13:04, Petr Viktorin wrote:
I'm quite interested in the rest of the story here. PySide is probably the biggest open-source user of the limited API, so IMO it is relevant to this list.
BTW., the Python 3.8 change to type refcounting is already breaking the Limited API a bit, because we have to dynamically figure that out in order to be version-independent. I am not so sure if that whole change was worth to break it? Cheers -- Chris -- Christian Tismer-Sperling :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Strandstraße 37 : https://github.com/PySide 24217 Schönberg : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
Pardon, I meant "there is no Python 3.8 version, yet". And this is wrong, the MacOS pip install shows PyQt5-5.13.2-5.13.2-cp35.cp36.cp37.cp38-abi3-macosx_10_6_intel.whl So probably we have some bad oversight, somewhere. Cheers -- Chris On 12.12.19 13:48, Christian Tismer wrote:
On 12.12.19 13:04, Petr Viktorin wrote:
I'm quite interested in the rest of the story here. PySide is probably the biggest open-source user of the limited API, so IMO it is relevant to this list.
I guess that PyQt5 is a similar huge user, and it may be that they have the same problem, since there is no 5.14 version yet ;-)
On 2019-12-11 23:48, Christian Tismer wrote:
Hi all,
Sorry for the noise, I was wrong, and I retract. I was somehow mislead and hunted a phantom.
Does that mean that there was never a problem? Or that a problem is still there, but we don't know if it's in CPython or PySide or their interaction? Or was the problem solved in PySide? Do we need to document something better, or just aware of some caveat?
The problem is still there. When PySide creates a new type, then PyType_Ready digs into the new type, fetches the "mro()" method and uses the Py_TPFLAGS_METHOD_DESCRIPTOR flag to decide how to handle it.
When I leave the flag in, we crash. So this is the current fix, and I have no idea why this affects us at all. Here is the last change to SbkObjectTypeTpNew() #503:
// The meta type creates a new type when the Python programmer extends a wrapped C++ class. auto type_new = reinterpret_cast<newfunc>(PyType_Type.tp_new);
// PYSIDE-939: This is a temporary patch that circumvents the problem // with Py_TPFLAGS_METHOD_DESCRIPTOR until this is finally solved. // PyType_Ready uses mro(). We need to temporarily remove the flag from it's type. // We cannot use PyMethodDescr_Type since it is not exported by Python 2.7 . static PyTypeObject *PyMethodDescr_TypePtr = Py_TYPE( PyObject_GetAttr(reinterpret_cast<PyObject *>(&PyType_Type), Shiboken::PyName::mro())); auto hold = PyMethodDescr_TypePtr->tp_flags; PyMethodDescr_TypePtr->tp_flags &= ~Py_TPFLAGS_METHOD_DESCRIPTOR; auto *newType = reinterpret_cast<SbkObjectType *>(type_new(metatype, args, kwds)); PyMethodDescr_TypePtr->tp_flags = hold;
I am still not sure where the bug is and who has it. My understanding is that this flag should have no impact on PySide at all, but it has.
Thanks for taking care -- Chris
On 10.12.19 00:29, Christian Tismer wrote:
On 09.12.19 23:26, Nick Coghlan wrote:
On Tue., 10 Dec. 2019, 5:17 am MRAB, <python@mrabarnett.plus.com <mailto:python@mrabarnett.plus.com>> wrote:
On 2019-12-09 18:22, Christian Tismer wrote: > > > 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".
I believe Christian's point is that this entire "if (custom) {" branch looks suspicious, as it assumes "lookup_maybe_method" will increment the refcount on the returned object. If that assumption is incorrect, we're going to get DECREFs without a preceding INCREF.
The specific code path is also obscure enough that it's plausible the test suite may not currently cover it (as it requires doing something that calls "type_mro_modified" on a type with a custom metaclass).
Thanks Nick for this nice analysis. And it's exactly this codepath that is taken in the case of PySide: custom types all the time :-)
what-a-relief - ly y'rs -- Chris
-- -- Christian Tismer-Sperling :^) tismer@stackless.com Software Consulting : http://www.stackless.com/ Strandstraße 37 : https://github.com/PySide 24217 Schönberg : GPG key -> 0xFB7BEE0E phone +49 173 24 18 776 fax +49 (30) 700143-0023
participants (5)
-
Abdur-Rahmaan Janhangeer -
Christian Tismer -
MRAB -
Nick Coghlan -
Petr Viktorin