object.h uses an anonymous union in a struct (older C incompatible)

@eduardo-elizondo

An anonymous union was added recently, making this header produce warnings/errors for C++ compilers that have not enabled the relevant extensions/standards.

https://github.com/python/cpython/blame/1668b41dc477bc9562e4c50ab36a232839b4621b/Include/object.h#L168-L173

This code looks like it's making a few too many assumptions anyway. I suspect we ought to be wrapping up accesses to ob_refcnt_split in a macro and masking, rather than using a union. Or at least setting up the union to not assume that SIZEOF_VOID_P > 4 implies SIZEOF_VOID_P == 8.

(Ping @eduardo-elizondo)

Linked PRs

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?

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.

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.

    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.

@Yhg1s

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.

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.

@zooba

@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!

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 |     };
      |      ^

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.

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;

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

gh-105059: Use GCC/clang extension for PyObject union (#107232)

pythongh-105059: Use GCC/clang extension for PyObject union (pythonGH…

pythongh-105059: Fix MSCV compiler warning on PyObject union

[3.12] gh-105059: Use GCC/clang extension for PyObject union (GH-107232…

gh-105059: Fix MSCV compiler warning on PyObject union (#107239)

pythongh-105059: Fix MSCV compiler warning on PyObject union (pythonG…

@encukou

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.

[3.12] gh-105059: Fix MSCV compiler warning on PyObject union (GH-107239

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.

pythongh-105059: Use GCC/clang extension for PyObject union (python#1…

pythongh-105059: Fix MSCV compiler warning on PyObject union (python#…

Navigation Menu

Repository navigation

object.h uses an anonymous union in a struct (older C incompatible) #105059

Linked PRs

43 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

object.h uses an anonymous union in a struct (older C incompatible) #105059

Description

Linked PRs

Activity

zooba commented on May 29, 2023

sunmy2019 commented on May 29, 2023

zooba commented on May 29, 2023

sunmy2019 commented on May 29, 2023

zooba commented on May 29, 2023

sunmy2019 commented on May 29, 2023

eduardo-elizondo commented on May 31, 2023

encukou commented on May 31, 2023

zooba commented on May 31, 2023

zooba commented on May 31, 2023

43 remaining items

vstinner commented on Jul 25, 2023

vstinner commented on Jul 25, 2023

vstinner commented on Jul 25, 2023

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions