Skip to content

The extern "C" ABI of SIMD vector types depends on target features (tracking issue for abi_unsupported_vector_types future-incompatibility lint) #116558

Open
1 of 5 issues completed
@RalfJung

Description

The following program has UB, for very surprising reasons:

use std::mem::transmute;
#[cfg(target_arch = "x86")]
use std::arch::x86::*;
#[cfg(target_arch = "x86_64")]
use std::arch::x86_64::*;

extern "C" fn no_target_feature(_dummy: f32, x: __m256) {
    let val = unsafe { transmute::<_, [u32; 8]>(x) };
    dbg!(val);
}

#[target_feature(enable = "avx")]
unsafe fn with_target_feature(x: __m256) {
  // Here we're seemingly just calling a safe function... but actually this call is UB!
  // Caller and callee do not agree on the ABI; specifically they disagree on how the
  // `__m256` argument gets passed.
  no_target_feature(0.0, x);
}

fn main() {
    assert!(is_x86_feature_detected!("avx"));
    // SAFETY: we checked that the `avx` feature is present.
    unsafe {
        with_target_feature(transmute([1; 8]));
    }
}

The reason is that the ABI of the __m256 type depends on the set of target features, so the caller (with_target_feature) and callee (no_target_feature) do not agree on how the argument should be passed. The result is a vector half-filled with junk. (The same issue also arises in the other direction, where the caller has fewer features than the callee: example here.)

Currently, we have no good way to do correct code generation here. See #132865 for a discussion of how we could support such code in the future; it will require some non-trivial work. So instead, the current plan is to reject such code entirely.

This is the tracking issue for the lint that moves us in that direction. The hope is that passing SIMD vectors across a C ABI is sufficiently rare, and most of the cases being rejected have anyway already been broken, that we can reject this without much of an ecosystem impact. Crater showed no regressions.

This is linted against since Rust 1.84, and shown in future breakage reports starting with Rust 1.85. The plan is to move to a hard error with Rust 1.87.

When this becomes a hard error:


Original issue text:

I'm not sure if this is currently even properly documented? We are mentioning it in #115476 but the program above doesn't involve any function pointers so we really cannot expect people to be aware of that part of the docs. We show a general warning about the type not being FFI-compatible, but that warning shows up a lot and anyway in this case both caller and callee are Rust functions!

I think we need to do better here, but backwards compatibility might make that hard. @chorman0773 suggested we should just reject functions like no_target_feature that take an AVX type by-val without having declared the AVX feature. That seems reasonable; a crater run would be needed to assess whether it breaks too much code. An alternative might be to have a deny-by-default lint that very clearly explains what is happening.

There are also details to work out wrt what exactly the lint should check. Newtypes around __m256 will have the same problem. What about other larger types that contain __m256? Behind a ptr indirection it's obviously fine, but what about (__m256, __m256)? If we apply the ScalarPair optimization this will be passed in registers even on x86.

A possible place to put the check could be somewhere around here.

In terms of process, I am not sure if an RFC is required; a t-compiler MCP might be sufficient. Currently we accept code that clearly doesn't do what it looks like it should do.

And finally -- are there any other targets (besides x86 and x86-64) that have target-features that affect the ABI? They should get the same treatment.

Note that this is different from #116344 in two ways:

  • This issue only affects non-"Rust" ABIs; the other issue affects even the "Rust" ABI. (The Rust ABI passes __m256 by-pointer exactly to work-around this issue.)
  • This issue comes about when the user enables extra features (which is possible also locally via #[target_feature]), the other issue comes about when the user disables features (which is only possible on a per-crate level via -C, and if you're mixing crates with different -C flags then you're already already on very shaky grounds -- we do that with std but nobody else really gets to do that, I think).

Cc @workingjubilee more ABI fun ;)

Sub-issues

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    A-ABIArea: Concerning the application binary interface (ABI)A-SIMDArea: SIMD (Single Instruction Multiple Data)A-target-featureArea: Enabling/disabling target features like AVX, Neon, etc.C-tracking-issueCategory: An issue tracking the progress of sth. like the implementation of an RFCF-simd_ffi`#![feature(simd_ffi)]`I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessL-abi_unsupported_vector_typesLint: abi_unsupported_vector_typesT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.T-opsemRelevant to the opsem team

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions