From: Michael Bradshaw Date: Wed, 22 May 2019 13:49:43 +0000 (-0700) Subject: Allow null-pointer-optimized enums in FFI if their underlying representation is FFI... X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=a31dc8e3b153ac3073f9fb14d8e523a350fe10f2;p=rust.git Allow null-pointer-optimized enums in FFI if their underlying representation is FFI safe This allows types like Option to be used in FFI without triggering the improper_ctypes lint. This works by changing the is_repr_nullable_ptr function to consider an enum E to be FFI-safe if: - E has no explicit #[repr(...)]. - It only has two variants. - One of those variants is empty (meaning it has no fields). - The other variant has only one field. - That field is one of the following: - &T - &mut T - extern "C" fn - core::num::NonZero* - core::ptr::NonNull - #[repr(transparent)] struct wrapper around one of the types in this list. - The size of E and its field are both known and are both the same size (implying E is participating in the nonnull optimization). --- diff --git a/src/libcore/num/mod.rs b/src/libcore/num/mod.rs index 562a7a4b3c7..932c0eaa4c7 100644 --- a/src/libcore/num/mod.rs +++ b/src/libcore/num/mod.rs @@ -50,6 +50,7 @@ macro_rules! nonzero_integers { #[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] + #[cfg_attr(not(stage0), rustc_nonnull_optimization_guaranteed)] pub struct $Ty($Int); } diff --git a/src/libcore/ptr.rs b/src/libcore/ptr.rs index 006b1e143ee..4bb4d3ee466 100644 --- a/src/libcore/ptr.rs +++ b/src/libcore/ptr.rs @@ -2938,6 +2938,7 @@ fn from(p: NonNull) -> Self { #[stable(feature = "nonnull", since = "1.25.0")] #[repr(transparent)] #[rustc_layout_scalar_valid_range_start(1)] +#[cfg_attr(not(stage0), rustc_nonnull_optimization_guaranteed)] pub struct NonNull { pointer: *const T, } diff --git a/src/librustc_lint/types.rs b/src/librustc_lint/types.rs index 38b6e2c1979..ac18e131c4a 100644 --- a/src/librustc_lint/types.rs +++ b/src/librustc_lint/types.rs @@ -1,10 +1,11 @@ #![allow(non_snake_case)] use rustc::hir::{ExprKind, Node}; +use crate::hir::def_id::DefId; use rustc::hir::lowering::is_range_literal; use rustc::ty::subst::SubstsRef; use rustc::ty::{self, AdtKind, ParamEnv, Ty, TyCtxt}; -use rustc::ty::layout::{self, IntegerExt, LayoutOf, VariantIdx}; +use rustc::ty::layout::{self, IntegerExt, LayoutOf, VariantIdx, SizeSkeleton}; use rustc::{lint, util}; use rustc_data_structures::indexed_vec::Idx; use util::nodemap::FxHashSet; @@ -14,11 +15,11 @@ use std::cmp; use std::{i8, i16, i32, i64, u8, u16, u32, u64, f32, f64}; -use syntax::{ast, attr}; +use syntax::{ast, attr, source_map}; use syntax::errors::Applicability; +use syntax::symbol::sym; use rustc_target::spec::abi::Abi; use syntax_pos::Span; -use syntax::source_map; use rustc::hir; @@ -522,42 +523,79 @@ enum FfiResult<'tcx> { }, } +fn is_zst<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, did: DefId, ty: Ty<'tcx>) -> bool { + tcx.layout_of(tcx.param_env(did).and(ty)).map(|layout| layout.is_zst()).unwrap_or(false) +} + +fn ty_is_known_nonnull<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, ty: Ty<'tcx>) -> bool { + match ty.sty { + ty::FnPtr(_) => true, + ty::Ref(..) => true, + ty::Adt(field_def, substs) if field_def.repr.transparent() && field_def.is_struct() => { + for field in &field_def.non_enum_variant().fields { + let field_ty = tcx.normalize_erasing_regions( + ParamEnv::reveal_all(), + field.ty(tcx, substs), + ); + if is_zst(tcx, field.did, field_ty) { + continue; + } + + let attrs = tcx.get_attrs(field_def.did); + if attrs.iter().any(|a| a.check_name(sym::rustc_nonnull_optimization_guaranteed)) || + ty_is_known_nonnull(tcx, field_ty) { + return true; + } + } + + false + } + _ => false, + } +} + /// Check if this enum can be safely exported based on the /// "nullable pointer optimization". Currently restricted -/// to function pointers and references, but could be -/// expanded to cover NonZero raw pointers and newtypes. +/// to function pointers, references, core::num::NonZero*, +/// core::ptr::NonNull, and #[repr(transparent)] newtypes. /// FIXME: This duplicates code in codegen. fn is_repr_nullable_ptr<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, - def: &'tcx ty::AdtDef, + ty: Ty<'tcx>, + ty_def: &'tcx ty::AdtDef, substs: SubstsRef<'tcx>) -> bool { - if def.variants.len() == 2 { - let data_idx; + if ty_def.variants.len() != 2 { + return false; + } - let zero = VariantIdx::new(0); - let one = VariantIdx::new(1); + let get_variant_fields = |index| &ty_def.variants[VariantIdx::new(index)].fields; + let variant_fields = [get_variant_fields(0), get_variant_fields(1)]; + let fields = if variant_fields[0].is_empty() { + &variant_fields[1] + } else if variant_fields[1].is_empty() { + &variant_fields[0] + } else { + return false; + }; - if def.variants[zero].fields.is_empty() { - data_idx = one; - } else if def.variants[one].fields.is_empty() { - data_idx = zero; - } else { - return false; - } + if fields.len() != 1 { + return false; + } - if def.variants[data_idx].fields.len() == 1 { - match def.variants[data_idx].fields[0].ty(tcx, substs).sty { - ty::FnPtr(_) => { - return true; - } - ty::Ref(..) => { - return true; - } - _ => {} - } - } + let field_ty = fields[0].ty(tcx, substs); + if !ty_is_known_nonnull(tcx, field_ty) { + return false; } - false + + // At this point, the field's type is known to be nonnull and the parent enum is Option-like. + // If the computed size for the field and the enum are different, the nonnull optimization isn't + // being applied (and we've got a problem somewhere). + let compute_size_skeleton = |t| SizeSkeleton::compute(t, tcx, ParamEnv::reveal_all()).unwrap(); + if !compute_size_skeleton(ty).same_size(compute_size_skeleton(field_ty)) { + bug!("improper_ctypes: Option nonnull optimization not applied?"); + } + + true } impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> { @@ -612,14 +650,8 @@ fn check_type_for_ffi(&self, ); // repr(transparent) types are allowed to have arbitrary ZSTs, not just // PhantomData -- skip checking all ZST fields - if def.repr.transparent() { - let is_zst = cx - .layout_of(cx.param_env(field.did).and(field_ty)) - .map(|layout| layout.is_zst()) - .unwrap_or(false); - if is_zst { - continue; - } + if def.repr.transparent() && is_zst(cx, field.did, field_ty) { + continue; } let r = self.check_type_for_ffi(cache, field_ty); match r { @@ -682,7 +714,7 @@ fn check_type_for_ffi(&self, // discriminant. if !def.repr.c() && def.repr.int.is_none() { // Special-case types like `Option`. - if !is_repr_nullable_ptr(cx, def, substs) { + if !is_repr_nullable_ptr(cx, ty, def, substs) { return FfiUnsafe { ty: ty, reason: "enum has no representation hint", diff --git a/src/libsyntax/feature_gate.rs b/src/libsyntax/feature_gate.rs index 5b1a9bb739f..b27f5b1495c 100644 --- a/src/libsyntax/feature_gate.rs +++ b/src/libsyntax/feature_gate.rs @@ -1134,6 +1134,13 @@ pub fn is_builtin_attr(attr: &ast::Attribute) -> bool { is just used to enable niche optimizations in libcore \ and will never be stable", cfg_fn!(rustc_attrs))), + (sym::rustc_nonnull_optimization_guaranteed, Whitelisted, template!(Word), + Gated(Stability::Unstable, + sym::rustc_attrs, + "the `#[rustc_nonnull_optimization_guaranteed]` attribute \ + is just used to enable niche optimizations in libcore \ + and will never be stable", + cfg_fn!(rustc_attrs))), (sym::rustc_regions, Normal, template!(Word), Gated(Stability::Unstable, sym::rustc_attrs, "the `#[rustc_regions]` attribute \ diff --git a/src/libsyntax_pos/symbol.rs b/src/libsyntax_pos/symbol.rs index 8b07e81e586..b59244283d7 100644 --- a/src/libsyntax_pos/symbol.rs +++ b/src/libsyntax_pos/symbol.rs @@ -491,6 +491,7 @@ rustc_layout_scalar_valid_range_end, rustc_layout_scalar_valid_range_start, rustc_mir, + rustc_nonnull_optimization_guaranteed, rustc_object_lifetime_default, rustc_on_unimplemented, rustc_outlives, diff --git a/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.rs b/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.rs index f7ff3eb3ac9..9f5c92349e0 100644 --- a/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.rs +++ b/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.rs @@ -4,5 +4,6 @@ #[rustc_variance] //~ ERROR the `#[rustc_variance]` attribute is just used for rustc unit tests and will never be stable #[rustc_error] //~ ERROR the `#[rustc_error]` attribute is just used for rustc unit tests and will never be stable +#[rustc_nonnull_optimization_guaranteed] //~ ERROR the `#[rustc_nonnull_optimization_guaranteed]` attribute is just used to enable niche optimizations in libcore and will never be stable fn main() {} diff --git a/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.stderr b/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.stderr index 882feb87f42..ed98484e13c 100644 --- a/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.stderr +++ b/src/test/ui/feature-gates/feature-gate-rustc-attrs-1.stderr @@ -16,6 +16,15 @@ LL | #[rustc_error] = note: for more information, see https://github.com/rust-lang/rust/issues/29642 = help: add #![feature(rustc_attrs)] to the crate attributes to enable -error: aborting due to 2 previous errors +error[E0658]: the `#[rustc_nonnull_optimization_guaranteed]` attribute is just used to enable niche optimizations in libcore and will never be stable + --> $DIR/feature-gate-rustc-attrs-1.rs:7:1 + | +LL | #[rustc_nonnull_optimization_guaranteed] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: for more information, see https://github.com/rust-lang/rust/issues/29642 + = help: add #![feature(rustc_attrs)] to the crate attributes to enable + +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0658`. diff --git a/src/test/ui/lint/lint-ctypes-enum.rs b/src/test/ui/lint/lint-ctypes-enum.rs index f347c2761c3..d3e11d2f7ed 100644 --- a/src/test/ui/lint/lint-ctypes-enum.rs +++ b/src/test/ui/lint/lint-ctypes-enum.rs @@ -1,6 +1,8 @@ #![deny(improper_ctypes)] #![allow(dead_code)] +use std::num; + enum Z { } enum U { A } enum B { C, D } @@ -15,14 +17,39 @@ enum U8 { A, B, C } #[repr(isize)] enum Isize { A, B, C } +#[repr(transparent)] +struct Transparent(T, std::marker::PhantomData); + +struct Rust(T); + extern { fn zf(x: Z); fn uf(x: U); //~ ERROR enum has no representation hint fn bf(x: B); //~ ERROR enum has no representation hint fn tf(x: T); //~ ERROR enum has no representation hint - fn reprc(x: ReprC); - fn u8(x: U8); - fn isize(x: Isize); + fn repr_c(x: ReprC); + fn repr_u8(x: U8); + fn repr_isize(x: Isize); + fn option_ref(x: Option<&'static u8>); + fn option_fn(x: Option); + fn nonnull(x: Option>); + fn nonzero_u8(x: Option); + fn nonzero_u16(x: Option); + fn nonzero_u32(x: Option); + fn nonzero_u64(x: Option); + fn nonzero_u128(x: Option); + //~^ ERROR 128-bit integers don't currently have a known stable ABI + fn nonzero_usize(x: Option); + fn nonzero_i8(x: Option); + fn nonzero_i16(x: Option); + fn nonzero_i32(x: Option); + fn nonzero_i64(x: Option); + fn nonzero_i128(x: Option); + //~^ ERROR 128-bit integers don't currently have a known stable ABI + fn nonzero_isize(x: Option); + fn repr_transparent(x: Option>); + fn repr_rust(x: Option>); //~ ERROR enum has no representation hint + fn no_result(x: Result<(), num::NonZeroI32>); //~ ERROR enum has no representation hint } pub fn main() { } diff --git a/src/test/ui/lint/lint-ctypes-enum.stderr b/src/test/ui/lint/lint-ctypes-enum.stderr index 92f76cfc38a..6b807f48aaa 100644 --- a/src/test/ui/lint/lint-ctypes-enum.stderr +++ b/src/test/ui/lint/lint-ctypes-enum.stderr @@ -1,5 +1,5 @@ error: `extern` block uses type `U` which is not FFI-safe: enum has no representation hint - --> $DIR/lint-ctypes-enum.rs:20:13 + --> $DIR/lint-ctypes-enum.rs:27:13 | LL | fn uf(x: U); | ^ @@ -11,36 +11,64 @@ LL | #![deny(improper_ctypes)] | ^^^^^^^^^^^^^^^ = help: consider adding a #[repr(...)] attribute to this enum note: type defined here - --> $DIR/lint-ctypes-enum.rs:5:1 + --> $DIR/lint-ctypes-enum.rs:7:1 | LL | enum U { A } | ^^^^^^^^^^^^ error: `extern` block uses type `B` which is not FFI-safe: enum has no representation hint - --> $DIR/lint-ctypes-enum.rs:21:13 + --> $DIR/lint-ctypes-enum.rs:28:13 | LL | fn bf(x: B); | ^ | = help: consider adding a #[repr(...)] attribute to this enum note: type defined here - --> $DIR/lint-ctypes-enum.rs:6:1 + --> $DIR/lint-ctypes-enum.rs:8:1 | LL | enum B { C, D } | ^^^^^^^^^^^^^^^ error: `extern` block uses type `T` which is not FFI-safe: enum has no representation hint - --> $DIR/lint-ctypes-enum.rs:22:13 + --> $DIR/lint-ctypes-enum.rs:29:13 | LL | fn tf(x: T); | ^ | = help: consider adding a #[repr(...)] attribute to this enum note: type defined here - --> $DIR/lint-ctypes-enum.rs:7:1 + --> $DIR/lint-ctypes-enum.rs:9:1 | LL | enum T { E, F, G } | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 3 previous errors +error: `extern` block uses type `u128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI + --> $DIR/lint-ctypes-enum.rs:40:23 + | +LL | fn nonzero_u128(x: Option); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: `extern` block uses type `i128` which is not FFI-safe: 128-bit integers don't currently have a known stable ABI + --> $DIR/lint-ctypes-enum.rs:47:23 + | +LL | fn nonzero_i128(x: Option); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: `extern` block uses type `std::option::Option>` which is not FFI-safe: enum has no representation hint + --> $DIR/lint-ctypes-enum.rs:51:20 + | +LL | fn repr_rust(x: Option>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding a #[repr(...)] attribute to this enum + +error: `extern` block uses type `std::result::Result<(), std::num::NonZeroI32>` which is not FFI-safe: enum has no representation hint + --> $DIR/lint-ctypes-enum.rs:52:20 + | +LL | fn no_result(x: Result<(), num::NonZeroI32>); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider adding a #[repr(...)] attribute to this enum + +error: aborting due to 7 previous errors