Skip to content

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Dec 10, 2024

This PR is a follow up to #125251 which made checking for immortal objects more robust.

This adds the ability to check whether objects are statically allocated or not, which should assist finalization and safety checks.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I've left a couple of minor comments. This will be a nice improvement.

@ZeroIntensity
Copy link
Member

Out of curiosity, how disruptive is adding a new field to the object structure?

@markshannon
Copy link
Member Author

Out of curiosity, how disruptive is adding a new field to the object structure?

It doesn't change the size of the PyObject struct or the offsets of ob_refcnt and ob_type.
Changing any of those would be very disruptive.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@markshannon markshannon merged commit bc262de into python:main Dec 11, 2024
43 checks passed
@AlexWaygood
Copy link
Member

This appears to be causing lots of compiler warnings on Windows. Here's what the "Files changed tab looks like on this PR:
image

These warnings are now showing up on the "files changed" tab on every PR

#if SIZEOF_VOID_P > 4
/* If an object has been freed, it will have a negative full refcnt
* If it has not it been freed, will have a very large refcnt */
if (op->ob_refcnt_full <= 0 || op->ob_refcnt > (UINT32_MAX - (1<<20))) {
Copy link
Member

Choose a reason for hiding this comment

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

Apparently UINT32_MAX is not defined in all builds, so this breaks a buildbot.

Copy link
Member Author

Choose a reason for hiding this comment

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

We use UINT32_MAX all over the place, so it must be defined. Maybe we're missing a #include somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it's a C++ build that failing, which might explain it

Copy link
Member Author

Choose a reason for hiding this comment

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

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
* Set a bit in the unused part of the refcount on 64 bit machines and the free-threaded build.

* Use the top of the refcount range on 32 bit machines
@markshannon markshannon deleted the refcnt-with-flags branch January 10, 2025 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants