-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
gh-111506: Implement Py_SET_REFCNT() as inline function #112798
Conversation
On macOS, the build fails with:
Windows build complains in
Oh, and Free Threading build complains with:
|
d4488ed
to
33b8a8d
Compare
As suggested by @pitrou, I added -inline void Py_SET_REFCNT(PyObject *ob, Py_ssize_t refcnt) {
+inline PyAPI_FUNC(void) Py_SET_REFCNT(PyObject *ob, Py_ssize_t refcnt) { It seems like test_cppext pass: C++ code including |
Is it even possible to define an inline function in a header file and export it from a DLL? Which compilation unit gets the implementation? Or does it just pick one at random (there's another declspec that does that, IIRC). The exported function doesn't actually have to be defined in a header file. But then, why not define the real function as There's no need to get into such obscure definitions. They'll inherently destabilise our code, so better to just avoid them and use what works reliably (and yes, a simple macro |
You can read about it here:
In this PR it doesn't, if the limited API is enabled.
Which obscure definition?
|
Convert Py_SET_REFCNT() and _Py_IsImmortal() static inline functions to inline functions. Py_SET_REFCNT() function is now exported as "Py_SET_REFCNT" name in the stable ABI, instead of "_Py_SetRefcnt". _Py_IsImmortal(), _Py_IsOwnedByCurrentThread() and _Py_ThreadId() have to be converted to an inline functions, since non-static Py_SET_REFCNT() function cannot call a static functions. _Py_IsImmortal(), _Py_IsOwnedByCurrentThread() and _Py_ThreadId() are not part of the stable ABI.
33b8a8d
to
49d6671
Compare
Oops, I replaced another |
Oh neat. That's... more intelligent than I ever expected from C++. I wonder if we can use |
@pitrou: Ah nice, all tests pass now. |
In the limited API, this PR declares Py_SET_REFCNT() as:
It's not declared as an inline function, but as a regular function. Implementation to export Py_SET_REFCNT() as a function, in extern inline PyAPI_FUNC(void) Py_SET_REFCNT(PyObject *ob, Py_ssize_t refcnt); |
Yes, I saw that. It's in its own |
The problem is not about adding an attribute to the function. The problem is that it's not technically possible to declare the function as The whole point of the limited C API is to not expose any implementation details. |
MSBuild emits the following error with Release mode:
|
This C extension is built for the limited C API version 3.5. In this version, Py_SET_REFCNT() is implemented as:
This implementation is very fragile for the limited C API version 3.5. Python 3.11 and older don't have immortal objects and should not call _Py_IsImmortal(). But Python 3.12 changed the limited C API making the assumption that "well, it should be nice" :-) See the Stable ABI section of PEP 683 – Immortal Objects, Using a Fixed Refcount. But at the ABI level, Or maybe I misunderstood something. Well, I'm not sure of what "unresolved external symbol _imp__Py_IsImmortal" means here. |
@pitrou: Do you have an explanation/fix for the error?
|
Well, it means that the implementation that you quoted above is inlined in the caller:
and that |
I looked at the |
If it's inlined, why does the linker expect getting the symbol from Python? |
I suppose that |
Oh interesting, when Python is built in debug mode, Py_ALWAYS_INLINE macro is always expanded as an empty expression: #if defined(Py_DEBUG)
// If Python is built in debug mode, usually compiler optimizations are
// disabled. In this case, Py_ALWAYS_INLINE can increase a lot the stack
// memory usage. For example, forcing inlining using gcc -O0 increases the
// stack usage from 6 KB to 15 KB per Python function call.
# define Py_ALWAYS_INLINE
|
This seems risky, especially because our header files are included by both C and C++ projects. I think |
This PR does two things:
Example with the current PR: $ ./python
Python 3.13.0a2+ (heads/set_refcnt_inline:49d6671183d, Dec 12 2023, 11:44:21) [GCC 13.2.1 20231205 (Red Hat 13.2.1-6)] on linux
>>> import ctypes
>>> python = ctypes.cdll.LoadLibrary(None)
>>> python.Py_SET_REFCNT
<_FuncPtr object at 0x7f8493c42440> |
Yeah, what I'm trying to say is that I think that (1) "Replace static inline with inline" is a risky way to achieve (2) "Export as regular functions" when we have other ways of exporting these as regular functions. I think it's risky because non-static inlines are treated differently in C than in C++, have meaningfully different behavior when compiling with optimizations vs. without, and the complexity and risk is increased because As a concrete example, an extension using This was not an issue when we used "static inline" because you don't get undefined symbols like you do with non-static inlines. |
Do you prefer PR #112794 approach which keeps |
@vstinner yes, although it seems like that could be simpler if we named the regular function differently from the |
Let's focus on the static inline API in this case. |
Convert Py_SET_REFCNT() and _Py_IsImmortal() static inline functions to inline functions.
Py_SET_REFCNT() function is now exported as "Py_SET_REFCNT" name in the stable ABI, instead of "_Py_SetRefcnt".
_Py_IsImmortal() has to be converted to an inline function, since non-static Py_SET_REFCNT() function cannot call a static _Py_IsImmortal() function. _Py_IsImmortal() is not part of the stable ABI.
--disable-gil
builds are not compatible with the limited API #111506📚 Documentation preview 📚: https://cpython-previews--112798.org.readthedocs.build/