GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-99298: Don't perform jumps before error handling by brandtbucher · Pull Request #99299 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Nov 9, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

My next steps:

  • This will need a manual backport to 3.11 (3.10 seems unaffected).
  • For 3.12, _Py_Specialize_LoadAttr and _Py_Specialize_StoreAttr don't actually need to handle errors (we can just fail to specialize "unready" types, which are probably super rare in practice).
  • Issue: Error handling for some instructions is incorrect #99298

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
brandtbucher added 2 commits November 9, 2022 11:45
@brandtbucher
Don't perform jumps before error handling
336f3e4
@brandtbucher
blurb add
02085ee
@brandtbucher brandtbucher added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 9, 2022
@brandtbucher
brandtbucher requested a review from markshannon November 9, 2022 20:11
@brandtbucher brandtbucher self-assigned this Nov 9, 2022
@bedevere-bot bedevere-bot mentioned this pull request Nov 9, 2022
Error handling for some instructions is incorrect #99298
Open
@bedevere-bot bedevere-bot added the awaiting core review label Nov 9, 2022
markshannon
markshannon reviewed Nov 10, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
next_instr--;
if (_Py_Specialize_StoreAttr(owner, next_instr, name)) {
// "undo" the rewind so end up in the correct handler:
next_instr++;

@markshannon markshannon Nov 10, 2022

Copy link
Copy Markdown
Member

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 is if (cond) goto fixup_error; where fixup_error does the necessary fix before jumping to error:. The C compiler will probably do this for us, but I think it best to be explicit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brandtbucher brandtbucher Nov 10, 2022

Copy link
Copy Markdown
Member Author

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 return void like the others in 3.12. Then we don't need the branch at all. :)

I'm hesitant to add additional labels and gotos to the 3.11 backport, especially since this is pretty cold code. Let me know if you think I should, though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Nov 10, 2022

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@markshannon
markshannon self-requested a review November 10, 2022 16:45
markshannon
markshannon approved these changes Nov 10, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Nov 10, 2022
@brandtbucher
brandtbucher merged commit 00ee6d5 into python:main Nov 10, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 10, 2022
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Nov 10, 2022
@brandtbucher @gvanrossum
pythonGH-99298: Don't perform jumps before error handling (pythonGH-9…
1aa0124 
…9299)
ethanfurman pushed a commit to ethanfurman/cpython that referenced this pull request Nov 12, 2022
@brandtbucher @ethanfurman
pythonGH-99298: Don't perform jumps before error handling (pythonGH-9…
887d708 
…9299)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@brandtbucher @markshannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.