GeezerVison

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

GeezerVison is showing github.com — gh-116098: Clean up frame allocation code and remove the invalid sneaky frame test by gaogaotiantian · Pull Request #116687 · python/cpython

Conversation

gaogaotiantian commented Mar 12, 2024 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

After a discussion with @brandtbucher , we believe the test is not testing what it's supposed to anymore. No new frame is created during the frame allocation and the test lost its meaning. Under no circumstance this test is valid or checking something reasonable. So we should just remove this test.

The fundamental reason the test is invalid is that the current frame allocation code is immune from being interrupted by Python code that can somehow create the same frame object. So we also remove the dead code in frame allocation, and replace it with an assert and some comments.

There's no user observable behavior changes so I'll skip the news, unless someone disagrees.

Issue: test_frame fails when running with -R 3:3 argument #116098

Sorry, something went wrong.

Uh oh!

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

All reactions


          Remove the invalid sneaky frame test

efbb9d0

gaogaotiantian requested a review from brandtbucher

March 12, 2024 22:20

bedevere-app Bot added the awaiting review label

Mar 12, 2024

bedevere-app Bot mentioned this pull request

Mar 12, 2024

test_frame fails when running with -R 3:3 argument #116098

Closed

gaogaotiantian added the skip news label

Mar 12, 2024

brandtbucher approved these changes

Mar 12, 2024

View reviewed changes

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

Thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/frame.c Resolved

Uh oh!

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

bedevere-app Bot added awaiting merge and removed awaiting review labels

Mar 12, 2024


          Grammar

8b9c1e1

brandtbucher enabled auto-merge (squash)

March 12, 2024 22:50

brandtbucher added tests

Tests in the Lib/test dir

interpreter-core (Objects, Python, Grammar, and Parser dirs) labels

Mar 12, 2024

brandtbucher self-assigned this

Mar 12, 2024

brandtbucher commented Mar 12, 2024

Copy link

Copy Markdown

Member

Probably not worth backporting.

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher merged commit a53cc3f into python:main

Mar 12, 2024

bedevere-app Bot removed the awaiting merge label

Mar 12, 2024

gaogaotiantian deleted the remove-sneaky-frame-test branch

March 12, 2024 23:41

vstinner pushed a commit to vstinner/cpython that referenced this pull request

Mar 20, 2024


          pythonGH-116098: Remove dead frame object creation code (pythonGH-116687

ccd58bf

gaogaotiantian mentioned this pull request

Mar 21, 2024

gh-107674: Improve performance of sys.settrace #117133

Merged

adorilson pushed a commit to adorilson/cpython that referenced this pull request

Mar 25, 2024


          pythonGH-116098: Remove dead frame object creation code (pythonGH-116687

5267e90

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request

Apr 17, 2024


          pythonGH-116098: Remove dead frame object creation code (pythonGH-116687

6be31cb

andersk mentioned this pull request

Sep 16, 2024

Remove asserts that confuse enum _framestate with enum _frameowner #124148

Merged

andersk reviewed

Sep 18, 2024

View reviewed changes

Comment thread

Python/frame.c

		// Just pretend that we have an owned, cleared frame so frame_dealloc
		// doesn't make the situation worse:
		f->f_frame = (_PyInterpreterFrame *)f->_f_frame_data;
		f->f_frame->owner = FRAME_CLEARED;

andersk Sep 18, 2024

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.

We should also remove the corresponding assert(frame->owner != FRAME_CLEARED) statements, especially since FRAME_CLEARED was never a valid member of enum _frameowner (it’s from enum _framestate).

Remove asserts that confuse enum _framestate with enum _frameowner #124148

Sorry, something went wrong.

Uh oh!

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

All reactions

colesbury pushed a commit that referenced this pull request

Jan 2, 2025


          Remove asserts that confuse enum _framestate with enum _frameowner (

a626f9a

GH-124148)

The `owner` field of `_PyInterpreterFrame` is supposed to be a member of
`enum _frameowner`, but `FRAME_CLEARED` is a member of `enum _framestate`.

At present, it happens that `FRAME_CLEARED` is not numerically equal to any
member of `enum _frameowner`, but that could change in the future. The code
that incorrectly assigned `owner = FRAME_CLEARED` was deleted in commit
a53cc3f (GH-116687). Remove the incorrect
checks for `owner != FRAME_CLEARED` as well.

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request

Jan 8, 2025


          Remove asserts that confuse enum _framestate with enum _frameowner (

f6fea23

pythonGH-124148)

The `owner` field of `_PyInterpreterFrame` is supposed to be a member of
`enum _frameowner`, but `FRAME_CLEARED` is a member of `enum _framestate`.

At present, it happens that `FRAME_CLEARED` is not numerically equal to any
member of `enum _frameowner`, but that could change in the future. The code
that incorrectly assigned `owner = FRAME_CLEARED` was deleted in commit
a53cc3f (pythonGH-116687). Remove the incorrect
checks for `owner != FRAME_CLEARED` as well.

Reviewers

brandtbucher

brandtbucher approved these changes

+1 more reviewer

andersk

andersk left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

brandtbucher

Labels

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

skip news tests

Tests in the Lib/test dir

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

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.

GeezerVison

Conversation

gaogaotiantian commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brandtbucher commented Mar 12, 2024

Uh oh!

andersk Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaogaotiantian commented Mar 12, 2024 •

edited

Loading

Uh oh!

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