Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-109118: Fix runtime crash when NameError happens in PEP 695 function#109123
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedSep 8, 2023
🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commit3c448e8 🤖 If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again. |
This is also wrong, trying to figure out the right way to handle the refcounting here now. |
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 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. |
bedevere-bot commentedSep 8, 2023
🤖 New build scheduled with the buildbot fleet by@JelleZijlstra for commit882ada7 🤖 If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again. |
A few buildbots are now segfaulting on |
The buildbot failures seem unrelated, filed#109143. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the general pattern inbytecodes.c
is supposed to be "decref all inputs prior to checking any error cases, and then useERROR_IF
pseudo-macro to jump to error" -- andERROR_IF
will jump topop_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 generatesgoto pop_1_error
in the error cases in bothLOAD_NAME
andLOAD_FROM_DICT_OR_GLOBALS
, even though in theLOAD_NAME
case the "input" to_LOAD_FROM_DICT_OR_GLOBALS
is not ever on the stack, sincemacro()
just hooks it up directly to the output of_LOAD_LOCAL
.
Ithink this is just a bug in the cases generator; it should realize in themacro()
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.
Uh oh!
There was an error while loading.Please reload this page.
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. |
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. |
Thanks@JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry,@JelleZijlstra, I could not cleanly backport this to |
bedevere-bot commentedSep 9, 2023
GH-109173 is a backport of this pull request to the3.12 branch. |
…function (python#109123)(cherry picked from commit17f9941)
…EP 695 function (pythonGH-109123).(cherry picked from commit17f9941)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 commit17f9941)* [3.12]gh-109118: Fix runtime crash when NameError happens in PEP 695 function (GH-109123).(cherry picked from commit17f9941)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.