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-94851: fix immortal objects refcounting in compiler by kumaraditya303 · Pull Request #95040 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Jul 20, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • Issue: Check for ref counting bugs in debug mode caused by immortal objects. #94851

Found while working on #95001

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@kumaraditya303
fix refcnt
da9d8a8
@kumaraditya303
kumaraditya303 requested a review from markshannon as a code owner July 20, 2022 07:03
@bedevere-bot bedevere-bot added the awaiting review label Jul 20, 2022
@kumaraditya303
kumaraditya303 requested a review from corona10 July 20, 2022 07:04
@kumaraditya303 kumaraditya303 added the skip news label Jul 20, 2022
@kumaraditya303 kumaraditya303 changed the title GH94851: fix immortal objects refcounting in compiler GH-94851: fix immortal objects refcounting in compiler Jul 20, 2022
@kumaraditya303 kumaraditya303 added the needs backport to 3.11 only security fixes label Jul 20, 2022
@corona10

corona10 commented Jul 20, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@kumaraditya303 Thanks Kumar

I was also curious about this code also when you submitted the related patch before.

  1. It will be helpful to review if you can suggest reproducible code as you did before.
  2. There are several _Py_STR related codes that don't increase reference count when they are used.
    Should they be updated? and if yes, Would you like to submit the one-shot patch?
All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 commented Jul 20, 2022

Copy link
Copy Markdown
Contributor Author

There are several _Py_STR related codes that don't increase reference count when they are used.

it depends on its usage, if you are owning the object or returning it from a function then incref is required else it is not required.
All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 commented Jul 20, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

It will be helpful to review if you can suggest reproducible code as you did before.

If you follow the macro, it is capturing a reference to the new object so incref is required.

See comment : /* Same as ADDOP_LOAD_CONST, but steals a reference. */
All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 commented Jul 20, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@kumaraditya303

a = 1
print(f'{a:d} {a:d}  {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d}')
./python.exe -X showrefcount tt.py
1 1  1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1
[-1 refs, 0 blocks]
All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303 commented Jul 20, 2022

Copy link
Copy Markdown
Contributor Author 
a = 1
print(f'{a:d} {a:d}  {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d} {a:d}')
./python.exe -X showrefcount tt.py
1 1  1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1
[-1 refs, 0 blocks]

Nice repro, thanks!
All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 commented Jul 20, 2022

Copy link
Copy Markdown
Member

And I checked that your patch fix this issue.

./python.exe -X showrefcount tt.py
1 1  1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1
[0 refs, 0 blocks]
kumaraditya303 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.
corona10
corona10 approved these changes Jul 20, 2022
View reviewed changes

@corona10 corona10 left a comment •
edited
Loading

Uh oh!

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

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.

Please submit the reproducible code if possible it can reduce the reviewer's time and help understanding context :)
I know your patch is correct but reproducible code makes sure what your patch fixes exactly.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Jul 20, 2022
@corona10
corona10 merged commit 7476154 into python:main Jul 20, 2022
@miss-islington

miss-islington commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

Thanks @kumaraditya303 for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖
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 Jul 20, 2022
@kumaraditya303
kumaraditya303 deleted the fix-refcnt branch July 20, 2022 09:53
@bedevere-bot

bedevere-bot commented Jul 20, 2022

Copy link
Copy Markdown

GH-95049 is a backport of this pull request to the 3.11 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.11 only security fixes label Jul 20, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 20, 2022
@kumaraditya303 @miss-islington
pythonGH-94851: fix immortal objects refcounting in compiler (pythong…
b745b27 
…h-95040)

(cherry picked from commit 7476154)

Co-authored-by: Kumar Aditya <[email protected]>
@corona10

corona10 commented Jul 20, 2022

Copy link
Copy Markdown
Member

I will merge backport patch after my dinner :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303 kumaraditya303 mentioned this pull request Jul 20, 2022
Check for ref counting bugs in debug mode caused by immortal objects. #94851
Closed
miss-islington added a commit that referenced this pull request Jul 20, 2022
@miss-islington @kumaraditya303
GH-94851: fix immortal objects refcounting in compiler (gh-95040)
16cb8ca 
(cherry picked from commit 7476154)

Co-authored-by: Kumar Aditya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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.

4 participants

@kumaraditya303 @corona10 @miss-islington @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.