Miri detected possible UB with CPU feature flags for x86

[mothran]

When running miri on a project that imported crc_fast I notice a possible UB due to missing checks of target features between caller / callee. I am not sure if this UB can be triggered but figured I would file the issue just in case. Thanks for the great library!

Repoduction:

rustup +nightly component add miri
cargo +nightly miri test test_combine

Output:

running 2 tests
test lib::test_combine ... error: Undefined Behavior: calling a function that requires unavailable target features: ssse3, sse4.1, pclmulqdq
    --> src/structs.rs:38:18
     |
  38 |         unsafe { arch::update(state, data, params) }
     |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
     |
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: BACKTRACE on thread `lib::test_combi`:
     = note: inside `<structs::Calculator as traits::CrcCalculator>::calculate` at src/structs.rs:38:18: 38:51
note: inside `checksum`
    --> src/lib.rs:592:5
     |
 592 |     calculator(params.init, buf, params) ^ params.xorout
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `lib::test_combine`
    --> src/lib.rs:1048:29
     |
1048 |             let checksum1 = checksum(algorithm, "1234".as_ref());
     |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure
    --> src/lib.rs:1042:22
     |
1041 |     #[test]
     |     ------- in this attribute macro expansion
1042 |     fn test_combine() {
     |                      ^

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

error: test failed, to rerun pass `--lib`

[onethumb]

Thanks for the bug report. Using 1.6.0 this behavior has changed, and will "correctly" panic when hardware intrinsics should be present, but are not. Is this sufficient?

I'm not familiar with miri so I'm not quite sure what the intent is, but reading through the bug reports, it's pretty clear that it doesn't handle hardware intrinsics well, especially on platforms like aarch64 (and x86_64 support seems recent and spotty), so I'm not sure what "success looks like" for a heavily hardware accelerated project like this, but if this isn't enough, I'd love to know more.

Or, better yet, submit a PR. :)

[saethlin]

I'm not familiar with miri so I'm not quite sure what the intent is

The intent is to test unsafe code, like this repo.

but reading through the bug reports, it's pretty clear that it doesn't handle hardware intrinsics well, especially on platforms like aarch64 (and x86_64 support seems recent and spotty)

Yup, every intrinsic needs to be hand-implemented.

so I'm not sure what "success looks like" for a heavily hardware accelerated project like this, but if this isn't enough, I'd love to know more.

Success looks like not encountering this UB. The quality of Miri's intrinsic implementations isn't even relevant here, because target_feature doesn't rely on intrinsics. They're separate parts of the interpreter.

I found this issue because I am currently debugging a case where on version 1.4.0, this code:

use crc_fast::{Digest, CrcAlgorithm};
fn main() {
    let mut digest = Digest::new(CrcAlgorithm::Crc64Nvme);
    digest.update(&[0]);
}

will try to execute AVX-512 instructions on CPUs that don't support those. I suspect this is related to the UB that Miri is reporting.

I've run the test suite on the tip of main and at least the test suite still reports target feature mismatches.

[saethlin]

Oh I see, in part I'm rediscovering #14

[onethumb]

@mothran @saethlin I'd appreciate some 👀 on #26, which should resolve this, and other Miri issues, if you don't mind and have the time. Thanks! 👍

[mothran]

@onethumb can confirm that PR completes the original tests + miri successfully for me, thanks!

[onethumb]

Seems to be resolved with #26 👍