Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
GH-99298: Don't perform jumps before error handling#99299
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
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.
LGTM. One minor issue about the error handling.
| @@ -1152,6 +1152,8 @@ dummy_func( | |||
| PyObject *name = GETITEM(names, oparg); | |||
| next_instr--; | |||
| if (_Py_Specialize_StoreAttr(owner, next_instr, name)) { | |||
| // "undo" the rewind so end up in the correct handler: | |||
| next_instr++; | |||
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.
Adding any code to a conditional block with just a goto adds extra branches.
It's not so important for these slower instructions, but its best avoided in general.
What we generally do isif (cond) goto fixup_error; wherefixup_error does the necessary fix before jumping toerror:. The C compiler will probably do this for us, but I think it best to be explicit.
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.
Yeah, see my comment at the top of this PR._Py_Specialize_StoreAttr and_Py_Specialize_LoadAttr don't really need error handling at all, so the next thing I'm going to do it make them returnvoid like the others in 3.12. Then we don't need the branch at all. :)
I'm hesitant to add additional labels andgotos to the 3.11 backport, especially since this is pretty cold code. Let me know if you think I should, though.
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.
Ok, leave this for this PR.
Maybe make a PR for 3.12 that removes the branch entirely?
Uh oh!
There was an error while loading.Please reload this page.
My next steps:
_Py_Specialize_LoadAttrand_Py_Specialize_StoreAttrdon't actually need to handle errors (we can just fail to specialize "unready" types, which are probably super rare in practice).