Skip to content

Commit

Permalink
Do not derive Copy/Debug for some opaque types
Browse files Browse the repository at this point in the history
When we treat a type as opaque, either because it is explicitly
annotated as such or because it is a template that has a non-type
parameter, we need to check if the size is larger than
RUST_DERIVE_IN_ARRAY_LIMIT.

Performing this check requires information that is up the stack, in the
`Item` rather than in the `CompInfo` or `Type`. Therefore, I introduced
two traits to encapsulate whether a thing can derive `Debug` and `Copy`
(the `CanDeriveDebug` and `CanDeriveCopy` traits, respectively) and
implemented them for ALL THE THINGS. This allowes us to perform our
various checks at the level where we have access to the necessary info,
and to let sub-levels do their own checks with their sub-info.

Fixes #372.
  • Loading branch information
fitzgen committed Dec 30, 2016
1 parent d49bae2 commit bc08b35
Show file tree
Hide file tree
Showing 10 changed files with 436 additions and 174 deletions.
6 changes: 3 additions & 3 deletions libbindgen/src/codegen/mod.rs
Expand Up @@ -6,6 +6,7 @@ use aster;
use ir::annotations::FieldAccessorKind;
use ir::comp::{CompInfo, CompKind, Field, Method, MethodKind};
use ir::context::{BindgenContext, ItemId};
use ir::derive::{CanDeriveCopy, CanDeriveDebug};
use ir::enum_ty::{Enum, EnumVariant, EnumVariantValue};
use ir::function::{Function, FunctionSig};
use ir::int::IntKind;
Expand Down Expand Up @@ -765,12 +766,11 @@ impl CodeGenerator for CompInfo {

let is_union = self.kind() == CompKind::Union;
let mut derives = vec![];
let ty = item.expect_type();
if ty.can_derive_debug(ctx) {
if item.can_derive_debug(ctx, ()) {
derives.push("Debug");
}

if item.can_derive_copy(ctx) && !item.annotations().disallow_copy() {
if item.can_derive_copy(ctx, ()) && !item.annotations().disallow_copy() {
derives.push("Copy");
if !applicable_template_args.is_empty() {
// FIXME: This requires extra logic if you have a big array in a
Expand Down
202 changes: 123 additions & 79 deletions libbindgen/src/ir/comp.rs
Expand Up @@ -6,6 +6,7 @@ use std::cell::Cell;
use std::cmp;
use super::annotations::Annotations;
use super::context::{BindgenContext, ItemId};
use super::derive::{CanDeriveCopy, CanDeriveDebug};
use super::item::Item;
use super::layout::Layout;
use super::ty::{RUST_DERIVE_IN_ARRAY_LIMIT, Type};
Expand Down Expand Up @@ -154,6 +155,26 @@ impl Field {
}
}

impl CanDeriveDebug for Field {
type Extra = ();

fn can_derive_debug(&self, ctx: &BindgenContext, _: ()) -> bool {
self.ty.can_derive_debug(ctx, ())
}
}

impl<'a> CanDeriveCopy<'a> for Field {
type Extra = ();

fn can_derive_copy(&self, ctx: &BindgenContext, _: ()) -> bool {
self.ty.can_derive_copy(ctx, ())
}

fn can_derive_copy_in_array(&self, ctx: &BindgenContext, _: ()) -> bool {
self.ty.can_derive_copy_in_array(ctx, ())
}
}

/// A compound type.
///
/// Either a struct or union, a compound type is built up from the combination
Expand Down Expand Up @@ -263,50 +284,6 @@ impl CompInfo {
}
}

/// Can we derive the `Debug` trait for this compound type?
pub fn can_derive_debug(&self,
ctx: &BindgenContext,
layout: Option<Layout>)
-> bool {
// We can reach here recursively via template parameters of a member,
// for example.
if self.detect_derive_debug_cycle.get() {
warn!("Derive debug cycle detected!");
return true;
}

if self.kind == CompKind::Union {
if ctx.options().unstable_rust {
return false;
}

let layout = layout.unwrap_or_else(Layout::zero);
let size_divisor = cmp::max(1, layout.align);
return layout.size / size_divisor <= RUST_DERIVE_IN_ARRAY_LIMIT;
}

self.detect_derive_debug_cycle.set(true);

let can_derive_debug = {
self.base_members
.iter()
.all(|ty| ctx.resolve_type(*ty).can_derive_debug(ctx)) &&
self.template_args
.iter()
.all(|ty| ctx.resolve_type(*ty).can_derive_debug(ctx)) &&
self.fields
.iter()
.all(|f| ctx.resolve_type(f.ty).can_derive_debug(ctx)) &&
self.ref_template.map_or(true, |template| {
ctx.resolve_type(template).can_derive_debug(ctx)
})
};

self.detect_derive_debug_cycle.set(false);

can_derive_debug
}

/// Is this compound type unsized?
pub fn is_unsized(&self, ctx: &BindgenContext) -> bool {
!self.has_vtable(ctx) && self.fields.is_empty() &&
Expand Down Expand Up @@ -356,41 +333,6 @@ impl CompInfo {
has_destructor
}

/// Can we derive the `Copy` trait for this type?
pub fn can_derive_copy(&self, ctx: &BindgenContext, item: &Item) -> bool {
// NOTE: Take into account that while unions in C and C++ are copied by
// default, the may have an explicit destructor in C++, so we can't
// defer this check just for the union case.
if self.has_destructor(ctx) {
return false;
}

if self.kind == CompKind::Union {
if !ctx.options().unstable_rust {
return true;
}

// https://github.com/rust-lang/rust/issues/36640
if !self.template_args.is_empty() || self.ref_template.is_some() ||
!item.applicable_template_args(ctx).is_empty() {
return false;
}
}

// With template args, use a safe subset of the types,
// since copyability depends on the types itself.
self.ref_template
.as_ref()
.map_or(true, |t| ctx.resolve_item(*t).can_derive_copy(ctx)) &&
self.base_members
.iter()
.all(|t| ctx.resolve_item(*t).can_derive_copy(ctx)) &&
self.fields.iter().all(|field| {
ctx.resolve_item(field.ty)
.can_derive_copy(ctx)
})
}

/// Is this type a template specialization?
pub fn is_template_specialization(&self) -> bool {
self.ref_template.is_some()
Expand Down Expand Up @@ -858,6 +800,108 @@ impl CompInfo {
}
}

impl CanDeriveDebug for CompInfo {
type Extra = Option<Layout>;

fn can_derive_debug(&self,
ctx: &BindgenContext,
layout: Option<Layout>)
-> bool {
if self.has_non_type_template_params() {
return layout.map_or(false, |l| l.opaque().can_derive_debug(ctx, ()));
}

// We can reach here recursively via template parameters of a member,
// for example.
if self.detect_derive_debug_cycle.get() {
warn!("Derive debug cycle detected!");
return true;
}

if self.kind == CompKind::Union {
if ctx.options().unstable_rust {
return false;
}

let layout = layout.unwrap_or_else(Layout::zero);
let size_divisor = cmp::max(1, layout.align);
return layout.size / size_divisor <= RUST_DERIVE_IN_ARRAY_LIMIT;
}

self.detect_derive_debug_cycle.set(true);

let can_derive_debug = {
self.base_members
.iter()
.all(|id| id.can_derive_debug(ctx, ())) &&
self.template_args
.iter()
.all(|id| id.can_derive_debug(ctx, ())) &&
self.fields
.iter()
.all(|f| f.can_derive_debug(ctx, ())) &&
self.ref_template.map_or(true, |id| {
id.can_derive_debug(ctx, ())
})
};

self.detect_derive_debug_cycle.set(false);

can_derive_debug
}
}

impl<'a> CanDeriveCopy<'a> for CompInfo {
type Extra = (&'a Item, Option<Layout>);

fn can_derive_copy(&self,
ctx: &BindgenContext,
(item, layout): (&Item, Option<Layout>))
-> bool {
if self.has_non_type_template_params() {
return layout.map_or(false, |l| l.opaque().can_derive_copy(ctx, ()));
}

// NOTE: Take into account that while unions in C and C++ are copied by
// default, the may have an explicit destructor in C++, so we can't
// defer this check just for the union case.
if self.has_destructor(ctx) {
return false;
}

if self.kind == CompKind::Union {
if !ctx.options().unstable_rust {
return true;
}

// https://github.com/rust-lang/rust/issues/36640
if !self.template_args.is_empty() || self.ref_template.is_some() ||
!item.applicable_template_args(ctx).is_empty() {
return false;
}
}

// With template args, use a safe subset of the types,
// since copyability depends on the types itself.
self.ref_template
.as_ref()
.map_or(true, |t| t.can_derive_copy(ctx, ())) &&
self.base_members
.iter()
.all(|t| t.can_derive_copy(ctx, ())) &&
self.fields.iter().all(|field| {
field.can_derive_copy(ctx, ())
})
}

fn can_derive_copy_in_array(&self,
ctx: &BindgenContext,
extra: (&Item, Option<Layout>))
-> bool {
self.can_derive_copy(ctx, extra)
}
}

impl TypeCollector for CompInfo {
type Extra = Item;

Expand Down
21 changes: 21 additions & 0 deletions libbindgen/src/ir/context.rs
Expand Up @@ -9,6 +9,7 @@ use std::cell::Cell;
use std::collections::{HashMap, VecDeque, hash_map};
use std::collections::btree_map::{self, BTreeMap};
use std::fmt;
use super::derive::{CanDeriveCopy, CanDeriveDebug};
use super::int::IntKind;
use super::item::{Item, ItemCanonicalPath};
use super::item_kind::ItemKind;
Expand All @@ -32,6 +33,26 @@ impl ItemId {
}
}

impl CanDeriveDebug for ItemId {
type Extra = ();

fn can_derive_debug(&self, ctx: &BindgenContext, _: ()) -> bool {
ctx.resolve_item(*self).can_derive_debug(ctx, ())
}
}

impl<'a> CanDeriveCopy<'a> for ItemId {
type Extra = ();

fn can_derive_copy(&self, ctx: &BindgenContext, _: ()) -> bool {
ctx.resolve_item(*self).can_derive_copy(ctx, ())
}

fn can_derive_copy_in_array(&self, ctx: &BindgenContext, _: ()) -> bool {
ctx.resolve_item(*self).can_derive_copy_in_array(ctx, ())
}
}

/// A key used to index a resolved type, so we only process it once.
///
/// This is almost always a USR string (an unique identifier generated by
Expand Down
67 changes: 67 additions & 0 deletions libbindgen/src/ir/derive.rs
@@ -0,0 +1,67 @@
//! Traits for determining whether we can derive traits for a thing or not.

use super::context::BindgenContext;

/// A trait that encapsulates the logic for whether or not we can derive `Debug`
/// for a given thing.
///
/// This should ideally be a no-op that just returns `true`, but instead needs
/// to be a recursive method that checks whether all the proper members can
/// derive debug or not, because of the limit rust has on 32 items as max in the
/// array.
pub trait CanDeriveDebug {
/// Implementations can define this type to get access to any extra
/// information required to determine whether they can derive `Debug`. If
/// extra information is unneeded, then this should simply be the unit type.
type Extra;

/// Return `true` if `Debug` can be derived for this thing, `false`
/// otherwise.
fn can_derive_debug(&self,
ctx: &BindgenContext,
extra: Self::Extra)
-> bool;
}

/// A trait that encapsulates the logic for whether or not we can derive `Copy`
/// for a given thing.
pub trait CanDeriveCopy<'a> {
/// Implementations can define this type to get access to any extra
/// information required to determine whether they can derive `Copy`. If
/// extra information is unneeded, then this should simply be the unit type.
type Extra;

/// Return `true` if `Copy` can be derived for this thing, `false`
/// otherwise.
fn can_derive_copy(&'a self,
ctx: &'a BindgenContext,
extra: Self::Extra)
-> bool;

/// For some reason, deriving copies of an array of a type that is not known
/// to be `Copy` is a compile error. e.g.:
///
/// ```rust
/// #[derive(Copy, Clone)]
/// struct A<T> {
/// member: T,
/// }
/// ```
///
/// is fine, while:
///
/// ```rust,ignore
/// #[derive(Copy, Clone)]
/// struct A<T> {
/// member: [T; 1],
/// }
/// ```
///
/// is an error.
///
/// That's the whole point of the existence of `can_derive_copy_in_array`.
fn can_derive_copy_in_array(&'a self,
ctx: &'a BindgenContext,
extra: Self::Extra)
-> bool;
}

0 comments on commit bc08b35

Please sign in to comment.