update object.h definitions for Python 3.12

[davidhewitt]

This fixes a flaky reference counting issue observed in tests which seems to be related to the new "immortal objects" of Python 3.12.

So many changes had happened to object.h due to the immortal objects I felt it better to do a full reconciliation of the definitions in that file against the 3.12 header.

The reference counting changes are complicated, so I'd like to see a full CI run and also add some more tests before calling this ready to merge.

[davidhewitt]

Ok, looks like we're there 🎉

I changed tests which interact with reference counting to use object() instead of None, because now that None is immortal on 3.12 it's not a suitable value to use in tests which try to modify reference counts!

[davidhewitt]

I'd quite like to merge this soon so that I can proceed with the patch release. The following summarises the diff, to help with reviewing:

• On Python 3.11 buffer.h was renamed to pybuffer.h, so I made the corresponding rename here.

• On Python 3.12 the definitions of getbufferproc and releasebufferproc have been moved to pybuffer.h. (So I moved them for all versions here.)

• On Python 3.12 the object.h file has new symbols _Py_IMMORTAL_REFCNT and _Py_IsImmortal. The implementation of these differs for 32-bit architectures:

  • On 32-bit architectures, the lower 30 bits of the 32-bit reference count are set to 1 to denote an immortal object.
  • On 64-bit architectures, the lower 32 bits are set instead.
  • I expect this will all change again in future when PEP 703 / nogil comes into the picture with thread-biased reference counting.

• I took the argument names out of typedefs for unaryfunc etc, because the Python header doesn't give those arguments names and the argument names were just arg1 arg2 rather than anything meaningful.

• I updated the implementations of a few inline macros in object.rs, adding helper definitions to pyport.rs where these were used. Most of these are trivial, except for Py_INCREF and Py_DECREF.

  • I added some tests to src/ffi/tests.rs to verify the behaviour of these functions. To exercise these with abi3 I had to change that file to exclude individual tests with #[cfg(not(Py_LIMITED_API))] rather than excluding the whole module.

• Finally, I updated other refcounting tests to use object() instead of None, as per comment above.

I appreciate this is quite a meaty patch 😢

[adamreichold]

I think this only works for target_pointer_width = "64" and would hence would prefer that as the cfg attribute here and above. (I don't think we can reasonably CPython on CHERI here in any case.)

[davidhewitt]

The C header used # if SIZEOF_VOID_P > 4, of which the closest analogy I could find was not(target_pointer_width = "32"), disregarding 16 bits which I don't think we would try to support.

[davidhewitt]

https://github.com/python/cpython/blob/e3b5ed7b1cb8ad12aa285cf78303c6dda0953181/Include/object.h#L182-L184

[adamreichold]

I think copying verbatim is not useful here and would prefer the direct formulation as the structure does not make sense for anything but 64-bit architectures.

[adamreichold]

I have to say that CPython is certainly not becoming less baroque.

Besides the inline questions on trying to better abstract out the differences in how the reference counts are handled, I wonder whether PyAny::get_refcnt would have to change as this looks more like Option<isize> then isize conceptionally?

[davidhewitt]

Pushed a commit with review corrections, I'll squash before merge.

I think there is an open question for PyAny::get_refcnt. I'll open an issue.

[adamreichold]

LGTM modulo the usage of target_point_width.

[davidhewitt]

🙏 thanks for the thorough reviewing!

Changed to use cfg(target_pointer_width = "64") instead of cfg(not(target_pointer_width = "32")). We won't support 16 or 128-bit systems for now, can cross that bridge then it seems like there's a reason to.