classification
Title: ctypes is not correctly handling bitfields backed by 64 bit integers on Windows
Type: behavior Stage: resolved
Components: ctypes Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: duplicate
Dependencies: Superseder: Can not set value for structure members larger than 32 bits
View: 6493
Assigned To: theller Nosy List: brian.curtin, higstar, meador.inge, r.david.murray, santoso.wijaya, terry.reedy, theller, tim.golden, vladris
Priority: normal Keywords: patch

Created on 2009-05-20 09:47 by higstar, last changed 2012-07-19 03:44 by meador.inge. This issue is now closed.

Files
File name Uploaded Description Edit
issue6068_unittest.diff vladris, 2011-06-24 22:32 review
unnamed higstar, 2011-06-24 22:37
issue6068_patch.diff vladris, 2011-06-25 05:54 review
Messages (14)
msg88110 - (view) Author: higstar (higstar) Date: 2009-05-20 09:47
When defining a structure with members of ctype type c_ulonglong, some
members appear read only?

I created this test.py and appended the results below:
-----
import ctypes
import time

class all_ulong(ctypes.BigEndianStructure):
    _fields_    = [
                   ("Data0",   ctypes.c_ulong, 4),
                   ("Data1",   ctypes.c_ulong, 4),
                  ]
class all_ulonglong(ctypes.BigEndianStructure):
    _fields_    = [
                   ("Data0",   ctypes.c_ulonglong, 4),
                   ("Data1",   ctypes.c_ulonglong, 4),
                  ]

def inc_both_members(test):
    test.Data0 += 1
    test.Data1 += 1
def print_both_members(test):
    print("Data0 is %d, Data1 is %d for %s"%(test.Data0, test.Data1,
test.__class__.__name__))

tests = [all_ulong(), all_ulonglong()]
for test in tests:
    print_both_members(test)

Failed = False
while not Failed:
    for test in tests:
        inc_both_members(test)
        print_both_members(test)
    if not tests[0].Data0 == tests[1].Data0: 
        Failed = True
    if not tests[0].Data1 == tests[1].Data1: 
        Failed = True
-----
>c:\python25\python.exe test.py
Data0 is 0, Data1 is 0 for all_ulong
Data0 is 0, Data1 is 0 for all_ulonglong
Data0 is 1, Data1 is 1 for all_ulong
Data0 is 1, Data1 is 1 for all_ulonglong
Data0 is 2, Data1 is 2 for all_ulong
Data0 is 2, Data1 is 1 for all_ulonglong

>c:\python26\python.exe test.py
Data0 is 0, Data1 is 0 for all_ulong
Data0 is 0, Data1 is 0 for all_ulonglong
Data0 is 1, Data1 is 1 for all_ulong
Data0 is 1, Data1 is 1 for all_ulonglong
Data0 is 2, Data1 is 2 for all_ulong
Data0 is 2, Data1 is 1 for all_ulonglong

>c:\python30\python.exe test.py
Data0 is 0, Data1 is 0 for all_ulong
Data0 is 0, Data1 is 0 for all_ulonglong
Data0 is 1, Data1 is 1 for all_ulong
Data0 is 1, Data1 is 1 for all_ulonglong
Data0 is 2, Data1 is 2 for all_ulong
Data0 is 2, Data1 is 1 for all_ulonglong


As you can see the second member Data1, is not incrementing correctly.

I found this issue because I started using c_ulonglong types for all
members so that when casting a byte stream to one of these ctype
structures, there are no incorrect values when casting to a member
crossing byte/word/long boundaries.
msg88144 - (view) Author: higstar (higstar) Date: 2009-05-21 00:42
Right...I've just found a caveat in section 16.15.1.12 of the html
documentation which means this should be a feature request:

"Bit fields are only possible for integer fields"
msg88149 - (view) Author: higstar (higstar) Date: 2009-05-21 03:10
I added the following to test_bitfields.py and got the results listed below:
def test_ulonglong_crossing_32_boundary(self):
    class X(BigEndianStructure):
        _fields_ = [("a", c_ulonglong, 16),
                    ("b", c_ulonglong, 32),
                    ("c", c_ulonglong, 16)]

    x = X()
    self.failUnlessEqual((x.a, x.b, x.c), (0, 0, 0))
    x.a, x.b, x.c = 1, 1, 1
    self.failUnlessEqual((x.a, x.b, x.c), (1, 1, 1))

======================================================================
FAIL: test_ulonglong_crossing_32_boundary (__main__.BitFieldTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "W:\ctypes\build\lib.win32-2.6\ctypes\test\test_bitfields.py",
line 98, in test_ulonglong_crossing_32_boundary
    self.failUnlessEqual((x.a, x.b, x.c), (1, 1, 1))
AssertionError: (1L, 0L, 1L) != (1, 1, 1)

----------------------------------------------------------------------
Ran 16 tests in 0.015s

FAILED (failures=1)
msg109673 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-07-09 03:33
I have never used ctypes, so I could have missed something, but when perusing this, I see data and statements, but I did not see a clear statement as to precisely what change you want. I am sure it is implied, but I am suggesting a simple statement that Thomas H. could quickly read.
msg138982 - (view) Author: Vlad Riscutia (vladris) Date: 2011-06-24 20:55
Changing title and type to better reflect issue.

On Windows MSVC build, ctypes is not correctly setting bitfields backed by 64 bit integers if specifying custom width. Simple repro:

from ctypes import *

class X(Structure):
    _fields_ = [("a", c_ulonglong, 16),
                ("b", c_ulonglong, 32),
                ("c", c_ulonglong, 16)]
s = X()
s.b = 1

print(s.b) # this prints 0

Whenever field width goes over 32 bits, setting or getting value of the field in cfield.c will only look at last (<actual size> - 32) bits of the field. So if we have a field of 34 bits, only least significant 2 bits will be operated on. The above repro results in an (<actual size> - 32) = 0 bits so nothing is getting set with the assignement.

This is not caught in unit test package because we have only this in test_bitfields.py:

    def test_ulonglong(self):
        class X(Structure):
            _fields_ = [("a", c_ulonglong, 1),
                        ("b", c_ulonglong, 62),
                        ("c", c_ulonglong, 1)]

        self.assertEqual(sizeof(X), sizeof(c_longlong))
        x = X()
        self.assertEqual((x.a, x.b, x.c), (0, 0, 0))
        x.a, x.b, x.c = 7, 7, 7
        self.assertEqual((x.a, x.b, x.c), (1, 7, 1))

For 62 bit width, we will actually operate on last 30 bits but this test passes as 7 fits in those bits. If we would actually try to set it to 0x3FFFFFFFFFFFFFFF, result will be different than expected (0x3FFFFFFF).

I will look into extending UT package with some tests that set full range of bits and check if they are actually being set correctly.

This issue reproes with latest bits but only on Windows. Root cause seems to be BIT_MASK macro in cfield.c which is ifdef-ed to something different on Windows. I patched on my machine by removing the special treatment:

@@ -429,12 +429,7 @@
 #define LOW_BIT(x)  ((x) & 0xFFFF)
 #define NUM_BITS(x) ((x) >> 16)
 
-/* This seems nore a compiler issue than a Windows/non-Windows one */
-#ifdef MS_WIN32
-#  define BIT_MASK(size) ((1 << NUM_BITS(size))-1)
-#else
-#  define BIT_MASK(size) ((1LL << NUM_BITS(size))-1)
-#endif
+#define BIT_MASK(size) ((1LL << NUM_BITS(size))-1)
 
 /* This macro CHANGES the first parameter IN PLACE. For proper sign handling,
    we must first shift left, then right.



Unittests still pass with this in place and now fields are handled correctly though I don't know why this was put in in the first place. Looking at file history it seems it was there from the start (from 2006). Could it be that it was addressing some MSVC bug which got fixed in the meantime? Things seem to be building and working fine now when using 1LL for shift.

Also related to this I have doubts about the SWAP_8 macro which is similarly changed for MSVC, also in cfield.c.

I am only able to build 32 bit version so I can't say whether this was put in place due to some x64 issue, maybe someone can check behavior on x64 build. If that's the case, maybe #ifdef should take that into account.
msg138988 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2011-06-24 21:41
Vlad, thanks for delving into this. Your two header changes are contradictory. If this is a feature request, then it is 3.3 only. If this is a behavior (bug) issue, then 3.2 should be included ;-). 'higstar's second comment indicates 'feature', but I am not sure what 'integer field means in this context'. I am guessing that UT = 'unittest'

Higstar, I see buried in your 3rd message the reference to Windows and 2.6. Please specify OS and version in initial messages.
msg138991 - (view) Author: Vlad Riscutia (vladris) Date: 2011-06-24 21:46
Terry, I am kind of new to this so I might not have marked header correctly. Please feel free to fix it.

This is 100% bug not feature request. Initial message was confusing because highstar thought Python has such bitfields as readonly and was asking for a feature to make them read-write. That is not the case, behavior is caused by a bug. By UT I mean unittest.

I reproed this on 64bit Windows 7 on latest sources with x32 build (as I said, I don't have x64 build).
msg138996 - (view) Author: Vlad Riscutia (vladris) Date: 2011-06-24 22:32
Attached is addition to unittests which repro the issue. They will currently pass on Linux but fail on Windows.
msg138997 - (view) Author: higstar (higstar) Date: 2011-06-24 22:37
Issue was originally found on do sp2 on Python 2.6.
Thanks Vlad, this rendered ctypes useless for my purposes, had to mask and
shift myself.
On Jun 25, 2011 8:32 AM, "Vlad Riscutia" <report@bugs.python.org> wrote:
>
> Vlad Riscutia <riscutiavlad@gmail.com> added the comment:
>
> Attached is addition to unittests which repro the issue. They will
currently pass on Linux but fail on Windows.
>
> ----------
> keywords: +patch
> Added file: http://bugs.python.org/file22447/issue6068_unittest.diff
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue6068>
> _______________________________________
msg139011 - (view) Author: Santoso Wijaya (santoso.wijaya) * Date: 2011-06-25 00:47
FWIW, I tested the patch on a 64-bit Python build and test_ctypes passes with the new unittest added.
msg139017 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-25 05:32
Adding our windows committers to nosy to see if this can be committed.
msg139018 - (view) Author: Vlad Riscutia (vladris) Date: 2011-06-25 05:54
Attaching patch as previous attachment is only unittest. I included change to SWAP_8 macro also as it looks like same issue and it will probably popup later.
msg140833 - (view) Author: Vlad Riscutia (vladris) Date: 2011-07-21 22:56
Ping? This also fixes 6493 (I believe in a cleaner way)
msg165823 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2012-07-19 03:44
I am closing this issue in favor of issue6493.  This patch attached to this issue is incorrect.  The proposed definition of BIT_MASK allows for undefined behavior when the number of bits shifted is 64 for a uint64.  The patch in issue6493 handles this case correctly.
History
Date User Action Args
2012-07-19 03:44:05meador.ingesetstatus: open -> closed

superseder: Can not set value for structure members larger than 32 bits

nosy: + meador.inge
messages: + msg165823
resolution: duplicate
stage: patch review -> resolved
2011-07-21 22:56:28vladrissetmessages: + msg140833
2011-06-25 05:54:29vladrissetfiles: + issue6068_patch.diff

messages: + msg139018
2011-06-25 05:32:16r.david.murraysetnosy: + r.david.murray, brian.curtin, tim.golden

messages: + msg139017
stage: needs patch -> patch review
2011-06-25 00:47:56santoso.wijayasetmessages: + msg139011
2011-06-25 00:38:28santoso.wijayasetnosy: + santoso.wijaya
2011-06-24 22:37:39higstarsetfiles: + unnamed

messages: + msg138997
2011-06-24 22:32:47vladrissetfiles: + issue6068_unittest.diff
keywords: + patch
messages: + msg138996
2011-06-24 22:16:05terry.reedysetversions: + Python 2.7, Python 3.2
2011-06-24 21:46:22vladrissetmessages: + msg138991
2011-06-24 21:41:18terry.reedysetmessages: + msg138988
2011-06-24 20:55:39vladrissetnosy: + vladris
title: support read/write c_ulonglong type bitfield structures -> ctypes is not correctly handling bitfields backed by 64 bit integers on Windows
messages: + msg138982

versions: + Python 3.3, - Python 3.2
type: enhancement -> behavior
2010-07-09 03:33:52terry.reedysetversions: + Python 3.2, - Python 2.6, Python 2.5, Python 3.0
nosy: + terry.reedy

messages: + msg109673

stage: needs patch
2009-05-21 03:10:23higstarsetmessages: + msg88149
2009-05-21 00:44:38higstarsettitle: c_ulonglong structure members appear read-only -> support read/write c_ulonglong type bitfield structures
2009-05-21 00:42:06higstarsettype: enhancement
messages: + msg88144
2009-05-20 10:32:54higstarsettype: compile error -> (no value)
2009-05-20 09:47:09higstarcreate