| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 3c448e8 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
This is also wrong, trying to figure out the right way to handle the refcounting here now. |
Sorry, something went wrong.
|
The problem is that the way we need to manage the refcount on mod_or_class_dict is different for the two instructions that use the _LOAD_FROM_DICT_OR_GLOBALS op: in the LOAD_NAME case, we own the only reference to the dict, so we need to decref it in all cases, including the error conditions. But in the LOAD_FROM_DICT_OR_GLOBALS case, the dict is still on the stack until the end of the instruction, so in the error case, it gets decrefed automatically during stack unwinding. The code that is currently in main is correct for the LOAD_NAME case, but we get a double decref for the goto error branches. The code in my initial PR is correct for LOAD_FROM_DICT_OR_GLOBALS, but it doesn't DECREF properly in the error case for LOAD_NAME, so we leak references. I couldn't figure out how to get the refcounting right in both cases with the existing macros, so I got rid of the macros and duplicated the code instead, which made it possible to get the correct refcounting in both conditions. |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 882ada7 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
A few buildbots are now segfaulting on test_sys_settrace. Doesn't reproduce locally on macOS. |
Sorry, something went wrong.
|
The buildbot failures seem unrelated, filed #109143. |
Sorry, something went wrong.
There was a problem hiding this comment.
I think the general pattern in bytecodes.c is supposed to be "decref all inputs prior to checking any error cases, and then use ERROR_IF pseudo-macro to jump to error" -- and ERROR_IF will jump to pop_X_error labels to ensure the inputs get popped off the stack before the stack-emptying-with-decref happens. But when I try that approach here (instead of the changes in this PR), it fails; the cases generator generates goto pop_1_error in the error cases in both LOAD_NAME and LOAD_FROM_DICT_OR_GLOBALS, even though in the LOAD_NAME case the "input" to _LOAD_FROM_DICT_OR_GLOBALS is not ever on the stack, since macro() just hooks it up directly to the output of _LOAD_LOCAL.
I think this is just a bug in the cases generator; it should realize in the macro() case that it needs to account for where the input actually came from in the previous uop when it is deciding how many items need to be popped off the stack at an ERROR_IF. cc @gvanrossum to confirm or contradict my analysis here.
But given that we want to minimize risk at this point, it may be best (at least for 3.12) to go with this code-duplication fix rather than trying to make changes to the cases generator.
Sorry, something went wrong.
|
Thanks for the review! Yes, there's probably a way to get this to work, but especially since the cases generator is quite different on main and on 3.12 by now, it's probably better to accept the code duplication and go with my current solution. |
Sorry, something went wrong.
Oh dear. I'm afraid I've been deep into other things, and it'll take me some time to confirm this.
That definitely feels like the safest approach -- a fix to the cases generator likely can't be backported, since the version on main was extensively refactored during the rc phase. |
Sorry, something went wrong.
|
Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry, something went wrong.
|
Sorry, @JelleZijlstra, I could not cleanly backport this to 3.12 due to a conflict. |
Sorry, something went wrong.
|
GH-109173 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
…function (python#109123) (cherry picked from commit 17f9941)
…EP 695 function (pythonGH-109123). (cherry picked from commit 17f9941) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
… function (GH-109123) (#109173) * gh-109118: Fix runtime crash when NameError happens in PEP 695 function (#109123) (cherry picked from commit 17f9941) * [3.12] gh-109118: Fix runtime crash when NameError happens in PEP 695 function (GH-109123). (cherry picked from commit 17f9941) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
| 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.