Issue46433
Created on 2022-01-19 15:46 by petr.viktorin, last changed 2022-04-11 14:59 by admin. This issue is now closed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 30696 | merged | petr.viktorin, 2022-01-19 16:09 | |
| PR 31262 | merged | petr.viktorin, 2022-02-10 16:37 | |
| Messages (11) | |||
|---|---|---|---|
| msg410962 - (view) | Author: Petr Viktorin (petr.viktorin) * ![]() |
Date: 2022-01-19 15:46 | |
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? |
|||
| msg411166 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-21 17:45 | |
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 <storchaka@gmail.com>
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
|
|||
| msg411167 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-01-21 17:47 | |
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. |
|||
| msg412370 - (view) | Author: miss-islington (miss-islington) | Date: 2022-02-02 15:57 | |
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 |
|||
| msg412482 - (view) | Author: Erlend E. Aasland (erlendaasland) * ![]() |
Date: 2022-02-03 22:47 | |
It looks like this can be closed. Petr? |
|||
| msg412498 - (view) | Author: Petr Viktorin (petr.viktorin) * ![]() |
Date: 2022-02-04 10:08 | |
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. |
|||
| msg412500 - (view) | Author: Erlend E. Aasland (erlendaasland) * ![]() |
Date: 2022-02-04 10:11 | |
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) |
|||
| msg413012 - (view) | Author: Petr Viktorin (petr.viktorin) * ![]() |
Date: 2022-02-10 16:41 | |
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! |
|||
| msg413058 - (view) | Author: Petr Viktorin (petr.viktorin) * ![]() |
Date: 2022-02-11 11:25 | |
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 |
|||
| msg413063 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2022-02-11 12:17 | |
> 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! |
|||
| msg413064 - (view) | Author: Erlend E. Aasland (erlendaasland) * ![]() |
Date: 2022-02-11 12:23 | |
> Do you plan to make the function public? It would be nice! See https://github.com/python/cpython/pull/31081 |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:59:54 | admin | set | github: 90591 |
| 2022-02-11 12:23:43 | erlendaasland | set | messages: + msg413064 |
| 2022-02-11 12:17:31 | vstinner | set | messages: + msg413063 |
| 2022-02-11 11:26:15 | petr.viktorin | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2022-02-11 11:25:49 | petr.viktorin | set | messages: + msg413058 |
| 2022-02-10 16:41:58 | petr.viktorin | set | messages:
+ msg413012 versions: - Python 3.9 |
| 2022-02-10 16:37:30 | petr.viktorin | set | stage: backport needed -> patch review pull_requests: + pull_request29429 |
| 2022-02-04 12:37:30 | AlexWaygood | set | stage: patch review -> backport needed |
| 2022-02-04 10:11:08 | erlendaasland | set | messages:
+ msg412500 versions: + Python 3.9, Python 3.10 |
| 2022-02-04 10:08:55 | petr.viktorin | set | status: pending -> open messages: + msg412498 |
| 2022-02-03 22:47:27 | erlendaasland | set | status: open -> pending messages: + msg412482 |
| 2022-02-02 15:57:56 | miss-islington | set | nosy:
+ miss-islington messages: + msg412370 |
| 2022-01-21 17:47:13 | vstinner | set | messages: + msg411167 |
| 2022-01-21 17:45:18 | vstinner | set | messages: + msg411166 |
| 2022-01-19 22:06:14 | erlendaasland | set | nosy:
+ erlendaasland |
| 2022-01-19 16:09:31 | petr.viktorin | set | keywords:
+ patch stage: patch review pull_requests: + pull_request28894 |
| 2022-01-19 15:46:37 | petr.viktorin | create | |

