Endianness was incorrectly assumed for GroupWord

[Erk-]

This assumption caused incorrect slots to chosen which would lead to it being unusable.

Likely also the cause for rust-lang/rust#90123, though I have not tested if it resolves it.

Tested on Linux on Z which is big-endian, I do not know if this happen on other BE systems.

[bjorn3]

Shouldn't the place where the original bytes are written be changed from native endian to little endian instead? This change asserts that the serialization format is endianness dependent, when I think it should use a fixed endianness.

[Erk-]

As far as I can tell with some smaller tests this does not change how the hash table is serialized. That said there may be a better solution, as far as I can tell this issues comes from some assumptions of layout of arrays in memory. The most simple test that is failing is probably this one https://github.com/rust-lang/odht/blob/main/src/swisstable_group_query/mod.rs#L58..L73

[cuviper]

I believe the underlying format is byte oriented, so endianness doesn't matter in the serialization format.

However, this GroupWord is batching the u8 control words to try to efficiently scan for matches or empties. It's using the bit-oriented trailing_zeros() to produce byte-oriented usize indexes into the control words. It makes sense that we would want from_le_bytes for that, to ensure low bytes are loaded at the correct "trailing" end. So I don't really understand what's happening here, why from_ne_bytes is fixing anything.

[cuviper]

Aha! The values eq_mask and empty_mask also have a to_le() on their construction, so they're effectively flipped twice. We can either flip with from_le_bytes(..) or these to_le(), but should not do both.

[michaelwoerister]

That's interesting. Thanks for the PR and bug report, @Erk-!

I'll need to take some time to actually understand what's going on here.

[cuviper]

I can confirm via qemu that a lot of tests fail before this change:

failures:
    raw_table::tests::stress
    swisstable_group_query::tests::match_iter
    swisstable_group_query::tests::match_iter_with_empty
    swisstable_group_query::tests::partially_filled
    tests::from_iterator
    tests::hash_table_at_different_alignments
    tests::init_in_place
    tests::quickchecks::k15_v0::lookup
    tests::quickchecks::k16_v0::lookup
    tests::quickchecks::k16_v16::lookup
    tests::quickchecks::k16_v17::lookup
    tests::quickchecks::k16_v1::lookup
    tests::quickchecks::k16_v2::lookup
    tests::quickchecks::k16_v3::lookup
    tests::quickchecks::k16_v4::lookup
    tests::quickchecks::k16_v8::lookup
    tests::quickchecks::k17_v0::lookup
    tests::quickchecks::k17_v4::lookup
    tests::quickchecks::k1_v0::insert_with_duplicates
    tests::quickchecks::k1_v0::lookup
    tests::quickchecks::k20_v4::lookup
    tests::quickchecks::k2_v0::insert_with_duplicates
    tests::quickchecks::k2_v0::lookup
    tests::quickchecks::k2_v4::insert_with_duplicates
    tests::quickchecks::k2_v4::lookup
    tests::quickchecks::k3_v0::lookup
    tests::quickchecks::k4_v0::lookup
    tests::quickchecks::k4_v4::lookup
    tests::quickchecks::k63_v0::lookup
    tests::quickchecks::k64_v0::lookup
    tests::quickchecks::k64_v4::lookup
    tests::quickchecks::k8_v0::lookup
    tests::quickchecks::k8_v4::lookup

test result: FAILED. 102 passed; 33 failed; 0 ignored; 0 measured; 0 filtered out; finished in 7.20s

and all tests pass with this pull request.
(I'll try to find a native machine to test on as well.)

[cuviper]

I've now tested and confirmed the fix on native s390x hardware as well.

It also works if we leave the from_le_bytes alone and instead remove the to_le calls, per #20 (comment). I think that's preferable, but I'll leave it to @michaelwoerister as the reviewer.

[michaelwoerister]

I finally had some time to take a closer look. I concur with @cuviper's diagnosis that this is a mismatch between working with addresses (which dependent on endianess) and bit-offsets within a u64 (which don't depend on endianess).

I think the correct solution is to keep GroupWord::from_le_bytes(*group) and instead remove the to_le() calls when computing eq_mask and empty_mask. We keep from_le_bytes because we want to construct a u64 where the first byte we read ends up in the lowest 8-bits, the second byte in the second lowest, and so on. This is exactly what from_le_bytes does.

Once we have converted the raw bytes into a u64 we are not working with anything endian-dependent anymore. We just work with bit-offsets within integer values -- so the to_le() calls should not be there.

@Erk-, would you mind updating the PR accordingly?

[michaelwoerister]

Once this is merged, I'll add a Miri-based regression tests (see #19).

[michaelwoerister]

Thanks, @Erk-!

[cuviper]

@michaelwoerister will you be publishing this fix? We should get this updated in rust master and beta to avoid shipping a regression in rust-lang/rust#90123.

[michaelwoerister]

@cuviper, the fix was merged into rustc in rust-lang/rust#90403.

[cuviper]

Thanks!