mark x86 intrinsics as safe

[usamoi]

This patch marks all x86 intrinsics that do not operate on registers or memory as safe.

ACP: rust-lang/libs-team#494

[rustbot]

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Amanieu]

Thanks!

Could you open a draft PR on rust-lang/rust and point the stdarch submodule to your branch? Considering the scope of the change, we would like to do a crater run to check for any breakage this may cause in the ecosystem.

[veluca93]

Thanks for this PR @usamoi ! I was going to do it but you beat me to it ;)

[Amanieu]

LGTM but this needs to be rebased due to conflicts.

[usamoi]

if I may ask, how was the list of "safe" intrinsics determined? I am slightly concerned about this introducing a subtle soundness issue by marking too make things as safe.

The parameters of intrinsics are __m* or primitive types, excluding intrinsics which operate registers (_MM_SET_* or _mm256_zeroupper).

[Amanieu]

These are purely SIMD arithmetic intrinsics. Any intrinsics that access memory via a pointer operand were not made safe.

[bjorn3]

There are vendor intrinsics which have UB on inputs like NaN or 0, right?

[RalfJung]

The parameters of intrinsics are __m* or primitive types, excluding intrinsics which operate registers (MM_SET* or _mm256_zeroupper).

How was the list of "intrinsics which operate on registers" determined?

These are purely SIMD arithmetic intrinsics. Any intrinsics that access memory via a pointer operand were not made safe.

Yes I understand. But there's thousands of them, so I assume this was done semi-automatically, and that seems to make it fairly easy to accidentally mark something as safe that shouldn't be safe. Or did I misunderstand?

[RalfJung]

Also, AFAIK _mm256_zeroupper is actually completely safe. I was told it is a NOP at the abstract machine level; the compiler is responsible for preserving the contents of all local variables across such a call. If that's not true we need to fix/remove the Miri implementation.

[usamoi]

How was the list of "intrinsics which operate on registers" determined?

I checked Intel intrinsics guide, and only _MM_SET_* mentions that it writes MXCSR.

Also, AFAIK _mm256_zeroupper is actually completely safe.

I'm not sure if it's safe so it's not marked safe.

[RalfJung]

I checked Intel intrinsics guide, and only MM_SET* mentions that it writes MXCSR.

Well we also had the non-temporal store debacle for the "streaming" intrinsics which turned out to be incredibly unsafe to the extent that they break the memory model itself, but Intel doesn't discuss such things. So I would not trust Intel when it comes to figuring out whether an intrinsic is safe in the context of a language with its own abstract semantics, which is different from describing what the intrinsic does "on the assembly level".

I'm not sure if it's safe so it's not marked safe.

That's the right default. :)
But I think this one is safe, so I opened #1736.

[RalfJung]

@usamoi so can you please describe the mechanical process you use to assemble the list of safe intrinsics? Is there a script you wrote? Did you hand-audit every one of these thousands of intrinsics? This kind of information seems quite relevant for judging the potential risks of a change like that for Rust. After all, even just one incorrectly-safe intrinsic is an I-unsound issue, one of the worst issues we can have in Rust. We should be quite sure to prevent that from happening.

[usamoi]

@usamoi so can you please describe the mechanical process you use to assemble the list of safe intrinsics? Is there a script you wrote? Did you hand-audit every one of these thousands of intrinsics? This kind of information seems quite relevant for judging the potential risks of a change like that for Rust.

All intrinsics except AVX512 are marked safe manually, referring to Intel intrinsics guide. AVX512 intrinsics are marked safe by a script and I checked the list manually.

[RalfJung]

There are vendor intrinsics which have UB on inputs like NaN or 0, right?

I checked the Miri implementation and none of the x86 intrinsics use throw_ub. So there's at least no obvious smoking gun in the subset that we implemented.

[RalfJung]

simd_div is an example of an intrinsic with a value-based precondition -- div-by-0 is UB. But only for integers. All uses of that in stdarch seem to be about floats so we're fine on that one, but this is quite subtle as it depends on the type of the argument passed to the intrinsics.

All intrinsics except AVX512 are marked safe manually, referring to Intel intrinsics guide. AVX512 intrinsics are marked safe by a script and I checked the list manually.

I see, thanks. Was it the same for the other 2 PRs?

Note that some (many?) intrinsics are implemented by invoking simd_* intrinsics, which are not directly mapping to hardware instructions and can have their own precondition. Some intrinsics don't involve pointers and still have special requirements:

• simd_div, simd_rem on integers: no div-by-0
• simd_shl, simd_shr: no shift-larger-than-bitwidth
• simd_cast on float-to-int casts acts like to_int_unchecked
• simd_reduce_all, simd_reduce_any: all vector elements must be 0 or !0
• simd_bitmask, simd_select: all mask elements must be 0 or !0
• simd_select_bitmask requires the "extra" bits in the mask to be 0

[RalfJung]

I found at least one case that contradicts our documented simd_* intrinsic preconditions: rust-lang/rust#137942.

Someone should do a proper audit of all uses of these intrinsics in stdarch; I don't have the time to do that currently.

[usamoi]

Was it the same for the other 2 PRs?

1. Arm intrinsics are marked safe manually. It seems that arm intrinsics do not operate a global floating register, and I checked if there are public functions that receives a pointer.
2. RISCV intrinsics whose safety docs said This function is safe to use if the target feature is present, are marked safe manually.

[RalfJung]

It seems that arm intrinsics do not operate a global floating register

I'm just concerned there may be other odd things these intrinsics do. Hardware vendors love to add weird stuff to their intrinsics (e.g. non-temporal stores, or magic global state affecting how other instructions behave) and then expose them in C as "does what the hardware does", but that's just not even a valid statement and blatantly unsound. So far at least only unsafe Rust code was at risk of being exposed to such nonsense behavior, hence I am quite concerned about the extra "attack surface" we are exposing by making these intrinsics all safe...

For the purely arithmetic ones, I agree they should be safe. We just have to be careful how they are implemented, since some may use Rust intrinsics that have preconditions which do not exist in x86.