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, <pyt...@mrabarnett.plus.com
>>>> <mailto:pyt...@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 :^) tis...@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
_______________________________________________
Python-Dev mailing list -- python-dev@python.org
To unsubscribe send an email to python-dev-le...@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/3LE4E363M4DBW5BDI3ROBBUYQLBBZDMR/
Code of Conduct: http://python.org/psf/codeofconduct/
