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

Inconsistent Bitfield bitrange behavior #1489

Closed
Tracked by #1481
aribl opened this issue Jul 28, 2023 · 4 comments · Fixed by #1662
Closed
Tracked by #1481

Inconsistent Bitfield bitrange behavior #1489

aribl opened this issue Jul 28, 2023 · 4 comments · Fixed by #1662
Assignees

Comments

@aribl
Copy link

aribl commented Jul 28, 2023

Running the following code results in the expected individual bit values based on the specified bit order, but unexpected bitrange values based on the specified bit order. Expected result for bitrange 0..7 in lsb bitfield is 55.

module bitorder;

import spicy;

public type Byte = unit { 
    data: bytes &size=1;

    lsb: bitfield(8) {
        bit0: 0;
        bit7: 7;
        range0_7: 0..7;
    } &parse-from=self.data &bit-order=spicy::BitOrder::LSB0;

    msb: bitfield(8) {
        bit0: 0;
        bit7: 7;
        range0_7: 0..7;
    } &parse-from=self.data &bit-order=spicy::BitOrder::MSB0;

    on %done { 
        print "LSB bit0: %x" % self.lsb.bit0;
        print "LSB bit7: %x" % self.lsb.bit7;
        print "LSB bitfield 0..7: %x" % self.lsb.range0_7;
        print "MSB bit0: %x" % self.msb.bit0;
        print "MSB bit7: %x" % self.msb.bit7;
        print "MSB bitfield 0..7: %x" % self.msb.range0_7;
    }
};
> printf '\xAA' | spicy-driver bitorder.spicy
LSB bit0: 0
LSB bit7: 1
LSB bitfield 0..7: aa
MSB bit0: 1
MSB bit7: 0
MSB bitfield 0..7: aa
@rsmmr
Copy link
Member

rsmmr commented Jul 31, 2023

I would actually expect aa for 0..7 with LSB0. Bit 7 (i.e., the msb) is one in that case, so the value must be > 0x80. Also, LSB0 is the default so getting a different value than put in when selecting all bits would be surprising.

But I'm wondering if there's a problem in the handling of MSB0: the way it works internally is that it first swaps the bit order of the whole integer value, the extracts the bits to yield the desired value. But that value is now in LSB0 order, so would it need to swap the bit order once more before continuing with the value? That would then yield the 55 value.

@aribl
Copy link
Author

aribl commented Jul 31, 2023

You're right about LSB0, it seems to be working as intended - my confusion. I see your point about how the bitfields operate - even though the location of each bit is different, the natures of MSB0 and LSB0 mean that the range starting at bit0 is built LSB first (right to left) for an LSB0 bitfield and MSB first (left to right) for an MSB0 bitfield. If the types containing the values respected bitorder, this would be fine, but since the type itself is LSB0 bitorder, the internal representation of the ranges becomes the same.

I'm a bit confused about your explanation of how MSB0 is handled internally - if the bit order is swapped shouldn't the behavior of an LSB0 bitfield with \x55 as the input be identical to an MSB0 bitfield with \xAA as the input? I think this is the behavior I would expect from an MSB0 bitfield, but in practice this doesn't appear to be the case:

> printf '\x55\xAA' | spicy-driver bitorder.spicy
LSB bit0: 1
LSB bit7: 0
LSB bitfield 0..7: 55
MSB bit0: 1
MSB bit7: 0
MSB bitfield 0..7: aa

For MSB0, if the integer is flipped and then treated like a normal LSB0 bitfield:

Integer passed into LSB0 bitfield: 1010 1010
bit0 = 0
0-7 range: 1010 1010 = aa

Same Integer flipped, then passed into a regular LSB0 bitfield: 0101 0101
bit0 = 1
0-7 range: 0101 0101 = 55

For MSB0, reversing the order of the input bits then treating them like a LSB0 bitfield seems like a good way of implementing the desired behavior, I'm not sure why it isn't working as implemented.

@rsmmr
Copy link
Member

rsmmr commented Aug 3, 2023

I'll add this to our bitfield tracking ticket #1481 to take a closer look and double-check what we're doing currently.

@rsmmr rsmmr mentioned this issue Jul 7, 2023
9 tasks
@rsmmr rsmmr self-assigned this Aug 7, 2023
@rsmmr
Copy link
Member

rsmmr commented Sep 7, 2023

Note to self: Once this is settled, add a test that bitfield constants work as expected, per #1511 (comment).

awelzel added a commit that referenced this issue Jan 26, 2024
@awelzel awelzel linked a pull request Jan 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants