object.h uses an anonymous union in a struct (older C incompatible) #105059
Comments
|
Seems the only use of it is later in object.h: Is this really better than this code?: Seems we can rely on the compilers to figure out efficient ways to do these, no? |
|
Note Both change the definition of immortality. Previously I don't mean this isn't desired; I just want to bring attention that something has changed here. |
You mean before you could (manually?) set a refcount to 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 The latter doesn't require a union at all, and so doesn't depend on the size of |
Indeed! But it should be Since there is indeed a compiler switch to control the behavior of signed int overflow. UB happens at But on the other side, unsigned int overflow is a well-defined behavior. So |
|
We don't actually overflow in the 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. |
If so, |
|
@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: Would eventually require us to do something like: ^ 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! |
It would! Alas, compilers are weird and For anonymous structs/unions, Linux/GCC, with Python 3.13 installed (so that |
|
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 Reproing using Petr's code from above with MSVC: All four warning disappear when you don't use 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. |
Then perhaps this, if "lower 32-bits set == saturated"? |
|
Some updates, I just removed the union in GH-105275. I will follow-up with a couple of other different options and benchmark numbers. Will let you all know once it's ready to be reviewed. |
|
Per https://peps.python.org/pep-0007/#c-dialect "Python 3.11 and later use C11 without optional features". Anonymous Structs and Unions are part of C11. |
gpshead
changed the title
@gpshead so does that mean that PEP007 guarantees that it's ok to have anonymous unions in Python 3.11? Hence making the original issue invalid now? |
Also @zooba to close the loop, I had tested this in the past but I just did it again to get some fresh numbers here. Looking at the generated code, it is not leveraging the saturated add which removes an extra instruction: https://godbolt.org/z/5rrdjPbMM. But generated code is just a proxy so I benchmarked it on a Linux machine with GCC-11.1 and it recorded a: Geometric mean: 1.01x slower. I'm testing it with the memset as well which looks neutral (since it generates almost the same code). Ultimately, I'm indifferent what structure we end up using, so I'm open to use the option you suggested if there's consensus on the regression. However, perf was a core part of the original upstreaming, so I went for the most perf efficient option. But let me know what you think! Benchmark ResultsAll benchmarks: ===============async_tree_eager: Mean +- std dev: [benchmark_cpython_baseline] 115 ms +- 1 ms -> [benchmark_cpython_no_union_v2] 116 ms +- 1 ms: 1.01x slower Benchmark hidden because not significant (27): async_generators, async_tree_none, async_tree_cpu_io_mixed, async_tree_eager_cpu_io_mixed, async_tree_eager_io, async_tree_io, async_tree_memoization, chameleon, comprehensions, bench_thread_ Geometric mean: 1.01x slower |
That is my interpretation. This seems to tie in with another discussion on https://discuss.python.org/t/requiring-compilers-c11-standard-mode-to-build-cpython/26481. Regardless of the primary issue about the union being modern, I do think the point about |
|
Is there a reason the union has to be anonymous? Why can't we just give it a name? |
That would mean all uses of Anonymous unions are a great tool for keeping API compatibility around changes like these. I wish we could use them :) |
|
Exactly as Petr said, it's just to keep the API compatibility so that we can still access |
|
okay, that's what I assumed given the public header file. |
While building Python requires a C11 compiler, we are trying to keep the Python C API compatible with ISO C89 whenever possible. I would prefer to upgrade to C99/C11, but some programs explicitly requires ISO C89 with warnings on C99 variable declarations in the middle of functions (pedantic mode). I don't know the rationale for treating these warnings as errors, but so far, it was cheap to keep ISO C89 compatibility. |
While it's not officially deprecated, I would strongly suggest to use Py_REFCNT() and Py_SET_REFCNT() functions instead. See issue #83754. |
|
I wrote PR #105767 to fix the C API for C99 and older:
Well, no longer inlining code on C99 and older can have an impact on performance which should be measured. For me, the first step is to fix the regression, and then design the most efficient implementation depending on the C or C++ standard, compiler flags, etc. |
I think something like |
I'm pretty sure that it can be avoided when building C extensions with GCC and clang which support |
It approximates the least common denominator of compilers we don't use in CI. |
test_cppext checks for C++ compiler warnings and treats them as errors. Not only to include Python.h, but also use the most common APIs. |
If it's not the case yet, we should have tests on these corner cases so they are no longer "undefined" behavior, but "tested" behavior 😁 |
Well, I would strongly suggest C extensions to move away accessing directly PyObject members and use Py_REFCNT() and Py_SET_REFCNT() functions instead. It would be a Python 3.12 incompatible change that we should also consider. |
|
FWIW, I'm using |
|
If you use |
|
Well, no. I was specifically referring to writing my extensions. I believe it has been highlighted in this issue that C11 should not be required for those. |
Anonymous union is new in C11. To prevent compiler warning, use Clang and GCC extension on C99 and older.
Anonymous union is new in C11. To prevent compiler warning when using -pedantic compiler option, use Clang and GCC extension on C99 and older.
Use pragram to ignore the MSCV compiler warning on the PyObject nameless union.
Use pragma to ignore the MSCV compiler warning on the PyObject nameless union.
…) (#107236) gh-105059: Use GCC/clang extension for PyObject union (GH-107232) Anonymous union is new in C11. To prevent compiler warning when using -pedantic compiler option, use Clang and GCC extension on C99 and older. (cherry picked from commit 6261585) Co-authored-by: Victor Stinner <vstinner@python.org>
Use pragma to ignore the MSCV compiler warning on the PyObject nameless union.
|
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 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 There is a simple fix to maximize backward compatibility: only expose opaque function 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 What I did is to fix the annoying side effect: compiler warnings when the most pedantic warning mode is enabled,
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 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. |
|
By the way, I fixed another class of compiler warnings when building |
…07232) Anonymous union is new in C11. To prevent compiler warning when using -pedantic compiler option, use Clang and GCC extension on C99 and older.
…107239) Use pragma to ignore the MSCV compiler warning on the PyObject nameless union.
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_splitin a macro and masking, rather than using a union. Or at least setting up the union to not assume thatSIZEOF_VOID_P > 4impliesSIZEOF_VOID_P == 8.(Ping @eduardo-elizondo)
Linked PRs
The text was updated successfully, but these errors were encountered: