Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Implement RFC2574 for FFI declarations #59238

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 96 additions & 20 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2256,6 +2256,95 @@ fn predicates_from_bound<'tcx>(
}
}

/// Returns the minimum required target-feature name to use a SIMD type on C FFI:
fn simd_ffi_min_target_feature(simd_width: usize) -> Option<&'static str> {
// FIXME: this needs to be architecture dependent and
// should probably belong somewhere else:
// * on arm: 8 => neon
// * on aarch64: 8 | 16 => neon,
// * on mips: 16 => msa,
// * on ppc: 16
// if vec elem is 64-bit wide => vsx
// otherwise => altivec
// * wasm: 16 => simd128
match simd_width {
16 => Some("sse"),
32 => Some("avx"),
64 => Some("avx512"),
_ => None,
}
}

/// Returns true if the target-feature allows using the SIMD type on C FFI:
fn simd_ffi_feature_check(simd_width: usize, feature: &'static str) -> bool {
// FIXME: see simd_ffi_min_target_feature
match simd_width {
16 => feature.contains("sse") || feature.contains("ssse") || feature.contains("avx"),
32 => feature.contains("avx"),
64 => feature.contains("avx512"),
_ => false,
}
}

fn simd_ffi_check<'a, 'tcx, 'b: 'tcx>(
tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId, ast_ty: &hir::Ty, ty: Ty<'b>,
attrs: &mut Option<CodegenFnAttrs>
) {
if !ty.is_simd() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that this won't handle I think is:

#[repr(transparent)]
struct MyType(__m128i);

but is that perhaps best left for a future implementation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not even sure if this is something we properly gate today

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, and no, we don't gate this today:

use std::arch::x86_64::__m128;

#[allow(improper_ctypes)]
extern "C" {
    //fn e(x: __m128); // ERROR
    pub fn a(x: A);
    pub fn b(x: B);
}

#[repr(transparent)] pub struct A(__m128);
#[repr(C)] pub struct B(__m128);

both of these are allowed on stable Rust, and both should be rejected.

Copy link
Contributor Author

@gnzlbg gnzlbg Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also should gate foreign function definitions:

use std::arch::x86_64::__m128;

pub extern "C" fn foo(x: __m128) -> __m128 { x }

These also work on stable Rust.

I'll try to fix all of these issues as part of this PR. We can do a crater run afterwards, and see if we need to change any of these to warn instead of error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me!

return;
}

// The use of SIMD types in FFI is feature-gated:
if !tcx.features().simd_ffi {
tcx.sess
.struct_span_err(
ast_ty.span,
&format!(
"use of SIMD type `{}` in FFI is unstable",
tcx.hir().hir_to_pretty_string(ast_ty.hir_id)
),
)
.help("add #![feature(simd_ffi)] to the crate attributes to enable")
.emit();
return;
}

if attrs.is_none() {
*attrs = Some(codegen_fn_attrs(tcx, def_id));
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
}

// Skip LLVM intrinsics:
if let Some(link_name) = attrs.as_ref().unwrap().link_name {
if link_name.as_str().get().starts_with("llvm.") {
return;
}
}

let features = &attrs.as_ref().unwrap().target_features;
let simd_len = tcx.layout_of(ty::ParamEnvAnd{
param_env: ty::ParamEnv::empty(),
value: ty,
}).unwrap().details.size.bytes() as usize;

if !features.iter().any(|f| simd_ffi_feature_check(simd_len, f.as_str().get())) {
let type_str = tcx.hir().hir_to_pretty_string(ast_ty.hir_id);
let error_msg = if let Some(min_feature) = simd_ffi_min_target_feature(simd_len) {
format!(
"use of SIMD type `{}` in FFI requires `#[target_feature(enable = \"{}\")]`",
type_str, min_feature,
)
} else {
format!(
"use of SIMD type `{}` in FFI not supported by any target features",
type_str
)
};
tcx.sess
.struct_span_err(ast_ty.span, &error_msg)
.emit();
}
}

fn compute_sig_of_foreign_fn_decl<'a, 'tcx>(
tcx: TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId,
Expand All @@ -2269,32 +2358,19 @@ fn compute_sig_of_foreign_fn_decl<'a, 'tcx>(
};
let fty = AstConv::ty_of_fn(&ItemCtxt::new(tcx, def_id), unsafety, abi, decl);

// feature gate SIMD types in FFI, since I (huonw) am not sure the
// ABIs are handled at all correctly.
// Using SIMD types in FFI signatures requires the
// signature to have appropriate `#[target_feature]`s enabled.
if abi != abi::Abi::RustIntrinsic
&& abi != abi::Abi::PlatformIntrinsic
&& !tcx.features().simd_ffi
{
let check = |ast_ty: &hir::Ty, ty: Ty<'_>| {
if ty.is_simd() {
tcx.sess
.struct_span_err(
ast_ty.span,
&format!(
"use of SIMD type `{}` in FFI is highly experimental and \
may result in invalid code",
tcx.hir().hir_to_pretty_string(ast_ty.hir_id)
),
)
.help("add #![feature(simd_ffi)] to the crate attributes to enable")
.emit();
}
};
// Compute function attrs at most once per FFI decl.
let mut attrs = None;

for (input, ty) in decl.inputs.iter().zip(*fty.inputs().skip_binder()) {
check(&input, ty)
simd_ffi_check(tcx, def_id, &input, ty, &mut attrs)
}
if let hir::Return(ref ty) = decl.output {
check(&ty, *fty.output().skip_binder())
simd_ffi_check(tcx, def_id, &ty, *fty.output().skip_binder(), &mut attrs)
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/feature-gates/feature-gate-simd-ffi.stderr
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
error: use of SIMD type `LocalSimd` in FFI is highly experimental and may result in invalid code
error: use of SIMD type `LocalSimd` in FFI is unstable
--> $DIR/feature-gate-simd-ffi.rs:9:17
|
LL | fn baz() -> LocalSimd;
| ^^^^^^^^^
|
= help: add #![feature(simd_ffi)] to the crate attributes to enable

error: use of SIMD type `LocalSimd` in FFI is highly experimental and may result in invalid code
error: use of SIMD type `LocalSimd` in FFI is unstable
--> $DIR/feature-gate-simd-ffi.rs:10:15
|
LL | fn qux(x: LocalSimd);
Expand Down
54 changes: 54 additions & 0 deletions src/test/ui/simd-ffi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
#![feature(repr_simd, simd_ffi)]
#![allow(non_camel_case_types)]

#[repr(simd)]
struct v1024(i128, i128, i128, i128, i128, i128, i128, i128);

extern {
fn foo(x: v1024); //~ ERROR use of SIMD type `v1024` in FFI not supported by any target-features
fn bar(x: i32, y: v1024); //~ ERROR use of SIMD type `v1024` in FFI not supported by any target-features
fn baz(x: i32) -> v1024; //~ ERROR use of SIMD type `v1024` in FFI not supported by any target-features

fn qux_fail(x: v128); //~ ERROR use of SIMD type `v128` in FFI requires `#[target_feature(enable = "sse")]`
#[target_feature(enable = "sse")]
fn qux(x: v128);
#[target_feature(enable = "sse4.2")]
fn qux2(x: v128);
#[target_feature(enable = "ssse3")]
fn qux3(x: v128);
#[target_feature(enable = "avx")]
fn qux4(x: v128);
#[target_feature(enable = "avx2")]
fn qux5(x: v128);
#[target_feature(enable = "avx512f")]
fn qux6(x: v128);

fn quux_fail(x: v256); //~ ERROR use of SIMD type `v256` in FFI requires `#[target_feature(enable = "avx")]`
#[target_feature(enable = "sse4.2")]
fn quux_fail2(x: v256); //~ ERROR use of SIMD type `v256` in FFI requires `#[target_feature(enable = "avx")]`
#[target_feature(enable = "avx")]
fn quux(x: v256);
#[target_feature(enable = "avx2")]
fn quux2(x: v256);
#[target_feature(enable = "avx512f")]
fn quux3(x: v256);

fn quuux_fail(x: v512); //~ ERROR use of SIMD type `v256` in FFI requires `#[target_feature(enable = "avx512")]`
#[target_feature(enable = "sse")]
fn quuux_fail2(x: v512); //~ ERROR use of SIMD type `v256` in FFI requires `#[target_feature(enable = "avx512")]`
#[target_feature(enable = "avx2")]
fn quuux_fail3(x: v512); //~ ERROR use of SIMD type `v256` in FFI requires `#[target_feature(enable = "avx512")]`
#[target_feature(enable = "avx512f")]
fn quuux(x: v512);
}

fn main() {}

#[repr(simd)]
struct v128(i128);

#[repr(simd)]
struct v256(i128, i128);

#[repr(simd)]
struct v512(i128, i128, i128, i128);
20 changes: 20 additions & 0 deletions src/test/ui/simd-ffi.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
error: use of SIMD type `v1024` in FFI not supported by any target features
--> $DIR/simd-ffi.rs:8:15
|
LL | fn foo(x: v1024);
| ^^^^^

error: use of SIMD type `v1024` in FFI not supported by any target features
--> $DIR/simd-ffi.rs:9:23
|
LL | fn bar(x: i32, y: v1024);
| ^^^^^

error: use of SIMD type `v1024` in FFI not supported by any target features
--> $DIR/simd-ffi.rs:10:23
|
LL | fn baz(x: i32) -> v1024;
| ^^^^^

error: aborting due to 3 previous errors