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-118934: Make PyEval_GetLocals return borrowed reference by gaogaotiantian · Pull Request #119769 · python/cpython

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented May 30, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Per discussion with @markshannon in Pittsburgh, we simulate what we did pre-PEP667. We store the locals dict in frame->f_locals so that we can return a borrowed reference. The frame->f_locals will be cleared when the frame is released. This way we can keep the behavior of PyEval_GetLocals().

  • Issue: PyEval_GetLocals() leaks locals #118934

Sorry, something went wrong.

Uh oh!

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

All reactions
@gaogaotiantian
Make PyEval_GetLocals return borrowed reference
60f38ad
@gaogaotiantian
gaogaotiantian requested a review from markshannon as a code owner May 30, 2024 04:11
@bedevere-app bedevere-app Bot added the awaiting review label May 30, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 30, 2024
PyEval_GetLocals() leaks locals #118934
Closed
blurb-it Bot and others added 2 commits May 30, 2024 04:11
@blurb-it
📜🤖 Added by blurb_it.
ca9c04f
@gaogaotiantian
Remove a blank line
2b90266
@gaogaotiantian gaogaotiantian added 3.13 only security fixes needs backport to 3.13 only security fixes and removed 3.13 only security fixes labels May 30, 2024
@gaogaotiantian
Fix refcnt check
10ad82c
@gaogaotiantian
gaogaotiantian marked this pull request as draft May 30, 2024 17:30
@bedevere-app bedevere-app Bot removed the awaiting review label May 30, 2024
@gaogaotiantian

gaogaotiantian commented May 30, 2024

Copy link
Copy Markdown
Member Author

Just realized this will overwrite f_locals when it's called inside a comprehension of a non-optimized scope. We might have to use a new member in FrameObject to store this.

All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury
colesbury reviewed May 31, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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.

A few formatting suggestions and a question about the return value

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Python/ceval.c Resolved

Uh oh!

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

Comment thread Python/ceval.c Resolved

Uh oh!

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

Comment thread Python/ceval.c Resolved

Uh oh!

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

gaogaotiantian added 2 commits May 31, 2024 19:34
@gaogaotiantian
Add an extra field to hold the locals dict
634ffd0
@gaogaotiantian
Fix lint
b48c279
@gaogaotiantian
gaogaotiantian marked this pull request as ready for review June 1, 2024 02:36
@gaogaotiantian
gaogaotiantian requested a review from ericsnowcurrently as a code owner June 1, 2024 02:36
@bedevere-app bedevere-app Bot added the awaiting review label Jun 1, 2024
@gaogaotiantian gaogaotiantian mentioned this pull request Jun 1, 2024
Local variable assignment is broken when combined with threads + tracing + closures #74929
Closed
@gaogaotiantian
Merge branch 'main' into fix-getlocals
da70ee1
ncoghlan
ncoghlan reviewed Jun 1, 2024
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

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

@gaogaotiantian
Remove deepfreeze file
1167c3d
ncoghlan
ncoghlan reviewed Jun 1, 2024
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

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

ncoghlan
ncoghlan reviewed Jun 2, 2024
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

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

This was referenced Jun 3, 2024
PEP 667: Clarify impact on PyEval_GetLocals python/peps#3809
Closed
PEP 667: correct/clarify handling of PyEval_GetLocals python/steering-council#245
Closed
ncoghlan added 2 commits July 16, 2024 17:57
@ncoghlan
Merge branch 'main' into fix-getlocals
e08209f
@ncoghlan
Revert to Python 3.12 semantics
edef0dd
@ncoghlan

ncoghlan commented Jul 16, 2024

Copy link
Copy Markdown
Contributor

@gaogaotiantian I merged the suggestion that reverts back to the Python 3.12 semantics, so we can get a CI run on that version of the refleak fix.

All reactions

Sorry, something went wrong.

Uh oh!

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

ncoghlan
ncoghlan approved these changes Jul 16, 2024
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

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.

@gaogaotiantian CI is happy with going back to the Python 3.12 behaviour, so if you're happy I think this is good to merge for the final beta.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jul 16, 2024
@ncoghlan ncoghlan assigned gaogaotiantian Jul 16, 2024
@gaogaotiantian
gaogaotiantian merged commit e65cb4c into python:main Jul 16, 2024
@miss-islington-app

miss-islington-app Bot commented Jul 16, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting merge label Jul 16, 2024
@gaogaotiantian
gaogaotiantian deleted the fix-getlocals branch July 16, 2024 19:17
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 16, 2024
@gaogaotiantian @ncoghlan @miss-islington
pythongh-118934: Make PyEval_GetLocals return borrowed reference (pyt…
ab2a568
…honGH-119769)

(cherry picked from commit e65cb4c)

Co-authored-by: Tian Gao <[email protected]>
Co-authored-by: Alyssa Coghlan <[email protected]>
@bedevere-app

bedevere-app Bot commented Jul 16, 2024

Copy link
Copy Markdown

GH-121869 is a backport of this pull request to the 3.13 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Jul 16, 2024
ncoghlan added a commit that referenced this pull request Jul 18, 2024
@miss-islington @gaogaotiantian @ncoghlan
[3.13] gh-118934: Make PyEval_GetLocals return borrowed reference (GH…
233ed46
…-119769) (#121869)

gh-118934: Make PyEval_GetLocals return borrowed reference (GH-119769)
(cherry picked from commit e65cb4c)

Co-authored-by: Tian Gao <[email protected]>
Co-authored-by: Alyssa Coghlan <[email protected]>
@bedevere-bot

bedevere-bot commented Jul 18, 2024

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Fedora Clang Installed 3.13 has failed when building commit 233ed46.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#builders/1392/builds/214) and take a look at the build logs.
  4. Check if the failure is related to this commit (233ed46) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#builders/1392/builds/214

Failed tests:

  • test_pyrepl

Failed subtests:

  • test_inspect_keeps_globals_from_inspected_file - test.test_pyrepl.test_pyrepl.TestMain.test_inspect_keeps_globals_from_inspected_file
  • test_inspect_keeps_globals_from_inspected_module - test.test_pyrepl.test_pyrepl.TestMain.test_inspect_keeps_globals_from_inspected_module

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.13.edelsohn-fedora-z.clang-installed/build/target/lib/python3.13/test/test_pyrepl/test_pyrepl.py", line 995, in _run_repl_globals_test
    self.fail(f"{var}= not found in output")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: FOO= not found in output


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.13.edelsohn-fedora-z.clang-installed/build/target/lib/python3.13/test/test_pyrepl/test_pyrepl.py", line 995, in _run_repl_globals_test
    self.fail(f"{var}= not found in output")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: __name__= not found in output


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.13.edelsohn-fedora-z.clang-installed/build/target/lib/python3.13/test/test_pyrepl/test_pyrepl.py", line 995, in _run_repl_globals_test
    self.fail(f"{var}= not found in output")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: __package__= not found in output
All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented Jul 18, 2024

Copy link
Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Fedora LTO + PGO 3.13 has failed when building commit 233ed46.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#builders/1511/builds/215) and take a look at the build logs.
  4. Check if the failure is related to this commit (233ed46) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#builders/1511/builds/215

Failed tests:

  • test_pyrepl

Failed subtests:

  • test_inspect_keeps_globals_from_inspected_module - test.test_pyrepl.test_pyrepl.TestMain.test_inspect_keeps_globals_from_inspected_module

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.13.edelsohn-fedora-z.lto-pgo/build/Lib/test/test_pyrepl/test_pyrepl.py", line 995, in _run_repl_globals_test
    self.fail(f"{var}= not found in output")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: __package__= not found in output


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.13.edelsohn-fedora-z.lto-pgo/build/Lib/test/test_pyrepl/test_pyrepl.py", line 995, in _run_repl_globals_test
    self.fail(f"{var}= not found in output")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: __file__= not found in output


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.13.edelsohn-fedora-z.lto-pgo/build/Lib/test/test_pyrepl/test_pyrepl.py", line 995, in _run_repl_globals_test
    self.fail(f"{var}= not found in output")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: FOO= not found in output
All reactions

Sorry, something went wrong.

Uh oh!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@ncoghlan ncoghlan ncoghlan approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

Uh oh!

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

Assignees

@gaogaotiantian gaogaotiantian

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.

4 participants

@gaogaotiantian @ncoghlan @bedevere-bot @colesbury
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.