GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-102304: Fix Py_INCREF() stable ABI in debug mode by vstinner · Pull Request #104763 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented May 22, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

When Python is built in debug mode (if the Py_REF_DEBUG macro is defined), the Py_INCREF() and Py_DECREF() function are now always implemented as opaque functions to avoid leaking implementation details like "_Py_RefTotal".

  • Issue: Move _Py_RefTotal to PyInterpreterState #102304

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner vstinner added the skip news label May 22, 2023
@bedevere-bot bedevere-bot added the awaiting core review label May 22, 2023
@bedevere-bot bedevere-bot mentioned this pull request May 22, 2023
Move _Py_RefTotal to PyInterpreterState #102304
Closed
@vstinner

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

I suggest backporting the change to the 3.12 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request May 22, 2023
gh-102304: Fix 2 New Stable ABI Functions #104762
Merged
@vstinner

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

Previous work on this issue: issue #87854 and commit 3359cab: "The limited C API is now supported if Python is built in debug mode".
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

This change is only about fixing the limited C API.

Python itself must not be built with the limited C API. It will still be built with _Py_IncRefTotal_DO_NOT_USE_THIS() and _Py_DecRefTotal_DO_NOT_USE_THIS() functions. But these functions should not be exposed as part of the stable ABI.
encukou 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.
@vstinner

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

In 2021, I was too shy to fix the limited C API in debug mode for any targeted Python version. I had worries about any potential performance drawbacks, even if the change only concerns Python built in debug mode (Py_DEBUG and Py_REF_DEBUG macros). IMO it's safe to remove this condition and use the opaque function call on all Python versions.

I was never happy about exposing _Py_RefTotal implementation detail as part of the stable ABI :-( Since Python 3.12, _Py_RefTotal became even more complicated: it's now a counter per interpreter, the global variable must no longer be used!
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

PR rebased on top of commit 421cbf3.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner force-pushed the incref_debug branch from fb05026 to f8a06d5 Compare May 22, 2023 23:13
@vstinner

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

Oh, test_enum currently fails on the main branch, as well on my PR.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

Oh, test_enum currently fails on the main branch, as well on my PR.

See #104765
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner force-pushed the incref_debug branch from f8a06d5 to 5298304 Compare May 22, 2023 23:28
@vstinner

vstinner commented May 22, 2023

Copy link
Copy Markdown
Member Author

PR rebased on main to get the test_enum fix.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented May 23, 2023

Copy link
Copy Markdown
Member Author

cc @ericsnowcurrently @encukou @Yhg1s
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented May 23, 2023

Copy link
Copy Markdown
Member Author

A temporary fix was merged in main for this issue: #104766. This PR removes modified code.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
encukou
encukou reviewed May 23, 2023
View reviewed changes
Comment thread Include/object.h
// Stable ABI for Python 3.10 built in debug mode.
#if defined(Py_REF_DEBUG) && defined(Py_LIMITED_API)
// Stable ABI for Python built in debug mode
_Py_IncRef(op);

@encukou encukou May 23, 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.

_Py_IncRef was only added in 3.10, so I don't think the Py_LIMITED_API+0 >= 0x030A0000 check can go away.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner May 31, 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.

The #else code path uses op->ob_refcnt_split and _Py_IsImmortal() which were added to Python 3.12. Choose your poison: no code path work on Python 3.9 and older.

I'm convinced that building a C extension targeting an old version of the stable ABI with a new Python never worked because of these subtle implementation details (especially around Py_INCREF/Py_DECREF). I would prefer to always convert Py_INCREF/Py_DECREF to opaque function calls in the stable ABI: the current implementation is badly broken because of that. Honestly, I don't think that relying on union, endianness, accessing direclty PyObject members, is a future proof. Extract of Py_INCREF implementation in Python 3.13:

    // Portable saturated add, branching on the carry flag and set low bits
    PY_UINT32_T cur_refcnt = op->ob_refcnt_split[PY_BIG_ENDIAN];
    PY_UINT32_T new_refcnt = cur_refcnt + 1;
    if (new_refcnt == 0) {
        return;
    }
    op->ob_refcnt_split[PY_BIG_ENDIAN] = new_refcnt;

It's not backward compatible at least.

Moreover, this change only impacts Python built in debug mode. That's really a corner case: nobody should distribute a wheel package with Python built in debug mode, especially when the stable ABI is targeted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
zooba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@zooba zooba May 31, 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.

That refcnt_split is going to change again anyway - @eduardo-elizondo FYI.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner May 31, 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.

That refcnt_split is going to change again anyway - @eduardo-elizondo FYI.

which makes my point even more relevant :-)

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner May 31, 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.

C extensions built for the stable ABI with Python 3.10 or older don't know about immortal objects :-(

Sorry, something went wrong.

Uh oh!

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

@eduardo-elizondo eduardo-elizondo Jun 7, 2023 •
edited
Loading

Uh oh!

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

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.

It's not backward compatible at least.

@vstinner let's dive a bit more into this! I don't think that this is actually an issue but I want to see if I'm missing something here.

To be more specific do you have an example of what exactly will not be backwards compatible? The semantics did change, but that does not mean that it will be a backwards incompatible change. The updates to the headers is done in a way that it doesn't matter if previous versions don't know about immortal headers, it will still work as intended.

^ Note that this comment is not necessarily about this PR as it totally makes sense to use _Py_IncRef directly in debug mode.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ericsnowcurrently

ericsnowcurrently commented May 30, 2023

Copy link
Copy Markdown
Member

FYI, I (mostly accidentally) merged gh-104762, even though I had meant to get some sort of resolution here first. At this point I'm going to backport that to 3.12 (and not revert it on main), since it accomplishes what we need for 3.12+.

However, I still think something along the lines of your PR, @vstinner, would be good for 3.12+, since there would be fewer symbols in the stable ABI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner force-pushed the incref_debug branch from 5298304 to 8f2ffe1 Compare May 31, 2023 11:37
@vstinner

vstinner commented May 31, 2023

Copy link
Copy Markdown
Member Author

I rebased my PR on the main branch.

This PR only makes sense in the main branch if it's also backported to Python 3.12.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented May 31, 2023

Copy link
Copy Markdown
Member Author

@encukou @ericsnowcurrently: Would you mind to review the updated PR?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner requested a review from a team as a code owner May 31, 2023 11:53
@vstinner
vstinner force-pushed the incref_debug branch from 8924b0f to d1cd7db Compare May 31, 2023 12:03
@vstinner

vstinner commented May 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

Oh, _Py_NegativeRefcount() is no longer needed in the limited C API: now it's only exported in the stable ABI.
All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently commented May 31, 2023

Copy link
Copy Markdown
Member

I'll try to take a look today.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently
ericsnowcurrently reviewed Jun 1, 2023
View reviewed changes

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

The change seems okay, aside from the discussion about the version check. I'd say this is good to go once that is resolved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner force-pushed the incref_debug branch from d1cd7db to 2f1b829 Compare June 6, 2023 07:26
@vstinner
pythongh-102304: Fix Py_INCREF() stable ABI in debug mode
a3bc4c9 
When Python is built in debug mode (if the Py_REF_DEBUG macro is
defined), the Py_INCREF() and Py_DECREF() function are now always
implemented as opaque functions to avoid leaking implementation
details like the "_Py_RefTotal" variable.

* Remove _Py_IncRefTotal_DO_NOT_USE_THIS() and
  _Py_DecRefTotal_DO_NOT_USE_THIS() from the stable ABI.
* Remove _Py_NegativeRefcount() from limited C API.
* Fix script name in Lib/test/test_stable_abi_ctypes.py
@vstinner
vstinner force-pushed the incref_debug branch from 2f1b829 to a3bc4c9 Compare June 6, 2023 08:42
@vstinner

vstinner commented Jun 6, 2023

Copy link
Copy Markdown
Member Author

I wrote a separated PR for limited API / stable ABI doc changes: PR #105345.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 92022d8 into python:main Jun 6, 2023
@bedevere-bot bedevere-bot removed the awaiting core review label Jun 6, 2023
@vstinner
vstinner deleted the incref_debug branch June 6, 2023 09:15
@vstinner vstinner added the needs backport to 3.12 only security fixes label Jun 6, 2023
@miss-islington

miss-islington commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

Thanks @vstinner 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

miss-islington commented Jun 6, 2023

Copy link
Copy Markdown
Contributor

Sorry, @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 92022d8416d9e175800b65c4d71d4e4fb47adcb0 3.12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned vstinner Jun 6, 2023
@vstinner vstinner mentioned this pull request Jun 6, 2023
gh-102304: doc: Add links to Stable ABI and Limited C API #105345
Merged
@vstinner

vstinner commented Jun 6, 2023

Copy link
Copy Markdown
Member Author

I merged my PR. I'm not comfortable with losing support for Python 3.9 and older, but IMO it's acceptable for a debug build. Moreover, I documented the change to communicate on it and explain how to work around it if needed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner added a commit to vstinner/cpython that referenced this pull request Jun 6, 2023
@vstinner
pythongh-102304: Fix Py_INCREF() stable ABI in debug mode (python#104763
222c6d9 
)

When Python is built in debug mode (if the Py_REF_DEBUG macro is
defined), the Py_INCREF() and Py_DECREF() function are now always
implemented as opaque functions to avoid leaking implementation
details like the "_Py_RefTotal" variable or the
_Py_DecRefTotal_DO_NOT_USE_THIS() function.

* Remove _Py_IncRefTotal_DO_NOT_USE_THIS() and
  _Py_DecRefTotal_DO_NOT_USE_THIS() from the stable ABI.
* Remove _Py_NegativeRefcount() from limited C API.

(cherry picked from commit 92022d8)
@bedevere-bot

bedevere-bot commented Jun 6, 2023

Copy link
Copy Markdown

GH-105352 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 bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 6, 2023
vstinner added a commit to vstinner/cpython that referenced this pull request Jun 6, 2023
@vstinner
pythongh-102304: Fix Py_INCREF() stable ABI in debug mode (python#104763
fee188d 
)

When Python is built in debug mode (if the Py_REF_DEBUG macro is
defined), the Py_INCREF() and Py_DECREF() function are now always
implemented as opaque functions to avoid leaking implementation
details like the "_Py_RefTotal" variable or the
_Py_DecRefTotal_DO_NOT_USE_THIS() function.

* Remove _Py_IncRefTotal_DO_NOT_USE_THIS() and
  _Py_DecRefTotal_DO_NOT_USE_THIS() from the stable ABI.
* Remove _Py_NegativeRefcount() from limited C API.

(cherry picked from commit 92022d8)
vstinner added a commit that referenced this pull request Jun 6, 2023
@vstinner
[3.12] gh-102304: Fix Py_INCREF() stable ABI in debug mode (#104763) (#…
2bbb557 
…105352)

gh-102304: Fix Py_INCREF() stable ABI in debug mode (#104763)

When Python is built in debug mode (if the Py_REF_DEBUG macro is
defined), the Py_INCREF() and Py_DECREF() function are now always
implemented as opaque functions to avoid leaking implementation
details like the "_Py_RefTotal" variable or the
_Py_DecRefTotal_DO_NOT_USE_THIS() function.

* Remove _Py_IncRefTotal_DO_NOT_USE_THIS() and
  _Py_DecRefTotal_DO_NOT_USE_THIS() from the stable ABI.
* Remove _Py_NegativeRefcount() from limited C API.

(cherry picked from commit 92022d8)
@encukou

encukou commented Jun 6, 2023

Copy link
Copy Markdown
Member

I'll ping myself to look at this later: @encukou
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eduardo-elizondo

eduardo-elizondo commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

Just a small nit that this is not necessarily about Py_INCREF only as it also moved Py_DECREF to use _Py_DecRef in debug mode 🙂
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Jun 9, 2023

Copy link
Copy Markdown
Member Author

Just a small nit that this is not necessarily about Py_INCREF only as it also moved Py_DECREF to use _Py_DecRef in debug mode slightly_smiling_face

Yes, the commit message mentions Py_INCREF and Py_DECREF. For issue and PR titles, I try to make them short, so mention a single function name.
eduardo-elizondo 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou left review comments
@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments
@zooba zooba zooba left review comments
+1 more reviewer
@eduardo-elizondo eduardo-elizondo eduardo-elizondo left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@vstinner vstinner

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.

7 participants

@vstinner @ericsnowcurrently @miss-islington @bedevere-bot @encukou @eduardo-elizondo @zooba
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.