[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

> Do you plan to make the function public? It would be nice!

See https://github.com/python/cpython/pull/31081

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[STINNER Victor]

STINNER Victor  added the comment:

> It also adds a precondition that's not feasible public API, which this was 
> meant to become

Do you plan to make the function public? It would be nice!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

Change by Petr Viktorin :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

Petr Viktorin  added the comment:


New changeset 8b8673fe940c4ebc4512bff5af180b66def3d1ae by Petr Viktorin in 
branch '3.10':
[3.10] bpo-46433: _PyType_GetModuleByDef: handle static types in MRO (GH-30696) 
(GH-31262)
https://github.com/python/cpython/commit/8b8673fe940c4ebc4512bff5af180b66def3d1ae


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

Petr Viktorin  added the comment:

Just 3.10, after all. 3.9 doesn't have the function yet.

I did the backport, but I'd welcome a review by a fresh set of eyes!

--
versions:  -Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

Change by Petr Viktorin :


--
pull_requests: +29429
stage: backport needed -> patch review
pull_request: https://github.com/python/cpython/pull/31262

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Alex Waygood]

Change by Alex Waygood :


--
stage: patch review -> backport needed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

Right, this of course affects 3.10 and 3.9. Let me know if you want me to do 
the backports :)


(Updating version field for this ticket)

--
versions: +Python 3.10, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

Petr Viktorin  added the comment:

Almost. It's a bugfix so it needs backports to 3.10 & 3.9.
Thanks for the reminder!
I should get to them next week.

--
status: pending -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Erlend E. Aasland]

Erlend E. Aasland  added the comment:

It looks like this can be closed. Petr?

--
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[miss-islington]

miss-islington  added the comment:


New changeset 0ef08530124c5ca13a9394f4ac18bee8e6c66409 by Petr Viktorin in 
branch 'main':
bpo-46433: _PyType_GetModuleByDef: handle static types in MRO (GH-30696)
https://github.com/python/cpython/commit/0ef08530124c5ca13a9394f4ac18bee8e6c66409


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[STINNER Victor]

STINNER Victor  added the comment:

An alternative would be to merge PyHeapTypeObject members into PyTypeObject. I 
don't get why there is a separated struture: PyTypeObject is excluded from the 
stable ABI. See my remark about that:
https://bugs.python.org/issue22079#msg391464

Having a separated structure just static types make the code more complex and 
more error prone.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[STINNER Victor]

STINNER Victor  added the comment:

Oh, it seems like I misunderstood type_ready_mro() change. The check is only 
done on static types type. The MRO of heap types is not checked.

--

In my change, I wrote:

// _PyType_GetModuleByDef() must only be called on a heap type created
// by PyType_FromModuleAndSpec() or on its subclasses.
// type_ready_mro() ensures that a static type cannot inherit from a
// heap type.

based on this function:

static int
type_ready_mro(PyTypeObject *type)
{
/* Calculate method resolution order */
if (mro_internal(type, NULL) < 0) {
return -1;
}
assert(type->tp_mro != NULL);
assert(PyTuple_Check(type->tp_mro));

/* All bases of statically allocated type should be statically allocated */
if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
PyObject *mro = type->tp_mro;
Py_ssize_t n = PyTuple_GET_SIZE(mro);
for (Py_ssize_t i = 0; i < n; i++) {
PyTypeObject *base = (PyTypeObject *)PyTuple_GET_ITEM(mro, i);
if (PyType_Check(base) && (base->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
PyErr_Format(PyExc_TypeError,
 "type '%.100s' is not dynamically allocated but "
 "its base type '%.100s' is dynamically allocated",
 type->tp_name, base->tp_name);
return -1;
}
}
}
return 0;
}

This code comes from bpo-22079:

commit e09bcc874a21ce351a7fe73b9a137e236550d03c
Author: Serhiy Storchaka 
Date:   Wed Jan 28 11:03:33 2015 +0200

Issue #22079: PyType_Ready() now checks that statically allocated type has
no dynamically allocated bases.

Rationale: https://bugs.python.org/issue22079#msg236830

--

Note: PyType_Ready() function was very big and complex. I splitted this huge 
function into sub-functions in bpo-43770. I hope that it's a little bit more 
readable yet. I tried but failed to find a bug about tp_getattro and 
tp_setattro inheritance

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Erlend E. Aasland]

Change by Erlend E. Aasland :


--
nosy: +erlendaasland

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

Change by Petr Viktorin :


--
keywords: +patch
pull_requests: +28894
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/30696

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue46433] _PyType_GetModuleByDef optimization is incorrect

[Petr Viktorin]

New submission from Petr Viktorin :

I'm looking at the _PyType_GetModuleByDef optimization in 
https://github.com/python/cpython/pull/25504/files -- previously I assumed it's 
OK since it passed review.
But it doesn't look correct: 
- in the `_PyType_HasFeature` assert, we should be looking at `super` rather 
than `type`
- it's definitely possible that a hear type has a static type in the MRO -- 
`object`, at least. The `(PyHeapTypeObject*)super` cast is invalid in the case 
when the module is not found. And it's *also* incorrect in some cases of 
diamond inheritance, when a static type comes before the type we're looking for 
in `bases`.

It also adds a precondition that's not feasible public API, which this was 
meant to become: it should be callable with any type object. That's possible to 
do by keeping faster internal API and adding a public version with checks, but 
the diamond inheritance problem remains. Py_TPFLAGS_HEAPTYPE needs to be 
checked at runtime (except for the first iteration, if we're sure we're 
handling a static type).

Does that analysis sound right?

--
messages: 410962
nosy: petr.viktorin, vstinner
priority: normal
severity: normal
status: open
title: _PyType_GetModuleByDef optimization is incorrect
versions: Python 3.11

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com