Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-73939: Fix bitfiled not matching C on mac/linux. #103052

Closed
wants to merge 2 commits into from

Conversation

thesamprice
Copy link

@thesamprice thesamprice 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.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Mar 27, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@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.

@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:

from ctypes import Structure, c_uint, c_ulonglong, c_ushort


class Foo(Structure):
    _fields_ = [("A", c_uint, 1), ("B", c_ushort, 16)]


class Bar(Structure):
    _fields_ = [("A", c_ulonglong, 1), ("B", c_uint, 32)]


if __name__ == "__main__":
    for a in [Foo(), Bar()]:
        a.A = 0
        a.B = 1
        print(a.A, a.B)

The above should print

0 1
0 1

But it actually prints

0 0
0 0

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

#include<stdio.h>

struct Foo {
  unsigned int A: 1;
  unsigned short B: 16;
};

struct Bar {
  unsigned long long int A: 1;
  unsigned int B: 32;
};

int main(int argc, char** argv) {
    struct Foo foo;
    foo.A = 0;
    foo.B = 1;
    printf("%d %d\n", foo.A, foo.B);

    struct Bar bar;
    bar.A = 0;
    bar.B = 1;
    printf("%d %d\n", bar.A, bar.B);
    return 0;
}

The C version prints what we expect:

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

More comprehensive test case

Using Hypothesis:

import ctypes
import string

from hypothesis import assume, example, given, note
from hypothesis import strategies as st

unsigned = [(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 + signed

unsigned_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.composite
def fields_and_set(draw):
    names_ = draw(names)
    ops = []
    results = []
    for name in names_:
        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()))
        for value in values:
            ops.append((res, value))
    ops = draw(st.permutations(ops))
    return results, ops


def fit_in_bits(value, type_, size):
    expect = value % (2**size)
    if type_ not in unsigned_types:
        if expect >= 2 ** (size - 1):
            expect -= 2**size
    return expect


@given(fops=fields_and_set())
def test(fops):
    (fields, ops) = fops

    class BITS(ctypes.Structure):
        _fields_ = fields

    b = BITS()
    for (name, type_, size), value in ops:

        expect = fit_in_bits(value, type_, size)
        setattr(b, name, value)
        j = getattr(b, name)
        assert expect == j, f"{expect} != {j}"


if __name__ == "__main__":
    test()

Running that one gives me:

Traceback (most recent call last):
  File "/home/matthias/prog/python/cpythons/sams/htest.py", line 58, in <module>
    test()
  File "/home/matthias/prog/python/cpythons/sams/htest.py", line 42, in test
    def test(fops):
                   ^
  File "/home/matthias/.local/lib/python3.12/site-packages/hypothesis/core.py", line 1256, in wrapped_test
    raise the_error_hypothesis_found
  File "/home/matthias/prog/python/cpythons/sams/htest.py", line 54, in test
    assert expect == j, f"{expect} != {j}"
AssertionError: 1 != 0
Falsifying example: 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:

import ctypes


class BITS(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 = 1
    print(b.C)
    assert 1 == 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

typedef struct{
    uint16_t A ;
    uint16_t B : 9;
    uint16_t C : 1;
    uint16_t D : 1;
    uint16_t E : 1;
    uint16_t F : 1;
    uint16_t G : 3;
    uint32_t H : 10;
    uint32_t I : 20;
    uint32_t J : 2;

    uint32_t K : 1;
    uint16_t L : 16;

    uint64_t M : 1;
    uint32_t N : 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 free to 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