gh-127545: Specify minimum PyGC_Head and PyObject alignment to fix build failure #135016
Conversation
As documented in InternalDocs/garbage_collector.md, the garbage collector stores flags in the least significant two bits of the _gc_prev pointer in struct PyGC_Head. Consequently, this pointer is only capable of storing a location that's aligned to a 4-byte boundary. This alignment requirement is documented but it's not actually encoded. The code only works when python happens to run on a platform that has a sufficiently large minimum alignment for the structs in question. The same problem arises with PyObject pointers because the least significant bits get used for PyStackRef tags. Since we know that 2 bits are needed, we also know the minimum alignment that's needed. Let's make that explicit, so the compiler can then make those bits available. This patch fixes a segfault in _bootstrap_python. In 3.14.0 beta 2 this fixes the "Assertion `!PyStackRef_IsTaggedInt(ref)' failed" when built with --config-pydebug. Also, making the requirements explicit improves clarity. This bug was previously investigated by Adrian Glaubitz here: https://lists.debian.org/debian-68k/2024/11/msg00020.html https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1087600 Although Adrian's patch isn't really correct (because natural alignment is not needed), he deserves full credit for finding the root cause.
|
@ZeroIntensity, please go ahead and add labels when convenient. |
|
🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 392f6c5 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135016%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Ugh, I forgot that we are now using two bits in _PyStackRef...
I am still concerned about adding the the alignment attributes to Include/object.h.
|
@colesbury you mentioned that you still have concerns about the If your concern is that this change might be insufficient, because the codebase makes further unstated alignment assumptions, I think you may be right, since |
| uintptr_t _gc_prev; | ||
| } PyGC_Head; | ||
| } PyGC_Head Py_ALIGNED(1 << _PyObject_ALIGNMENT_SHIFT); |
There was a problem hiding this comment.
Note that Py_ALIGNED is only for GCC. I think we want the _Py_ALIGN_AS macro, but that seems to only be on the free-threaded build for some reason. See capi-workgroup/decisions#61.
As documented in
InternalDocs/garbage_collector.md, the garbage collector stores flags in the least significant two bits of the_gc_prevpointer instruct PyGC_Head. Consequently, this pointer is only capable of storing a location that's aligned to a 4-byte boundary.This alignment requirement is documented but it's not actually encoded. The code only works when python happens to run on a platform that has a sufficiently large minimum alignment for the structs in question.
The same problem arises with
PyObjectpointers because the least significant bits get used forPyStackReftags.Since we know that 2 bits are needed, we also know the minimum alignment that's needed. Let's make that explicit, so the compiler can then make those bits available.
This patch fixes a segfault in
_bootstrap_python. In 3.14.0 beta 2 this fixes the error, "Assertion!PyStackRef_IsTaggedInt(ref)' failed" when building with--config-pydebug`.Also, making the requirements explicit improves clarity.
This bug was previously investigated by Adrian Glaubitz here:
https://lists.debian.org/debian-68k/2024/11/msg00020.html
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1087600
Although Adrian's patch isn't really correct (because natural alignment is not needed), he deserves full credit for finding the root cause.
Please see PR#127546 for the previous pull request.