From bc68f4a4abcfbea60bb1db1ccadb07613561931c Mon Sep 17 00:00:00 2001 From: Diego Russo Date: Tue, 5 Dec 2023 15:07:50 +0000 Subject: [PATCH] gh-110190: Fix ctypes structs with array on Arm (#112604) Set MAX_STRUCT_SIZE to 32 in stgdict.c when on Arm platforms. This because on Arm platforms structs with at most 4 elements of any floating point type values can be passed through registers. If the type is double the maximum size of the struct is 32 bytes. On x86-64 Linux, it's maximum 16 bytes hence we need to differentiate. --- Lib/test/test_ctypes/test_structures.py | 123 +++++++++++++++++- ...-12-01-18-05-09.gh-issue-110190.5bf-c9.rst | 1 + Modules/_ctypes/_ctypes_test.c | 36 +++++ Modules/_ctypes/stgdict.c | 53 +++++--- 4 files changed, 193 insertions(+), 20 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-12-01-18-05-09.gh-issue-110190.5bf-c9.rst diff --git a/Lib/test/test_ctypes/test_structures.py b/Lib/test/test_ctypes/test_structures.py index f05ee5e491a..771205ffb32 100644 --- a/Lib/test/test_ctypes/test_structures.py +++ b/Lib/test/test_ctypes/test_structures.py @@ -2,7 +2,7 @@ import _ctypes_test import struct import sys import unittest -from ctypes import (CDLL, Structure, Union, POINTER, sizeof, byref, alignment, +from ctypes import (CDLL, Array, Structure, Union, POINTER, sizeof, byref, alignment, c_void_p, c_char, c_wchar, c_byte, c_ubyte, c_uint8, c_uint16, c_uint32, c_short, c_ushort, c_int, c_uint, @@ -494,12 +494,59 @@ class StructureTestCase(unittest.TestCase): ('more_data', c_float * 2), ] + class Test3C1(Structure): + _fields_ = [ + ("data", c_double * 4) + ] + + class DataType4(Array): + _type_ = c_double + _length_ = 4 + + class Test3C2(Structure): + _fields_ = [ + ("data", DataType4) + ] + + class Test3C3(Structure): + _fields_ = [ + ("x", c_double), + ("y", c_double), + ("z", c_double), + ("t", c_double) + ] + + class Test3D1(Structure): + _fields_ = [ + ("data", c_double * 5) + ] + + class DataType5(Array): + _type_ = c_double + _length_ = 5 + + class Test3D2(Structure): + _fields_ = [ + ("data", DataType5) + ] + + class Test3D3(Structure): + _fields_ = [ + ("x", c_double), + ("y", c_double), + ("z", c_double), + ("t", c_double), + ("u", c_double) + ] + + # Load the shared library + dll = CDLL(_ctypes_test.__file__) + s = Test2() expected = 0 for i in range(16): s.data[i] = i expected += i - dll = CDLL(_ctypes_test.__file__) func = dll._testfunc_array_in_struct1 func.restype = c_int func.argtypes = (Test2,) @@ -540,6 +587,78 @@ class StructureTestCase(unittest.TestCase): self.assertAlmostEqual(s.more_data[0], -3.0, places=6) self.assertAlmostEqual(s.more_data[1], -2.0, places=6) + # Tests for struct Test3C + expected = (1.0, 2.0, 3.0, 4.0) + func = dll._testfunc_array_in_struct_set_defaults_3C + func.restype = Test3C1 + result = func() + # check the default values have been set properly + self.assertEqual( + (result.data[0], + result.data[1], + result.data[2], + result.data[3]), + expected + ) + + func = dll._testfunc_array_in_struct_set_defaults_3C + func.restype = Test3C2 + result = func() + # check the default values have been set properly + self.assertEqual( + (result.data[0], + result.data[1], + result.data[2], + result.data[3]), + expected + ) + + func = dll._testfunc_array_in_struct_set_defaults_3C + func.restype = Test3C3 + result = func() + # check the default values have been set properly + self.assertEqual((result.x, result.y, result.z, result.t), expected) + + # Tests for struct Test3D + expected = (1.0, 2.0, 3.0, 4.0, 5.0) + func = dll._testfunc_array_in_struct_set_defaults_3D + func.restype = Test3D1 + result = func() + # check the default values have been set properly + self.assertEqual( + (result.data[0], + result.data[1], + result.data[2], + result.data[3], + result.data[4]), + expected + ) + + func = dll._testfunc_array_in_struct_set_defaults_3D + func.restype = Test3D2 + result = func() + # check the default values have been set properly + self.assertEqual( + (result.data[0], + result.data[1], + result.data[2], + result.data[3], + result.data[4]), + expected + ) + + func = dll._testfunc_array_in_struct_set_defaults_3D + func.restype = Test3D3 + result = func() + # check the default values have been set properly + self.assertEqual( + (result.x, + result.y, + result.z, + result.t, + result.u), + expected) + def test_38368(self): class U(Union): _fields_ = [ diff --git a/Misc/NEWS.d/next/Library/2023-12-01-18-05-09.gh-issue-110190.5bf-c9.rst b/Misc/NEWS.d/next/Library/2023-12-01-18-05-09.gh-issue-110190.5bf-c9.rst new file mode 100644 index 00000000000..730b9d49119 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-12-01-18-05-09.gh-issue-110190.5bf-c9.rst @@ -0,0 +1 @@ +Fix ctypes structs with array on Arm platform by setting ``MAX_STRUCT_SIZE`` to 32 in stgdict. Patch by Diego Russo. diff --git a/Modules/_ctypes/_ctypes_test.c b/Modules/_ctypes/_ctypes_test.c index d33e6fc7586..fc9fc131f62 100644 --- a/Modules/_ctypes/_ctypes_test.c +++ b/Modules/_ctypes/_ctypes_test.c @@ -150,6 +150,42 @@ _testfunc_array_in_struct2a(Test3B in) return result; } +/* + * See gh-110190. structs containing arrays of up to four floating point types + * (max 32 bytes) are passed in registers on Arm. + */ + +typedef struct { + double data[4]; +} Test3C; + +EXPORT(Test3C) +_testfunc_array_in_struct_set_defaults_3C(void) +{ + Test3C s; + s.data[0] = 1.0; + s.data[1] = 2.0; + s.data[2] = 3.0; + s.data[3] = 4.0; + return s; +} + +typedef struct { + double data[5]; +} Test3D; + +EXPORT(Test3D) +_testfunc_array_in_struct_set_defaults_3D(void) +{ + Test3D s; + s.data[0] = 1.0; + s.data[1] = 2.0; + s.data[2] = 3.0; + s.data[3] = 4.0; + s.data[4] = 5.0; + return s; +} + typedef union { long a_long; struct { diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index 6fbcf77a115..04dd9bae32c 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -697,29 +697,43 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct stgdict->align = total_align; stgdict->length = len; /* ADD ffi_ofs? */ -#define MAX_STRUCT_SIZE 16 +/* + * On Arm platforms, structs with at most 4 elements of any floating point + * type values can be passed through registers. If the type is double the + * maximum size of the struct is 32 bytes. + * By Arm platforms it is meant both 32 and 64-bit. +*/ +#if defined(__aarch64__) || defined(__arm__) +# define MAX_STRUCT_SIZE 32 +#else +# define MAX_STRUCT_SIZE 16 +#endif if (arrays_seen && (size <= MAX_STRUCT_SIZE)) { /* - * See bpo-22273. Arrays are normally treated as pointers, which is - * fine when an array name is being passed as parameter, but not when - * passing structures by value that contain arrays. On 64-bit Linux, - * small structures passed by value are passed in registers, and in - * order to do this, libffi needs to know the true type of the array - * members of structs. Treating them as pointers breaks things. + * See bpo-22273 and gh-110190. Arrays are normally treated as + * pointers, which is fine when an array name is being passed as + * parameter, but not when passing structures by value that contain + * arrays. + * On x86_64 Linux and Arm platforms, small structures passed by + * value are passed in registers, and in order to do this, libffi needs + * to know the true type of the array members of structs. Treating them + * as pointers breaks things. * - * By small structures, we mean ones that are 16 bytes or less. In that - * case, there can't be more than 16 elements after unrolling arrays, - * as we (will) disallow bitfields. So we can collect the true ffi_type - * values in a fixed-size local array on the stack and, if any arrays - * were seen, replace the ffi_type_pointer.elements with a more - * accurate set, to allow libffi to marshal them into registers - * correctly. It means one more loop over the fields, but if we got - * here, the structure is small, so there aren't too many of those. + * By small structures, we mean ones that are 16 bytes or less on + * x86-64 and 32 bytes or less on Arm. In that case, there can't be + * more than 16 or 32 elements after unrolling arrays, as we (will) + * disallow bitfields. So we can collect the true ffi_type values in + * a fixed-size local array on the stack and, if any arrays were seen, + * replace the ffi_type_pointer.elements with a more accurate set, + * to allow libffi to marshal them into registers correctly. + * It means one more loop over the fields, but if we got here, + * the structure is small, so there aren't too many of those. * - * Although the passing in registers is specific to 64-bit Linux, the - * array-in-struct vs. pointer problem is general. But we restrict the - * type transformation to small structs nonetheless. + * Although the passing in registers is specific to x86_64 Linux + * and Arm platforms, the array-in-struct vs. pointer problem is + * general. But we restrict the type transformation to small structs + * nonetheless. * * Note that although a union may be small in terms of memory usage, it * could contain many overlapping declarations of arrays, e.g. @@ -745,6 +759,9 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct * struct { uint_32 e1; uint_32 e2; ... uint_32 e_4; } f6; * } * + * The same principle applies for a struct 32 bytes in size like in + * the case of Arm platforms. + * * So the struct/union needs setting up as follows: all non-array * elements copied across as is, and all array elements replaced with * an equivalent struct which has as many fields as the array has