BUG: fix crash on 32 bit systems using abi3t#31771
Conversation
There was a problem hiding this comment.
This is a neat idea! Inspired I had a bit of a brainstorming search:
How about we embed the "public" fields here as an anonymous struct? I feel like that may be even cleaner (it doesn't encode the actual alignment guarantees yet, see below).
That is C11 but major compilers support it earlier, that might be somewhat annoying (I hope not, but...). However, NPY_DECL_ALIGNED is also not defined in a fully compatible way, currently (i.e. also would need fixing).
With an anonymous struct, but also in general, I think it might be good to do this for all our structs maybe. And it would be nice to add static asserts that would tell us in case we ever break our assumptions.
(I think a static_assert(alignof(object_struct) <= 8/16) would do the trick. It is a bit more strict than needed, but that could be addressed if it ever breaks...)
There was a problem hiding this comment.
How about we embed the "public" fields here as an anonymous struct? I feel like that may be even cleaner (it doesn't encode the actual alignment guarantees yet, see below).
That is C11 but major compilers support it earlier, that might be somewhat annoying (I hope not, but...).
I think we have considered anonymous struct before but avoided that because it is incompatible with C++ and requires C11. See #31091 (comment) where it was discussed previously.
However, NPY_DECL_ALIGNED is also not defined in a fully compatible way, currently (i.e. also would need fixing).
Yeah, I'll fix it, thanks!
There was a problem hiding this comment.
I've added the asserts
There was a problem hiding this comment.
Thanks, I forgot about that and we do need C++ of course.
Three points, mostly because I felt they are worth noting/considering once:
_Alignasis also C11, so either we require C11 for downstream for compilers other than gcc/clang/msvc, or we'd have to omit it and stop compiling for free-thread and opaque builds.
I am not exactly sure how unreasonable C11 + fun compiler is?!- Reducing alignment is a one way road for public fields (i.e. the asserts I asked for). We aren't reducing it on 32bit systems but are doing so on 64bit systems.
I don't know that there is a realistic need for 16 byte aligned types public types but if we do this we can only add them by telling the compiler to not only align them to 8 bytes. - As this is an ABI choice, it also (potentially) affects structs that are public but not exposed on opaque builds.
I have not checked, I suspect this is only the scalars and I think that is fine. So this may mostly be about diffusing knowledge: Adding this alignment is vital for all new types we might want to expose on the opaque API (sure for many it does nothing but that isn't obvious).
EDIT: I think we can make such structs public actually, but it is limited to future Python versions (i.e. right now we can for 3.15+ which is all there is anyway for opaque builds).
There was a problem hiding this comment.
_Alignas is also C11, so either we require C11 for downstream for compilers other than gcc/clang/msvc, or we'd have to omit it and stop compiling for free-thread and opaque builds.
I am not exactly sure how unreasonable C11 + fun compiler is?!
I think it is reasonable to use C11 _Alignas because CPython itself require it for defining PyObject layout. https://github.com/python/cpython/blob/2670cb062c9ec31cd6df7be645f929a8398601c7/Include/pymacro.h#L65-L86
Reducing alignment is a one way road for public fields (i.e. the asserts I asked for). We aren't reducing it on 32bit systems but are doing so on 64bit systems.
I don't think we are actually reducing it on 64 bits, as noted in the CPython implementation, alignment can only be increased and the macros becomes no-op when it will reduce the alignment.
There was a problem hiding this comment.
I think it is reasonable to use C11 _Alignas because CPython itself require it for defining PyObject layout.
An nice, shouldn't we match CPython and add that #ifndef to allow compilers that don't do this right? (Not that I expect this is actually used.)
I don't think we are actually reducing it on 64 bits, as noted in the CPython implementation, alignment can only be increased and the macros becomes no-op when it will reduce the alignment.
This assumes that Python promises that on 64bit systems the size of PyObject_HEAD will always remain a multiple of 16. Unless that is a promise that Python is willing to make officially, I don't think we should rely on it.
(Sure, we could just add a static_assert in our code with a comment "if this fails we have to think about ABI" in practice.)
Sorry if I seem pedantic, but I feel we need to be very clear about what ABI decisions we are making here that we cannot change easily.
EDIT: I'll note that I think there are one or two more issues to think about. But I need a pause from quick iteration and probably Nathan will find them as well anyway.
There was a problem hiding this comment.
Sure, we could just add a static_assert in our code with a comment "if this fails we have to think about ABI" in practice.
IMO this is the most practical thing to do. There is overlap between the CPython team and the NumPy team (me and Kumar) and IMO we would be able to handle that static assert failing if it ever does come up in the future.

PR summary
Fixes #31762
Fixes #31764
This PR fixes the crash on 32 bit systems when using abi3t ABI. In this PR, the first member of the
PyArrayDescris now aligned to 8 bytes so that regardless of the size of the actualPyObject, the field accesses remain correct. 8 bytes is used instead ofalignof(max_align_t)to preserve backwards compatibility with exisiting abi3 extensions wheresizeof(PyObject)is a multiple of 8 thereby the added alignment is no-op and does not changes the offset for such extensions.AI Disclosure
AI was used to create the test.