Fix bit_offset for big-endian structs (where bitfields are laid out f…

…rom the big end)
python · encukou · Mar 24, 2025 · Dec 18, 2024 · Jan 9, 2025 · Jan 10, 2025
commit 34865e8efc953b59fd3e7650bd893829246d7acc
diff --git a/Doc/library/ctypes.rst b/Doc/library/ctypes.rst
@@ -2826,14 +2826,15 @@ fields, or any other data types containing pointer type fields.
 
       Offset of the field, in bytes.
 
-      For bitfields, this excludes any :attr:`~CField.bit_offset`.
+      For bitfields, this is the offset of the underlying byte-aligned
+      *storage unit*; see :attr:`~CField.bit_offset`.
 
    .. attribute:: byte_size
 
       Size of the field, in bytes.
 
-      For bitfields, this is the size of the underlying type, which may be
-      much larger than the field itself.
+      For bitfields, this is the size of the underlying *storage unit*.
+      Typically, it has the same size as the bitfield's type.
 
    .. attribute:: size
 
@@ -2849,19 +2850,18 @@ fields, or any other data types containing pointer type fields.
       True if this is a bitfield.
 
    .. attribute:: bit_offset
+                  bit_size
 
-      Additional offset of a bitfield, in bits.
-      The value is relative to :attr:`byte_offset`. That is, the *total* bit
-      offset, from the start of the structure,
-      is ``byte_offset * 8 + bit_offset``.
+      The location of a bitfield within its *storage unit*, that is, within
+      :attr:`~CField.byte_size` bytes of memory starting at
+      :attr:`~CField.byte_offset`.
 
-      Zero for non-bitfields.
+      To get the field's value, read the storage unit as an integer,
+      :ref:`shift left <shifting>` by :attr:`!bit_offset` and
+      take the :attr:`!bit_size` least significant bits.
 
-   .. attribute:: bit_size
-
-      Size of the field, in bits.
-
-      For non-bitfields, this is equal to ``byte_size * 8``.
+      For non-bitfields, :attr:`!bit_offset` is zero
+      and :attr:`!bit_size` is equal to ``byte_size * 8``.
 
    .. attribute:: is_anonymous
 

diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h
diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h
@@ -444,7 +444,6 @@ struct _Py_global_strings {
         STRUCT_FOR_ID(flush)
         STRUCT_FOR_ID(fold)
         STRUCT_FOR_ID(follow_symlinks)
-        STRUCT_FOR_ID(for_big_endian)
         STRUCT_FOR_ID(format)
         STRUCT_FOR_ID(format_spec)
         STRUCT_FOR_ID(from_param)

diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h
diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h
diff --git a/Lib/ctypes/_layout.py b/Lib/ctypes/_layout.py
@@ -192,9 +192,8 @@ def get_layout(cls, input_fields, is_struct, base):
 
             offset = round_down(next_bit_offset, type_bit_align) // 8
             if is_bitfield:
-                effective_bit_offset = next_bit_offset - 8 * offset
-                bit_offset = effective_bit_offset
-                assert effective_bit_offset <= type_bit_size
+                bit_offset = next_bit_offset - 8 * offset
+                assert bit_offset <= type_bit_size
             else:
                 assert offset == next_bit_offset / 8
 
@@ -237,6 +236,11 @@ def get_layout(cls, input_fields, is_struct, base):
             next_bit_offset += bit_size
             struct_size = next_byte_offset
 
+        if is_bitfield and big_endian:
+            # On big-endian architectures, bit fields are also laid out
+            # starting with the big end.
+            bit_offset = type_bit_size - bit_size - bit_offset
+
         # Add the format spec parts
         if is_struct:
             padding = offset - last_size
@@ -268,7 +272,6 @@ def get_layout(cls, input_fields, is_struct, base):
             bit_size=bit_size if is_bitfield else None,
             bit_offset=bit_offset if is_bitfield else None,
             index=i,
-            for_big_endian=big_endian,
 
             # Do not use CField outside ctypes, yet.
             # The constructor is internal API and may change without warning.

diff --git a/Lib/test/test_ctypes/_support.py b/Lib/test/test_ctypes/_support.py
@@ -3,6 +3,7 @@
 import ctypes
 from _ctypes import Structure, Union, _Pointer, Array, _SimpleCData, CFuncPtr
 import sys
+from test import support
 
 
 _CData = Structure.__base__
@@ -43,9 +44,10 @@ def _check_struct_or_union(self, cls, is_struct):
         # Check that fields are not overlapping (for structs),
         # and that their metadata is consistent.
 
-        # offset of the last checked bit, from start of struct
-        # stays 0 for unions
-        next_bit = 0
+        used_bits = 0
+
+        is_little_endian = (
+            hasattr(cls, '_swappedbytes_') ^ (sys.byteorder == 'little'))
 
         anon_names = getattr(cls, '_anonymous_', ())
         cls_size = ctypes.sizeof(cls)
@@ -75,17 +77,7 @@ def _check_struct_or_union(self, cls, is_struct):
                 self.assertGreaterEqual(field.size, 0)
                 if is_bitfield:
                     # size has backwards-compatible bit-packed info
-                    is_big_endian = (
-                        hasattr(cls, '_swappedbytes_')
-                        ^ (sys.byteorder == 'big')
-                    )
-                    if is_big_endian:
-                        offset_for_size = (8 * field.byte_size
-                                           - field.bit_offset
-                                           - field.bit_size)
-                    else:
-                        offset_for_size = field.bit_offset
-                    expected_size = (field.bit_size << 16) + offset_for_size
+                    expected_size = (field.bit_size << 16) + field.bit_offset
                     self.assertEqual(field.size, expected_size)
                 else:
                     # size == byte_size
@@ -102,7 +94,11 @@ def _check_struct_or_union(self, cls, is_struct):
                 else:
                     self.assertEqual(field.bit_offset, 0)
                 if not is_struct:
-                    self.assertEqual(field.bit_offset, 0)
+                    if is_little_endian:
+                        self.assertEqual(field.bit_offset, 0)
+                    else:
+                        self.assertEqual(field.bit_offset,
+                                         field.byte_size * 8 - field.bit_size)
 
                 # bit_size
                 if is_bitfield:
@@ -116,17 +112,27 @@ def _check_struct_or_union(self, cls, is_struct):
                 # is_anonymous (bool)
                 self.assertIs(field.is_anonymous, name in anon_names)
 
-                # field is not overlapping earlier members in a struct.
-                # (this assumes fields are laid out in order)
-                self.assertGreaterEqual(
-                    field.byte_offset * 8 + field.bit_offset,
-                    next_bit)
-                next_bit = (field.byte_offset * 8
-                            + field.bit_offset
-                            + field.bit_size)
+                # In a struct, field should not overlap.
+                # (Test skipped if the structs is enormous.)
+                if is_struct and cls_size < 10_000:
+                    # Get a mask indicating where the field is within the struct
+                    if is_little_endian:
+                        tp_shift = field.byte_offset * 8
+                    else:
+                        tp_shift = (cls_size
+                                    - field.byte_offset
+                                    - field.byte_size) * 8
+                    mask = (1 << field.bit_size) - 1
+                    mask <<= (tp_shift + field.bit_offset)
+                    assert mask.bit_count() == field.bit_size
+                    # Check that these bits aren't shared with previous fields
+                    self.assertEqual(used_bits & mask, 0)
+                    # Mark the bits for future checks
+                    used_bits |= mask
+
                 # field is inside cls
-                self.assertLessEqual(next_bit, cls_size * 8)
+                bit_end = (field.byte_offset * 8
+                           + field.bit_offset
+                           + field.bit_size)
+                self.assertLessEqual(bit_end, cls_size * 8)
 
-                if not is_struct:
-                    # union fields may overlap
-                    next_bit = 0
diff --git a/Lib/test/test_ctypes/test_generated_structs.py b/Lib/test/test_ctypes/test_generated_structs.py
@@ -10,10 +10,11 @@
 """
 
 import unittest
-from test.support import import_helper
+from test.support import import_helper, verbose
 import re
 from dataclasses import dataclass
 from functools import cached_property
+import sys
 
 import ctypes
 from ctypes import Structure, Union
@@ -449,6 +450,8 @@ def test_generated_data(self):
         Common compilers seem to do so.
         """
         for name, cls in TESTCASES.items():
+            is_little_endian = (
+                hasattr(cls, '_swappedbytes_') ^ (sys.byteorder == 'little'))
             with self.subTest(name=name):
                 self.check_struct_or_union(cls)
                 if _maybe_skip := getattr(cls, '_maybe_skip', None):
@@ -464,7 +467,7 @@ def test_generated_data(self):
                 obj = cls()
                 ptr = pointer(obj)
                 for field in iterfields(cls):
-                    for value in -1, 1, 0:
+                    for value in -1, 1, 2926941915, 0:
                         with self.subTest(field=field.full_name, value=value):
                             field.set_to(obj, value)
                             py_mem = string_at(ptr, sizeof(obj))
@@ -475,6 +478,17 @@ def test_generated_data(self):
                                 m = "\n".join([str(field), 'in:', *lines])
                                 self.assertEqual(py_mem.hex(), c_mem.hex(), m)
 
+                            descriptor = field.descriptor
+                            field_mem = py_mem[
+                                field.byte_offset
+                                : field.byte_offset + descriptor.byte_size]
+                            field_int = int.from_bytes(field_mem, sys.byteorder)
+                            mask = (1 << descriptor.bit_size) - 1
+                            self.assertEqual(
+                                (field_int >> descriptor.bit_offset) & mask,
+                                value & mask)
+
+
 
 # The rest of this file is generating C code from a ctypes type.
 # This is only meant for (and tested with) the known inputs in this file!
@@ -572,6 +586,8 @@ class FieldInfo:
     bits: int | None  # number if this is a bit field
     parent_type: type
     parent: 'FieldInfo' #| None
+    descriptor: object
+    byte_offset: int
 
     @cached_property
     def attr_path(self):
@@ -603,10 +619,6 @@ def root(self):
         else:
             return self.parent
 
-    @cached_property
-    def descriptor(self):
-        return getattr(self.parent_type, self.name)
-
     def __repr__(self):
         qname = f'{self.root.parent_type.__name__}.{self.full_name}'
         try:
@@ -624,7 +636,11 @@ def iterfields(tp, parent=None):
     else:
         for fielddesc in fields:
             f_name, f_tp, f_bits = unpack_field_desc(*fielddesc)
-            sub = FieldInfo(f_name, f_tp, f_bits, tp, parent)
+            descriptor = getattr(tp, f_name)
+            byte_offset = descriptor.byte_offset
+            if parent:
+                byte_offset += parent.byte_offset
+            sub = FieldInfo(f_name, f_tp, f_bits, tp, parent, descriptor, byte_offset)
             yield from iterfields(f_tp, sub)
 
 
@@ -660,12 +676,13 @@ def output(string):
                 (char*)&value, sizeof(value)));         \\
         }
 
-        // Set a field to -1, 1 and 0; append a snapshot of the memory
+        // Set a field to test values; append a snapshot of the memory
         // after each of the operations.
-        #define TEST_FIELD(TYPE, TARGET) {              \\
-            SET_AND_APPEND(TYPE, TARGET, -1)            \\
-            SET_AND_APPEND(TYPE, TARGET, 1)             \\
-            SET_AND_APPEND(TYPE, TARGET, 0)             \\
+        #define TEST_FIELD(TYPE, TARGET) {                  \\
+            SET_AND_APPEND(TYPE, TARGET, -1)                \\
+            SET_AND_APPEND(TYPE, TARGET, 1)                 \\
+            SET_AND_APPEND(TYPE, TARGET, (TYPE)2926941915)  \\
+            SET_AND_APPEND(TYPE, TARGET, 0)                 \\
         }
 
         #if defined(__GNUC__) || defined(__clang__)

diff --git a/Lib/test/test_ctypes/test_structunion.py b/Lib/test/test_ctypes/test_structunion.py
@@ -1,6 +1,7 @@
 """Common tests for ctypes.Structure and ctypes.Union"""
 
 import unittest
+import sys
 from ctypes import (Structure, Union, POINTER, sizeof, alignment,
                     c_char, c_byte, c_ubyte,
                     c_short, c_ushort, c_int, c_uint,
@@ -239,11 +240,12 @@ class X(self.cls):
         # is_bitfield, bit_size, bit_offset
         # size
 
+        little_endian = (sys.byteorder == 'little')
         expected_bitfield_info = dict(
             # (bit_size, bit_offset)
-            b=(1, 0),
-            c=(2, 1),
-            y=(1, 0),
+            b=(1, 0 if little_endian else 7),
+            c=(2, 1 if little_endian else 5),
+            y=(1, 0 if little_endian else 15),
         )
         for name in field_names:
             with self.subTest(name=name):

diff --git a/Modules/_ctypes/_ctypes_test_generated.c.h b/Modules/_ctypes/_ctypes_test_generated.c.h
@@ -25,12 +25,13 @@
                 (char*)&value, sizeof(value)));         \
         }
 
-        // Set a field to -1, 1 and 0; append a snapshot of the memory
+        // Set a field to test values; append a snapshot of the memory
         // after each of the operations.
-        #define TEST_FIELD(TYPE, TARGET) {              \
-            SET_AND_APPEND(TYPE, TARGET, -1)            \
-            SET_AND_APPEND(TYPE, TARGET, 1)             \
-            SET_AND_APPEND(TYPE, TARGET, 0)             \
+        #define TEST_FIELD(TYPE, TARGET) {                  \
+            SET_AND_APPEND(TYPE, TARGET, -1)                \
+            SET_AND_APPEND(TYPE, TARGET, 1)                 \
+            SET_AND_APPEND(TYPE, TARGET, (TYPE)2926941915)  \
+            SET_AND_APPEND(TYPE, TARGET, 0)                 \
         }
 
         #if defined(__GNUC__) || defined(__clang__)

diff --git a/Modules/_ctypes/cfield.c b/Modules/_ctypes/cfield.c
@@ -61,7 +61,6 @@ _ctypes.CField.__new__ as PyCField_new
     byte_size: Py_ssize_t
     byte_offset: Py_ssize_t
     index: Py_ssize_t
-    for_big_endian: bool
     _internal_use: bool
     bit_size as bit_size_obj: object = None
     bit_offset as bit_offset_obj: object = None
@@ -71,9 +70,9 @@ _ctypes.CField.__new__ as PyCField_new
 static PyObject *
 PyCField_new_impl(PyTypeObject *type, PyObject *name, PyObject *proto,
                   Py_ssize_t byte_size, Py_ssize_t byte_offset,
-                  Py_ssize_t index, int for_big_endian, int _internal_use,
+                  Py_ssize_t index, int _internal_use,
                   PyObject *bit_size_obj, PyObject *bit_offset_obj)
-/*[clinic end generated code: output=79505dee1dad9b8e input=a6376bdec96976b8]*/
+/*[clinic end generated code: output=3f2885ee4108b6e2 input=b343436e33c0d782]*/
 {
     CFieldObject* self = NULL;
 
@@ -188,7 +187,6 @@ PyCField_new_impl(PyTypeObject *type, PyObject *name, PyObject *proto,
     self->byte_offset = byte_offset;
     self->bitfield_size = (uint8_t)bitfield_size;
     self->bit_offset = (uint8_t)bit_offset;
-    self->_for_big_endian = for_big_endian;
 
     self->index = index;
 
@@ -237,9 +235,6 @@ _pack_legacy_size(CFieldObject *field)
 {
     if (field->bitfield_size) {
         Py_ssize_t bit_offset = field->bit_offset;
-        if (field->_for_big_endian) {
-            bit_offset = 8 * field->byte_size - bit_offset - field->bitfield_size;
-        }
         return (field->bitfield_size << 16) | bit_offset;
     }
     return field->byte_size;