From 7d36030042b5075720b6f3d09599ce674d3b2059 Mon Sep 17 00:00:00 2001 From: Diego Russo Date: Wed, 13 Dec 2023 16:08:15 +0000 Subject: [PATCH] [3.11] gh-110190: Fix ctypes structs with array on PPCLE64 (GH-112959) Fix the same issue of PR #112604 on PPC64LE platform Refactor tests to make easier to add more platfroms if needed. (cherry picked from commit 6644ca45cde9ca1b80513a90dacccfeea2d98620) Change-Id: I1ada30808c0d593a43eca3fa7a628c26bc276310 --- Lib/ctypes/test/test_structures.py | 232 +++++++----------- ...-12-11-14-12-46.gh-issue-110190.e0iEUa.rst | 1 + Modules/_ctypes/_ctypes_test.c | 138 ++++++++--- Modules/_ctypes/stgdict.c | 45 ++-- 4 files changed, 209 insertions(+), 207 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2023-12-11-14-12-46.gh-issue-110190.e0iEUa.rst diff --git a/Lib/ctypes/test/test_structures.py b/Lib/ctypes/test/test_structures.py index f57bb2b5c178c0..369e326b49c72b 100644 --- a/Lib/ctypes/test/test_structures.py +++ b/Lib/ctypes/test/test_structures.py @@ -9,6 +9,7 @@ c_long, c_ulong, c_longlong, c_ulonglong, c_float, c_double) from struct import calcsize import _ctypes_test +from collections import namedtuple from test import support # The following definition is meant to be used from time to time to assist @@ -482,36 +483,53 @@ class X(Structure): def test_array_in_struct(self): # See bpo-22273 + # Load the shared library + dll = CDLL(_ctypes_test.__file__) + # These should mirror the structures in Modules/_ctypes/_ctypes_test.c class Test2(Structure): _fields_ = [ ('data', c_ubyte * 16), ] - class Test3(Structure): + class Test3AParent(Structure): + _fields_ = [ + ('data', c_float * 2), + ] + + class Test3A(Test3AParent): + _fields_ = [ + ('more_data', c_float * 2), + ] + + class Test3B(Structure): _fields_ = [ ('data', c_double * 2), ] - class Test3A(Structure): + class Test3C(Structure): _fields_ = [ - ('data', c_float * 2), + ("data", c_double * 4) ] - class Test3B(Test3A): + class Test3D(Structure): _fields_ = [ - ('more_data', c_float * 2), + ("data", c_double * 8) + ] + + class Test3E(Structure): + _fields_ = [ + ("data", c_double * 9) ] - # Load the shared library - dll = CDLL(_ctypes_test.__file__) + # Tests for struct Test2 s = Test2() expected = 0 for i in range(16): s.data[i] = i expected += i - func = dll._testfunc_array_in_struct1 + func = dll._testfunc_array_in_struct2 func.restype = c_int func.argtypes = (Test2,) result = func(s) @@ -520,29 +538,16 @@ class Test3B(Test3A): for i in range(16): self.assertEqual(s.data[i], i) - s = Test3() - s.data[0] = 3.14159 - s.data[1] = 2.71828 - expected = 3.14159 + 2.71828 - func = dll._testfunc_array_in_struct2 - func.restype = c_double - func.argtypes = (Test3,) - result = func(s) - self.assertEqual(result, expected) - # check the passed-in struct hasn't changed - self.assertEqual(s.data[0], 3.14159) - self.assertEqual(s.data[1], 2.71828) - - s = Test3B() + # Tests for struct Test3A + s = Test3A() s.data[0] = 3.14159 s.data[1] = 2.71828 s.more_data[0] = -3.0 s.more_data[1] = -2.0 - - expected = 3.14159 + 2.71828 - 5.0 - func = dll._testfunc_array_in_struct2a + expected = 3.14159 + 2.71828 - 3.0 - 2.0 + func = dll._testfunc_array_in_struct3A func.restype = c_double - func.argtypes = (Test3B,) + func.argtypes = (Test3A,) result = func(s) self.assertAlmostEqual(result, expected, places=6) # check the passed-in struct hasn't changed @@ -551,129 +556,60 @@ class Test3B(Test3A): self.assertAlmostEqual(s.more_data[0], -3.0, places=6) self.assertAlmostEqual(s.more_data[1], -2.0, places=6) - @unittest.skipIf( - 'ppc64le' in platform.uname().machine, - "gh-110190: currently fails on ppc64le", - ) - def test_array_in_struct_registers(self): - dll = CDLL(_ctypes_test.__file__) - - 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) - ] - - # 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 + # Test3B, Test3C, Test3D, Test3E have the same logic with different + # sizes hence putting them in a loop. + StructCtype = namedtuple( + "StructCtype", + ["cls", "cfunc1", "cfunc2", "items"] ) - - 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) + structs_to_test = [ + StructCtype( + Test3B, + dll._testfunc_array_in_struct3B, + dll._testfunc_array_in_struct3B_set_defaults, + 2), + StructCtype( + Test3C, + dll._testfunc_array_in_struct3C, + dll._testfunc_array_in_struct3C_set_defaults, + 4), + StructCtype( + Test3D, + dll._testfunc_array_in_struct3D, + dll._testfunc_array_in_struct3D_set_defaults, + 8), + StructCtype( + Test3E, + dll._testfunc_array_in_struct3E, + dll._testfunc_array_in_struct3E_set_defaults, + 9), + ] + + for sut in structs_to_test: + s = sut.cls() + + # Test for cfunc1 + expected = 0 + for i in range(sut.items): + float_i = float(i) + s.data[i] = float_i + expected += float_i + func = sut.cfunc1 + func.restype = c_double + func.argtypes = (sut.cls,) + result = func(s) + self.assertEqual(result, expected) + # check the passed-in struct hasn't changed + for i in range(sut.items): + self.assertEqual(s.data[i], float(i)) + + # Test for cfunc2 + func = sut.cfunc2 + func.restype = sut.cls + result = func() + # check if the default values have been set correctly + for i in range(sut.items): + self.assertEqual(result.data[i], float(i+1)) def test_38368(self): class U(Union): diff --git a/Misc/NEWS.d/next/Library/2023-12-11-14-12-46.gh-issue-110190.e0iEUa.rst b/Misc/NEWS.d/next/Library/2023-12-11-14-12-46.gh-issue-110190.e0iEUa.rst new file mode 100644 index 00000000000000..3bfed1e0f1dc91 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-12-11-14-12-46.gh-issue-110190.e0iEUa.rst @@ -0,0 +1 @@ +Fix ctypes structs with array on PPC64LE platform by setting ``MAX_STRUCT_SIZE`` to 64 in stgdict. Patch by Diego Russo. diff --git a/Modules/_ctypes/_ctypes_test.c b/Modules/_ctypes/_ctypes_test.c index 5f9ba0c42aa5ef..f5fe258e188aab 100644 --- a/Modules/_ctypes/_ctypes_test.c +++ b/Modules/_ctypes/_ctypes_test.c @@ -81,11 +81,10 @@ typedef struct { } Test2; EXPORT(int) -_testfunc_array_in_struct1(Test2 in) +_testfunc_array_in_struct2(Test2 in) { int result = 0; - - for (unsigned i = 0; i < 16; i++) + for (unsigned i = 0; i < sizeof(in.data)/sizeof(in.data[0]); i++) result += in.data[i]; /* As the structure is passed by value, changes to it shouldn't be * reflected in the caller. @@ -94,22 +93,25 @@ _testfunc_array_in_struct1(Test2 in) return result; } -typedef struct { - double data[2]; -} Test3; - +/* + * Test3A struct test the MAX_STRUCT_SIZE 16 with single precision floats. + * These structs should be passed via registers on all platforms and they are + * used for within bounds tests. + */ typedef struct { float data[2]; float more_data[2]; -} Test3B; +} Test3A; EXPORT(double) -_testfunc_array_in_struct2(Test3 in) +_testfunc_array_in_struct3A(Test3A in) { double result = 0; - for (unsigned i = 0; i < 2; i++) + for (unsigned i = 0; i < sizeof(in.data)/sizeof(in.data[0]); i++) result += in.data[i]; + for (unsigned i = 0; i < sizeof(in.more_data)/sizeof(in.more_data[0]); i++) + result += in.more_data[i]; /* As the structure is passed by value, changes to it shouldn't be * reflected in the caller. */ @@ -117,56 +119,116 @@ _testfunc_array_in_struct2(Test3 in) return result; } +/* The structs Test3B..Test3E use the same functions hence using the MACRO + * to define their implementation. + */ + +#define _TESTFUNC_ARRAY_IN_STRUCT_IMPL \ + double result = 0; \ + \ + for (unsigned i = 0; i < sizeof(in.data)/sizeof(in.data[0]); i++) \ + result += in.data[i]; \ + /* As the structure is passed by value, changes to it shouldn't be \ + * reflected in the caller. \ + */ \ + memset(in.data, 0, sizeof(in.data)); \ + return result; \ + +#define _TESTFUNC_ARRAY_IN_STRUCT_SET_DEFAULTS_IMPL \ + for (unsigned i = 0; i < sizeof(s.data)/sizeof(s.data[0]); i++) \ + s.data[i] = (double)i+1; \ + return s; \ + + +/* + * Test3B struct test the MAX_STRUCT_SIZE 16 with double precision floats. + * These structs should be passed via registers on all platforms and they are + * used for within bounds tests. + */ +typedef struct { + double data[2]; +} Test3B; + EXPORT(double) -_testfunc_array_in_struct2a(Test3B in) +_testfunc_array_in_struct3B(Test3B in) { - double result = 0; + _TESTFUNC_ARRAY_IN_STRUCT_IMPL +} - for (unsigned i = 0; i < 2; i++) - result += in.data[i]; - for (unsigned i = 0; i < 2; i++) - result += in.more_data[i]; - /* As the structure is passed by value, changes to it shouldn't be - * reflected in the caller. - */ - memset(in.data, 0, sizeof(in.data)); - return result; +EXPORT(Test3B) +_testfunc_array_in_struct3B_set_defaults(void) +{ + Test3B s; + _TESTFUNC_ARRAY_IN_STRUCT_SET_DEFAULTS_IMPL } /* - * See gh-110190. structs containing arrays of up to four floating point types - * (max 32 bytes) are passed in registers on Arm. + * Test3C struct tests the MAX_STRUCT_SIZE 32. Structs containing arrays of up + * to four floating point types are passed in registers on Arm platforms. + * This struct is used for within bounds test on Arm platfroms and for an + * out-of-bounds tests for platfroms where MAX_STRUCT_SIZE is less than 32. + * See gh-110190. */ - typedef struct { double data[4]; } Test3C; +EXPORT(double) +_testfunc_array_in_struct3C(Test3C in) +{ + _TESTFUNC_ARRAY_IN_STRUCT_IMPL +} + EXPORT(Test3C) -_testfunc_array_in_struct_set_defaults_3C(void) +_testfunc_array_in_struct3C_set_defaults(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; + _TESTFUNC_ARRAY_IN_STRUCT_SET_DEFAULTS_IMPL } +/* + * Test3D struct tests the MAX_STRUCT_SIZE 64. Structs containing arrays of up + * to eight floating point types are passed in registers on PPC64LE platforms. + * This struct is used for within bounds test on PPC64LE platfroms and for an + * out-of-bounds tests for platfroms where MAX_STRUCT_SIZE is less than 64. + * See gh-110190. + */ typedef struct { - double data[5]; + double data[8]; } Test3D; +EXPORT(double) +_testfunc_array_in_struct3D(Test3D in) +{ + _TESTFUNC_ARRAY_IN_STRUCT_IMPL +} + EXPORT(Test3D) -_testfunc_array_in_struct_set_defaults_3D(void) +_testfunc_array_in_struct3D_set_defaults(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; + _TESTFUNC_ARRAY_IN_STRUCT_SET_DEFAULTS_IMPL +} + +/* + * Test3E struct tests the out-of-bounds for all architectures. + * See gh-110190. + */ +typedef struct { + double data[9]; +} Test3E; + +EXPORT(double) +_testfunc_array_in_struct3E(Test3E in) +{ + _TESTFUNC_ARRAY_IN_STRUCT_IMPL +} + +EXPORT(Test3E) +_testfunc_array_in_struct3E_set_defaults(void) +{ + Test3E s; + _TESTFUNC_ARRAY_IN_STRUCT_SET_DEFAULTS_IMPL } typedef union { diff --git a/Modules/_ctypes/stgdict.c b/Modules/_ctypes/stgdict.c index 7746f95692f346..ca774157311cd2 100644 --- a/Modules/_ctypes/stgdict.c +++ b/Modules/_ctypes/stgdict.c @@ -663,13 +663,12 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct 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. -*/ + * The value of MAX_STRUCT_SIZE depends on the platform Python is running on. + */ #if defined(__aarch64__) || defined(__arm__) # define MAX_STRUCT_SIZE 32 +#elif defined(__powerpc64__) +# define MAX_STRUCT_SIZE 64 #else # define MAX_STRUCT_SIZE 16 #endif @@ -680,24 +679,29 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct * 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. + * 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. + * + * Small structures have different sizes depending on the platform + * where Python is running on: + * + * * x86-64: 16 bytes or less + * * Arm platforms (both 32 and 64 bit): 32 bytes or less + * * PowerPC 64 Little Endian: 64 bytes or less * - * 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. + * In that case, there can't be more than 16, 32 or 64 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 x86_64 Linux - * and Arm platforms, the array-in-struct vs. pointer problem is - * general. But we restrict the type transformation to small structs + * Although the passing in registers is specific to the above + * 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 @@ -724,8 +728,7 @@ 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. + * The same principle applies for a struct 32 or 64 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