X Tutup
Skip to content

gh-101266: Fix __sizeof__ for subclasses of int#101394

Merged
mdickinson merged 7 commits intopython:mainfrom
mdickinson:fix-sizeof-for-int-subtypes
Feb 5, 2023
Merged

gh-101266: Fix __sizeof__ for subclasses of int#101394
mdickinson merged 7 commits intopython:mainfrom
mdickinson:fix-sizeof-for-int-subtypes

Conversation

@mdickinson
Copy link
Member

@mdickinson mdickinson commented Jan 28, 2023 •
edited by bedevere-bot
Loading

This PR fixes the behaviour of the __sizeof__ method (and hence the results returned by sys.getsizeof) for subclasses of int.

@mdickinson mdickinson added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jan 28, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 28, 2023
Closed
@mdickinson
Copy link
Member Author

mdickinson commented Jan 28, 2023

@ionite34 This should fix the int portion of the bug you reported.

@corona10 Do you have enough spare cycles to review?

@mdickinson mdickinson requested a review from corona10 January 28, 2023 13:26
@mdickinson
Copy link
Member Author

mdickinson commented Jan 28, 2023

Darnit; this fails for bool; looks like we'll have to special case in boolobject.c. Working on it.

@mdickinson mdickinson marked this pull request as draft January 28, 2023 14:01
@mdickinson
Copy link
Member Author

mdickinson commented Jan 28, 2023

this fails for bool

That's because we had somewhat bogus numbers for tp_basicsize and tp_itemsize for the bool class. Now fixed.

I'm going to remove the "backport to 3.10" label: the bug does technically still exist in 3.10, but it's messier to fix - it looks like we would need to add a dedicated __sizeof__ implementation for bool in that case, and it doesn't seem worth the effort.

@corona10
Copy link
Member

corona10 commented Jan 28, 2023

Do you have enough spare cycles to review?

Thanks I will take a look

@mdickinson mdickinson marked this pull request as ready for review January 28, 2023 14:45
@mdickinson mdickinson removed the needs backport to 3.10 only security fixes label Jan 28, 2023
@corona10 corona10 self-assigned this Jan 29, 2023
@corona10
Copy link
Member

corona10 commented Jan 30, 2023

@mdickinson We have to resolve the conflict before reviewing this PR.
Mark Shannon refactored the longobject ;)
c1b1f51

@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 30, 2023
@bedevere-bot
Copy link

bedevere-bot commented Jan 30, 2023

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 9cd5d01 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 30, 2023
@mdickinson
Copy link
Member Author

mdickinson commented Jan 30, 2023

@corona10

@mdickinson We have to resolve the conflict before reviewing this PR.

Thanks! Updated.

@corona10 corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 31, 2023
@bedevere-bot
Copy link

bedevere-bot commented Jan 31, 2023

🤖 New build scheduled with the buildbot fleet by @corona10 for commit ae6229b 🤖

If you want to schedule another build, you need to add the :hammer: test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jan 31, 2023
corona10
corona10 approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm,

Just one more thing,
Would you like to change the PyBool_Type initialization with C99 style through this chance?

PyTypeObject PyBool_Type = {
    PyVarObject_HEAD_INIT(&PyType_Type, 0)
    "bool",
     .tp_basicsize = offsetof(struct _longobject, long_value.ob_digit),
     .tp_itemsize = sizeof(digit),
     .tp_dealloc = bool_dealloc,
      ...
     .tp_vectorcall = bool_vectorcall

@mdickinson
Copy link
Member Author

mdickinson commented Jan 31, 2023

Would you like to change the PyBool_Type initialization with C99 style through this chance?

Good idea! It may make sense to do that in a separate PR, though: I was hoping to backport this bugfix PR to 3.11, and the C99 style change sounds like a cosmetic update that shouldn't be backported.

@corona10
Copy link
Member

corona10 commented Jan 31, 2023

It may make sense to do that in a separate PR,

Great! Feel free to merge anytime you want :)

@mdickinson mdickinson merged commit 39017e0 into python:main Feb 5, 2023
@miss-islington
Copy link
Contributor

miss-islington commented Feb 5, 2023

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

@miss-islington
Copy link
Contributor

miss-islington commented Feb 5, 2023

Sorry, @mdickinson, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 39017e04b55d4c110787551dc9a0cb753f27d700 3.11

@miss-islington miss-islington assigned mdickinson and unassigned corona10 Feb 5, 2023
@mdickinson
Copy link
Member Author

mdickinson commented Feb 5, 2023

Hmm. Looks like I'll need to do a manual cherry-pick backport to 3.11, excluding the last few commits. I'll do that later today.

@mdickinson mdickinson mentioned this pull request Feb 5, 2023
Merged
@bedevere-bot
Copy link

bedevere-bot commented Feb 5, 2023

GH-101579 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Feb 5, 2023
@mdickinson
Copy link
Member Author

mdickinson commented Feb 5, 2023

See #101579 for a manual backport to 3.11 (cherry-picking the commits up to but excluding the main merge).

mdickinson added a commit that referenced this pull request Feb 5, 2023
@mdickinson
[3.11] gh-101266: Fix __sizeof__ for subclasses of int (GH-101394) (#…
cf89c16 
…101579)

Fix the behaviour of the `__sizeof__` method (and hence the results returned
by `sys.getsizeof`) for subclasses of `int`. Previously, `int` subclasses gave
identical results to the `int` base class, ignoring the presence of the instance
dictionary.

(Manual backport of #101394 to the Python 3.11 branch.)
mdickinson added a commit to mdickinson/cpython that referenced this pull request Feb 7, 2023
@mdickinson
Revert "[3.11] pythongh-101266: Fix __sizeof__ for subclasses of int (p…
2c5ae16 
…ythonGH-101394) (python#101579)"

This reverts commit cf89c16.
mdickinson added a commit that referenced this pull request Feb 7, 2023
@mdickinson
[3.11] gh-101266: Revert fix __sizeof__ for subclasses of int (#101638)
358b02d 
Revert "[3.11] gh-101266: Fix __sizeof__ for subclasses of int (GH-101394) (#101579)"

This reverts commit cf89c16.
@sobolevn
Copy link
Member

sobolevn commented Feb 12, 2023

This PR was the first one to fail refleak bot (test_importlib for some reason): https://buildbot.python.org/all/#/builders/802/builds/582

See #101766

@sobolevn sobolevn mentioned this pull request Feb 12, 2023
Closed
@mdickinson
Copy link
Member Author

mdickinson commented Feb 12, 2023 •
edited
Loading

@sobolevn

Hmm. We could try reverting it in a PR, and then running the refleak buildbots on that PR to see what happens. It seems rather implausible that it was this particular PR that introduced the refleak, though.

@sobolevn
Copy link
Member

sobolevn commented Feb 12, 2023

Yes, seems suspicious. I will investigate!

sobolevn added a commit to sobolevn/cpython that referenced this pull request Feb 12, 2023
@sobolevn
Revert "pythongh-101266: Fix __sizeof__ for subclasses of int (python…
357c542 
…#101394)"

This reverts commit 39017e0.
@sobolevn sobolevn mentioned this pull request Feb 12, 2023
Closed
@hroncok hroncok mentioned this pull request Mar 2, 2023
Closed
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

Assignees

@mdickinson mdickinson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mdickinson @corona10 @bedevere-bot @miss-islington @sobolevn
X Tutup