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.

[newpavlov]

you release something that's supposed to be SemVer compatible, and it no longer compiles, that's a breaking change. Period.

Nope. There are various carve outs for this naive rule. For example, if you add const { assert_eq!(size_of::<Foo>(), 16) } to your crate, by default it's not my fault if your build gets broken after I change size of Foo in an upstream crate to something else. The Rust compiler also explicitly reserves the right to break builds for things like soundness issues. The code containing UB also could break builds at any moment with a new Miri release.

In this particular case the change did the intended job. I am fine with taking measures to limit the impact (note that it was already extremely limited), but moving this change into a new breaking release is IMO taking it a bit too far. Instead of yanking cpufeatures v0.3.1 I believe we should've fixed chacha20 and cut a new release ASAP.

[newpavlov]

Please note that I went ahead and un-yanked v0.3.1.

[tarcieri]

@newpavlov in this case, it's changing the backend selected on Miri from a working pure Rust one to a broken one that doesn't even compile on Miri.

That's not the kind of breaking change you can defensibly make an exception for. It took a good implementation for that particular target and switched it out for a broken one that doesn't even compile.

[newpavlov]

It took a good implementation for that particular target and switched it out for a broken one that doesn't even compile.

I disagree. We expanded what Miri can check in our crates and it has helped to find UB. Both Miri and this change did their intended job. The broken CI build is not a bad thing, quite an opposite, it did CI's job and notified us about the lurking issue. And a somewhat serious one at that. I believe this firmly falls under the soundness issue exception.

With your stance new Miri releases which expand scope of checks (e.g. imagine if runtime target feature detection is added) would be also "breaking".

[newpavlov]

cc @RalfJung

[tarcieri]

Alright, it's a reasonable argument that Miri is nightly-only and might change what is considered UB in the future, and therefore it's unreasonable to expect stability guarantees.

I still think we could ensure all of our code is tested and working under Miri with this change first before we unleash it on the world. That said, if chacha20 winds up being the only issue in practice I'm fine with the change.

If more issues pop up, perhaps we should reconsider.

[RalfJung]

The comment seems outdated now.

[RalfJung]

cc @RalfJung

Sorry, what's the question?

[RalfJung]

The std macro honors statically enabled target features under Miri so IMO it makes sense for this crate to do the same.

[newpavlov]

@RalfJung
I wanted your input on:

1. Should this change be considered breaking or not?
2. Does Miri implement support for all intrinsics gated on enabled-by-default target features? Unfortunately, it does not seem so, see Disable AES intrinsics on aarch64 Miri block-ciphers#585. Ideally, it would be nice to have a full list of not-yet-implemented such features.

[RalfJung]

Should this change be considered breaking or not?

I think that's up to the crate maintainers. I view it as bringing cpufeatures in line with the std macro, but I can see how it could be annoying for downstream users. std macro consumers had to deal with that already and I think typically crates just carry their own cfg(miri) then, to disable e.g. the aarch64.aes codepaths in Miri. So IMO that is the right fix for RustCrypto/block-ciphers#585: if the block-ciphers crate wants to work in Miri on aarch64 it should have a cfg (maybe with a FIXME pointing to a Miri issue tracking support for the missing intrinsics).

If you want to take a more conservative stance, you could do this change only for x86-32/64 where we have much better intrinsic coverage in Miri. Maybe that's more appropriate for a semver-compatible bump. But given the std macro precedent that could cause its own confusion.

Does Miri implement support for all intrinsics gated on enabled-by-default target features? Unfortunately, it does not seem so, see RustCrypto/block-ciphers#585. Ideally, it would be nice to have a full list of not-yet-implemented such features.

I don't know enough about which target features even exist, let alone which ones are enabled by default where, to provide such a list. Generally we have pretty good support on x86 and barely anything for other targets (though aarch64 has been getting better). But even on x86 I don't know how I'd even figure out whether we support everything that's enabled by default on any (tier 1?) target. And obviously with -Ctarget-cpu you can easily enable target features Miri does not support even on x86 (our AVX512 support is very incomplete).

[tarcieri]

So RustCrypto/block-ciphers#585 was exactly the sort of thing I was worried about.

I think we really need to roll this change out more gradually in CI instead of using our end users as testers to discover where these gaps exist.

I would continue to suggest this change either be made cfg gated in cpufeatures v0.3 and/or rolled out in cpufeatures v0.4 so we can turn it on for ourselves in CI to e.g. discover UB in the SIMD backends where Miri supports the relevant intrinsics, but I don't think it makes sense to make it on-by-default after we've already cut stable releases of crates. We can get the benefits of this change without causing downstream breakages for our users.

In general with crates like cpufeatures (or even cpubits) I think we need to be very careful making changes like this after stable releases to avoid breakages.

[newpavlov]

I fixed aes in RustCrypto/block-ciphers#586, IMO we should do the same for any other similar breakage in future. We do not break any production builds with this, so impact is relatively small.

[tarcieri]

RustCrypto/block-ciphers#588 is another example, and it seems we're going to keep playing whack-a-mole with these for awhile.

I think that issue in particular is a good example of why cfg-gating this is a good idea. @RustCrypto developers are the only ones who actually care about this and benefit from it. Everyone else just wants their code to build, and this is just getting in the way.

I don't understand the opposition to cfg gating. We should really validate this stuff before setting it loose in the wild and breaking a bunch of people's builds.

cc @nazar-pc