gh-102304: Fix Py_INCREF() stable ABI in debug mode #104763
Conversation
|
I suggest backporting the change to the 3.12 branch. |
|
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. |
|
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 |
|
PR rebased on top of commit 421cbf3. |
|
Oh, test_enum currently fails on the main branch, as well on my PR. |
See #104765 |
|
PR rebased on main to get the test_enum fix. |
|
A temporary fix was merged in main for this issue: #104766. This PR removes modified code. |
| #if defined(Py_REF_DEBUG) && defined(Py_LIMITED_API) && Py_LIMITED_API+0 >= 0x030A0000 | ||
| // 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); |
There was a problem hiding this comment.
_Py_IncRef was only added in 3.10, so I don't think the Py_LIMITED_API+0 >= 0x030A0000 check can go away.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That refcnt_split is going to change again anyway - @eduardo-elizondo FYI.
There was a problem hiding this comment.
That refcnt_split is going to change again anyway - @eduardo-elizondo FYI.
which makes my point even more relevant :-)
There was a problem hiding this comment.
C extensions built for the stable ABI with Python 3.10 or older don't know about immortal objects :-(
There was a problem hiding this comment.
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.
|
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. |
|
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. |
|
@encukou @ericsnowcurrently: Would you mind to review the updated PR? |
|
Oh, _Py_NegativeRefcount() is no longer needed in the limited C API: now it's only exported in the stable ABI. |
|
I'll try to take a look today. |
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
|
I wrote a separated PR for limited API / stable ABI doc changes: PR #105345. |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
|
Sorry, @vstinner, I could not cleanly backport this to |
|
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. |
) 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)
|
GH-105352 is a backport of this pull request to the 3.12 branch. |
) 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)
…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)
|
I'll ping myself to look at this later: @encukou |
|
Just a small nit that this is not necessarily about Py_INCREF only as it also moved Py_DECREF to use |
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. |
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".