Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-110190: Fix ctypes structs with array on Arm #112604

Merged
merged 6 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 121 additions & 2 deletions Lib/test/test_ctypes/test_structures.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,)
Expand Down Expand Up @@ -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_ = [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix ctypes structs with array on Arm platform by setting ``MAX_STRUCT_SIZE`` to 32 in stgdict. Patch by Diego Russo.
36 changes: 36 additions & 0 deletions Modules/_ctypes/_ctypes_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
53 changes: 35 additions & 18 deletions Modules/_ctypes/stgdict.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down