GeezerVison

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

GeezerVison is showing github.com — gh-105020: Share tp_bases and tp_mro Between Interpreters For All Static Builtin Types by ericsnowcurrently · Pull Request #105115 · python/cpython

Conversation

ericsnowcurrently commented May 30, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry. However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses). We address that here by reverting back to shared objects, making them immortal in the process.

Issue: tp_bases of object is NULL: undocumented or unintentional behavior change in 3.12? #105020

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently added 5 commits

May 30, 2023 13:06


          Make tp_bases immortal for static builtin types.

7c3e588


          Add _Py_ClearImmortal().

ff411b2


          Drop PyInterpreterState.types.builtins[i].tp_bases.

354ff5f


          Make sure tp_bases is a tuple.

5be53f4


          Make tp_mro immortal for static builtin types.

bf1578e

ericsnowcurrently added skip news needs backport to 3.12 only security fixes labels

May 30, 2023

ericsnowcurrently requested review from gpshead and lysnikolaou

May 30, 2023 20:36

bedevere-bot mentioned this pull request

May 30, 2023

tp_bases of object is NULL: undocumented or unintentional behavior change in 3.12? #105020

Closed

ericsnowcurrently marked this pull request as ready for review

May 30, 2023 21:00

ericsnowcurrently requested a review from markshannon as a code owner

May 30, 2023 21:00

bedevere-bot added the awaiting core review label

May 30, 2023

gpshead reviewed

May 30, 2023

View reviewed changes

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

Do we have a meaningful place to put a regression test to assert that tp_bases and tp_mro are never NULL in the situations user code was encountering that in 3.12beta1?

Sorry, something went wrong.

Uh oh!

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

All reactions

gpshead reviewed

May 30, 2023

View reviewed changes

Comment thread

Include/internal/pycore_typeobject.h Outdated

		@@ -49,8 +49,6 @@ typedef struct {
		// XXX tp_dict, tp_bases, and tp_mro can probably be statically

gpshead May 30, 2023

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.

should this comment (added by the original PR) be updated now that bases and mro don't exist here?

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently and erlend-aasland reacted with thumbs up emoji

All reactions

👍 2 reactions

ericsnowcurrently May 30, 2023

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.

fixed

Sorry, something went wrong.

Uh oh!

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

All reactions

ericsnowcurrently commented May 30, 2023

Copy link

Copy Markdown

Member Author

Do we have a meaningful place to put a regression test to assert that tp_bases and tp_mro are never NULL in the situations user code was encountering that in 3.12beta1?

I'll add a test in test_capi.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently added 2 commits

May 30, 2023 15:40


          Update a TODO comment.

f4a5da1


          Add a test to avoid more regressions.

e054bbe

ericsnowcurrently commented May 30, 2023

Copy link

Copy Markdown

Member Author

I've added a test. Thanks for the suggestion.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently requested a review from Yhg1s

May 30, 2023 22:04

gpshead approved these changes

May 30, 2023

View reviewed changes

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

This looks good to me.

I do wonder if anything is going to be tripped up by the tp_dict as well, but that seems like a separate category of change given I believe it could've been NULL in the past - if it comes up it can be addressed separately.

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot added awaiting merge and removed awaiting core review labels

May 30, 2023

gpshead commented May 30, 2023

Copy link

Copy Markdown

Member

you might want a NEWS entry fwiw... as people do read the changelog between beta1 and beta2.

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


          Add a NEWS entry.

78c7d0e

ericsnowcurrently enabled auto-merge (squash)

May 30, 2023 23:46

ericsnowcurrently merged commit 7be667d into python:main

May 31, 2023

miss-islington commented May 31, 2023

Copy link

Copy Markdown

Contributor

Thanks @ericsnowcurrently 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 removed the awaiting merge label

May 31, 2023

miss-islington commented May 31, 2023

Copy link

Copy Markdown

Contributor

Sorry @ericsnowcurrently, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 7be667dfafa2465df6342d72dca9c1f82dd830d0 3.12

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned ericsnowcurrently

May 31, 2023

ericsnowcurrently added needs backport to 3.12 only security fixes and removed needs backport to 3.12 only security fixes labels

May 31, 2023

miss-islington commented May 31, 2023

Copy link

Copy Markdown

Contributor

Thanks @ericsnowcurrently 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.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

May 31, 2023


          pythongh-105020: Share tp_bases and tp_mro Between Interpreters For A…

c28c929

…ll Static Builtin Types (pythongh-105115)

In pythongh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry.  However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses).  We address that here by reverting back to shared objects, making them immortal in the process.
(cherry picked from commit 7be667d)

Co-authored-by: Eric Snow <[email protected]>

bedevere-bot commented May 31, 2023

Copy link

Copy Markdown

GH-105124 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 removed the needs backport to 3.12 only security fixes label

May 31, 2023

ericsnowcurrently deleted the fix-tp-bases branch

May 31, 2023 00:23

ericsnowcurrently pushed a commit that referenced this pull request

Jun 1, 2023


          [3.12] gh-105020: Share tp_bases and tp_mro Between Interpreters For …

c38ceb0

…All Static Builtin Types (gh-105115) (gh-105124)

In gh-103912 we added tp_bases and tp_mro to each PyInterpreterState.types.builtins entry.  However, doing so ignored the fact that both PyTypeObject fields are public API, and not documented as internal (as opposed to tp_subclasses).  We address that here by reverting back to shared objects, making them immortal in the process.
(cherry picked from commit 7be667d)

Co-authored-by: Eric Snow [email protected]

erlend-aasland mentioned this pull request

Jun 6, 2023

3.12 backport gh 105236 #105358

Closed

Reviewers

gpshead

gpshead approved these changes

lysnikolaou

Awaiting requested review from lysnikolaou

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Yhg1s

Awaiting requested review from Yhg1s

Uh oh!

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

Assignees

ericsnowcurrently

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

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

ericsnowcurrently commented May 30, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

gpshead May 30, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently May 30, 2023

Choose a reason for hiding this comment

Uh oh!

ericsnowcurrently commented May 30, 2023

Uh oh!

ericsnowcurrently commented May 30, 2023

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

gpshead commented May 30, 2023

Uh oh!

miss-islington commented May 31, 2023

Uh oh!

miss-islington commented May 31, 2023

Uh oh!

miss-islington commented May 31, 2023

Uh oh!

bedevere-bot commented May 31, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericsnowcurrently commented May 30, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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