-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-127545: Specify minimum PyGC_Head alignment to fix build failure #127546
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a blurb entry. Could you add one using one of the links the bot gave?
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 works when python happens to run on a platform that has a large minimum alignment, but fails otherwise. 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 sure those 2 bits are available. This patch fixes a segfault in _bootstrap_python. It also clarifies the code by making the actual requirement explicit. This bug was 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to force push--everything is squashed at the end anyway.
Misc/NEWS.d/next/Build/2024-12-04-10-00-35.gh-issue-127545.t0THjE.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provisionally, this LGTM. (I say "provisionally" because I don't totally understand the issue--there's just nothing wrong with this PR from a triage standpoint.)
…han a plain number. Move the definition accordingly.
I think the patch can made more understandable, by making use of the existing macro definitions. I will update the branch. |
#127545