gh-127443: Update refcounts data with stable ABI functions. #127444

picnixz · 2024-11-30T10:15:59Z

Since the changes were done manually and are mechanical, I'm pretty sure there are some +1 missing or incorrectly deduced. I've added some comments here and there about questions I had.

Issue: Improve and maintain Doc/data/refcounts.dat #127443

📚 Documentation preview 📚: https://cpython-previews--127444.org.readthedocs.build/

serhiy-storchaka

Thank you for your work. I left some questions, but more thoughtful review I can only do in a week.

Doc/data/refcounts.dat

serhiy-storchaka · 2024-11-30T12:28:11Z

Doc/data/refcounts.dat

+PyDict_GetItemRef:int:::
+PyDict_GetItemRef:PyObject *:p:0:
+PyDict_GetItemRef:PyObject *:key:0:
+PyDict_GetItemRef:PyObject **:result:+1:


I am not sure how does it work with double pointers. Are there precedences?

BTW, are there any recirds with non-empty 5th field, and if there are any, what is its meaning?

I am not sure how does it work with double pointers. Are there precedences?

One precedent is

PyErr_GetExcInfo:void::: PyErr_GetExcInfo:PyObject**:ptype:+1: PyErr_GetExcInfo:PyObject**:pvalue:+1: PyErr_GetExcInfo:PyObject**:ptraceback:+1:

I think the convention here is that we set the destination pointer to a new reference of the item we found, effectively increasing the reference count of the underlying object.

BTW, are there any recirds with non-empty 5th field, and if there are any, what is its meaning?

Yes, the meaning is just a "comment".

Doc/data/refcounts.dat

serhiy-storchaka · 2024-11-30T12:41:26Z

Doc/data/refcounts.dat

+PyModule_Add:int:::
+PyModule_Add:PyObject *:module:0:
+PyModule_Add:const char *:name:0:
+PyModule_Add:PyObject *:value:0:stolen


Should not the refcount be -1?

There is a comment at the top of the file that says:

XXX NOTE: the 0/+1/-1 refcount information for arguments is
confusing! Much more useful would be to indicate whether the
function "steals" a reference to the argument or not. Take for
example PyList_SetItem(list, i, item). This lists as a 0 change for
both the list and the item arguments. However, in fact it steals a
reference to the item argument!

Technically, the reference count of the value itself does not change even though you're stealing the reference.

Tangentially related: can we add a stolen comment to the PyList_SetItem? Though the comment at the beginning of the file explicitly says this, I still think it would be useful to add it.

I think it would be good but I'd like to address the stealing references separately (easier for reviewers I think).

Doc/data/refcounts.dat

ZeroIntensity · 2024-11-30T22:32:40Z

Upon reviewing, I've discovered that I hate this format 😄. I'm going to throw together a script to convert this to TOML or something. Would others be interested in using such a script?

picnixz · 2024-11-30T22:52:41Z

Upon reviewing, I've discovered that I hate this format

I also don't like this format. We can convert it to TOML but what do you suggest?

ZeroIntensity · 2024-11-30T22:54:00Z

TOML is probably the way to go here. Let's deal with converting later though.

ZeroIntensity

Some other things that I found that you didn't modify:

PyErr_NormalizeException is missing a reference count effect for a parameter.
So is PySeqIter_New.
PySequence_FAST_ITEMS is missing an effect for its return value (double check me on that, it returns PyObject **).
PyUnicode_AsUTF8AndSize erroneously has a reference count effect for a Py_ssize_t * parameter.
Some functions that take PyModuleDef are missing reference count effects for the passed module def. Namely, PyModule_Create, PyModule_Create2, PyModule_ExecDef, PyModule_FromDefAndSpec, PyModule_FromDefAndSpec2, PyState_AddModule, PyState_FindModule, PyState_RemoveModule, and PyType_GetModuleByDef.

Doc/data/refcounts.dat

ZeroIntensity · 2024-12-01T01:58:01Z

Doc/data/refcounts.dat

@@ -1878,6 +2234,26 @@ PyObject_TypeCheck:int:::
 PyObject_TypeCheck:PyObject*:o:0:
 PyObject_TypeCheck:PyTypeObject*:type:0:

+PyObject_Vectorcall:PyObject *::+1:
+PyObject_Vectorcall:PyObject *:callable:0:
+PyObject_Vectorcall:PyObject *const *:args:0:


We don't have a reference count effect for any of the other PyObject *const * functions. Let's make a decision on whether to have it or not for these.

Suggested change

PyObject_Vectorcall:PyObject *const *:args:0:

PyObject_Vectorcall:PyObject *const *:args::

I prefer to have them. We can think of args as being loosely typed as PyObject ** and since we are also managing double pointers like that, I think we can also manage those.

Could you fix the other cases then?

I'd like to do it in a separate PR. I don't want to change existing lines in this specific PR.

Doc/data/refcounts.dat

ZeroIntensity · 2024-12-01T02:07:16Z

Doc/data/refcounts.dat

+PyUnicodeTranslateError_SetStart:int:::
+PyUnicodeTranslateError_SetStart:PyObject *:exc:0:
+PyUnicodeTranslateError_SetStart:Py_ssize_t:start::
+
 PyWeakref_Check:int:::
 PyWeakref_Check:PyObject*:ob::


This, as well as PyWeakref_CheckRef and PyWeakref_CheckProxy below it, need reference count effects.

Suggested change

PyWeakref_Check:PyObject*:ob::

PyWeakref_Check:PyObject*:ob:0:

Addressed in #127451.

picnixz · 2024-12-01T06:43:42Z

Some of those are done in the other PR! I'll address the remaining in a few hours . Thanks!

In PyErr_SetHandledException, the exception's reference count is incremented by 1 since it's being used as `Py_XSETREF(..., Py_NewRef(exc))`. In PyType_FromModuleAndSpec, the module's reference count is incremented by 1 since it's being stored in the new class being created.

…e-127443

encukou · 2024-12-04T14:26:00Z

Doc/data/refcounts.dat

+PyOS_string_to_double:double:::
+PyOS_string_to_double:const char *:s::
+PyOS_string_to_double:char **:endptr::
+PyOS_string_to_double:PyObject *:overflow_exception:+1:


Is +1 correct here? It's ±0 on success.

encukou · 2024-12-04T14:32:00Z

Doc/data/refcounts.dat

@@ -3016,10 +3812,50 @@ _PyBytes_Resize:int:::
 _PyBytes_Resize:PyObject**:bytes:0:
 _PyBytes_Resize:Py_ssize_t:newsize::

+_PyObject_CallFunction_SizeT:PyObject *::+1:abi-only


Please pick only one of abi_only or abi-only.

bedevere-app bot mentioned this pull request Nov 30, 2024

Improve and maintain Doc/data/refcounts.dat #127443

Open

picnixz added skip news docs Documentation in the Doc dir labels Nov 30, 2024

picnixz force-pushed the doc/refcounts/update-127443 branch 6 times, most recently from d1e6a24 to b798ba3 Compare November 30, 2024 11:57

Update refcounts data with stable ABI functions.

ef9793d

picnixz force-pushed the doc/refcounts/update-127443 branch from b798ba3 to ef9793d Compare November 30, 2024 12:07

picnixz marked this pull request as ready for review November 30, 2024 12:08

bedevere-app bot added the awaiting review label Nov 30, 2024

picnixz requested review from encukou, serhiy-storchaka, ZeroIntensity and Eclips4 November 30, 2024 12:09

serhiy-storchaka reviewed Nov 30, 2024

View reviewed changes

address Serhiy's review

da56c71

ZeroIntensity reviewed Dec 1, 2024

View reviewed changes

picnixz added 5 commits December 1, 2024 08:46

address Peter's review

f0ea3df

Address Peter's review (round 2)

3aa16c9

add missing stable ABI entries

f30c34c

complete TODOs

d9fa7b9

picnixz mentioned this pull request Dec 1, 2024

gh-127443: Add semantics for stolen references in Doc/data/refcounts.dat #127468

Open

Merge remote-tracking branch 'upstream/main' into doc/refcounts/updat…

9b25179

…e-127443

encukou reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-127443: Update refcounts data with stable ABI functions. #127444

gh-127443: Update refcounts data with stable ABI functions. #127444

picnixz commented Nov 30, 2024 •

edited

Loading

serhiy-storchaka left a comment

serhiy-storchaka Nov 30, 2024

picnixz Nov 30, 2024

picnixz Nov 30, 2024

serhiy-storchaka Nov 30, 2024

picnixz Nov 30, 2024 •

edited

Loading

Eclips4 Nov 30, 2024

picnixz Nov 30, 2024

ZeroIntensity commented Nov 30, 2024 •

edited

Loading

picnixz commented Nov 30, 2024

ZeroIntensity commented Nov 30, 2024

ZeroIntensity left a comment

ZeroIntensity Dec 1, 2024

picnixz Dec 1, 2024

ZeroIntensity Dec 1, 2024

picnixz Dec 1, 2024

ZeroIntensity Dec 1, 2024

picnixz Dec 1, 2024

picnixz commented Dec 1, 2024

encukou Dec 4, 2024

encukou Dec 4, 2024

	PyObject_Vectorcall:PyObject const :args:0:
	PyObject_Vectorcall:PyObject const :args::

	PyWeakref_Check:PyObject*:ob::
	PyWeakref_Check:PyObject*:ob:0:

gh-127443: Update refcounts data with stable ABI functions. #127444

Are you sure you want to change the base?

gh-127443: Update refcounts data with stable ABI functions. #127444

Conversation

picnixz commented Nov 30, 2024 • edited Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroIntensity commented Nov 30, 2024 • edited Loading

picnixz commented Nov 30, 2024

ZeroIntensity commented Nov 30, 2024

ZeroIntensity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz commented Dec 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz commented Nov 30, 2024 •

edited

Loading

picnixz Nov 30, 2024 •

edited

Loading

ZeroIntensity commented Nov 30, 2024 •

edited

Loading