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-97588: Fix ctypes structs #97702

Merged
merged 128 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from 60 commits
Commits
Show all changes
128 commits
Select commit Hold shift + click to select a range
7922585
gh-97588: Fix ctypes structs
matthiasgoergens Oct 1, 2022
2307932
📜🤖 Added by blurb_it.
blurb-it[bot] Oct 1, 2022
47f826c
Handling pack as well
matthiasgoergens Oct 1, 2022
5a32211
Handle basedict, too
matthiasgoergens Oct 1, 2022
8d79d8f
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 2, 2022
2d07375
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 3, 2022
8bc8535
Merge remote-tracking branch 'matthias/fix-bitfield-clean' into fix-b…
matthiasgoergens Oct 5, 2022
c3d162b
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Oct 5, 2022
e5ed9ac
Split windows and linux
matthiasgoergens Oct 5, 2022
b0f9819
Compiles
matthiasgoergens Oct 5, 2022
2dee0e3
Abstract out proto stuff
matthiasgoergens Oct 5, 2022
79ef347
Clean align
matthiasgoergens Oct 5, 2022
6170dad
Hypothesis tests pass
matthiasgoergens Oct 5, 2022
871ca1a
Formatting
matthiasgoergens Oct 5, 2022
c162144
Clean up
matthiasgoergens Oct 5, 2022
a57cf2c
Adapt tests for Windows
matthiasgoergens Oct 5, 2022
3f7c4cc
Fix order
matthiasgoergens Oct 5, 2022
e39a271
Fix alignment test
matthiasgoergens Oct 5, 2022
359ed58
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Oct 5, 2022
52ef8d2
Avoid casting
matthiasgoergens Oct 5, 2022
e8102c4
Fixup
matthiasgoergens Oct 5, 2022
600e144
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 5, 2022
9bad706
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 5, 2022
5121857
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Oct 6, 2022
6cd27ea
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Oct 8, 2022
6b6fa8a
Add ability to force msvc compatibility even when not doing any packing
matthiasgoergens Oct 8, 2022
ca9d580
More tests
matthiasgoergens Oct 8, 2022
1401ee4
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 9, 2022
235fa68
Big endian works
matthiasgoergens Oct 9, 2022
a534e18
Merge remote-tracking branch 'matthias/fix-bitfield-clean' into fix-b…
matthiasgoergens Oct 9, 2022
53db061
More tests
matthiasgoergens Oct 9, 2022
9c249c3
More asserts
matthiasgoergens Oct 9, 2022
3cf4747
Drop unneeded parameters
matthiasgoergens Oct 9, 2022
8beddb9
Drop unneeded parameters
matthiasgoergens Oct 9, 2022
9b64ff6
Drop unneeded parameters
matthiasgoergens Oct 9, 2022
4d0dab5
Clean up
matthiasgoergens Oct 9, 2022
bf98667
Explain
matthiasgoergens Oct 9, 2022
8223063
More cleanup
matthiasgoergens Oct 9, 2022
e80a09a
More cleanup
matthiasgoergens Oct 9, 2022
33a10fb
More cleanup
matthiasgoergens Oct 9, 2022
0f3c5a3
Move common assert
matthiasgoergens Oct 9, 2022
47eca3c
Break line
matthiasgoergens Oct 9, 2022
456afd0
Clean TODO
matthiasgoergens Oct 9, 2022
bc799ac
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 9, 2022
212cf13
Fix test for Windows
matthiasgoergens Oct 9, 2022
e7e5ae9
Merge remote-tracking branch 'matthias/fix-bitfield-clean' into fix-b…
matthiasgoergens Oct 9, 2022
45e26ec
Remove warning
matthiasgoergens Oct 9, 2022
ab42187
Port test from PR-19850
matthiasgoergens Oct 9, 2022
640c062
Fix assert
matthiasgoergens Oct 9, 2022
f3e04af
Revert "Port test from PR-19850"
matthiasgoergens Oct 9, 2022
bff34a1
Support gcc's packed structs
matthiasgoergens Oct 9, 2022
5d6f0f7
Remove gcc_packed experiment
matthiasgoergens Oct 9, 2022
d9c1fca
Fix test
matthiasgoergens Oct 10, 2022
cc9ea8a
Fix ms_struct
matthiasgoergens Oct 10, 2022
1554083
Fix ms_struct
matthiasgoergens Oct 10, 2022
a86eadc
Indenting
matthiasgoergens Oct 10, 2022
eefa1ad
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Oct 10, 2022
28c7fe7
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 10, 2022
f0a92b0
Fix miscounted parens
matthiasgoergens Oct 10, 2022
a3a390b
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Oct 10, 2022
e997b5f
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Mar 14, 2023
5a7ea09
make regen-all
matthiasgoergens Mar 14, 2023
0f73919
clean up
matthiasgoergens Mar 14, 2023
cdfc3c6
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Mar 15, 2023
6226e55
make regen-all
matthiasgoergens Mar 15, 2023
4d48eba
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens Mar 16, 2023
20b5582
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Mar 17, 2023
eb502d7
Explain where 999 comes from
matthiasgoergens Mar 17, 2023
0cf5f4e
fix typo
matthiasgoergens Mar 17, 2023
3457558
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Mar 17, 2023
fd3cd0b
Explain magic 999
matthiasgoergens Mar 17, 2023
2d8492e
Incorporate Sam's tests
matthiasgoergens Mar 27, 2023
a9f7f14
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens May 24, 2023
bee6c53
Tickle CI/CD
matthiasgoergens May 24, 2023
220e19e
Merge branch 'main' into fix-bitfield-clean
matthiasgoergens May 24, 2023
60cebe2
Merge branch 'main' into fix-bitfield-clean
gpshead May 24, 2023
3424a7d
Merge commit '698a0da7d440856a90b45964e9082b5a55387b80' into fix-bitf…
matthiasgoergens Apr 14, 2024
2506eea
Merge commit '0841ca7932987f30a2a23d39f3e6e141622d6fea' into fix-bitf…
matthiasgoergens Apr 14, 2024
65654b4
Merge commit '9f7176d360b5898003d5ca78bab1822ad67867c4' into fix-bitf…
matthiasgoergens Apr 14, 2024
98767da
Fix compile
matthiasgoergens Apr 14, 2024
3ca703f
Merge commit '298bcdc185d1a9709271e61a4cc529d33483add4' into fix-bitf…
matthiasgoergens Apr 14, 2024
c40ef7d
Merge commit 'dcaf33a41d5d220523d71c9b35bc08f5b8405dac' into fix-bitf…
matthiasgoergens Apr 14, 2024
d840f01
Merge commit '7e2fef865899837c47e91ef0180fa59eb03e840b' into fix-bitf…
matthiasgoergens Apr 14, 2024
8b9f0eb
Merge commit 'ea94b3b149eeadf33c2f7c46f16dcda0adc7cf4e' into fix-bitf…
matthiasgoergens Apr 14, 2024
4e8c220
Merge remote-tracking branch 'origin/main' into fix-bitfield-clean
matthiasgoergens Apr 14, 2024
0369d0d
Fix merge
matthiasgoergens Apr 14, 2024
d4dc2c0
Clean up
matthiasgoergens Apr 14, 2024
f75d7d6
Fix tests
matthiasgoergens Apr 14, 2024
cdc5cdc
Merge in the main branch
encukou Apr 22, 2024
0da36ad
Add Hypothesis test
encukou Apr 22, 2024
b6f7117
Merge in the main branch
encukou Apr 29, 2024
5e47d5f
dict -> info
encukou Apr 30, 2024
de22b39
Conditionalize ms_struct
encukou Apr 30, 2024
cdd1860
Use fixed-width types in tests
encukou Apr 30, 2024
dd84ac3
Use subTest
encukou Apr 30, 2024
8a6fb67
fixup! dict
encukou Apr 30, 2024
07ea42d
Turn `info->size != info->align` assertion into a TypeError
encukou Apr 30, 2024
3fa7f55
PEP 7
encukou Apr 30, 2024
eb2c4fb
Change _ms_struct_ to _layout_
encukou Apr 30, 2024
22cd86c
Merge in the main branch
encukou Apr 30, 2024
637b961
PEP 7
encukou Apr 30, 2024
5309b7e
TMP
encukou Apr 30, 2024
489c5c8
Add generated tests
encukou May 2, 2024
3594473
Merge in the main branch
encukou May 2, 2024
f780496
Remove ideas
encukou May 2, 2024
6ade022
Regen
encukou May 2, 2024
cfa6647
Do not set _layout_
encukou May 2, 2024
df162b0
We don't always get union alignment right
encukou May 2, 2024
836a5cc
Remove unused stuff
encukou May 2, 2024
b07ae33
Packing implies ms layout
encukou May 2, 2024
6dd7a8d
Docs & whitespace
encukou May 2, 2024
0cf1049
Use limited API again
encukou May 2, 2024
bc91549
Appease the linter
encukou May 3, 2024
70bbc26
Fix ReST typo
encukou May 3, 2024
6e23b3d
Use nicer error output for memory dumps
encukou May 3, 2024
1b90841
Regen global strings
encukou May 3, 2024
2c4874b
Allow alignment < size, as in int64_t on x86 (32-bit), GCC layout
encukou May 3, 2024
738323f
Add examples from MS docs
encukou May 4, 2024
af7487c
Use correct spelling for test skips
encukou May 5, 2024
5353352
Only use 'attribute((ms_struct))' on x86_64 & ppc; on GCC and clang
encukou May 5, 2024
fe76d45
Remove the `info->size == info->align` assertion for _layout_='ms'
encukou May 5, 2024
c4714f1
Fix silly mistake in the macro
encukou May 5, 2024
c79725d
Regen
encukou May 5, 2024
cace0c9
Include field name in error message
encukou May 5, 2024
61e92bf
Conditionally skip tests that assume sizeof(int64) == alignment(int64)
encukou May 5, 2024
ba61051
Use ms_struct only on Windows and x86/ppc GCC/clang
encukou May 5, 2024
8991444
Use proper PyArg_ParseTuple code for Py_ssize_t
encukou May 6, 2024
bc1225b
Merge in the main branch
encukou May 6, 2024
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
197 changes: 194 additions & 3 deletions Lib/test/test_ctypes/test_bitfields.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from test import support
import unittest
import os
import sys

import _ctypes_test

Expand All @@ -28,6 +29,30 @@ class BITS(Structure):
func = CDLL(_ctypes_test.__file__).unpack_bitfields
func.argtypes = POINTER(BITS), c_char


class BITS_msvc(Structure):
_ms_struct_ = 1
_fields_ = [("A", c_int, 1),
("B", c_int, 2),
("C", c_int, 3),
("D", c_int, 4),
("E", c_int, 5),
("F", c_int, 6),
("G", c_int, 7),
("H", c_int, 8),
("I", c_int, 9),

("M", c_short, 1),
("N", c_short, 2),
("O", c_short, 3),
("P", c_short, 4),
("Q", c_short, 5),
("R", c_short, 6),
("S", c_short, 7)]

func_msvc = CDLL(_ctypes_test.__file__).unpack_bitfields_msvc
func_msvc.argtypes = POINTER(BITS_msvc), c_char

##for n in "ABCDEFGHIMNOPQRS":
## print n, hex(getattr(BITS, n).size), getattr(BITS, n).offset

Expand All @@ -40,8 +65,6 @@ def test_ints(self):
setattr(b, name, i)
self.assertEqual(getattr(b, name), func(byref(b), name.encode('ascii')))

# bpo-46913: _ctypes/cfield.c h_get() has an undefined behavior
@support.skip_if_sanitizer(ub=True)
def test_shorts(self):
b = BITS()
name = "M"
Expand All @@ -51,7 +74,24 @@ def test_shorts(self):
for name in "MNOPQRS":
b = BITS()
setattr(b, name, i)
self.assertEqual(getattr(b, name), func(byref(b), name.encode('ascii')))
self.assertEqual(
getattr(b, name),
func(byref(b), (name.encode('ascii'))),
(name, i))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider updating tests like this that loop over byte values into sub-tests via the https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests API call so that you don't need to pass msg=(name, i) here to get a meaningful error message.


def test_shorts_msvc_mode(self):
b = BITS_msvc()
name = "M"
if func_msvc(byref(b), name.encode('ascii')) == 999:
matthiasgoergens marked this conversation as resolved.
Show resolved Hide resolved
self.skipTest("Compiler does not support signed short bitfields")
for i in range(256):
for name in "MNOPQRS":
b = BITS_msvc()
setattr(b, name, i)
self.assertEqual(
getattr(b, name),
func_msvc(byref(b), name.encode('ascii')),
(name, i))

signed_int_types = (c_byte, c_short, c_int, c_long, c_longlong)
unsigned_int_types = (c_ubyte, c_ushort, c_uint, c_ulong, c_ulonglong)
Expand Down Expand Up @@ -236,6 +276,157 @@ class X(Structure):
else:
self.assertEqual(sizeof(X), sizeof(c_int) * 2)

def test_mixed_5(self):
class X(Structure):
_fields_ = [
('A', c_uint, 1),
('B', c_ushort, 16)]
a = X()
a.A = 0
a.B = 1
self.assertEqual(1, a.B)

def test_mixed_6(self):
class X(Structure):
_fields_ = [
('A', c_ulonglong, 1),
('B', c_uint, 32)]
a = X()
a.A = 0
a.B = 1
self.assertEqual(1, a.B)

def test_mixed_7(self):
class X(Structure):
_fields_ = [
("A", c_uint),
('B', c_uint, 20),
('C', c_ulonglong, 24)]
self.assertEqual(16, sizeof(X))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes specific sizeof(c_uint) & sizeof(c_ulonglong).


def test_mixed_8(self):
class Foo(Structure):
_fields_ = [
("A", c_uint),
("B", c_uint, 32),
("C", c_ulonglong, 1),
]

class Bar(Structure):
_fields_ = [
("A", c_uint),
("B", c_uint),
("C", c_ulonglong, 1),
]
self.assertEqual(sizeof(Foo), sizeof(Bar))

def test_mixed_9(self):
class X(Structure):
_fields_ = [
("A", c_uint8),
("B", c_uint, 1),
]
if sys.platform == 'win32':
self.assertEqual(8, sizeof(X))
else:
self.assertEqual(4, sizeof(X))

def test_mixed_10(self):
class X(Structure):
_fields_ = [
("A", c_uint32, 1),
("B", c_uint64, 1),
]
if sys.platform == 'win32':
self.assertEqual(8, alignment(X))
self.assertEqual(16, sizeof(X))
else:
self.assertEqual(8, alignment(X))
self.assertEqual(8, sizeof(X))

def test_gh_95496(self):
for field_width in range(1, 33):
class TestStruct(Structure):
_fields_ = [
("Field1", c_uint32, field_width),
("Field2", c_uint8, 8)
]

cmd = TestStruct()
cmd.Field2 = 1
self.assertEqual(1, cmd.Field2)

def test_gh_84039(self):
class Bad(Structure):
_pack_ = 1
_fields_ = [
("a0", c_uint8, 1),
("a1", c_uint8, 1),
("a2", c_uint8, 1),
("a3", c_uint8, 1),
("a4", c_uint8, 1),
("a5", c_uint8, 1),
("a6", c_uint8, 1),
("a7", c_uint8, 1),
("b0", c_uint16, 4),
("b1", c_uint16, 12),
]


class GoodA(Structure):
_pack_ = 1
_fields_ = [
("a0", c_uint8, 1),
("a1", c_uint8, 1),
("a2", c_uint8, 1),
("a3", c_uint8, 1),
("a4", c_uint8, 1),
("a5", c_uint8, 1),
("a6", c_uint8, 1),
("a7", c_uint8, 1),
]


class Good(Structure):
_pack_ = 1
_fields_ = [
("a", GoodA),
("b0", c_uint16, 4),
("b1", c_uint16, 12),
]

self.assertEqual(3, sizeof(Bad))
self.assertEqual(3, sizeof(Good))

def test_gh_73939(self):
class MyStructure(Structure):
_pack_ = 1
_fields_ = [
("P", c_uint16),
("L", c_uint16, 9),
("Pro", c_uint16, 1),
("G", c_uint16, 1),
("IB", c_uint16, 1),
("IR", c_uint16, 1),
("R", c_uint16, 3),
("T", c_uint32, 10),
("C", c_uint32, 20),
("R2", c_uint32, 2)
]
self.assertEqual(8, sizeof(MyStructure))

def test_gh_86098(self):
class X(Structure):
_fields_ = [
("a", c_uint8, 8),
("b", c_uint8, 8),
("c", c_uint32, 16)
]
if sys.platform == 'win32':
self.assertEqual(8, sizeof(X))
else:
self.assertEqual(4, sizeof(X))

def test_anon_bitfields(self):
# anonymous bit-fields gave a strange error message
class X(Structure):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix ctypes construction of structs from description.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand a bit here? This change could presumably impact anyone using packed bit fields through ctypes, so it would be good to mention at least some of those keywords here. A couple of sentences are okay (and feel free to add "Contributed by <your name>" at the end if you like - you've earned it 😉 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact maybe a what's new entry would be appreciated too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe... "we're getting it right now" isn't the sort of thing we'd usually advertise as "new", but on the other hand, there's a good chance people have discovered it was buggy before and are now avoiding it. A note there may get noticed.

We still have someone edit what's new before it goes out, right? It won't hurt to add it, and the editor can choose whether to keep it or rephrase/reframe it.

For @matthiasgoergens, we're talking about Doc/whatsnew/3.13.rst as I mentioned for porting notes, but in the "Improved Modules" section (ctypes probably needs to be added as a heading).

45 changes: 44 additions & 1 deletion Modules/_ctypes/_ctypes_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ struct BITS {
*/
#ifndef __xlc__
#define SIGNED_SHORT_BITFIELDS
short M: 1, N: 2, O: 3, P: 4, Q: 5, R: 6, S: 7;
signed short M: 1, N: 2, O: 3, P: 4, Q: 5, R: 6, S: 7;
#endif
};

Expand Down Expand Up @@ -622,6 +622,49 @@ EXPORT(int) unpack_bitfields(struct BITS *bits, char name)
return 999;
}

struct
#ifndef MS_WIN32
__attribute__ ((ms_struct))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__attribute__ is a GCC extension. (Also supported by clang, but I'm not immediately sure if clang has ms_struct.)
It should be guarded by something like defined(__GNUC__), and the tests skipped on unknown platforms.

#endif
BITS_msvc
{
signed int A: 1, B:2, C:3, D:4, E: 5, F: 6, G: 7, H: 8, I: 9;
/*
* The test case needs/uses "signed short" bitfields, but the
* IBM XLC compiler does not support this
*/
#ifndef __xlc__
#define SIGNED_SHORT_BITFIELDS
signed short M: 1, N: 2, O: 3, P: 4, Q: 5, R: 6, S: 7;
#endif
};

EXPORT(int) unpack_bitfields_msvc(struct BITS_msvc *bits, char name)
{
switch (name) {
case 'A': return bits->A;
case 'B': return bits->B;
case 'C': return bits->C;
case 'D': return bits->D;
case 'E': return bits->E;
case 'F': return bits->F;
case 'G': return bits->G;
case 'H': return bits->H;
case 'I': return bits->I;

#ifdef SIGNED_SHORT_BITFIELDS
case 'M': return bits->M;
case 'N': return bits->N;
case 'O': return bits->O;
case 'P': return bits->P;
case 'Q': return bits->Q;
case 'R': return bits->R;
case 'S': return bits->S;
#endif
}
return 999;
}

static PyMethodDef module_methods[] = {
/* {"get_last_tf_arg_s", get_last_tf_arg_s, METH_NOARGS},
{"get_last_tf_arg_u", get_last_tf_arg_u, METH_NOARGS},
Expand Down
Loading