Remove zero or negative reference counter check in PyObject_Print

[MonadChains]

Feature or enhancement

Remove the section of the PyObject_Print function that handles input objects that have a negative reference counter:

cpython/Objects/object.c

Lines 274 to 278 in 88297e2

	if (Py_REFCNT(op) <= 0) {
	Py_BEGIN_ALLOW_THREADS
	fprintf(fp, "<refcnt %zd at %p>", Py_REFCNT(op), (void *)op);
	Py_END_ALLOW_THREADS
	}

Previous discussion

The PR #98749 that improves the coverage of PyObject_Print is blocked due to a failing pipeline. After investigation, the test that is failing is associated with the Address Sanitizer. In particular, it fails the test that covers the aforementioned section. It consists in passing an object (specifically a Python string) with its reference counter set to zero and comparing the output string to the expected result: https://github.com/MonadChains/cpython/blob/562b9d26db7e766741c954146d4423eda414d2f6/Modules/_testcapimodule.c#L2053-L2080
However, when the reference counter of the object goes to zero, it is deallocated, and the Address Sanitizer does not like it.
After the discussion in https://mail.python.org/archives/list/[email protected]/thread/7BP55KL6VB3H2ZZ5JZP2NV6RX72HBUCC/, a possible solution to this is to clean up the function and remove this section entirely. Normally this kind of check is performed only in specific situations like for example _Py_ForgetReference or Py_DECREF so it seems a bit weird to have it in PyObject_Print.
Alternatively, another solution could be to remove the failing test from the PR and don't cover this section of the function.

• PR: gh-98928: Remove negative reference counter check section from PyObject_Print #98929

[JelleZijlstra]

The section you pointed to seems useful as a debugging aid. Obviously objects with nonpositive refcounts shouldn't exist, but bugs happen, and if so this code could make the bug easier to diagnose.

Maybe we can just not worry about covering this branch. Alternatively, your PR could set the refcount to 0 directly instead of using DECREF.

[MonadChains]

Hi @JelleZijlstra. Thank you for your suggestion. I've modified the PR and now the pipeline is working. I will close this issue as not necessary anymore.

[ericvsmith]

I'm glad you got it working.

Here are my thoughts: if debugging negative reference counts is a useful feature (and it is), then having PyObject_Print being the only place it's used seems odd to me. Maybe we should add a more general purpose version of this that can be used elsewhere, and add a test helper for testing it.

[MonadChains]

Hi @ericvsmith, thank you for your help. What do you mean by a more general-purpose version of this check? Do you mean a function that asserts that an object has a positive reference count? Or do you mean something for logging such cases?
I would be more than happy to participate in the design and implantation of such functionality.

[ericvsmith]

I don't have any particular ideas: I'm just thinking out loud. I just know that I've written code with reference count errors, and if I ever called PyObject_Print this would have found them. But that's not an API I ever call. What if other APIs had this functionality?