| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
No need to use tp_alloc and tp_free, because these types are not valid base types. Just make them immutable.
Sorry, something went wrong.
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). |
Sorry, something went wrong.
| type = (PyTypeObject *)Tktt_Type; | ||
| assert(type != NULL); | ||
| assert(type->tp_alloc != NULL); | ||
| v = (TkttObject *)type->tp_alloc(type, 0); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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);
}
Sorry, something went wrong.
There was a problem hiding this comment.
At this point, I would prefer having a macro to avoid the extra cast everytime... Something that unifies PyObject_[GC_]New(typename, type).
Sorry, something went wrong.
|
Ok, now I'm utterly confused because I don't understand why I get a segfault. The segfault happens in PyObject_GC_UnTrack |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
| type = (PyTypeObject *)Tkapp_Type; | ||
| assert(type != NULL); | ||
| assert(type->tp_alloc != NULL); | ||
| v = (TkappObject *)type->tp_alloc(type, 0); |
There was a problem hiding this comment.
I would prefer to move the variable definition aside to their first assignment:
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
I followed the existing style. Do you want me to move the char *arg as well?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
I am not comfortable with replacing a single line with 5 lines. Why not simply use public API?
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Please create a DPO post to gauge feedback on whether to use tp_alloc/tp_free vs manual allocation functions. |
Sorry, something went wrong.
|
Well, if you want to use tp_alloc/tp_free, can you just use them, without adding unnecessary lines? |
Sorry, something went wrong.
| type = (PyTypeObject *)Tktt_Type; | ||
| assert(type != NULL); | ||
| assert(type->tp_alloc != NULL); | ||
| v = (TkttObject *)type->tp_alloc(type, 0); |
There was a problem hiding this comment.
| 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); |
Sorry, something went wrong.
There was a problem hiding this comment.
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))
Sorry, something went wrong.
| PyTypeObject *tp = Py_TYPE(op); | ||
| PyObject_GC_UnTrack(op); | ||
| (void)Tktt_Clear(op); | ||
| tp->tp_free(op); |
There was a problem hiding this comment.
| tp->tp_free(op); | |
| Py_TYPE(op)->tp_free(op); |
Sorry, something went wrong.
There was a problem hiding this comment.
Unfortunately, this temporary variable is needed as we need Py_DECREF(Py_TYPE(op)) otherwise. The rest of the code base also does this.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Debuggers are typically pretty good at catching null pointer dereferences anyway. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM.
Sorry, something went wrong.
|
(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). |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
…{app,tt}Object` (python#138331)"
This reverts commit 283380a.
⚠️⚠️⚠️ 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:
You can take a look at the buildbot page here: https://buildbot.python.org/#/builders/1610/builds/2057 Failed tests:
Test leaking resources:
Summary of the results of the build (if available): == Click to see traceback logsremote: 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 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.