Skip to content

Conversation

@thesamprice
Copy link

@thesampricethesamprice commented Mar 27, 2023

Adds a simple unit test to verify C matches python bitfield.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Mar 27, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@thesampricethesampriceforce-pushed the 73939-bitfield-problem branch from ece411b to 624b481CompareMarch 27, 2023 03:17
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@thesampricethesampriceforce-pushed the 73939-bitfield-problem branch from 624b481 to 2b82490CompareMarch 27, 2023 03:41
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@matthiasgoergens
Copy link
Contributor

matthiasgoergens commented Mar 27, 2023

As requested, I have run the tests I posted in #97588 against this PR. I ran everything on x86_64 Archlinux. Summary: this PR seems to fail the tests in the same way as main does.

First:

fromctypesimportStructure, c_uint, c_ulonglong, c_ushortclassFoo(Structure): _fields_= [("A", c_uint, 1), ("B", c_ushort, 16)] classBar(Structure): _fields_= [("A", c_ulonglong, 1), ("B", c_uint, 32)] if__name__=="__main__": forain [Foo(), Bar()]: a.A=0a.B=1print(a.A, a.B)

The above should print

0 1 0 1 

But it actually prints

0000

For comparison and to test my understanding, I expect the following C code to be equivalent to the Python code above:

#include<stdio.h>structFoo{unsigned intA: 1; unsigned shortB: 16}; structBar{unsigned long long intA: 1; unsigned intB: 32}; intmain(intargc, char**argv){structFoofoo; foo.A=0; foo.B=1; printf("%d %d\n", foo.A, foo.B); structBarbar; bar.A=0; bar.B=1; printf("%d %d\n", bar.A, bar.B); return0}

The C version prints what we expect:

$ gcc -fsanitize=undefined test.c && ./a.out 0 1 0 1 

More comprehensive test case

Using Hypothesis:

importctypesimportstringfromhypothesisimportassume, example, given, notefromhypothesisimportstrategiesasstunsigned= [(ctypes.c_ushort, 16), (ctypes.c_uint, 32), (ctypes.c_ulonglong, 64)] signed= [(ctypes.c_short, 16), (ctypes.c_int, 32), (ctypes.c_longlong, 64)] types=unsigned+signedunsigned_types=list(zip(*unsigned))[0] signed_types=list(zip(*signed))[0] names=st.lists(st.text(alphabet=string.ascii_letters, min_size=1), unique=True) @st.compositedeffields_and_set(draw): names_=draw(names) ops= [] results= [] fornameinnames_: t, l=draw(st.sampled_from(types)) res= (name, t, draw(st.integers(min_value=1, max_value=l))) results.append(res) values=draw(st.lists(st.integers())) forvalueinvalues: ops.append((res, value)) ops=draw(st.permutations(ops)) returnresults, opsdeffit_in_bits(value, type_, size): expect=value% (2**size) iftype_notinunsigned_types: ifexpect>=2** (size-1): expect-=2**sizereturnexpect@given(fops=fields_and_set())deftest(fops): (fields, ops) =fopsclassBITS(ctypes.Structure): _fields_=fieldsb=BITS() for (name, type_, size), valueinops: expect=fit_in_bits(value, type_, size) setattr(b, name, value) j=getattr(b, name) assertexpect==j, f"{expect} != {j}"if__name__=="__main__": test()

Running that one gives me:

Traceback (mostrecentcalllast): File"/home/matthias/prog/python/cpythons/sams/htest.py", line58, in<module>test() File"/home/matthias/prog/python/cpythons/sams/htest.py", line42, intestdeftest(fops): ^File"/home/matthias/.local/lib/python3.12/site-packages/hypothesis/core.py", line1256, inwrapped_testraisethe_error_hypothesis_foundFile"/home/matthias/prog/python/cpythons/sams/htest.py", line54, intestassertexpect==j, f"{expect} != {j}"AssertionError: 1!=0Falsifyingexample: test( fops=([('A', ctypes.c_ushort, 2), ('B', ctypes.c_uint, 7), ('C', ctypes.c_ushort, 8)], [(('C', ctypes.c_ushort, 8), 1)]), )

To make that easier to read, the above error is equivalent to this example:

importctypesclassBITS(ctypes.Structure): _fields_= [('A', ctypes.c_ushort, 2), ('B', ctypes.c_uint, 7), ('C', ctypes.c_ushort, 8)] if__name__=="__main__": b=BITS() b.C=1print(b.C) assert1==b.C

matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this pull request Mar 27, 2023
@thesamprice
Copy link
Author

Adding @matthiasgoergens tests cases to my test logic does not match the C abi.

@matthiasgoergens
Copy link
Contributor

@thesamprice Sorry, could you explain what you mean? I'm a bit dense.

1 similar comment
@matthiasgoergens
Copy link
Contributor

@thesamprice Sorry, could you explain what you mean? I'm a bit dense.

@thesamprice
Copy link
Author

@matthiasgoergens gitlab is preventing me from pushing at the moment.
I extend my test case with your example. python looks similar, its breaks on my test.
We prob need to document what the current logic is doing if its not matching the system compiler ABI

typedefstruct{uint16_tA ; uint16_tB : 9; uint16_tC : 1; uint16_tD : 1; uint16_tE : 1; uint16_tF : 1; uint16_tG : 3; uint32_tH : 10; uint32_tI : 20; uint32_tJ : 2; uint32_tK : 1; uint16_tL : 16; uint64_tM : 1; uint32_tN : 32} Test9;

@matthiasgoergens
Copy link
Contributor

@thesamprice Thanks for explaining!

Well, I hope we can push my fix in #97702 over the line soon, and then we can match what the C compilers are doing.

@thesamprice
Copy link
Author

Closing merge request will look at @matthiasgoergens version

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thesamprice@bedevere-bot@matthiasgoergens