gh-98724: Fix Py_CLEAR() macro side effects (#99100) by vstinner · Pull Request #100070 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The Py_CLEAR(), Py_SETREF() and Py_XSETREF() macros now only evaluate their arguments once. If an argument has side effects, these side effects are no longer duplicated.
Use temporary variables to avoid duplicating side effects of macro arguments side effects. Use memcpy() for assignment to prevent a miscompilation with strict aliasing caused by type punning.
Add test_py_clear() and test_py_setref() unit tests to _testcapi.
cc @serge-sans-paille @erlend-aasland: This version should no longer miscompile Python: it avoids the type punning issue thanks to memcpy(). I wrote a comment in Py_CLEAR() to explain the rationale for memcpy().
This PR no longer reintroduces the miscompilation bug: issue #99701. I tested manually.
I tested this PR with this local patch (otherwise the output is just too verbose!):
diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 8209132ebc..cf243c8ef9 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1891,9 +1891,6 @@ Py_FinalizeEx(void) /* Destroy all modules / finalize_modules(tstate); - / Print debug stats if any */ - _PyEval_Fini();
/* Flush sys.stdout and sys.stderr (again, in case more was printed) */
if (flush_std_files() < 0) {
status = -1;Test command:
rm ~/pyprefix -rf; git clean -fdx
./configure --enable-pystats --prefix ~/pyprefix CC=gcc CFLAGS="-O3 -fstrict-aliasing"
make && make install
./python -m test test_capi test_itertools
The test succeeded with CC=gcc CFLAGS="-O3 -fstrict-aliasing" and CC=clang LD=clang CFLAGS="-O3 -fstrict-aliasing".
I updated my PR to use __typeof__() if available to still emit compiler warnings in Py_SETREF(dst, src) if src has not the same type than dst. On Windows, the memcpy() implementation is used since __typeof__() is not available: memcpy() avoids the type punning issue, but it does not emit a compiler warning if src has the same type.
My previous attempt PR #99739 always used __typeof__(), but this function is not available on Windows with MSVC.
I ran my test for 4 cases and all succeeded:
- GCC with
__typeof__() - GCC with
memcpy() - clang with
__typeof__() - clang with
memcpy()
To also emit a warning in the memcpy() implementation, @serge-sans-paille suggested to me using sizeof(dst=src). It works because sizeof() does not evalute its argument at runtime, it only infers the argument type. MSVC emits a warning with sizeof(dst=src).
I'm not sure about this one. Maybe this enhancement can be made later, once the initial issue #98724 is fixed with my current PR implementation. So it's easy to revert if something goes wrong.
🤖 New build scheduled with the buildbot fleet by @vstinner for commit 86d8878 🤖
If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
buildbot/ARM64 Windows PR
test_ast.test_recursion_direct() crash with a stack overflow. It sounds unrelated to this change. The ARM64 Windows 3.x buildbot is sick for 2 days: it fails with "exception" (don't build+test Python).
test_parse_in_error (test.test_ast.ASTHelpers_Test.test_parse_in_error) ... ok
Windows fatal exception: stack overflow
Current thread 0x0000142c (most recent call first):
File "C:\Workspace\buildarea\pull_request.linaro-win-arm64\build\Lib\test\test_ast.py", line 1252 in test_recursion_direct
The Py_CLEAR(), Py_SETREF() and Py_XSETREF() macros now only evaluate their arguments once. If an argument has side effects, these side effects are no longer duplicated.
Use temporary variables to avoid duplicating side effects of macro arguments side effects. If available, use _Py_TYPEOF() to avoid type punning. Otherwise, use memcpy() for the assignment to prevent a miscompilation with strict aliasing caused by type punning.
Add _Py_TYPEOF() macro: typeof() on GCC and clang.
Add test_py_clear() and test_py_setref() unit tests to _testcapi.
Apart of the sick "ARM64 Windows 3.x" buildbot, all buildbots were happy with this change.
I fixed a fix typos. I amended the commit message.
buildbot/ARM64 Windows PR: test_ast.test_recursion_direct() crash with a stack overflow
FYI I proposed #100104 to fix this bug (unrelated to this change).
2 participants
