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

Fix BitVec on Big-Endian Systems #43

Closed
Alexhuszagh opened this issue Jan 24, 2020 · 6 comments
Closed

Fix BitVec on Big-Endian Systems #43

Alexhuszagh opened this issue Jan 24, 2020 · 6 comments
Assignees

Comments

@Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jan 24, 2020

Relevant Code Failing

let u16b = u16::from_ne_bytes(0x1234u16.to_le_bytes());
bytes[5 ..][.. 14].store_le(u16b);
assert_eq!(bytes[5 ..][.. 14].load_le::<u16>(), 0x1234u16);

Result

---- fields::tests::lsb0 stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `13330`,
 right: `4660`', src/fields.rs:999:3

---- fields::tests::msb0 stdout ----
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `13330`,
 right: `4660`', src/fields.rs:1073:3

On big-endian PPC, the above code cails, but on little-endian PPC, it passes.

@myrrlyn
Copy link
Collaborator

@myrrlyn myrrlyn commented Jan 24, 2020

Awesome. I've always been afraid these were subtly incorrect and I just didn't have a machine on which to test them.

@myrrlyn myrrlyn self-assigned this Jan 24, 2020
@Alexhuszagh
Copy link
Contributor Author

@Alexhuszagh Alexhuszagh commented Jan 25, 2020

Some further debugging with a lot of printlns shows that it's store_le that's failing:

println!("0x1234u16.to_le_bytes()={:?}", 0x1234u16.to_le_bytes());
// 0x1234u16.to_le_bytes()=[52, 18]

let u16b = u16::from_ne_bytes(0x1234u16.to_le_bytes());
println!("u16b={:?}", u16b);
// u16b=13330

bytes[..16].store_le(u16b);
println!("bytes[..16]={:?}", bytes[..16]);
// ACTUAL:   bytes[..16]=BitSlice<Lsb0, u8> [01001000, 00101100]
// EXPECTED: bytes[..16]=BitSlice<Lsb0, u8> [00101100, 01001000]

Now, I"m not sure if this is a real issue, or faulty assumptions during unittesting, since to_le_bytes() properly switches the byteorder of the integer from big-endian to little-endian, and store_le properly switches the byteorder again, assuming it's storing a big-endian value as little-endian.

@Alexhuszagh
Copy link
Contributor Author

@Alexhuszagh Alexhuszagh commented Jan 25, 2020

The more I look at this, the more I believe this is simply a logic bug in the testing suite. The following works, as expected:

let u16b = u16::from_ne_bytes(0x1234u16.to_ne_bytes());
bytes[..16].store_le(u16b);
assert_eq!(bytes[..16].load_le::<u16>(), 0x1234u16);
@Alexhuszagh Alexhuszagh changed the title store_le or load_le fails on PowerPC64 (But Passes on PPC64LE). Fix BitVec on Big-Endian Systems Jan 25, 2020
@Alexhuszagh
Copy link
Contributor Author

@Alexhuszagh Alexhuszagh commented Jan 25, 2020

On big-endian, one more issue:

let bits: &BitSlice<Local, usize> = bits![1, 0, 1, 1, 0, 1, 0, 0, 1, 0];
println!("bits={:?}", bits);
// bits=BitSlice<Msb0, usize> [0000000000]

The bits macro is therefore broken on big-endian architectures.

@obeah
Copy link

@obeah obeah commented Mar 4, 2020

In my case (little-endian) bitvec! makes wrong initialization after 48th bit.
Probably completely different problem, just found this issue while searching for solution.

let bits = bitvec![0, 1, 0, 1, 1, 0, 1, 1, 0, 0,
                   0, 0, 0, 0, 0, 0, 1, 1, 0, 0,
                   0, 0, 0, 0, 0, 0, 1, 1, 0, 0,
                   0, 1, 0, 1, 1, 0, 1, 1, 0, 0,
                   0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                   0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
                   0, 0, 0, 0, 0, 0, 0, 0, 0, 0];

println!("{:?}", bits);
// BitVec<Lsb0, usize> [
//    0b0101101100000000110000000011000101011011000000001100000000110001,
//    0b000000,
//]
@myrrlyn myrrlyn closed this in #46 Mar 4, 2020
@myrrlyn myrrlyn reopened this Mar 4, 2020
@myrrlyn myrrlyn closed this in 9557bc6 Mar 4, 2020
@myrrlyn
Copy link
Collaborator

@myrrlyn myrrlyn commented Mar 4, 2020

Thank you @obeah for this issue report.

The behavior is now fixed on trunk. I will cherry-pick it down into a 0.17.3 patch release later today or this week.

myrrlyn added a commit that referenced this issue Mar 4, 2020
Closes #43
The byte-reärranging functions in `macros/internals.rs` had
copy-paste errors in the `u64` variants.
myrrlyn added a commit that referenced this issue Mar 5, 2020
The byte-reärranging functions in `macros/internals.rs` had
copy-paste errors in the `u64` variants.
dingelish pushed a commit to mesalock-linux/bitvec-sgx that referenced this issue Apr 13, 2020
The byte-reärranging functions in `macros/internals.rs` had
copy-paste errors in the `u64` variants.

(cherry picked from commit af5ee02)
dingelish pushed a commit to mesalock-linux/bitvec-sgx that referenced this issue Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants