Skip to content

gh-111506: Implement Py_SET_REFCNT() as opaque function in limited C API #111508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 30, 2023

In the limited C API version 3.13, Py_SET_REFCNT() function is now implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.

…ted C API

In the limited C API version 3.13, Py_SET_REFCNT() function is now
implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.
@vstinner vstinner requested a review from a team as a code owner October 30, 2023 17:23
@encukou
Copy link
Member

encukou commented Oct 31, 2023

Why is this particular function necessary?
I'd rather not commit to supporting setting the reference count to an arbitrary value in the long term.

abi3 makes it possible -- it exposes ob_refcnt directly. But exposing the fields directly was perhaps the biggest flaw of abi3. We now have a chance to revisit it, and tighten the set of supported operations.

@vstinner
Copy link
Member Author

Why is this particular function necessary?

It's needed to be able to remove non-portable assembly code from Include/*.h. I would prefer to keep these header files as simple C code, no inline assembly.

The code was just change for free threading. This change follows Py_INCREF/DECREF which already use an opaque function call in limited C API version 3.12.

@vstinner
Copy link
Member Author

I'd rather not commit to supporting setting the reference count to an arbitrary value in the long term.

I also have concerns about that. But this change is a practical change to make the situation less bad.

I plan to work on a whole PEP to disallow accessing directly PyObject.ob_refcnt and convert more functions to opaque function calls in the limited C API, as Sam describes in the issue.

@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2023

@colesbury @serhiy-storchaka @erlend-aasland: Would you mind to review this change?

@erlend-aasland
Copy link
Contributor

I'll leave this to @colesbury.

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

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

LGTM

I think this only improves our compatibility story going forward. We have already committed to this functionality as part of the API and ABI. Using an opaque function instead of an inline function does not change this.

@encukou
Copy link
Member

encukou commented Nov 3, 2023

+1 API-wise -- keeping it abi_only is the way to go.

@vstinner vstinner merged commit 20cfab9 into python:main Nov 3, 2023
@vstinner vstinner deleted the py_setrefcnt branch November 3, 2023 17:18
@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2023

Merged, thanks for reviews.

@bedevere-bot
Copy link

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

Hi! The buildbot s390x Fedora 3.x has failed when building commit 20cfab9.

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/all/#builders/223/builds/4734) and take a look at the build logs.
  4. Check if the failure is related to this commit (20cfab9) 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/all/#builders/223/builds/4734

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

==

Click to see traceback logs
remote: Enumerating objects: 17, done.        
remote: Counting objects:   7% (1/13)        
remote: Counting objects:  15% (2/13)        
remote: Counting objects:  23% (3/13)        
remote: Counting objects:  30% (4/13)        
remote: Counting objects:  38% (5/13)        
remote: Counting objects:  46% (6/13)        
remote: Counting objects:  53% (7/13)        
remote: Counting objects:  61% (8/13)        
remote: Counting objects:  69% (9/13)        
remote: Counting objects:  76% (10/13)        
remote: Counting objects:  84% (11/13)        
remote: Counting objects:  92% (12/13)        
remote: Counting objects: 100% (13/13)        
remote: Counting objects: 100% (13/13), done.        
remote: Compressing objects:  10% (1/10)        
remote: Compressing objects:  20% (2/10)        
remote: Compressing objects:  30% (3/10)        
remote: Compressing objects:  40% (4/10)        
remote: Compressing objects:  50% (5/10)        
remote: Compressing objects:  60% (6/10)        
remote: Compressing objects:  70% (7/10)        
remote: Compressing objects:  80% (8/10)        
remote: Compressing objects:  90% (9/10)        
remote: Compressing objects: 100% (10/10)        
remote: Compressing objects: 100% (10/10), done.        
remote: Total 17 (delta 3), reused 3 (delta 3), pack-reused 4        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '20cfab903db70cf952128bc6b606e3ec4a216498'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 20cfab903d gh-111506: Implement Py_SET_REFCNT() as opaque function in limited C API (#111508)
Switched to and reset branch 'main'

make: *** [Makefile:2065: buildbottest] Error 3

aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…ted C API (python#111508)

In the limited C API version 3.13, Py_SET_REFCNT() function is now
implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…ted C API (python#111508)

In the limited C API version 3.13, Py_SET_REFCNT() function is now
implemented as an opaque function call.

Add _Py_SetRefcnt() to the stable ABI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants