Skip to content

gh-127545: Replace _Py_ALIGN_AS(V) by _Py_ALIGNED_DEF(N, T) #135209

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

encukou
Copy link
Member

@encukou encukou commented Jun 6, 2025

_Py_ALIGNED_DEF(N, T) might be a better way to make the various _Alignas alternatives,
which behave in interesting ways, increase (rather than set) a variable's alignment.

The standard alignas/_Alignas error out if they would decrease the alignment. To prevent that, they can be used again with the defined variable's type.

The standard alignas/_Alignas can't be used on a struct definition, the workaround is to use it on one of the struct's members.

MSVC declspec(aligned) only takes integer literals.

MSVC declspec(aligned) had a bug when applied to a combined struct+member definition; a workaround is to separate the struct definition. This avoids a potentially ABI-breaking bug in older MSVC versions (which newer versions warn about, using code C5274).
This PR does that for PyASCIIObject.state.

fthain and others added 2 commits June 2, 2025 10:37
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.
This is a common façade for the various `_Alignas` alternatives,
which behave in interesting ways.

The standard `alignas` errors if it would decrease the alignment;
to prevent that it can be used again with the defined variable's
type.

The standard `alignas` can't be used on a `struct` definition,
the workaround is to use it on one of the `struct`'s members.

MSVC `declspec(aligned)` only takes integer literals.

MSVC `declspec(aligned)` had a bug when applied to a combined
struct+member definition.
The workaround is to separate the struct definition.
Do that for `PyASCIIObject.state`.
@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 6, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 512fe58 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135209%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 6, 2025
@encukou
Copy link
Member Author

encukou commented Jun 7, 2025

@fthain, would you be able to test this on a m68k?

[edit] Buildbot failures look unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants