gh-116946: fully implement GC protocol for _tkinter.Tk{app,tt}Object

[picnixz]

• tkinter.Tcl_Obj is empty but not immutable so we make it immutable to avoid the needs of GC protocol.
• _tkinter.tktimertoken and _tkinter.tkapp are not immutable but they are not empty either as they have a strong reference to a callable, so we should still make them support the full GC protocol.

• Issue: Heap types implemented in C must have Py_TPFLAGS_HAVE_GC and implement tp_traverse #116946

[serhiy-storchaka]

No need to use tp_alloc and tp_free, because these types are not valid base types. Just make them immutable.

[picnixz]

No need to use tp_alloc and tp_free, because these types are not valid base types. Just make them immutable.

Doesn't it mean I would leave the possibility of introducing a cycle in bug fix versions and simply make them all immutable in main? unlike the select.[e]poll and _random.Random, the trace function can have a reference to the current app.

Now, you added tk.settrace as an experimental feature so it's internal only, but I believe that making it non-experimental would still require tp_clear/tp_traverse (and thus tp_alloc/tp_free as recommended by the docs; otherwise I would need GC_New/GC_Del + track).

[serhiy-storchaka]

All these types cannot be subclassed. So we can simply replace PyObject_New/PyObject_Free with PyObject_GC_New/PyObject_GC_Del. I think that this would look clearer than with tp_alloc/tp_free.

[picnixz]

Yes, but now the docs explicitly say "don't call those functions directly, use tp_alloc/tp_free". I agree that when possible, we could directly call them, but @ZeroIntensity suggested to follow the docs here.

[ZeroIntensity]

I like using tp_alloc/tp_free more because it's less refactoring if we need to change the type flags. People follow CPython's source code for inspiration in their extensions, so we should follow what's documented. If we want to change the preference, let's update the docs.

Alternatively, we could have a function like this for limited API users:

PyObject *
PyType_InvokeAlloc(PyTypeObject *tp)
{
    assert(tp != NULL);
    assert(type->tp_alloc != NULL);
    return type->tp_alloc(type, 0);
}

[picnixz]

At this point, I would prefer having a macro to avoid the extra cast everytime... Something that unifies PyObject_[GC_]New(typename, type).

[picnixz]

Ok, now I'm utterly confused because I don't understand why I get a segfault. The segfault happens in PyObject_GC_UnTrack

[vstinner]

LGTM

[vstinner]

I would prefer to move the variable definition aside to their first assignment:

Suggested change

	type = (PyTypeObject *)Tkapp_Type;
	assert(type != NULL);
	assert(type->tp_alloc != NULL);
	v = (TkappObject *)type->tp_alloc(type, 0);
	PyTypeObject *type = (PyTypeObject *)Tkapp_Type;
	assert(type != NULL);
	assert(type->tp_alloc != NULL);
	TkappObject *v = (TkappObject *)type->tp_alloc(type, 0);

Same below.

[picnixz]

I followed the existing style. Do you want me to move the char *arg as well?

[vstinner]

Do you want me to move the char *arg as well?

As you want. You can leave argv0 as it is to keep the diff small.

[picnixz]

I think I would be more comfortable not changing the style of this function. It's a long function, and if we were to use gotos instead for some refactoring, we could benefit for having everything declared at the top. Do you mind I leave it as is?

[serhiy-storchaka]

I am not comfortable with replacing a single line with 5 lines. Why not simply use public API?

[picnixz]

Well.. that's what I wanted to do but now the docs say to do things differently and considering people take inspiration for their extension modules with CPython code it'd be better to match our recommendations. Personally, I would prefer using the exact functions instead of tp_free/tp_alloc.

[ZeroIntensity]

Please create a DPO post to gauge feedback on whether to use tp_alloc/tp_free vs manual allocation functions.

[serhiy-storchaka]

Well, if you want to use tp_alloc/tp_free, can you just use them, without adding unnecessary lines?

[serhiy-storchaka]

Suggested change

	type = (PyTypeObject *)Tktt_Type;
	assert(type != NULL);
	assert(type->tp_alloc != NULL);
	v = (TkttObject *)type->tp_alloc(type, 0);
	v = (TkttObject *)((PyTypeObject *)Tktt_Type)->tp_alloc(type, 0);

[picnixz]

Unfortunately, we would need

 (TkttObject *)((PyTypeObject *)Tktt_Type)->tp_alloc((PyTypeObject *)Tktt_Type, 0);

which is a bit unreadable IMO. So I'll keep the temporary type variable. I would prefer having a macro for calling tp_alloc because it becomes quite annoying if we need to cast to PyTypeObject* everytime twice.

#define _PyObject_New(T, type)	\
	((T *)((PyTypeObject *)type)->tp_alloc((PyTypeObject *)type, 0))

[serhiy-storchaka]

Suggested change

	tp->tp_free(op);
	Py_TYPE(op)->tp_free(op);

[picnixz]

Unfortunately, this temporary variable is needed as we need Py_DECREF(Py_TYPE(op)) otherwise. The rest of the code base also does this.

[picnixz]

Oh you mean, removing the asserts. Well, I can do it though tracking segfaults would be a bit more annoying (at least with asserts, we don't have this issue). I'll just shorten the diff then and let it segfault normally.

[ZeroIntensity]

Debuggers are typically pretty good at catching null pointer dereferences anyway.

[serhiy-storchaka]

LGTM.

[picnixz]

(I merged main because of conflicts; for the backports, I will need to do them manually as those types are not immutable on 3.13 and 3.14).

[picnixz]

Mmh, something is actually wrong now. Previously, those leaked:

import tkinter
w = tkinter.Tk()
trace = lambda *_, **__: None
trace.evil = type(w.tk)
w.tk.settrace(trace)
w.mainloop()

For the timer, it's also possible to make something bad as follows:

import tkinter
w = tkinter.Tk()
func = lambda *_, **__: None
func.evil = type(w.tk.createtimerhandler(1234567, print))
w.tk.createtimerhandler(1234567, func)
w.mainloop()

But now I have the following crash for the timer reproducer:

python: ./Modules/_tkinter.c:2784: int Tktt_Traverse(PyObject *, visitproc, void *): Assertion `TkttObject_Check(op)' failed.

The first reproducer is fixed however. I think it's because of the extra reference created by the timer handler that is not properly visited or decrefed. I'll first patch this crash and move to backports afterwards.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 CentOS9 NoGIL Refleaks 3.x (tier-1) has failed when building commit f96f7c9.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1610/builds/2057) and take a look at the build logs.
4. Check if the failure is related to this commit (f96f7c9) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1610/builds/2057

Failed tests:

• test_free_threading

Test leaking resources:

• test_free_threading: file descriptors

Summary of the results of the build (if available):

==

Click to see traceback logs

remote: Enumerating objects: 3, done.        
remote: Counting objects:  50% (1/2)        
remote: Counting objects: 100% (2/2)        
remote: Counting objects: 100% (2/2), done.        
remote: Total 3 (delta 1), reused 1 (delta 1), pack-reused 1 (from 1)        
From https://github.com/python/cpython
 * branch                    main       -> FETCH_HEAD
Note: switching to 'f96f7c9f5b5db35e8a22f29b4e20f386cdde64f5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at f96f7c9f5b5 Revert "gh-116946: fully implement GC protocol for `_tkinter.Tk{app,tt}Object` (#138331)" (#138807)
Switched to and reset branch 'main'

configure: WARNING: no system libmpdec found; falling back to pure-Python version for the decimal module

make: *** [Makefile:2486: buildbottest] Error 2