]> git.ipfire.org Git - thirdparty/Python/cpython.git/commitdiff
gh-110190: Fix ctypes structs with array on PPCLE64 (GH-112959)
authorDiego Russo <diego.russo@arm.com>
Wed, 13 Dec 2023 16:08:15 +0000 (16:08 +0000)
committerGitHub <noreply@github.com>
Wed, 13 Dec 2023 16:08:15 +0000 (17:08 +0100)
Fix the same issue of PR #112604 on PPC64LE platform
Refactor tests to make easier to add more platfroms if needed.

Lib/test/test_ctypes/test_structures.py
Misc/NEWS.d/next/Library/2023-12-11-14-12-46.gh-issue-110190.e0iEUa.rst [new file with mode: 0644]
Modules/_ctypes/_ctypes_test.c
Modules/_ctypes/stgdict.c

index 57ae9240b3c165bae1866ad71af2ba97ed06e3b3..21039f049475077d9acbeef4fe044b855d6a91e1 100644 (file)
@@ -9,6 +9,7 @@ from ctypes import (CDLL, Array, Structure, Union, POINTER, sizeof, byref, align
                     c_short, c_ushort, c_int, c_uint,
                     c_long, c_ulong, c_longlong, c_ulonglong, c_float, c_double)
 from struct import calcsize
+from collections import namedtuple
 from test import support
 
 
@@ -474,36 +475,53 @@ class StructureTestCase(unittest.TestCase):
     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)
@@ -512,29 +530,16 @@ class StructureTestCase(unittest.TestCase):
         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
@@ -543,129 +548,60 @@ class StructureTestCase(unittest.TestCase):
         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 (file)
index 0000000..3bfed1e
--- /dev/null
@@ -0,0 +1 @@
+Fix ctypes structs with array on PPC64LE platform by setting ``MAX_STRUCT_SIZE`` to 64 in stgdict. Patch by Diego Russo.
index 2681b9c58ecb9da74a2a832fc207e4d06dd90d73..ecc60417790417ced0db8275b5aeff1f51f8596b 100644 (file)
@@ -96,11 +96,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.
@@ -109,22 +108,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.
      */
@@ -132,56 +134,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 {
index 04dd9bae32cd5ee1f168debc250c90013322156b..dfdb96b0e7258a79b1ce69d639127b5a1ca0f5a2 100644 (file)
@@ -698,13 +698,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
@@ -715,24 +714,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
@@ -759,8 +763,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