@kumaraditya303@mdickinson Would you please have a look? Thanks!

I'm not super-keen on this fix - I agree that it would be good to fix the undefined behaviour, but I wonder whether there's a better way. The multiplication bugs me on both conceptual grounds and technical grounds: conceptually, the intent really is just to shift bits around (unlike the similar changes inaudioop.c, where arithmeticis what's intended), and it would be nice if the code reflected that. Technically, reasoning about correctness becomes even harder when arithmetic is brought into the mix, since now you're worrying about the usual arithmetic conversions aswell as the integer promotion rules.

It feels as though the right solution is to ensure that we always do the bitwise operations (or at the very least, the shifts) on unsigned types rather than signed types, so that everything's well-defined, and then do sign-extension as necessary. So forh_get, for example, the current code is this:

staticPyObject*h_get(void*ptr,Py_ssize_tsize){shortval;memcpy(&val,ptr,sizeof(val));GET_BITFIELD(val,size);returnPyLong_FromLong((long)val);}

Could we arrange for theh_get code to be something that looks more like this, after macro expansion (it can almost certainly be streamlined with appropriate macros)? The key change here is thatval now has typeunsigned short instead ofshort, so that we do all of the bit manipulation with unsigned types. There's still implementation-defined behaviour in the very last line, in the cast fromunsigned short toshort, but I can live with that.

staticPyObject*h_get(void*ptr,Py_ssize_tsize){unsigned shortval,sign_bit;memcpy(&val,ptr,sizeof(val));if (NUM_BITS(size)) {// Assumes that 0 < NUM_BITS(size) <= width of shortassert(0<NUM_BITS(size));assert(NUM_BITS(size) <=16);sign_bit= (unsigned short)1 <<NUM_BITS(size)-1;// Assumes that 0 <= LOW_BIT(size) < width of shortassert(0 <=LOW_BIT(size));// assert(LOW_BIT(size) < 16);val=val >>LOW_BIT(size)& (sign_bit |sign_bit-1U);val= (val ^sign_bit)-sign_bit;    }returnPyLong_FromLong((long)(short)val);}

N.B. Theassert(LOW_BIT(size) < 16), when uncommented, fails on my machine when I runtest_ctypes - there are apparently test cases whereLOW_BIT(size) = 17. I'm not sure whether this is a bug in the test, or whether it's genuinely considered okay to have a low bit >= 16 for a bit field in ashort. There isn't undefined behaviour here because thanks to the integer promotions we're actually shifting something of typeint in this case, and17 is a legal right shift forint on my machine.

Thanks for having a look.
@mdickinson

I actually had a similar idea at first: work in unsigned for the left shift, then convert to signed for the right shift.

But I had some bugs in my implementation and things were getting complicated. So instead of thinking too hard, I went with the multiplication instead, which was much simpler for me to wrap my head around.

Your code looks good. Probably better than what I was trying to do. If it's similar enough to other fix, then our conpilers can probably see through it just as well, and compile it to the same code we used to have.

Btw, I am running benchmarks on main vs a version with -fstrict-overflow enabled, and so far it seems like I'm getting about 1% speedup on (geometric) average across the board for all benchmarks that have a 'significant' difference.

It's nothing too exciting, but considering that this is a fairly trivial compiler flag change, I guess we shouldn't complain.

(I'm rerunning the benchmarks with slightly different settings, I don't really have much experience with pyperformance.)

Drive-by comment: there's a use of thesupport.skip_if_sanitizer decorator intest_bitfields.py that should be removable as part of this PR:

it can almost certainly be streamlined with appropriate macros [...]

I'm thinking something along the lines of this:

staticPyObject*h_get(void*ptr,Py_ssize_tsize){unsigned shortval;memcpy(&val,ptr,sizeof(val));GET_SIGNED_BITFIELD(unsigned short,val,size);returnPyLong_FromLong((long)(short)val);}

where:

#defineGET_SIGNED_BITFIELD(unsigned_type,v,size)                      \    if (NUM_BITS(size)) {                                                \        unsigned_type sign_bit = (unsigned_type)1 << NUM_BITS(size) - 1; \        v = v >> LOW_BIT(size) & (sign_bit | sign_bit - 1U);             \        v = (v ^ sign_bit) - sign_bit;                                   \    }

there are apparently test cases where LOW_BIT(size) = 17

I'm a bit worried that there's a deeper bug here. The relevant test case arises from testing this structure:

class BITS(Structure):    _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)]

It seems possible that we're trying to cram the bitfield forM into the same int thatH andI live in. (WithA throughG having been put into 28 bits in a separateint.)

In any case, if there is a bug there it would be orthogonal to the fixes in this PR.

In any case, if there is a bug there it would be orthogonal to the fixes in this PR.

Many bugs.#73939,#84039,#86098,#59324 (which apparently I opened, a long time ago), and more ... In the case of thatBITS structure, we have (on my machine):

(Pdb) BITS.M<Field type=c_short, ofs=6:17, bits=1>(Pdb) BITS.N<Field type=c_short, ofs=6:18, bits=2>(Pdb) BITS.O<Field type=c_short, ofs=6:20, bits=3>(Pdb) BITS.P<Field type=c_short, ofs=6:23, bits=4>(Pdb) BITS.Q<Field type=c_short, ofs=6:27, bits=5>(Pdb) BITS.R

and none of those make much sense. But discussion should be deferred to the other issues mentioned above.

@matthiasgoergens I don't suppose you'd be interested in becoming a ctypes bitfield expert / champion? There's a definite maintenance hole here. (I'm ruling myself out, I'm afraid: I do have a small portion of the necessary expertise, but unfortunately close to zero of the necessary energy and time.)

@mdickinson I have a bit of time until mid October, as I am waiting out a non-compete before starting my next job. I'll have a look at the bitfield bugs already mentioned here, let's see how much I can learn.

As far as I can tell, it's trying to fit values inrange(256) into signed bit-fields of sizes 1 to 7 bits. There's not enough space to fit those value (and we aren't checking negative numbers, either?)

I think the best we can do in this case, is to throw an error?

Throwing an error may be a backwards incompatible change. Having assignments to bit fields wrap modulo the appropriate power of two wouldn't be unreasonable behaviour; after all, that's what most compilers would do for direct assignments to an integer type that's too small (and is defined behaviour in the case of an unsigned type, and implementation-defined but typical behaviour for signed types).

But one of the things that makes ctypes development hard is that we don't just need something reasonable in a self-contained sense; we need something that also matches the behaviour of commonly-used C compilers for the relevant system.

So it may be worth a test at C level: if you assign7 to an unsigned bit field of bit width2 withgcc (say) and then retrieve the value, what's the behaviour? And then the same question for a signed bit field.

So it may be worth a test at C level: if you assign7 to an unsigned bit field of bit width2 withgcc (say) and then retrieve the value, what's the behaviour? And then the same question for a signed bit field.

Here's what happens with Clang 14.0.0. Using this test code:

#include<stdio.h>structbitfields {unsignedintsome_val:2;signedintother_val:2;};intmain(void) {structbitfieldsx;x.some_val=7;x.other_val=7;printf("%u %d\n",x.some_val,x.other_val);return0;}

when I compile and run on my laptop, I get3 -1. (Not surprisingly, I also get compile-time warnings about implicit truncation.)

@mdickinson Yes, the modulo behaviour is exactly what you get on the C level. (Not sure if that's what the standard says, but it's definitely what GCC and clang give you.)

I figured that out after I wrote that comment, that's why I hid it afterwards.

However, I found some other problems. See the new issue and failing-test PR that I made.