cpython
cpython copied to clipboard
python
gh-98724: Fix Py_CLEAR() macro side effects
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: gh-98724
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 https://github.com/python/cpython/issues/98724#issuecomment-1292591490 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?
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.
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.
I created a 3.11 backport without documentation changes: https://github.com/python/cpython/pull/99288
I created a 3.11 backport without documentation changes: #99288
But that will invalidate the versionchanged in the docs...?
But that will invalidate the versionchanged in the docs...?
I fixed the bug silently, but I cannot document that it's the default behavior in Python 3.11.0 since the fix will only be done in later 3.11.x bugfix versions.
Hum. I read again the discussion and now I'm confused.
@serhiy-storchaka @erlend-aasland: Should I backport the fix to 3.10 and 3.11 or not?
I just backported the change to 3.11 as: https://github.com/python/cpython/commit/108289085719db8b227d65ce945e806f91be8f80
I created backport to 3.10 but then I closed it before it got merged: https://github.com/python/cpython/pull/99292
My idea is to fix the bug in Python 3.10 and 3.11 with a changelog entry, but don't document the behavior in the C API documentation with versionchanged. Is it a bad thing to fix this bug in 3.10 and 3.11?
People who need to use Py_CLEAR(), Py_SETREF() or Py_XSETREF() with side effects on old Python versions can redefine the macros with a flavor which has the bugfix, no?
I created https://github.com/python/pythoncapi-compat/pull/50 to fix the two macros in the pythoncapi-compat project.
I proposed to add them to the stable API. But there were objections.
https://mail.python.org/archives/list/[email protected]/thread/HGBT2Y4VAZOOU5XI2RPSURDGFBMRJKPO/
If you want to reconsider this, please open a new discussion.
By the way, Python 3.12 contains an important change related to memory allocation of objects tracked by the GC:
The Garbage Collector now runs only on the eval breaker mechanism of the Python bytecode evaluation loop instead on object allocations. The GC can also run when PyErr_CheckSignals() is called so C extensions that need to run for a long time without executing any Python code also have a chance to execute the GC periodically. (Contributed by Pablo Galindo in gh-97922.)
See https://github.com/python/cpython/issues/97922 for details. The change was done in _PyObject_GC_New() and _PyObject_GC_NewVar(), not on Py_DECREF().
See also issue #89060 and @nascheme's PR #27738 "WIP: Integrate trashcan into _Py_Dealloc". But again, this WIP change is only for objects tracked by the GC.