classification
Title: Ctypes is confused by bitfields of varying integer types
Type: behavior Stage:
Components: ctypes Versions: Python 3.0, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: theller Nosy List: effbot, mgiuca, rpetrov, skip.montanaro, theller, tim.maxwell
Priority: release blocker Keywords: needs review, patch

Created on 2008-08-12 16:20 by tim.maxwell, last changed 2008-09-29 20:05 by theller. This issue is now closed.

Files
File name Uploaded Description Edit
bitfields-3.patch theller, 2008-09-18 08:17
bitfields-mingw.patch theller, 2008-09-26 18:59 Additional patch for MingW
Messages (17)
msg71060 - (view) Author: Tim Maxwell (tim.maxwell) Date: 2008-08-12 16:20
Steps to reproduce:

Python 2.5.2 (r252:60911, Feb 22 2008, 07:57:53) 
[GCC 4.0.1 (Apple Computer, Inc. build 5363)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from ctypes import *     
>>> fields = [('a', c_short, 4), ('b', c_short, 4), ('c', c_long, 24)]
>>> class Foo(Structure):
...     _fields_ = fields
... 
>>> Foo.a
<Field type=c_short, ofs=0:0, bits=4>
>>> Foo.b
<Field type=c_short, ofs=0:4, bits=4>
>>> Foo.c
<Field type=c_long, ofs=-2:8, bits=24> # Wrong!

More about my machine:
>>> sizeof(c_short)
2
>>> sizeof(c_long)
4

This particular example comes from a 32-bit Mac OS X Intel machine. The 
bug has been reproduced on Linux as well, but could not be reproduced on 
Windows XP.
msg71196 - (view) Author: Matt Giuca (mgiuca) Date: 2008-08-16 05:56
Confirmed in HEAD for Python 2.6 and 3.0, on Linux.

Python 2.6b2+ (trunk:65708, Aug 16 2008, 15:04:13) 
[GCC 4.2.3 (Ubuntu 4.2.3-2ubuntu7)] on linux2

Python 3.0b2+ (py3k:65708, Aug 16 2008, 15:09:19) 
[GCC 4.2.3 (Ubuntu 4.2.3-2ubuntu7)] on linux2

I was also able to simplify the test case. I get this issue just using
one c_short and one c_long, with nonstandard bit lengths. eg:

fields = [('a', c_short, 16), ('b', c_long, 16)]

(sizeof(c_short) == 2 and sizeof(c_long) == 4).

But it's somewhat sporadic under which conditions it happens and which
it doesn't.

One might imagine this was a simple calculation. But the _ctypes module
is so big (5000 lines of C); at an initial glance I can't find the code
responsible! Any hints? (Modules/_ctypes/ctypes.c presumably is where
this takes place).
msg73341 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-17 18:48
Here is a patch, with test, that fixes this problem.
msg73361 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-18 06:16
Updated patch.
msg73364 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-18 08:17
Updated the unittest so that it works on Windows, too.
msg73629 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-23 12:39
Make this a release blocker so hopefully someone will review it.
msg73735 - (view) Author: Fredrik Lundh (effbot) * (Python committer) Date: 2008-09-24 17:30
Looks fine to me, except for the comment in the test suite.  Should

+        # MS compilers do NOT combine c_short and c_int into
+        # one field, gcc doesn't.

perhaps be

+        # MS compilers do NOT combine c_short and c_int into
+        # one field, gcc do.

?

Is using explicit tests for MSVC vs. GCC a good idea, btw?  What about 
other compilers?  Can the test be changed to accept either value?
msg73736 - (view) Author: Skip Montanaro (skip.montanaro) * (Python triager) Date: 2008-09-24 17:36
Looks reasonable, though I'm no ctypes maven.  Trivial little nit:
In the comment just before the start of CField_FromDesc it says,
in part:

 * Expects the size, index and offset for the current field in *psize and
 * *poffset, stores the total size so far in *psize, the offset for the next

Note that it identifies three values (size, index, offset) as stored
in two locations (*psize and *poffset).  Seems like some sort of
mismatch to me.
msg73738 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-24 18:00
Fredrik Lundh schrieb:
> Looks fine to me, except for the comment in the test suite.  Should
> 
> +        # MS compilers do NOT combine c_short and c_int into
> +        # one field, gcc doesn't.
> 
> perhaps be
> 
> +        # MS compilers do NOT combine c_short and c_int into
> +        # one field, gcc do.

Sure.  But isn't this correct (or better) english, instead?

                            ^^^^

> Is using explicit tests for MSVC vs. GCC a good idea, btw?  What about 
> other compilers?  Can the test be changed to accept either value?

Well, MSVC and GCC are the only compilers that I use (and that are tested
on the buildbots, afaik).  If a cygwin compiled Python, for example, fails
this test then of course the test must be changed.

Thanks.
msg73739 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-24 18:01
Skip Montanaro schrieb:
> Looks reasonable, though I'm no ctypes maven.  Trivial little nit:
> In the comment just before the start of CField_FromDesc it says,
> in part:
> 
>  * Expects the size, index and offset for the current field in *psize and
>  * *poffset, stores the total size so far in *psize, the offset for the next
> 
> Note that it identifies three values (size, index, offset) as stored
> in two locations (*psize and *poffset).  Seems like some sort of
> mismatch to me.

Unfortunately this is not the only comment that is out of date in the
ctypes sources.  I wanted to touch as little code as possible in this patch.

Thanks.
msg73740 - (view) Author: Fredrik Lundh (effbot) * (Python committer) Date: 2008-09-24 18:02
^^^^

"Do" should be "does", right.  Not enough coffee today :)
msg73802 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-09-25 16:50
test_mixed_4 fail on:
Python 2.6rc2+ (trunk:66617M, Sep 25 2008, 16:32:44) 
[GCC 3.4.5 (mingw special)] on win32
Type "help", "copyright", "credits" or "license" for more information.

sizeof(X) return 12.
msg73808 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-25 19:12
Does the following patch fix the test failure with MingW?

<patch>
Index: cfield.c
===================================================================
--- cfield.c	(revision 66611)
+++ cfield.c	(working copy)
@@ -65,10 +65,10 @@
 	}
 	if (bitsize /* this is a bitfield request */
 	    && *pfield_size /* we have a bitfield open */
-#if defined(MS_WIN32) && !defined(__MINGW32__)
-	    && dict->size * 8 == *pfield_size /* MSVC */
+#if defined(MS_WIN32)
+	    && dict->size * 8 == *pfield_size /* Windows */
 #else
-	    && dict->size * 8 <= *pfield_size /* GCC, MINGW */
+	    && dict->size * 8 <= *pfield_size /* GCC */
 #endif
 	    && (*pbitofs + bitsize) <= *pfield_size) {
 		/* continue bit field */
<end patch>

Also, can you please post the output of the following code snippet?

<test script>
from ctypes import *

class X(Structure):
    _fields_ = [("a", c_short, 4),
                ("b", c_short, 4),
                ("c", c_int, 24),
                ("d", c_short, 4),
                ("e", c_short, 4),
                ("f", c_int, 24)]

print X.a
print X.b
print X.c
print X.d
print X.e
print X.f
<end test script>
msg73824 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-09-25 23:36
Yes this extra define was the problem.
Instead hint /* Windows */ what about /* MSVC, GCC(with -mms-bitfields) */ ?

The option -mms-bitfields is available for GCC compiler on mingw and
cygwin targets.


About test_bitfields.py:
- comment in test_mixed_4: if GCC compiler has option -mms-bitfields it
will produce code compatible with MSVC. May be comment has to include
this information. 

- may be method test_mixed_1 need similar comment as test_mixed_4, but
even without comment is fine with me.
msg73877 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-26 18:59
Ok, here is an additional patch bitfields-mingw.patch.
It fixes the problem reported by rpetrov, and updates the comments.

Please review again ;-).

The previous patch bitfields-3.patch has already been applied.
msg73892 - (view) Author: Roumen Petrov (rpetrov) * Date: 2008-09-26 21:40
Flag start only with one minus: -mms-bitfields
Fine with me.
msg74042 - (view) Author: Thomas Heller (theller) * (Python committer) Date: 2008-09-29 20:05
Committed as SVN rev 66683 (trunk), 66684 (py3k), and 66685
(release25-maint).
History
Date User Action Args
2008-09-29 20:05:07thellersetstatus: open -> closed
resolution: fixed
messages: + msg74042
2008-09-26 21:40:29rpetrovsetmessages: + msg73892
2008-09-26 18:59:30thellersetfiles: + bitfields-mingw.patch
messages: + msg73877
2008-09-25 23:36:23rpetrovsetmessages: + msg73824
2008-09-25 19:12:55thellersetmessages: + msg73808
2008-09-25 16:50:11rpetrovsetnosy: + rpetrov
messages: + msg73802
2008-09-24 18:02:55effbotsetmessages: + msg73740
2008-09-24 18:01:32thellersetmessages: + msg73739
2008-09-24 18:00:27thellersetmessages: + msg73738
2008-09-24 17:36:14skip.montanarosetnosy: + skip.montanaro
messages: + msg73736
2008-09-24 17:30:37effbotsetnosy: + effbot
messages: + msg73735
2008-09-23 12:39:54thellersetpriority: release blocker
messages: + msg73629
2008-09-18 08:17:32thellersetfiles: - bitfields-2.patch
2008-09-18 08:17:21thellersetfiles: + bitfields-3.patch
messages: + msg73364
2008-09-18 06:16:57thellersetfiles: - bitfields.patch
2008-09-18 06:16:39thellersetfiles: + bitfields-2.patch
messages: + msg73361
2008-09-17 18:48:55thellersetkeywords: + patch, needs review
files: + bitfields.patch
messages: + msg73341
2008-08-16 05:56:40mgiucasetnosy: + mgiuca
messages: + msg71196
versions: + Python 2.6, Python 3.0
2008-08-12 16:20:03tim.maxwellcreate