gh-98724: Fix Py_CLEAR() macro side effects

[vstinner]

The Py_CLEAR(), Py_SETREF() and Py_XSETREF() macros now only evaluate
their argument once. If the argument has side effects, these side
effects are no longer duplicated.

Add test_py_clear() and test_py_setref() unit tests to _testcapi.

• Issue: Macro Py_CLEAR references argument two times. #98724

[vstinner]

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. 

[vstinner]

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

[hochl]

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

[vstinner]

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) 

[vstinner]

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

[serhiy-storchaka]

Py_SETREF?

[vstinner]

Py_SETREF?

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

[serhiy-storchaka]

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

[hochl]

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.

[vstinner]

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).

[vstinner]

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.

[serhiy-storchaka]

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.

[vstinner]

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.

[erlend-aasland]

I would consider adding a What's New item.

[vstinner]

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?

[vstinner]

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 :-(

[vstinner]

I tried to make it clear that Py_SETREF() and Py_XSETREF() are implemented as macros.

[serhiy-storchaka]

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

[vstinner]

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.

[vstinner]

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.

[hochl]

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.

[vstinner]

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".

[erlend-aasland]

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.

[erlend-aasland]

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.

[vstinner]

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"?

[erlend-aasland]

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?

[serhiy-storchaka]

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

[vstinner]

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.

[vstinner]

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.

[erlend-aasland]

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.

[serhiy-storchaka]

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

[pitrou]

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

What's the question exactly?

[serhiy-storchaka]

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?

[pitrou]

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

[vstinner]

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.

[serhiy-storchaka]

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.

[erlend-aasland]

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.

[vstinner]

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.

[vstinner]

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.

[vstinner]

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

[bedevere-bot]

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

[vstinner]

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

[vstinner]

I reverted the PR with commit: 3a803bc

Rationale: #99701 (comment)

[bedevere-bot]

⚠️⚠️⚠️ 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):

==

Click to see traceback logs

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