cpufeatures: Enable compile time target features under Miri

[elichai]

Miri already supports a lot of intrinsics: https://github.com/rust-lang/miri/tree/master/src/intrinsics so I think it make sense to return true for compile time enabled features, so if someone runs RUSTFLAGS=+avx2 cargo miri run it will actually enable those features, as the user explicitly asked for them

Also added tests to show that it works outside and inside of miri

tested everything locally too(including loongarch64 via qemu)

[tarcieri]

So, while this would be interesting, seems it's a breaking change: RustCrypto/stream-ciphers#579

We can consider it for cpufeatures v0.4 but for now we should revert.

I went ahead and yanked cpufeatures v0.3.1 which included this PR.

[newpavlov]

As I wrote here, it's probably an issue in the chacha20 crate, so we probably can un-yank the release.

[tarcieri]

@newpavlov but code that previously built under miri (because the gated backend was disabled) will now break.

That’s a breaking change. I think it should be cfg gated for now at least.

chacha20 is a widely used crate and we’ll break everyone’s build until it’s fixed.

[newpavlov]

we’ll break everyone’s build until it’s fixed

It breaks build only for people who use Miri, and only old versions of it at that. So IMO the impact is pretty limited, especially considering that the breakage is caused by a genuine UB. So arguably it's not a "breaking change", but a change which has exposed previously existing UB in downstream code, so if anything the breakage speaks in favor of this PR.

I am fine with postponing un-yanking until we release a fixed version of chacha20, but I believe it should be done in the near future.

[tarcieri]

I'm fine with giving it another go after chacha20 is fixed but if we get more reports of problems after that we should probably at least cfg gate it until the next breaking release (which could happen immediately if we wanted it to, no reason not to bump the version)

[newpavlov]

I don't see how a breaking release of cpufeatures would help. Let's say a new patch release of chacha20 migrates to cpufeatures v0.4 with this change, the end result would still be the same.

We could've considered it a breaking change if Miri did not support intrinsics for target features enabled by default, but IIUC it's not the case today. I agree that we should decide based on reports, but I don't expect much trouble.

[tarcieri]

A minor version bump to v0.4 would ensure that all code which builds with cpufeatures v0.3.0 on Miri continues to build, even if it contains problems that should ultimately be fixed.

I'm worried about things like blocking CI workflows of major downstream crates, such as uuid in the case of RustCrypto/stream-ciphers#579.

[newpavlov]

A minor version bump to v0.4 would ensure that all code which builds with cpufeatures v0.3.0 on Miri continues to build,

From a downstream crate perspective, I don't see any difference between pinning cpufeatures to v0.3.0 or pinning chacha20 to v0.10.1. In both cases it would "fix" the Miri build and updating dependencies would result in a "breakage".

I opened RustCrypto/stream-ciphers#580 to the fix the root issue, so we should proceed with its release and un-yanking of cpufeatures following it.

[tarcieri]

I'm not suggesting any "pinning" on the part of the end user.

Instead making it a breaking change means we can individually update our crates to the new breaking version after we've confirmed they build under Miri with the various backends, without worrying about it causing downstream breakage in the interim.

[newpavlov]

chacha20 v0.10.2 with the fix is published, so cpufeatures v0.3.1 can be un-yanked.

after we've confirmed they build under Miri with the various backends

You do not work around UB in downstream by demanding upstream dependencies to do breaking releases on each change which potentially breaks incorrect unsafe code (unless behavior of safe code was explicitly bromised). It could've been a different story if breakage has happened in a correct code.

And I don't think we need to check that Miri works with every supported backend, but only those which require enabled by default target features (like SSE2 on x86 targets).

[tarcieri]

If the code compiled under one version, and you release something that's supposed to be SemVer compatible, and it no longer compiles, that's a breaking change. Period.

Note that the previous Miri behavior was to fall back to the pure Rust version, which works. This PR switches Miri to trying to use the SIMD backend which doesn't compile. So it definitely changes things from "working" to "broken" in what's supposed to be a SemVer-compatible update.

Despite this being UB, having it cause new compile errors under Miri and only Miri in what's supposed to be a SemVer compatible release doesn't help downstream users. It's breaking under Miri runs for people who are trying to run Miri their own projects and pulling in chacha20 for an RNG. Miri users generally aren't going to be production users of cryptography, and the UB will still be there in release builds, so having it break isn't really helping in such a case except for discovering these issues, which we can also do in CI in a more careful manner if we do breaking cpufeatures updates.

We should absolutely fix the issues, but in the interim we shouldn't be breaking people's builds.