From c70e93c182ae46122e6f97a001566d37a78c47c2 Mon Sep 17 00:00:00 2001 From: Diego Russo Date: Mon, 27 Nov 2023 17:32:37 +0000 Subject: [PATCH 1/3] gh-110190: Fix ctypes structs with array on Arm 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 +++++++++++++++++++++++- Modules/_ctypes/_ctypes_test.c | 36 +++++++ Modules/_ctypes/stgdict.c | 47 +++++---- 3 files changed, 186 insertions(+), 20 deletions(-) diff --git a/Lib/test/test_ctypes/test_structures.py b/Lib/test/test_ctypes/test_structures.py index f05ee5e491a41e..771205ffb32ecc 100644 --- a/Lib/test/test_ctypes/test_structures.py +++ b/Lib/test/test_ctypes/test_structures.py @@ -2,7 +2,7 @@ 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 Test3B(Test3A): ('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 Test3B(Test3A): 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/Modules/_ctypes/_ctypes_test.c b/Modules/_ctypes/_ctypes_test.c index d33e6fc7586d28..fc9fc131f6249a 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 6fbcf77a115371..07325c4e44b730 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -697,29 +697,38 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct stgdict->align = total_align; stgdict->length = len; /* ADD ffi_ofs? */ -#define MAX_STRUCT_SIZE 16 + +#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 +754,8 @@ 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. + * * 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 From 34f9c99acbdaa1dc38050ebf33ecd1e6d3102343 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 1 Dec 2023 18:05:10 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2023-12-01-18-05-09.gh-issue-110190.5bf-c9.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2023-12-01-18-05-09.gh-issue-110190.5bf-c9.rst 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 00000000000000..730b9d49119805 --- /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. From 4368b2fa978bf98d235a47441ef88c690b4b092d Mon Sep 17 00:00:00 2001 From: Diego Russo Date: Tue, 5 Dec 2023 10:21:55 +0000 Subject: [PATCH 3/3] Address comments in the PR --- Modules/_ctypes/stgdict.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index 07325c4e44b730..04dd9bae32cd5e 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -697,7 +697,12 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct stgdict->align = total_align; stgdict->length = len; /* ADD ffi_ofs? */ - +/* + * 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 @@ -754,7 +759,8 @@ 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. + * 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