GeezerVison is showing github.com — object.h uses an anonymous union in a struct (older C incompatible) · Issue #105059 · python/cpython
object.h uses an anonymous union in a struct (older C incompatible) #105059Activityzooba commented on May 29, 2023 Seems the only use of it is later in object.h:
#if SIZEOF_VOID_P > 4
// Portable saturated add, branching on the carry flag and set low bits
PY_UINT32_T cur_refcnt = op->ob_refcnt_split[PY_BIG_ENDIAN];
PY_UINT32_T new_refcnt = cur_refcnt + 1;
if (new_refcnt == 0) {
return;
}
op->ob_refcnt_split[PY_BIG_ENDIAN] = new_refcnt;
Is this really better than this code?:
Py_ssize_t new_refcnt = op->ob_refcnt + 1;
if (new_refcnt & ~0xFFFFFFFF) return;
// or
if ((PY_UINT32_T)new_refcnt != new_refcnt) return;
Seems we can rely on the compilers to figure out efficient ways to do these, no? sunmy2019 commented on May 29, 2023 Note
if (new_refcnt & ~0xFFFFFFFF) return;
if ((PY_UINT32_T)new_refcnt != new_refcnt) return;
Both change the definition of immortality. Previously refcnt % 2^32 == -1, now refcnt >= 2^32 -1 or refcnt < -1 (impossible).
I don't mean this isn't desired; I just want to bring attention that something has changed here. zooba commented on May 29, 2023
Both change the definition of immortality. Previously refcnt % 2^32 == -1, now refcnt >= 2^32 -1 or refcnt < -1 (impossible).
You mean before you could (manually?) set a refcount to 0x00000001_00000000 and it wouldn't saturate, but now it will? I guess that's true, but also irrelevant. The definition of immortality isn't changed, just the condition under which Py_INCREF stops incrementing the refcount. And at least by my reading, that's going to apply to all objects, it's just that immortal objects start there and non-immortal ones will never reach it.
Incidentally, after a bit of playing with godbolt.org, it seems that gcc generates the same code for these two snippets (the first is the current code, and remember to add -O2 if you're trying to reproduce):
PY_UINT32_T cur_refcnt = op->ob_refcnt_split[PY_BIG_ENDIAN];
PY_UINT32_T new_refcnt = cur_refcnt + 1;
PY_UINT32_T new_refcnt = (PY_UINT32)(op->ob_refcnt + 1);
The latter doesn't require a union at all, and so doesn't depend on the size of void *. Simpler code all around. sunmy2019 commented on May 29, 2023
PY_UINT32_T new_refcnt = (PY_UINT32)(op->ob_refcnt + 1);
The latter doesn't require a union at all, and so doesn't depend on the size of void *. Simpler code all around.
Indeed! But it should be (PY_UINT32)(op->ob_refcnt) + 1;
Since there is indeed a compiler switch to control the behavior of signed int overflow. UB happens at op->ob_refcnt + 1.
But on the other side, unsigned int overflow is a well-defined behavior. So (PY_UINT32)(op->ob_refcnt) + 1; is good. zooba commented on May 29, 2023 We don't actually overflow in the op->ob_refcnt + 1 case (we've taken a different path if it's only 32 bits). So we get truncation at the cast, not an overflow.
Either way, I think we ought to get something else in for b2, as this can block people from testing. Marking as a release blocker, and @Yhg1s can decide. sunmy2019 commented on May 29, 2023
So we get truncation at the cast, not an overflow.
If so, (PY_UINT32)(op->ob_refcnt) + 1 should be equal to (PY_UINT32)(op->ob_refcnt + 1). And the former is a little bit safer without any possible UB. eduardo-elizondo commented on May 31, 2023 @zooba @sunmy2019 thanks for bringing up the issue! I think I might be able to get rid of the union and I'll try to get a patch in the next couple of days.
Separately, let's be careful about making any changes into the IncRef logic, the current code was very carefully crafted to maximize performance in windows, linux, and mac and was thoroughly benchmarked. There are also subtle side effects that can happen if we don't perform the operations correctly. For example, the suggested fix:
PY_UINT32_T new_refcnt = (PY_UINT32)(op->ob_refcnt + 1);
Would eventually require us to do something like:
op->ob_refcnt = new_refcnt;
^ this will cause an implicit conversion from 32 to 64 bit as the generated code assigns the 32 bit register to the 64 bit register by padding it with zeros. This causes incorrect behavior since we can have Extension code that has increased the reference count beyond the maximum value and this implicit conversion would cause the reference count of the extension to be lost. Then, it will result in a crash since we will decref this to deletion even when there are valid references to the object. I've seen this exact crash manifested when testing in a large application.
Anyways, give me a couple of days to explore a couple of options and I'll get back to this thread! In the meantime, if we can get some repro examples, that would be great! Reacted by Itamar Oren and sunmy2019 encukou commented on May 31, 2023
if we can get some repro examples, that would be great!
It would! Alas, compilers are weird and distutils is pining for fjords, so...
For anonymous structs/unions, Linux/GCC, with Python 3.13 installed (so that python3.13-config is in PATH):
$ cat repro.c
#include <Python.h>
int main () {
return 0;
}
$ gcc $(python3.13-config --cflags --ldflags) -lpython3.13$(python3.13-config --abiflags) -std=c99 -pedantic repro.c
In file included from .../include/python3.13d/Python.h:44,
from repro.c:1:
.../include/python3.13d/object.h:173:6: warning: ISO C99 doesn’t support unnamed structs/unions [-Wpedantic]
173 | };
| ^
Reacted by Eddie Elizondo zooba commented on May 31, 2023 Yeah, it looks like it's actually C compatibility, not C++. When I dug further into my failing build, it's actually one of the .c files.
Reproing using Petr's code from above with MSVC:
> cl /c /I(python3.12 -c "import sysconfig; print(sysconfig.get_config_var('INCLUDEPY'), end='')") /W4 .\t.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.37.32619.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
t.c
C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.177.0_x64__3847v3x7pw1km\Include\object.h(173): warning C4201: nonstandard extension used: nameless struct/union
C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.177.0_x64__3847v3x7pw1km\Include\cpython/unicodeobject.h(203): warning C4100: '_unused_op': unreferenced formal parameter
C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.177.0_x64__3847v3x7pw1km\Include\cpython/unicodeobject.h(393): warning C4100: '_unused_op': unreferenced formal parameter
C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.177.0_x64__3847v3x7pw1km\Include\cpython/pytime.h(192): warning C4115: 'timeval': named type definition in parentheses
All four warning disappear when you don't use /W4, which is our standard, but we shouldn't be warning even on high levels. The non-standard extension should definitely be avoided - I think the other two have been in there for a while.
I think we need a test that basically includes the headers and spits out warnings or errors. Shouldn't need to be anything more clever than a make target. That doesn't have to be part of this fix. zooba commented on May 31, 2023
Would eventually require us to do something like:
op->ob_refcnt = new_refcnt;
Then perhaps this, if "lower 32-bits set == saturated"?
Py_ssize_t new_refcnt = op->ob_refcnt + 1;
if ((PY_UINT32)new_refcnt == 0) { // might have to be & 0xFFFFFFFF instead of the cast
return;
}
op->ob_refcnt = new_refcnt;
vstinner commented on Jul 25, 2023 I wrote PR #105767 to implement Py_INCREF() and Py_DECREF() as opaque function call on C99 and older. I'm worried that it can have a significant negative impact on performance. So I abandon it, and instead I try to get rid of the compiler warning: PR #107232 (use GCC and clang __extension__). vstinner commented on Jul 25, 2023 I close the issue. Thanks everybody for your very useful feedback! I fixed the known compiler warnings (MSVC, GCC, clang).
Thanks @encukou for your reproducer, it was very useful on Linux and Windows.
C compilers try to implement latest standards, but they also have many extensions which may or may not be enabled by default. The GCC is famous for enabling many extensions by default: they are only disabled by -pedantic. It took many years until the Linux kernel could be built with LLVM clang, time for clang to implement some GCC extensions, and to update the Linux kernel to avoid some other extensions.
While anonymous union is now standard in C11, the feature existed in C compilers before it was standardized.
In a perfect world, we should respect strictly the C standard and maximize C backward compatibility of the Python C API, by trying to stay compatible with C89. Well, in practice we use static inline which was not standardized in C89, but in C99.
There is a simple fix to maximize backward compatibility: only expose opaque function void Py_INCREF(PyObject *op) in the C API, and then use whatever we want in the hidden implementation. That's exactly what I did in the limited C API version 3.12 and newer. But it has a significant negative impact on performance since Py_INCREF/Py_DECREF are called frequently in C extensions.
For Python 3.12, we are trying to implement immortal objects with the minimum performance overhead. The chosen implementation is to use an anonymous union on 64-bit platforms (if sizeof(void*) is larger than 4 bytes).
What I did is to fix the annoying side effect: compiler warnings when the most pedantic warning mode is enabled, gcc -Wall -Wextra -pedantic (GCC) and cl /W4 (MSC).
- For GCC and clang, I used
__extension__ to make the warning quiet: 6261585
- For MSVC, I used
__pragma to make the warning quiet: 1c8fe9b
These changes are being backported to Python 3.12.
Maybe there is a another efficient implementation of Py_INCREF/Py_DECREF which can be inlined and be compatible with C99 or event C89. But apparently, @eduardo-elizondo spent significant time on designing the current implementation, and he failed to find a better silver bullet.
Well, at least now we know how to check for pedantic compiler warnings :-) If someone has a new clever implementation idea, please open a new issue!
I fail to reproduce the issue with C++. Program in C++: (...)
I didn't get any warning with g++. If someone gets a new compiler warning with Python 3.12 Py_INCREF/Py_DECREF, please open a separated issue with details how to reproduce it. Reacted by Eric Snow, Itamar Oren and Erlend E. Aasland vstinner commented on Jul 25, 2023 By the way, I fixed another class of compiler warnings when building Python.h with MSVC cl /W4. I implemented Py_UNUSED() for MSVC in the main branch: PR #107250. It fix 2 warnings about unused arguments in unicodeobject.h.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|