BUG: fix crash on 32 bit systems using abi3t - #31771
Conversation
|
The lint problem is unrelated and fixed in main. |
| * object. | ||
| */ | ||
| PyTypeObject *typeobj; | ||
| NPY_DECL_ALIGNED(8) PyTypeObject *typeobj; |
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.
|
I've changed the code to only align the first field for 3.15+ as we discussed in community meeting. I tested it with https://github.com/kumaraditya303/np-test against 3.12 stable ABI extensions and 3.14t extensions and it passes on all of them.
|
Sorry, but this is winding me up: Can you explain what changed now? About it: I was OK with just adding an assert to defer the decision to the future poor sods if it ever happens (hopefully us still, but who knows). However, this is not the assert we have. FWIW, I am wondering if we can't add a bit more docs around the core problem we have to ensure: so that others don't have to think backwards from the bug condition. I am thinking there are likely cargo-culters and we spend enough time on this to leave more breadcrumbs for them/us (e.g. point out that for new objects From there we can explain all of our choices:
|
Sorry for the noise, that comment is not correct. I messed up the testing ignore that part.
Honestly given how difficult it is to get this right, I would prefer if for new objects, we do not expose it the way it is done here. For new objects you are correct and we should use max_align_t as you suggest but also I would suggest to not expose the struct at all in the ABI instead add functions to access the fields. We should also define a separate fields struct and object struct which embed the fields structs to be standard compliant, we couldn't do it for existing types because we did not want break extensions which access fields directly. |
I mean something like this for new types: struct NewNumpyStructFields {
... // Define the fields of the new struct here
};
struct NewNumpyStruct {
PyObject_HEAD
alignas(max_align_t) struct NewNumpyStructFields fields;
}; |
|
Review wise, there are still two things mentioned earlier that I am not sure we aligned on (maybe, maybe not); I may also be forgetting something, but hopefully nothing big:
But, I guess I'll just let you and Nathan mop that up unless you wonder e.g. if I think a struct should get the padding. |
|
Thanks @kumaraditya303! |
Co-authored-by: Sebastian Berg <sebastianb@nvidia.com> Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
BUG: fix crash on 32 bit systems using abi3t (#31771)
This MR contains the following updates: | Package | Type | Update | Change | OpenSSF | |---|---|---|---|---| | [numpy](https://github.com/numpy/numpy) ([changelog](https://numpy.org/doc/stable/release)) | dependencies | patch | `2.5.0` → `2.5.2` | [](https://securityscorecards.dev/viewer/?uri=github.com/numpy/numpy) | | [uvicorn](https://github.com/Kludex/uvicorn) ([changelog](https://uvicorn.dev/release-notes)) | dependencies | patch | `0.52.1` → `0.52.3` | [](https://securityscorecards.dev/viewer/?uri=github.com/Kludex/uvicorn) | --- ### Release Notes <details> <summary>numpy/numpy (numpy)</summary> ### [`v2.5.2`](https://github.com/numpy/numpy/releases/tag/v2.5.2): (Aug 9, 2026) [Compare Source](numpy/numpy@v2.5.1...v2.5.2) ### NumPy 2.5.2 Release Notes The NumPy 2.5.2 is a patch release that fixes bugs discovered after the 2.5.1 release. The big news is that it includes wheels for the newly released Python 3.15.0rc1. This release supports Python versions 3.12-3.15 #### C API changes ##### `PyArray_StringDTypeObject` is opaque under the abi3t stable ABI The `PyArray_StringDTypeObject` was accidentally exposed in NumPy 2.5 when targeting the free-threading-compatible stable ABI (`Py_TARGET_ABI3T`). `PyArray_StringDTypeObject` is now an opaque struct: extensions compiled that way cannot access its fields, since the struct layout depends on the size of the object header. Any code that accessed `PyArray_StringDTypeObject` fields in an abi3t build would have crashed, so we are making this API change in a bugfix release. The `NpyString` allocator API remains usable by passing the descriptor object pointer, e.g. `NpyString_acquire_allocator((PyArray_StringDTypeObject *)descr)`. ([gh-31771](numpy/numpy#31771)) #### Contributors A total of 16 people contributed to this release. People with a "+" by their names contributed a patch for the first time. - Abhijeetsingh Meena + - Charalampos Stratakis - Charles Harris - Chris Ninham + - David Woods - Geonho + - Gopu Yeshwanth Reddy + - Iason Krommydas - Ijtihed Kilani - Jelle Zijlstra + - Joren Hammudoglu - Kumar Aditya - Mike Boyle - Nathan Goldbaum - Raghuveer Devulapalli - Sebastian Berg #### Pull requests merged A total of 28 pull requests were merged for this release. - [#​31864](numpy/numpy#31864): MAINT: Prepare 2.5.x for further development - [#​31889](numpy/numpy#31889): TYP: Backport multiple static typing fixes 1. - [#​31900](numpy/numpy#31900): TST: add tests for stable ABI numpy extensions ([#​31822](numpy/numpy#31822)) - [#​31901](numpy/numpy#31901): BUG: fix `StringDType` coerce flag in binary ufunc promotion... - [#​31902](numpy/numpy#31902): BLD: fix meson deprecation warnings ([#​31892](numpy/numpy#31892)) - [#​31921](numpy/numpy#31921): TYP: Backport multiple typing fixes 2. - [#​31947](numpy/numpy#31947): MAINT: Update x86-simd-sort subproject ([`5adb334`](numpy/numpy@5adb334) → [`fa944ef`](numpy/numpy@fa944ef)) ([#​31908](numpy/numpy#31908)) - [#​31949](numpy/numpy#31949): BUG: fix crash on 32 bit systems using abi3t ([#​31771](numpy/numpy#31771)) - [#​31950](numpy/numpy#31950): MNT: remove some obsolete string to bool workarounds ([#​31859](numpy/numpy#31859)) - [#​31952](numpy/numpy#31952): BUG: centralized helper for output coerce and na\_object in stringdtype... - [#​31953](numpy/numpy#31953): BUG: fix CPU feature env diagnostic buffer overruns ([#​31905](numpy/numpy#31905)) - [#​31954](numpy/numpy#31954): BUG: restore ndarray.conjugate() for legacy user-defined dtypes... - [#​31955](numpy/numpy#31955): TYP: Avoid shadowed dtype annotations - [#​32077](numpy/numpy#32077): MAINT: Update verdored-meson/meson to match main. - [#​32114](numpy/numpy#32114): BUG: fix refcount leak on overlapping copyto with where=False - [#​32115](numpy/numpy#32115): BUG: fix swallowed cast error in fancy indexing assignment ([#​31975](numpy/numpy#31975)) - [#​32116](numpy/numpy#32116): BUG: Fix buffered iterator stride after removing multi-index - [#​32117](numpy/numpy#32117): BUG: fix `np.fromiter` corruption when reusing a `StringDType`... - [#​32119](numpy/numpy#32119): BUG: add a special case for StringDType in np.isdtype ([#​32030](numpy/numpy#32030)) - [#​32121](numpy/numpy#32121): BUG: reference leak in `simd_sequence_from_iterable` ([#​32038](numpy/numpy#32038)) - [#​32122](numpy/numpy#32122): BUG: ensure lock is held when accessing or writing to RNG state... - [#​32123](numpy/numpy#32123): BUG: fully reset cached RNG state for non-MT19937 RNGs ([#​32062](numpy/numpy#32062)) - [#​32135](numpy/numpy#32135): TYP: type capabilities max dimensions - [#​32158](numpy/numpy#32158): BUG: avoid possible stack overflow in arraydescr\_dealloc ([#​32133](numpy/numpy#32133)) - [#​32206](numpy/numpy#32206): MAINT: Update cibuildwheel to v4.2.0 - [#​32214](numpy/numpy#32214): MAINT: Skip limited\_api tests on some platforms. - [#​32220](numpy/numpy#32220): TYP: `isclose` shape-typing fix for 2d array-likes ([#​32205](numpy/numpy#32205)) - [#​32221](numpy/numpy#32221): BUG: avoid segfaults when legacy copyswap slot is not defined... ### [`v2.5.1`](https://github.com/numpy/numpy/releases/tag/v2.5.1): (July 4, 2026) [Compare Source](numpy/numpy@v2.5.0...v2.5.1) ### NumPy 2.5.1 Release Notes The NumPy 2.5.1 is a patch release that fixes bugs discovered after the 2.5.0 release. The most noticeable is the fix is to the numpy datetime cython API which should allow downstream to support NumPy versions older than 2.5. Preparation for Python 3.15 continues along with typing improvements. This release supports Python versions 3.12-3.14 #### Changes - The minimum supported GCC version has been updated from 9.3.0 to 10.3.0 ([gh-31843](numpy/numpy#31843)) #### Contributors A total of 10 people contributed to this release. People with a "+" by their names contributed a patch for the first time. - Adhyan Gupta + - Ankit Ahlawat - Charles Harris - Iason Krommydas - Joren Hammudoglu - Kumar Aditya - Nathan Goldbaum - Sebastian Berg - Ties Jan Hefting + - Vineet Kumar #### Pull requests merged A total of 20 pull requests were merged for this release. - [#​31707](numpy/numpy#31707): MAINT: Prepare 2.5.x for further development - [#​31721](numpy/numpy#31721): CI: fix new `cython-lint` errors ([#​31711](numpy/numpy#31711)) - [#​31723](numpy/numpy#31723): MAINT: Update meson to match main - [#​31729](numpy/numpy#31729): TST: use setup-sde instead of curl to get SDE binaries ([#​31727](numpy/numpy#31727)) - [#​31829](numpy/numpy#31829): BUG: Relax finfo to be easier accessible for all user dtypes... - [#​31831](numpy/numpy#31831): TYP: Fix `flatiter.__next__` return type for `object_` and... - [#​31832](numpy/numpy#31832): BUG: avoid deadlocks using NpyString API ([#​31682](numpy/numpy#31682)) - [#​31833](numpy/numpy#31833): BUG: fix out array leak in reduceat and accumulate when dtype... - [#​31835](numpy/numpy#31835): BUG: fix numpy datetime cython APIs to be compatible with older... - [#​31836](numpy/numpy#31836): TYP: Fix incorrect dtype inference of `asarray([])` ([#​31732](numpy/numpy#31732)) - [#​31837](numpy/numpy#31837): TYP: Fix `np.ma.masked_array` 2.5.0 regression - [#​31838](numpy/numpy#31838): FIX: Refactor error handling in array\_setstate to prevent typecode... - [#​31839](numpy/numpy#31839): TST: xfail multithreaded BLAS test more generously - [#​31840](numpy/numpy#31840): MAINT: Rename subroutine for crackfortran tests - [#​31842](numpy/numpy#31842): BUG: fix leak in reductions when a ufunc override errors or is... - [#​31849](numpy/numpy#31849): BLD: set minimum required gcc version to 10.3 ([#​31843](numpy/numpy#31843)) - [#​31855](numpy/numpy#31855): CI: fix hangs on MacOS ASan CI ([#​31853](numpy/numpy#31853)) - [#​31856](numpy/numpy#31856): BUG: fix several bugs in StringDType operations ([#​31846](numpy/numpy#31846)) - [#​31857](numpy/numpy#31857): BUG: Fix segfault in MT19937 by preventing recursive seed lists... - [#​31858](numpy/numpy#31858): BUG: Fix signed integer overflow in datetime.c ([#​31688](numpy/numpy#31688)) </details> <details> <summary>Kludex/uvicorn (uvicorn)</summary> ### [`v0.52.3`](https://github.com/Kludex/uvicorn/releases/tag/0.52.3): Version 0.52.3 [Compare Source](Kludex/uvicorn@0.52.2...0.52.3) ##### Changed - Update `zttp` to 0.0.24 and use its combined receive path, improving HTTP/1.1 request parsing performance ([#​3067](Kludex/uvicorn#3067)) **Full Changelog**: <Kludex/uvicorn@0.52.2...0.52.3> ### [`v0.52.2`](https://github.com/Kludex/uvicorn/releases/tag/0.52.2): Version 0.52.2 [Compare Source](Kludex/uvicorn@0.52.1...0.52.2) ##### Fixed - Update `zttp` to 0.0.22, fixing bodyless request receives and improving HTTP/1 request parsing performance ([#​3063](Kludex/uvicorn#3063)) **Full Changelog**: <Kludex/uvicorn@0.52.1...0.52.2> </details> --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4yNTEuMyIsInVwZGF0ZWRJblZlciI6IjQ0LjMwLjQiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbImRlcGVuZGVuY2llcyIsInJlbm92YXRlIl19--> See merge request swiss-armed-forces/cyber-command/cea/loom!639 Co-authored-by: Loom MR Pipeline Trigger <group_103951964_bot_9504bb8dead6d4e406ad817a607f24be@noreply.gitlab.com>
PR summary
Fixes #31762
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.