"Fancy new algorithm" doesn't work on big-endian machines

[infinity0]

Originally from BurntSushi/ripgrep#1144, we traced down the issue to bytecount, specifically commit c6668a3:

git bisect start master f2a67d5
git bisect run sh -c 'RUSTFLAGS="-L $LD_LIBRARY_PATH" RUSTDOCFLAGS="-L $LD_LIBRARY_PATH" cargo test'
[..]
running 9 tests
test check_count_correct ... FAILED
test check_count_large ... ok
test check_count_overflow ... ok
test check_count_some ... FAILED
test check_num_chars_correct ... ok
test check_count_large_rand ... ok
test check_num_chars_overflow ... ok
test check_num_chars_some ... ok
test check_num_chars_large ... ok

failures:

---- check_count_correct stdout ----
thread 'check_count_correct' panicked at '[quickcheck] TEST FAILED. Arguments: (([0, 0, 0, 1, 0, 0, 0, 0, 0, 0], 0))', /home/infinity0/.cargo/registry/src/github.com-eae4ba8cbf2ce1c7/quickcheck-0.6.2/src/tester.rs:179:28
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- check_count_some stdout ----
thread 'check_count_some' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `1`', tests/check.rs:43:5


failures:
    check_count_correct
    check_count_some

test result: FAILED. 7 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test check'
c6668a3264ec208ae9733f83eb94b6312518f14a is the first bad commit
commit c6668a3264ec208ae9733f83eb94b6312518f14a
Author: Veedrac <joshua@landau.ws>
Date:   Wed Jan 31 02:54:17 2018 +0000

    Fancy new algorithm on stable SIMD

:100644 100644 f37644090b9886a0ed33293c5ac353b52631d2e3 c4b93928eda2ec61972beef49ed8c40860639d89 M      .travis.yml
:100644 100644 1eecc67f7677cdb843eba653d689faef0b3d7b13 91395c0abca0a56e7bf0f8075dbf83b25fc1a1e0 M      Cargo.toml
:100644 100644 29d6473403e8fcfe5ecb6d4b2d144cdd782fc7ea e76dfae2c603a0e87bb29efe4904200f09a56a4e M      appveyor.yml
:040000 040000 d0ac6b0edbf5fe91a4c2d499eb4e2f94ba91b799 47a3eb4dc78afe60b98f2b1f88f1ebd724ce690f M      src
bisect run success

[Veedrac]

Ah dangit, thanks so much for reporting this. I'm guessing the issue is

let mask = !(!0 >> ((utf8_chars.len() % chunksize) * 8));

and I have a patch at PR #54. I'm installing some cross platform testing stuff and should have confirmation soon.

[Veedrac]

Yep, I can repro the issue and the fix works for me. Thanks again!

@llogiq We should CI this.

[llogiq]

Is there a CI that gives us native big-endian machines? Or do we need to setup qemu on a Travis job?

[Veedrac]

https://github.com/japaric/trust looks nice. Looking through it mostly just seems to wrap https://github.com/rust-embedded/cross, which is what I used locally (so maybe just use cross?).

[infinity0]

The fix works for me over here too, thanks for the quick response!

[llogiq]

version 0.5.1 with the fix will be published soon.

[llogiq]

0.5.1 is published to crates.io.

[BurntSushi]

Both the memchr and byteorder crates have a CI target that uses a big endian target, so you could reuse that.

…

On Sun, Feb 10, 2019, 04:52 llogiq ***@***.*** wrote: version 0.5.1 with the fix will be published soon. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#53 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAb34kgCBbhGAI5PGAnLEN7UWBXiMyDXks5vL-vjgaJpZM4ayvZg> .

[llogiq]

What architectures should we add to CI?

[BurntSushi]

I used mips, but any big-endian arch should be fine. https://github.com/BurntSushi/byteorder/blob/64c03fb383289f8e42b2892bf72297879777a238/.travis.yml#L8

[infinity0]

ppc64 and s390x if available are typically much faster than mips.

[infinity0]

Looking at that travis patch, does CROSS_TARGET actually run the tests? I would not have normally expected so... the bad code does compile successfully, but the test fails.

[alex]

FWIW, travis recently gained s390x support, so testing there should be easy-ish.