I looked at the x86-64 machine code of test_py_clear() built by gcc -O3. I don't see any PyObject** type anymore, GCC works directly on pointers (PyObject*): inefficient was removed by the compiler, as expected.

(gdb) disassemble test_py_clear
Dump of assembler code for function test_py_clear:
   0x00007fffea27ba90 <+0>:	sub    rsp,0x8

   # obj = PyList_New(0);
   0x00007fffea27ba94 <+4>:	xor    edi,edi
   0x00007fffea27ba96 <+6>:	call   0x7fffea279080 <PyList_New@plt>

   # if (!obj) return NULL;
   0x00007fffea27ba9b <+11>:	test   rax,rax
   0x00007fffea27ba9e <+14>:	je     0x7fffea27bab1 <test_py_clear+33>

   # Py_DECREF(obj);
   0x00007fffea27baa0 <+16>:	sub    QWORD PTR [rax],0x1
   0x00007fffea27baa4 <+20>:	je     0x7fffea27bac0 <test_py_clear+48>

   # Py_RETURN_NONE;
   0x00007fffea27baa6 <+22>:	mov    rax,QWORD PTR [rip+0x224fb]        # 0x7fffea29dfa8
   0x00007fffea27baad <+29>:	add    QWORD PTR [rax],0x1
   0x00007fffea27bab1 <+33>:	add    rsp,0x8
   0x00007fffea27bab5 <+37>:	ret    

   # _Py_Dealloc()
   0x00007fffea27bab6 <+38>:	cs nop WORD PTR [rax+rax*1+0x0]
   0x00007fffea27bac0 <+48>:	mov    rdi,rax
   0x00007fffea27bac3 <+51>:	call   0x7fffea279400 <_Py_Dealloc@plt>
   0x00007fffea27bac8 <+56>:	jmp    0x7fffea27baa6 <test_py_clear+22>
End of assembler dump.

I completed test_py_clear() to test calling Py_CLEAR() with a side effect: test based on #98724 (comment) idea.

I have some suggestion for the test: It should crash with the old Py_CLEAR version so you do not get any regressions.

I have some suggestion for the test: It should crash with the old Py_CLEAR version so you do not get any regressions.

It does crash with the old Py_CLEAR() macro:

$ ./python -m test -v test_capi  -m test_py_clear 
(...)
test_py_clear (test.test_capi.Test_testcapi.test_py_clear) ...
Fatal Python error: Segmentation fault
(...)
Erreur de segmentation (core dumped)

cc @serhiy-storchaka: there is now a unit test which shows that the old macro had a bug.

Py_SETREF?

Py_SETREF?

I don't understand your comment. This PR fix a bug in Py_CLEAR(). Can you please elaborate?

Py_SETREF and Py_XSETREF are similar to Py_CLEAR. If change the latter, why not change the formers?

I think all macros should be changed so they only reference their arguments once. This avoids the whole issue with side effects applied multiple times.

I think all macros should be changed so they only reference their arguments once. This avoids the whole issue with side effects applied multiple times.

That's one of the purpose of PEP 670. It's just that @erlend-aasland and me missed these macros (Py_CLEAR, Py_SETREF, Py_XSETREF).

Py_SETREF and Py_XSETREF are similar to Py_CLEAR. If change the latter, why not change the formers?

You're right. I completed my PR to fix also Py_SETREF() and Py_XSETREF() macros.

Py_SETREF() and Py_XSETREF() macros require an additional _PyObject_CAST() since the l-value type becomes a PyObject*, whereas previously it had the type of the op argument:

PyObject **_py_tmp_ptr = _Py_CAST(PyObject**, &(op));
...
(*_py_tmp_ptr) = _PyObject_CAST(op2);  // <==== HERE

I added unit tests.

I would document it as a change in 3.12. And older versions need documenting that the first argument can be evaluated more than once.

I would document it as a change in 3.12.

Ok, I documented the Py_CLEAR() change with a "versionchanged".

But Py_SETREF() and Py_XSETREF() are not documented. Maybe it's no purpose, so I didn't modify their (non existing) documentation.

I would consider adding a What's New item.

I would consider adding a What's New item.

Ok, I added a What's New in Python 3.12 entry. I copied the same text everywhere.

Would you mind to review the update documentation text?

I renamed Py_SETREF() and Py_XSETREF() arguments, I documented Py_SETREF() and Py_XSETREF(). I fixed a typo in the documentation.

@erlend-aasland: Would you mind to review the updated PR?

Sorry, I abused git commit --amend && git rebase main && git push --force on this PR, I should use multiple commits to ease review instead :-(

Py_SETREF() and Py_XSETREF() are intentionally not documented because they are not in the public API.

Py_SETREF() and Py_XSETREF() are intentionally not documented because they are not in the public API.

If I include <Python.h>, I get these macros. What do you mean by "not part of the public API"? Do you want to move them to the internal C API?

Multiple projects use these macros. Code search in PyPI top 5000 projects using (Py_SETREF|Py_XSETREF) regex:

16 projects (177 matching lines in total):

• bitarray-2.6.0
• coverage-6.4.4
• datatable-1.0.0
• gnureadline-8.1.2
• immutables-0.18
• inflate64-0.3.0
• iteration_utilities-0.11.0
• mypy-0.971
• numpy-1.23.2
• pandas-1.4.4
• pickle5-0.0.12
• python-rapidjson-1.8
• python-snappy-0.6.1
• scipy-1.9.1
• typed_ast-1.5.4
• zstd-1.5.2.5

numpy and the pythoncapi-compat project (code) define these macros on Python older than 3.5.2.

IMO it's too late, people are now consuming the macros, so it's good to document them. Not documenting them doesn't prevent people from using them.

Oh, on these 16 projects, 4 projects only define Py_SETREF() and Py_XSETREF() in a copy of the pythoncapi_capi.h header file:

• bitarray-2.6.0
• mypy-0.971
• python-snappy-0.6.1
• zstd-1.5.2.5

They don't use these macros.

Doc/c-api/refcounting.rst contains two times an error: If the argument has side effects, there are no longer duplicated. Further down it is correct with these are no longer duplicated.

Doc/c-api/refcounting.rst contains two times an error: If the argument has side effects, there are no longer duplicated. Further down it is correct with these are no longer duplicated.

Ah, I hesitated between both :-) I changed for "these are".

Py_SETREF() and Py_XSETREF() are intentionally not documented because they are not in the public API.

Unfortunately, anything that's included by Python.h is de facto part of the public C API, documented or not.

Adding these functions to the Limited API should be discussed in a separate issue. It is out of scope for gh-98724, and this PR is definitely not the correct place to discuss it.

Regarding the backports, which should be discussed here (or on the linked issue): if we are not to update the docs in the backports, I don't think the backports should be made. It is unfortunate if the docs are inaccurate because of the backports.

Regarding the backports, which should be discussed here (or on the linked issue): if we are not to update the docs in the backports, I don't think the backports should be made.

What if I put a "versionchanged" with Python 3.11.x and Python 3.10.x bugfix versions, rather than "3.11" and "3.10"?

What if I put a "versionchanged" with Python 3.11.x and Python 3.10.x bugfix versions, rather than "3.11" and "3.10"?

I'm not sure how to handle that correctly in Sphinx. In order to be accurate, the versionchanged in main should include both 3.10.x, 3.11.1, and 3.12, so the rendered version should read "Changed in version 3.10.x, 3.11.1, and 3.12". Is that even possible?

I just feel uncomfortable that these macros were documented without discussion. What is they official status?

I just feel uncomfortable that these macros were documented without discussion. What is they official status?

I don't understand your point about the fact that functions which are excluded from the limited C API should not be documented.

Most of the C API is excluded from the limited C API and it is documented. For example, https://docs.python.org/dev/c-api/frame.html documents many PyFrame functions, PyFrame_GetBack() is documented and excluded from the limited C API. Only some functions have the "Part of the Stable ABI" label (see this Frame C API doc).

By the way, many C API macros are documented.

What is they official status?

Py_CLEAR(), Py_SETREF(), Py_XSETREF() are part of the public C API, are tested and documented in Python 3.12. How is it an issue? Sorry, I still don't get your point.

I'm not sure how to handle that correctly in Sphinx. In order to be accurate, the versionchanged in main should include both 3.10.x, 3.11.1, and 3.12, so the rendered version should read "Changed in version 3.10.x, 3.11.1, and 3.12". Is that even possible?

That's problematic in practice if 3.10 or 3.11 get a special release just for a single security fix, not based on the current 3.10 and 3.11 branch. Well, I don't recall that it ever happened. But I would prefer to not attempt to which today what will be the 3.x.y release in 3.10, 3.11 and main branches which will include the change. I prefer to only put 3.10.x in 3.10 doc, 3.11.x in the 3.11 doc, and 3.12 in the main branch. We did that multiple times in the past for security fixes. See also:

• https://docs.python.org/dev/whatsnew/3.9.html#notable-changes-in-python-3-9-2
• https://docs.python.org/dev/whatsnew/3.8.html#notable-changes-in-python-3-8-1
• https://docs.python.org/dev/whatsnew/3.7.html#notable-changes-in-python-3-7-1
• https://docs.python.org/dev/whatsnew/3.6.html#notable-changes-in-python-3-6-4

I'm not suggesting to document Py_SETREF() change as a "notable change".

I would prefer to not even document it in 3.10 and 3.11 branches. I don't get why you guys consider that Py_SETREF() is special enough to require that the bugfix is documented. But I'm fine with document it if you prefer.

I prefer to only put 3.10.x in 3.10 doc, 3.11.x in the 3.11 doc, and 3.12 in the main branch. We did that multiple times in the past for security fixes.

Ok, that makes sense.

I'm not suggesting to document Py_SETREF() change as a "notable change".

+1

I would prefer to not even document it in 3.10 and 3.11 branches.

The easiest thing to do would of course be to just keep this bugfix/feature in main, and not backport it :) I see I lack the historical insight in how you've previously tackled such backport issues, so I'm not sure my opinion should matter much here.

Since there were objections and concerns, you should ask at least for opinions of @rhettinger, @pitrou and @ncoghlan.

Since there were objections and concerns, you should ask at least for opinions of @rhettinger, @pitrou and @ncoghlan.

What's the question exactly?

What is the status of Py_SETREF and Py_XSETREF? Should they be the part of the public C API, or private internal CPython helpers? And if they are public, from which version?

I think they can be part of the public API, as they are useful helpers.

What is the status of Py_SETREF and Py_XSETREF? Should they be the part of the public C API, or private internal CPython helpers?

Did you even read my messages? These macros are public and are used in the wild. Removing these macros would break projects.

They were added as non-public with intention to make them public in the next feature release. But the latter thing did never happen. They are not officially public.

They are not officially public.

Only in docs. But they are part of whats included in Python.h, so they are de facto public. Might as well document them. As Victor points out, removing them from Python.h is a breaking change, which proves the fact that they are public.

By the way, I created issue #99300 to replace Py_INCREF() with Py_NewRef(), and issue #99481 to propose adding a "Py_HOLD_REF()" macro.

The easiest thing to do would of course be to just keep this bugfix/feature in main, and not backport it :)

Py_CLEAR() was added to Python 2.4 in 2004 with commit 8c5aeaa.

Py_SETREF() was added to Python 3.6 (and 3.5.2) in 2016 with commit 57a01d3.

The issue #98724 was only reported in 2022. So people were fine with duplicated side effects in Py_CLEAR() for 18 years and in Py_SETREF() for 6 years.

Well, ok, I revert my Python 3.11 change: #99573

In Python 3.11 and older, there is simple workaround: avoid side effects in these macros.

I also suspect that the issue #98724 doesn't come from a real application, but was just spotted as a theorical issue. So yeah, there is no urgency to fix it. And again, there are trivial ways to workaround the issue.

I created issue #99574 "Add Py_SETREF() and Py_XSETREF() macros to the limited C API 3.12".

GH-99573 is a backport of this pull request to the 3.11 branch.

Ok, I reverted my 3.11 change to keep the duplication of side effects in Python 3.11.

I reverted the PR with commit: 3a803bc

Rationale: #99701 (comment)

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot ARM64 Windows 3.x has failed when building commit b11a384.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/729/builds/3114) and take a look at the build logs.
4. Check if the failure is related to this commit (b11a384) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/729/builds/3114

Failed tests:

• test_ast

Summary of the results of the build (if available):

==

remote: Enumerating objects: 17, done.        
remote: Counting objects:   5% (1/17)        
remote: Counting objects:  11% (2/17)        
remote: Counting objects:  17% (3/17)        
remote: Counting objects:  23% (4/17)        
remote: Counting objects:  29% (5/17)        
remote: Counting objects:  35% (6/17)        
remote: Counting objects:  41% (7/17)        
remote: Counting objects:  47% (8/17)        
remote: Counting objects:  52% (9/17)        
remote: Counting objects:  58% (10/17)        
remote: Counting objects:  64% (11/17)        
remote: Counting objects:  70% (12/17)        
remote: Counting objects:  76% (13/17)        
remote: Counting objects:  82% (14/17)        
remote: Counting objects:  88% (15/17)        
remote: Counting objects:  94% (16/17)        
remote: Counting objects: 100% (17/17)        
remote: Counting objects: 100% (17/17), done.        
remote: Compressing objects:   6% (1/16)        
remote: Compressing objects:  12% (2/16)        
remote: Compressing objects:  18% (3/16)        
remote: Compressing objects:  25% (4/16)        
remote: Compressing objects:  31% (5/16)        
remote: Compressing objects:  37% (6/16)        
remote: Compressing objects:  43% (7/16)        
remote: Compressing objects:  50% (8/16)        
remote: Compressing objects:  56% (9/16)        
remote: Compressing objects:  62% (10/16)        
remote: Compressing objects:  68% (11/16)        
remote: Compressing objects:  75% (12/16)        
remote: Compressing objects:  81% (13/16)        
remote: Compressing objects:  87% (14/16)        
remote: Compressing objects:  93% (15/16)        
remote: Compressing objects: 100% (16/16)        
remote: Compressing objects: 100% (16/16), done.        
remote: Total 17 (delta 1), reused 11 (delta 1), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to 'b11a384dc7471ffc16de4b86e8f5fdeef151f348'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at b11a384dc7 gh-98724: Fix Py_CLEAR() macro side effects (#99100) (#100070)
Switched to and reset branch 'main'

Could Not Find C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\*.pyc
The system cannot find the file specified.
Could Not Find C:\Workspace\buildarea\3.x.linaro-win-arm64\build\PCbuild\python*.zip

Windows fatal exception: stack overflow

Current thread 0x00003664 (most recent call first):
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\test_ast.py", line 1252 in test_recursion_direct
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\case.py", line 579 in _callTestMethod
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\case.py", line 623 in run
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\case.py", line 678 in __call__
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 122 in run
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 84 in __call__
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 122 in run
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 84 in __call__
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 122 in run
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\suite.py", line 84 in __call__
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\unittest\runner.py", line 208 in run
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\support\__init__.py", line 1100 in _run_suite
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\support\__init__.py", line 1226 in run_unittest
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 281 in _test_module
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 317 in _runtest_inner2
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 360 in _runtest_inner
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 235 in _runtest
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\runtest.py", line 265 in runtest
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 352 in rerun_failed_tests
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 754 in _main
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 709 in main
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\libregrtest\main.py", line 773 in main
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\test\__main__.py", line 2 in <module>
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\runpy.py", line 88 in _run_code
  File "C:\Workspace\buildarea\3.x.linaro-win-arm64\build\Lib\runpy.py", line 198 in _run_module_as_main

Cannot open file 'C:\Workspace\buildarea\3.x.linaro-win-arm64\build\test-results.xml' for upload

Could Not Find C:\Workspace\buildarea\3.x.linaro-win-arm64\build\PCbuild\python*.zip