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

lint/ctypes: Don't warn on sized structs with PhantomData. #39462

Merged
merged 1 commit into from
Feb 8, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
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
46 changes: 39 additions & 7 deletions src/librustc_lint/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ struct ImproperCTypesVisitor<'a, 'tcx: 'a> {

enum FfiResult {
FfiSafe,
FfiPhantom,
FfiUnsafe(&'static str),
FfiBadStruct(DefId, &'static str),
FfiBadUnion(DefId, &'static str),
Expand Down Expand Up @@ -385,8 +386,11 @@ fn is_repr_nullable_ptr<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
/// Check if the given type is "ffi-safe" (has a stable, well-defined
/// representation which can be exported to C code).
fn check_type_for_ffi(&self, cache: &mut FxHashSet<Ty<'tcx>>, ty: Ty<'tcx>) -> FfiResult {
fn check_type_for_ffi(&self,
cache: &mut FxHashSet<Ty<'tcx>>,
ty: Ty<'tcx>) -> FfiResult {
use self::FfiResult::*;

let cx = self.cx.tcx;

// Protect against infinite recursion, for example
Expand All @@ -399,6 +403,9 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {

match ty.sty {
ty::TyAdt(def, substs) => {
if def.is_phantom_data() {
return FfiPhantom;
}
match def.adt_kind() {
AdtKind::Struct => {
if !cx.lookup_repr_hints(def.did).contains(&attr::ReprExtern) {
Expand All @@ -407,18 +414,22 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
consider adding a #[repr(C)] attribute to the type");
}

// We can't completely trust repr(C) markings; make sure the
// fields are actually safe.
if def.struct_variant().fields.is_empty() {
return FfiUnsafe("found zero-size struct in foreign module, consider \
adding a member to this struct");
}

// We can't completely trust repr(C) markings; make sure the
// fields are actually safe.
let mut all_phantom = true;
for field in &def.struct_variant().fields {
let field_ty = cx.normalize_associated_type(&field.ty(cx, substs));
let r = self.check_type_for_ffi(cache, field_ty);
match r {
FfiSafe => {}
FfiSafe => {
all_phantom = false;
}
FfiPhantom => {}
FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => {
return r;
}
Expand All @@ -427,7 +438,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
}
}
FfiSafe

if all_phantom { FfiPhantom } else { FfiSafe }
}
AdtKind::Union => {
if !cx.lookup_repr_hints(def.did).contains(&attr::ReprExtern) {
Expand All @@ -436,11 +448,20 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
consider adding a #[repr(C)] attribute to the type");
}

if def.struct_variant().fields.is_empty() {
return FfiUnsafe("found zero-size union in foreign module, consider \
adding a member to this union");
}

let mut all_phantom = true;
for field in &def.struct_variant().fields {
let field_ty = cx.normalize_associated_type(&field.ty(cx, substs));
let r = self.check_type_for_ffi(cache, field_ty);
match r {
FfiSafe => {}
FfiSafe => {
all_phantom = false;
}
FfiPhantom => {}
FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => {
return r;
}
Expand All @@ -449,7 +470,8 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
}
}
}
FfiSafe

if all_phantom { FfiPhantom } else { FfiSafe }
}
AdtKind::Enum => {
if def.variants.is_empty() {
Expand Down Expand Up @@ -500,6 +522,10 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {
FfiBadStruct(..) | FfiBadUnion(..) | FfiBadEnum(..) => {
return r;
}
FfiPhantom => {
return FfiBadEnum(def.did,
"Found phantom data in enum variant");
}
FfiUnsafe(s) => {
return FfiBadEnum(def.did, s);
}
Expand Down Expand Up @@ -593,6 +619,12 @@ impl<'a, 'tcx> ImproperCTypesVisitor<'a, 'tcx> {

match self.check_type_for_ffi(&mut FxHashSet(), ty) {
FfiResult::FfiSafe => {}
FfiResult::FfiPhantom => {
self.cx.span_lint(IMPROPER_CTYPES,
sp,
&format!("found zero-sized type composed only \
of phantom-data in a foreign-function."));
Copy link
Contributor

Choose a reason for hiding this comment

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

@emilio why is it important to highlight the fact that the zero-sized type consists only of phantom data? Isn't it true that any zero-sized type is suspicious, and that is the real problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessarily important. It's just that we happen to have that info. I can omit it if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Note that other kind of zero-sized types get reported with different messages by the struct or union checks).

}
FfiResult::FfiUnsafe(s) => {
self.cx.span_lint(IMPROPER_CTYPES, sp, s);
}
Expand Down
6 changes: 6 additions & 0 deletions src/test/compile-fail/lint-ctypes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ pub type RustBadRet = extern fn() -> Box<u32>;
pub type CVoidRet = ();
pub struct Foo;

#[repr(C)]
pub struct ZeroSizeWithPhantomData(::std::marker::PhantomData<i32>);

extern {
pub fn ptr_type1(size: *const Foo); //~ ERROR: found struct without
pub fn ptr_type2(size: *const Foo); //~ ERROR: found struct without
Expand All @@ -40,6 +43,9 @@ extern {
pub fn tuple_type(p: (i32, i32)); //~ ERROR found Rust tuple type
pub fn tuple_type2(p: I32Pair); //~ ERROR found Rust tuple type
pub fn zero_size(p: ZeroSize); //~ ERROR found zero-size struct
pub fn zero_size_phantom(p: ZeroSizeWithPhantomData); //~ ERROR found zero-sized type
pub fn zero_size_phantom_toplevel()
-> ::std::marker::PhantomData<bool>; //~ ERROR: found zero-sized type
pub fn fn_type(p: RustFn); //~ ERROR found function pointer with Rust
pub fn fn_type2(p: fn()); //~ ERROR found function pointer with Rust
pub fn fn_contained(p: RustBadRet); //~ ERROR: found struct without
Expand Down
34 changes: 34 additions & 0 deletions src/test/run-pass/issue-34798.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![forbid(improper_ctypes)]
#![allow(dead_code)]

#[repr(C)]
pub struct Foo {
size: u8,
__value: ::std::marker::PhantomData<i32>,
}

#[repr(C)]
pub struct ZeroSizeWithPhantomData<T>(::std::marker::PhantomData<T>);

#[repr(C)]
pub struct Bar {
size: u8,
baz: ZeroSizeWithPhantomData<i32>,
}

extern "C" {
pub fn bar(_: *mut Foo, _: *mut Bar);
}

fn main() {
}