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-109118: Fix runtime crash when NameError happens in PEP 695 function by JelleZijlstra · Pull Request #109123 · python/cpython

Conversation

@JelleZijlstra

@JelleZijlstra JelleZijlstra commented Sep 8, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Annotation scopes containing nested scopes #109118

Sorry, something went wrong.

Uh oh!

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

All reactions
@JelleZijlstra
pythongh-109118: Fix runtime crash when NameError happens in PEP 695 …
3c448e8
…function
@JelleZijlstra
JelleZijlstra requested a review from carljm September 8, 2023 01:31
@bedevere-bot bedevere-bot added the awaiting core review label Sep 8, 2023
@bedevere-bot bedevere-bot mentioned this pull request Sep 8, 2023
Annotation scopes containing nested scopes #109118
Closed
@JelleZijlstra JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 8, 2023
@bedevere-bot

bedevere-bot commented Sep 8, 2023

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 8, 2023
@JelleZijlstra JelleZijlstra added the needs backport to 3.12 only security fixes label Sep 8, 2023
@JelleZijlstra

JelleZijlstra commented Sep 8, 2023

Copy link
Copy Markdown
Member Author

This is also wrong, trying to figure out the right way to handle the refcounting here now.

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra
Duplicate the code instead
882ada7
@JelleZijlstra

JelleZijlstra commented Sep 8, 2023

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra JelleZijlstra added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 8, 2023
@bedevere-bot

bedevere-bot commented Sep 8, 2023

Copy link
Copy Markdown

🤖 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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 8, 2023
@JelleZijlstra

JelleZijlstra commented Sep 8, 2023

Copy link
Copy Markdown
Member Author

A few buildbots are now segfaulting on test_sys_settrace. Doesn't reproduce locally on macOS.

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra JelleZijlstra mentioned this pull request Sep 8, 2023
test_sys_settrace segfaults on main #109143
Closed
@JelleZijlstra

JelleZijlstra commented Sep 8, 2023

Copy link
Copy Markdown
Member Author

The buildbot failures seem unrelated, filed #109143.

All reactions

Sorry, something went wrong.

Uh oh!

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

carljm
carljm approved these changes Sep 8, 2023
View reviewed changes

@carljm carljm 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.

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.

Uh oh!

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

All reactions
Comment thread Python/bytecodes.c Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Sep 8, 2023
@JelleZijlstra

JelleZijlstra commented Sep 8, 2023

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra
DECREF_INPUTS
ab90023
@gvanrossum

gvanrossum commented Sep 9, 2023

Copy link
Copy Markdown
Member

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.

Oh dear. I'm afraid I've been deep into other things, and it'll take me some time to confirm this.

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.

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.

carljm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@JelleZijlstra
JelleZijlstra enabled auto-merge (squash) September 9, 2023 01:14
@JelleZijlstra
Merge branch 'main' into fixinterpcrash
5deb0b3
@JelleZijlstra
JelleZijlstra merged commit 17f9941 into python:main Sep 9, 2023
@miss-islington

miss-islington commented Sep 9, 2023

Copy link
Copy Markdown
Contributor

Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label Sep 9, 2023
@miss-islington

miss-islington commented Sep 9, 2023

Copy link
Copy Markdown
Contributor

Sorry, @JelleZijlstra, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 17f994174de9211b2baaff217eeb1033343230fc 3.12

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington miss-islington assigned JelleZijlstra Sep 9, 2023
@bedevere-bot

bedevere-bot commented Sep 9, 2023

Copy link
Copy Markdown

GH-109173 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Sep 9, 2023
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Sep 9, 2023
@JelleZijlstra
pythongh-109118: Fix runtime crash when NameError happens in PEP 695 …
546f56d
…function (python#109123)

(cherry picked from commit 17f9941)
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Sep 9, 2023
@JelleZijlstra
[3.12] pythongh-109118: Fix runtime crash when NameError happens in P…
e28d114
…EP 695 function (pythonGH-109123).

(cherry picked from commit 17f9941)

Co-authored-by: Jelle Zijlstra <[email protected]>
@JelleZijlstra JelleZijlstra mentioned this pull request Sep 10, 2023
SystemError for pep695 type parameter with the same name as the inner class #109219
Closed
Yhg1s pushed a commit that referenced this pull request Sep 12, 2023
@JelleZijlstra
[3.12] gh-109118: Fix runtime crash when NameError happens in PEP 695…
acde502
… 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 <[email protected]>
@JelleZijlstra JelleZijlstra mentioned this pull request Nov 2, 2023
redundant decref on the eval stack value in LOAD_FROM_DICT_OR_DEREF in error path #111654
Closed
@aisk aisk mentioned this pull request Nov 2, 2023
gh-111654: remove redundant decref on the eval stack value #111655
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm carljm approved these changes

Uh oh!

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

Assignees

@JelleZijlstra JelleZijlstra

Labels

None yet

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.

5 participants

@JelleZijlstra @bedevere-bot @gvanrossum @miss-islington @carljm
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.