BUG: fix crash on 32 bit systems using abi3t by kumaraditya303 · Pull Request #31771 · numpy/numpy · GitHub
Skip to content

BUG: fix crash on 32 bit systems using abi3t#31771

Open
kumaraditya303 wants to merge 12 commits into
numpy:mainfrom
kumaraditya303:32bit-crash
Open

BUG: fix crash on 32 bit systems using abi3t#31771
kumaraditya303 wants to merge 12 commits into
numpy:mainfrom
kumaraditya303:32bit-crash

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

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 PyArrayDescr is now aligned to 8 bytes so that regardless of the size of the actual PyObject, the field accesses remain correct. 8 bytes is used instead of alignof(max_align_t) to preserve backwards compatibility with exisiting abi3 extensions where sizeof(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.

@charris charris added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported labels Jun 27, 2026
@charris

charris commented Jun 27, 2026

Copy link
Copy Markdown
Member

@seberg seberg added the 63 - C API Changes or additions to the C API. Mailing list should usually be notified. label Jun 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the asserts

@seberg seberg Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I forgot about that and we do need C++ of course.

Three points, mostly because I felt they are worth noting/considering once:

  1. _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?!
  2. 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.
  3. 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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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.

@seberg seberg Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kumaraditya303 kumaraditya303 marked this pull request as ready for review June 28, 2026 11:45
@kumaraditya303 kumaraditya303 requested a review from seberg June 28, 2026 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

00 - Bug 09 - Backport-Candidate PRs tagged should be backported 63 - C API Changes or additions to the C API. Mailing list should usually be notified.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dealing with opaque API, 32bit builds and Fields Getting BUG: descriptor accessors should account for struct padding on 32bit abi3t builds

4 participants