-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
tp_dict
slot of static builtin types is NULL
in 3.12, without mention in the changelog or an alternative
#105227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@Yhg1s : Would it be acceptable to introduce such a new API in an upcoming 3.12 beta release? |
@Yhg1s, I've added the deferred blocker label because this issue is a seemingly intentional regression from 3.11 without a documented replacement. I'm currently using a private API and that doesn't feel right. |
FWIW, I'm in favor of adding |
I agree that its is better to return a new reference instead of a borrowed reference in APIs like this, but that can make life harder for anyone transitioning to the new way of working. In my particular case it does make transitioning harder because I'll have to add refcount updates where I didn't have to do before. That said, this is not a big problem. |
This compensates for static builtin types having `tp_dict` set to `NULL`. (cherry picked from commit a840806) Co-authored-by: Eric Snow <[email protected]> Co-authored-by: Petr Viktorin <[email protected]>
This compensates for static builtin types having `tp_dict` set to `NULL`. Co-authored-by: Petr Viktorin <[email protected]>
@ericsnowcurrently, when you get back, could you review (and maybe improve) the docs? |
gh-105227: Add PyType_GetDict() (GH-105747) This compensates for static builtin types having `tp_dict` set to `NULL`. (cherry picked from commit a840806) Co-authored-by: Eric Snow <[email protected]> Co-authored-by: Petr Viktorin <[email protected]>
The docs seem fine to me, after your changes. My original changes were likely unnecessarily specific. :) Was there something in particular you were concerned about? |
If you think more should be done, feel free to re-open this. |
See python/cpython#105227 Signed-off-by: Emanuele Giaquinta <[email protected]>
Trying to import golang on py3.12 crashes with segmentation fault with the following traceback: #0 0x00007f5579f6bfec in Py_INCREF (op=0x0) at /home/kirr/local/py3.12/include/python3.12/object.h:641 #1 __pyx_f_6golang_7_golang__patch_slot (__pyx_v_typ=0x563b309bd6c0 <PyBytes_Type>, __pyx_v_name=0x563b30ac0e00 <_PyRuntime+32128>, __pyx_v_func_or_descr=0x7f557a4e42c0) at golang/_golang.cpp:43758 #2 0x00007f5579f641b9 in __pyx_pf_6golang_7_golang_34_ (__pyx_self=0x0) at golang/_golang.cpp:40581 #3 0x00007f5579f637ba in __pyx_pw_6golang_7_golang_35_ (__pyx_self=0x0, unused=0x0) at golang/_golang.cpp:40427 ... 43750 /* "golang/_golang_str.pyx":1562 43751 * cdef DEL = object() 43752 * cdef _patch_slot(PyTypeObject* typ, str name, object func_or_descr): 43753 * typdict = <dict>(typ.tp_dict) # <<<<<<<<<<<<<< 43754 * #print("\npatching %s.%s with %r" % (typ.tp_name, name, func_or_descr)) 43755 * #print("old: %r" % typdict.get(name)) 43756 */ 43757 __pyx_t_1 = ((PyObject *)__pyx_v_typ->tp_dict); 43758 __Pyx_INCREF(__pyx_t_1); <--- HERE 43759 __pyx_v_typdict = ((PyObject*)__pyx_t_1); 43760 __pyx_t_1 = 0; Here we see that _patch_slots uses PyBytes_Type.tp_dict and hits NULL pointer dereference on it. Before Python3.12 typ.tp_dict was never NULL for any ready type and this code was correct, but starting since py3.12, as Boxiang explains By checking [Python 3.12 type object document](https://docs.python.org/3.12/c-api/typeobj.html#c.PyTypeObject.tp_dict), it said "Changed in version 3.12: Internals detail: For static builtin types, this is always NULL" What is going on here is that in py3.12 they continued the work on subinterpreters (PEP 684) and introduced per-interpreter GIL (https://docs.python.org/3.12/whatsnew/3.12.html#pep-684-a-per-interpreter-gil). The subinterpreters all have their own set of imported modules and objects, but for builtin types like e.g. bytes, str, dict and exception types, those types are shared in between the interpreters since the types are effectively immutable. The sharing is a bit delicate though because there is .__subclasses__ attribute of a type and that attribute can have different values in different subinterpreters. https://peps.python.org/pep-0684/#global-objects explains: Builtin static types are a special case of global objects that will be shared. They are effectively immutable except for one part: __subclasses__ (AKA tp_subclasses). We expect that nothing else on a builtin type will change, even the content of __dict__ (AKA tp_dict). So starting from py3.12 .tp_subclasses is handled specially: for heap types (type objects that are allocated on the heap instead of being static), .tp_subclasses continues to be a pointer. However for static builtin types, .tp_subclasses is now an integer "pointing" to per-interpreter data inside PyInterpreterState.types.builtins array keeping mutable type state there (python/cpython@47e75a00). And even though they were initially saying that the __dict__ might be left intact, later they also discovered that there are ways to modify typ.__dict__ even from python code python/cpython#94673 (comment) python/cpython#88004 and thus that for static types typ.tp_dict should be also considered mutable and kept in per-interpreter state inside PyInterpreterState instead of typ.tp_dict (python/cpython@de64e756). Which caused a bit of backward compatibility pain for tools that were previously accessing .tp_dict directly (python/cpython#105227) and was resolved via exposing PyType_GetDict utility (python/cpython@a840806d). In our crash above we were hitting such NULL .tp_dict for bytes type. -------- The simplest fix is relatively straightforward: use PyType_GetDict instead of directly accessing type's .tp_dict field. A more correct fix when patching slots of a type is to go through all created interpreters and adjust corresponding slot everywhere. This is left as TODO since we do not currently use subinterpreters in practice and with "free threaded" python builds coming, one can achieve GIL-less threading without subinterpreters at all. This patch takes into account original bug report and fix attempt by Boxiang: https://lab.nexedi.com/nexedi/pygolang/-/merge_requests/33#note_231278 https://lab.nexedi.com/Daetalus/pygolang/-/commit/b6a09755 /cc @Daetalus /reviewed-by @jerome, @tomo /reviewed-on https://lab.nexedi.com/nexedi/pygolang/-/merge_requests/35
PyObjC contains some functionality that needs to walk the entire MRO of classes and access the
tp_dict
slot. As of Python 3.12 beta 1 this code crashes due to the slot beingNULL
on static builtin types.The code in PyObjC walks the MRO to implement
tp_getattro
for the proxy of Objective-C classes to replicatePyObject_GenericGetAttr
with some customisations, as wel as in the implementation of an alternative tosuper
(again with customisations in the attribute resolution path). All code paths only need read-only access to thetp_dict
of static builtin types, although some code needs to be able to update thetp_dict
of classes created by PyObjC.There is currently no documented alternative for directly accessing
tp_dict
, and because of this I've changed PyObjC to use the private API_PyType_GetDict
.Some alternatives I've looked into:
Use
PyObject_GetAttrString(someType, "__dict__")
: This increases the complexity of the PyObjC code bases and requires changes to long stable code:Needs to introduce a different code path for accessing the
__dict__
slot of non-PyObjC subclassesNeeds to switch to the abstract Mapping API instead of the PyDict API, which likely has a performance impact
Changes to reference counting invariants in the code
Use
_PyType_GetDict
on Python 3.12 or later: This works, but introduces a reliance on a private CPython API that might disappear at any moment (even micro releases), for example by no longer exporting the function from shared libraries.Proposal: Rename
_PyType_GetDict
to eitherPyUnstable_Type_GetDict
orPyType_GetDict
, while keeping the current interface (e.g. returning a borrowed reference and never raising an exception).This is a follow-up from #105020
@ericsnowcurrently
Linked PRs
The text was updated successfully, but these errors were encountered: