8000 bpo-22273: Update ctypes to correctly handle arrays in small structur… · python/cpython@ce62dcc · GitHub
[go: up one dir, main page]
Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit ce62dcc

Browse files
miss-islingtonvsajip
miss-islington
authored and
vsajip
committed
bpo-22273: Update ctypes to correctly handle arrays in small structur… (GH-15839) (GH-16370)
(cherry picked from commit 12f209e)
1 parent 1a17a05 commit ce62dcc

File tree

3 files changed

+187
-0
lines changed

3 files changed

+187
-0
lines changed

‎Lib/ctypes/test/test_structures.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,47 @@ class X(Structure):
477477
self.assertEqual(s.first, got.first)
478478
self.assertEqual(s.second, got.second)
479479

480+
def test_array_in_struct(self):
481+
# See bpo-22273
482+
483+
# These should mirror the structures in Modules/_ctypes/_ctypes_test.c
484+
class Test2(Structure):
485+
_fields_ = [
486+
('data', c_ubyte * 16),
487+
]
488+
489+
class Test3(Structure):
490+
_fields_ = [
491+
('data', c_double * 2),
492+
]
493+
494+
s = Test2()
495+
expected = 0
496+
for i in range(16):
497+
s.data[i] = i
498+
expected += i
499+
dll = CDLL(_ctypes_test.__file__)
500+
func = dll._testfunc_array_in_struct1
501+
func.restype = c_int
502+
func.argtypes = (Test2,)
503+
result = func(s)
504+
self.assertEqual(result, expected)
505+
# check the passed-in struct hasn't changed
506+
for i in range(16):
507+
self.assertEqual(s.data[i], i)
508+
509+
s = Test3()
510+
s.data[0] = 3.14159
511+
s.data[1] = 2.71828
512+
expected = 3.14159 + 2.71828
513+
func = dll._testfunc_array_in_struct2
514+
func.restype = c_double
515+
func.argtypes = (Test3,)
516+
result = func(s)
517+
self.assertEqual(result, expected)
518+
# check the passed-in struct hasn't changed
519+
self.assertEqual(s.data[0], 3.14159)
520+
self.assertEqual(s.data[1], 2.71828)
480521

481522
class PointerMemberTestCase(unittest.TestCase):
482523

‎Modules/_ctypes/_ctypes_test.c

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,45 @@ _testfunc_reg_struct_update_value(TestReg in)
7474
((volatile TestReg *)&in)->second = 0x0badf00d;
7575
}
7676

77+
/*
78+
* See bpo-22273. Structs containing arrays should work on Linux 64-bit.
79+
*/
80+
81+
typedef struct {
82+
unsigned char data[16];
83+
} Test2;
84+
85+
EXPORT(int)
86+
_testfunc_array_in_struct1(Test2 in)
87+
{
88+
int result = 0;
89+
90+
for (unsigned i = 0; i < 16; i++)
91+
result += in.data[i];
92+
/* As the structure is passed by value, changes to it shouldn't be
93+
* reflected in the caller.
94+
*/
95+
memset(in.data, 0, sizeof(in.data));
96+
return result;
97+
}
98+
99+
typedef struct {
100+
double data[2];
101+
} Test3;
102+
103+
EXPORT(double)
104+
_testfunc_array_in_struct2(Test3 in)
105+
{
106+
double result = 0;
107+
108+
for (unsigned i = 0; i < 2; i++)
109+
result += in.data[i];
110+
/* As the structure is passed by value, changes to it shouldn't be
111+
* reflected in the caller.
112+
*/
113+
memset(in.data, 0, sizeof(in.data));
114+
return result;
115+
}
77116

78117
EXPORT(void)testfunc_array(int values[4])
79118
{

‎Modules/_ctypes/stgdict.c

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,9 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct
350350
int pack;
351351
Py_ssize_t ffi_ofs;
352352
int big_endian;
353+
#if defined(X86_64)
354+
int arrays_seen = 0;
355+
#endif
353356

354357
/* HACK Alert: I cannot be bothered to fix ctypes.com, so there has to
355358
be a way to use the old, broken semantics: _fields_ are not extended
@@ -501,6 +504,10 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct
501504
Py_XDECREF(pair);
502505
return -1;
503506
}
507+
#if defined(X86_64)
508+
if (PyCArrayTypeObject_Check(desc))
509+
arrays_seen = 1;
510+
#endif
504511
dict = PyType_stgdict(desc);
505512
if (dict == NULL) {
506513
Py_DECREF(pair);
@@ -641,6 +648,106 @@ PyCStructUnionType_update_stgdict(PyObject *type, PyObject *fields, int isStruct
641648
stgdict->align = total_align;
642649
stgdict->length = len; /* ADD ffi_ofs? */
643650

651+
#if defined(X86_64)
652+
653+
#define MAX_ELEMENTS 16
654+
655+
if (arrays_seen && (size <= 16)) {
656+
/*
657+
* See bpo-22273. Arrays are normally treated as pointers, which is
658+
* fine when an array name is being passed as parameter, but not when
659+
* passing structures by value that contain arrays. On 64-bit Linux,
660+
* small structures passed by value are passed in registers, and in
661+
* order to do this, libffi needs to know the true type of the array
662+
* members of structs. Treating them as pointers breaks things.
663+
*
664+
* By small structures, we mean ones that are 16 bytes or less. In that
665+
* case, there can't be more than 16 elements after unrolling arrays,
666+
* as we (will) disallow bitfields. So we can collect the true ffi_type
667+
* values in a fixed-size local array on the stack and, if any arrays
668+
* were seen, replace the ffi_type_pointer.elements with a more
669+
* accurate set, to allow libffi to marshal them into registers
670+
* correctly. It means one more loop over the fields, but if we got
671+
* here, the structure is small, so there aren't too many of those.
672+
*/
673+
ffi_type *actual_types[MAX_ELEMENTS + 1];
674+
int actual_type_index = 0;
675+
676+
memset(actual_types, 0, sizeof(actual_types));
677+
for (i = 0; i < len; ++i) {
678+
PyObject *name, *desc;
679+
PyObject *pair = PySequence_GetItem(fields, i);
680+
StgDictObject *dict;
681+
int bitsize = 0;
682+
683+
if (pair == NULL) {
684+
return -1;
685+
}
686+
if (!PyArg_ParseTuple(pair, "UO|i", &name, &desc, &bitsize)) {
687+
PyErr_SetString(PyExc_TypeError,
688+
"'_fields_' must be a sequence of (name, C type) pairs");
689+
Py_XDECREF(pair);
690+
return -1;
691+
}
692+
dict = PyType_stgdict(desc);
693+
if (dict == NULL) {
694+
Py_DECREF(pair);
695+
PyErr_Format(PyExc_TypeError,
696+
"second item in _fields_ tuple (index %zd) must be a C type",
697+
i);
698+
return -1;
699+
}
700+
if (!PyCArrayTypeObject_Check(desc)) {
701+
/* Not an array. Just copy over the element ffi_type. */
702+
actual_types[actual_type_index++] = &dict->ffi_type_pointer;
703+
assert(actual_type_index <= MAX_ELEMENTS);
704+
}
705+
else {
706+
int length = dict->length;
707+
StgDictObject *edict;
708+
709+
edict = PyType_stgdict(dict->proto);
710+
if (edict == NULL) {
711+
Py_DECREF(pair);
712+
PyErr_Format(PyExc_TypeError,
713+
"second item in _fields_ tuple (index %zd) must be a C type",
714+
i);
715+
return -1;
716+
}
717+
/* Copy over the element's type, length times. */
718+
while (length > 0) {
719+
actual_types[actual_type_index++] = &edict->ffi_type_pointer;
720+
assert(actual_type_index <= MAX_ELEMENTS);
721+
length--;
722+
}
723+
}
724+
Py_DECREF(pair);
725+
}
726+
727+
actual_types[actual_type_index++] = NULL;
728+
/*
729+
* Replace the old elements with the new, taking into account
730+
* base class elements where necessary.
731+
*/
732+
assert(stgdict->ffi_type_pointer.elements);
733+
PyMem_Free(stgdict->ffi_type_pointer.elements);
734+
stgdict->ffi_type_pointer.elements = PyMem_New(ffi_type *,
735+
ffi_ofs + actual_type_index);
736+
if (stgdict->ffi_type_pointer.elements == NULL) {
737+
PyErr_NoMemory();
738+
return -1;
739+
}
740+
if (ffi_ofs) {
741+
memcpy(stgdict->ffi_type_pointer.elements,
742+
basedict->ffi_type_pointer.elements,
743+
ffi_ofs * sizeof(ffi_type *));
744+
745+
}
746+
memcpy(&stgdict->ffi_type_pointer.elements[ffi_ofs], actual_types,
747+
actual_type_index * sizeof(ffi_type *));
748+
}
749+
#endif
750+
644751
/* We did check that this flag was NOT set above, it must not
645752
have been set until now. */
646753
if (stgdict->flags & DICTFLAG_FINAL) {

0 commit comments

Comments
 (0)
0