Skip to content

Commit

Permalink
Allow marking specific template instantiations as opaque
Browse files Browse the repository at this point in the history 
If a template has a specialization that bindgen doesn't understand, it can be
helpful to mark it as opaque and continue making forward progress in the
meantime. This is something we need in the SpiderMonkey bindings.
  • Loading branch information
@fitzgen
fitzgen committed Jun 21, 2017
1 parent 480c2d1 commit 1fb0b32
Show file tree
Hide file tree
Showing 13 changed files with 311 additions and 72 deletions.
39 changes: 23 additions & 16 deletions src/codegen/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use ir::dot;
use ir::enum_ty::{Enum, EnumVariant, EnumVariantValue};
use ir::function::{Abi, Function, FunctionSig};
use ir::int::IntKind;
use ir::item::{Item, ItemAncestors, ItemCanonicalName, ItemCanonicalPath,
ItemSet};
use ir::item::{IsOpaque, Item, ItemAncestors, ItemCanonicalName,
ItemCanonicalPath, ItemSet};
use ir::item_kind::ItemKind;
use ir::layout::Layout;
use ir::module::Module;
Expand Down Expand Up @@ -580,7 +580,7 @@ impl CodeGenerator for Type {
}

let mut used_template_params = item.used_template_params(ctx);
let inner_rust_type = if item.is_opaque(ctx) {
let inner_rust_type = if item.is_opaque(ctx, &()) {
used_template_params = None;
self.to_opaque(ctx, item)
} else {
Expand Down Expand Up @@ -759,8 +759,11 @@ impl CodeGenerator for TemplateInstantiation {
item: &Item) {
// Although uses of instantiations don't need code generation, and are
// just converted to rust types in fields, vars, etc, we take this
// opportunity to generate tests for their layout here.
if !ctx.options().layout_tests {
// opportunity to generate tests for their layout here. If the
// instantiation is opaque, then its presumably because we don't
// properly understand it (maybe because of specializations), and so we
// shouldn't emit layout tests either.
if !ctx.options().layout_tests || self.is_opaque(ctx, item) {
return
}

Expand Down Expand Up @@ -1568,7 +1571,7 @@ impl CodeGenerator for CompInfo {
}

// Yeah, sorry about that.
if item.is_opaque(ctx) {
if item.is_opaque(ctx, &()) {
fields.clear();
methods.clear();

Expand Down Expand Up @@ -1704,7 +1707,7 @@ impl CodeGenerator for CompInfo {
})
.count() > 1;

let should_skip_field_offset_checks = item.is_opaque(ctx) ||
let should_skip_field_offset_checks = item.is_opaque(ctx, &()) ||
too_many_base_vtables;

let check_field_offset = if should_skip_field_offset_checks {
Expand Down Expand Up @@ -2816,7 +2819,7 @@ impl TryToRustTy for Type {
.build())
}
TypeKind::TemplateInstantiation(ref inst) => {
inst.try_to_rust_ty(ctx, self)
inst.try_to_rust_ty(ctx, item)
}
TypeKind::ResolvedTypeRef(inner) => inner.try_to_rust_ty(ctx, &()),
TypeKind::TemplateAlias(inner, _) |
Expand All @@ -2828,7 +2831,7 @@ impl TryToRustTy for Type {
.collect::<Vec<_>>();

let spelling = self.name().expect("Unnamed alias?");
if item.is_opaque(ctx) && !template_params.is_empty() {
if item.is_opaque(ctx, &()) && !template_params.is_empty() {
self.try_to_opaque(ctx, item)
} else if let Some(ty) = utils::type_from_named(ctx,
spelling,
Expand All @@ -2841,7 +2844,7 @@ impl TryToRustTy for Type {
TypeKind::Comp(ref info) => {
let template_params = item.used_template_params(ctx);
if info.has_non_type_template_params() ||
(item.is_opaque(ctx) && template_params.is_some()) {
(item.is_opaque(ctx, &()) && template_params.is_some()) {
return self.try_to_opaque(ctx, item);
}

Expand Down Expand Up @@ -2895,23 +2898,27 @@ impl TryToRustTy for Type {
}

impl TryToOpaque for TemplateInstantiation {
type Extra = Type;
type Extra = Item;

fn try_get_layout(&self,
ctx: &BindgenContext,
self_ty: &Type)
item: &Item)
-> error::Result<Layout> {
self_ty.layout(ctx).ok_or(error::Error::NoLayoutForOpaqueBlob)
item.expect_type().layout(ctx).ok_or(error::Error::NoLayoutForOpaqueBlob)
}
}

impl TryToRustTy for TemplateInstantiation {
type Extra = Type;
type Extra = Item;

fn try_to_rust_ty(&self,
ctx: &BindgenContext,
_: &Type)
item: &Item)
-> error::Result<P<ast::Ty>> {
if self.is_opaque(ctx, item) {
return Err(error::Error::InstantiationOfOpaqueType);
}

let decl = self.template_definition();
let mut ty = decl.try_to_rust_ty(ctx, &())?.unwrap();

Expand All @@ -2924,7 +2931,7 @@ impl TryToRustTy for TemplateInstantiation {
extra_assert!(decl.into_resolver()
.through_type_refs()
.resolve(ctx)
.is_opaque(ctx));
.is_opaque(ctx, &()));
return Err(error::Error::InstantiationOfOpaqueType);
}
};
Expand Down
4 changes: 2 additions & 2 deletions src/ir/comp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::annotations::Annotations;
use super::context::{BindgenContext, ItemId};
use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault};
use super::dot::DotAttributes;
use super::item::Item;
use super::item::{IsOpaque, Item};
use super::layout::Layout;
use super::traversal::{EdgeKind, Trace, Tracer};
use super::template::TemplateParameters;
Expand Down Expand Up @@ -1585,7 +1585,7 @@ impl Trace for CompInfo {
// We unconditionally trace `CompInfo`'s template parameters and inner
// types for the the usage analysis. However, we don't want to continue
// tracing anything else, if this type is marked opaque.
if item.is_opaque(context) {
if item.is_opaque(context, &()) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions src/ir/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault};
use super::int::IntKind;
use super::item::{Item, ItemAncestors, ItemCanonicalPath, ItemSet};
use super::item::{IsOpaque, Item, ItemAncestors, ItemCanonicalPath, ItemSet};
use super::item_kind::ItemKind;
use super::module::{Module, ModuleKind};
use super::named::{UsedTemplateParameters, analyze};
Expand Down Expand Up @@ -339,7 +339,7 @@ impl<'ctx> BindgenContext<'ctx> {
location);
debug_assert!(declaration.is_some() || !item.kind().is_type() ||
item.kind().expect_type().is_builtin_or_named() ||
item.kind().expect_type().is_opaque(),
item.kind().expect_type().is_opaque(self, &item),
"Adding a type without declaration?");

let id = item.id();
Expand Down
54 changes: 39 additions & 15 deletions src/ir/item.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@ pub trait ItemCanonicalPath {
fn canonical_path(&self, ctx: &BindgenContext) -> Vec<String>;
}

/// A trait for determining if some IR thing is opaque or not.
pub trait IsOpaque {
/// Extra context the IR thing needs to determine if it is opaque or not.
type Extra;

/// Returns `true` if the thing is opaque, and `false` otherwise.
///
/// May only be called when `ctx` is in the codegen phase.
fn is_opaque(&self, ctx: &BindgenContext, extra: &Self::Extra) -> bool;
}

/// A trait for iterating over an item and its parents and up its ancestor chain
/// up to (but not including) the implicit root module.
pub trait ItemAncestors {
Expand Down Expand Up @@ -231,7 +242,7 @@ impl Trace for Item {
// don't want to stop collecting types even though they may be
// opaque.
if ty.should_be_traced_unconditionally() ||
!self.is_opaque(ctx) {
!self.is_opaque(ctx, &()) {
ty.trace(ctx, tracer, self);
}
}
Expand Down Expand Up @@ -269,7 +280,7 @@ impl CanDeriveDebug for Item {
let result = ctx.options().derive_debug &&
match self.kind {
ItemKind::Type(ref ty) => {
if self.is_opaque(ctx) {
if self.is_opaque(ctx, &()) {
ty.layout(ctx)
.map_or(true, |l| l.opaque().can_derive_debug(ctx, ()))
} else {
Expand All @@ -292,7 +303,7 @@ impl CanDeriveDefault for Item {
ctx.options().derive_default &&
match self.kind {
ItemKind::Type(ref ty) => {
if self.is_opaque(ctx) {
if self.is_opaque(ctx, &()) {
ty.layout(ctx)
.map_or(false,
|l| l.opaque().can_derive_default(ctx, ()))
Expand All @@ -317,7 +328,7 @@ impl<'a> CanDeriveCopy<'a> for Item {

let result = match self.kind {
ItemKind::Type(ref ty) => {
if self.is_opaque(ctx) {
if self.is_opaque(ctx, &()) {
ty.layout(ctx)
.map_or(true, |l| l.opaque().can_derive_copy(ctx, ()))
} else {
Expand All @@ -335,7 +346,7 @@ impl<'a> CanDeriveCopy<'a> for Item {
fn can_derive_copy_in_array(&self, ctx: &BindgenContext, _: ()) -> bool {
match self.kind {
ItemKind::Type(ref ty) => {
if self.is_opaque(ctx) {
if self.is_opaque(ctx, &()) {
ty.layout(ctx)
.map_or(true, |l| {
l.opaque().can_derive_copy_in_array(ctx, ())
Expand Down Expand Up @@ -593,15 +604,6 @@ impl Item {
ctx.hidden_by_name(&self.canonical_path(ctx), self.id)
}

/// Is this item opaque?
pub fn is_opaque(&self, ctx: &BindgenContext) -> bool {
debug_assert!(ctx.in_codegen_phase(),
"You're not supposed to call this yet");
self.annotations.opaque() ||
self.as_type().map_or(false, |ty| ty.is_opaque()) ||
ctx.opaque_by_name(&self.canonical_path(ctx))
}

/// Is this a reference to another type?
pub fn is_type_ref(&self) -> bool {
self.as_type().map_or(false, |ty| ty.is_type_ref())
Expand Down Expand Up @@ -858,6 +860,28 @@ impl Item {
}
}

impl IsOpaque for ItemId {
type Extra = ();

fn is_opaque(&self, ctx: &BindgenContext, _: &()) -> bool {
debug_assert!(ctx.in_codegen_phase(),
"You're not supposed to call this yet");
ctx.resolve_item(*self).is_opaque(ctx, &())
}
}

impl IsOpaque for Item {
type Extra = ();

fn is_opaque(&self, ctx: &BindgenContext, _: &()) -> bool {
debug_assert!(ctx.in_codegen_phase(),
"You're not supposed to call this yet");
self.annotations.opaque() ||
self.as_type().map_or(false, |ty| ty.is_opaque(ctx, self)) ||
ctx.opaque_by_name(&self.canonical_path(ctx))
}
}

/// A set of items.
pub type ItemSet = BTreeSet<ItemId>;

Expand All @@ -874,7 +898,7 @@ impl DotAttributes for Item {
self.id,
self.name(ctx).get()));

if self.is_opaque(ctx) {
if self.is_opaque(ctx, &()) {
writeln!(out, "<tr><td>opaque</td><td>true</td></tr>")?;
}

Expand Down
32 changes: 31 additions & 1 deletion src/ir/template.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@

use super::context::{BindgenContext, ItemId};
use super::derive::{CanDeriveCopy, CanDeriveDebug};
use super::item::{Item, ItemAncestors};
use super::item::{IsOpaque, Item, ItemAncestors, ItemCanonicalName,
ItemCanonicalPath};
use super::layout::Layout;
use super::traversal::{EdgeKind, Trace, Tracer};
use clang;
Expand Down Expand Up @@ -305,6 +306,35 @@ impl TemplateInstantiation {
}
}

impl IsOpaque for TemplateInstantiation {
type Extra = Item;

/// Is this an opaque template instantiation?
fn is_opaque(&self, ctx: &BindgenContext, item: &Item) -> bool {
if self.template_definition().is_opaque(ctx, &()) {
return true;
}

// TODO(#<insert issue here>): this doesn't properly handle opaque
// instantiations where an argument is itself an instantiation because
// `canonical_name` does not insert the template arguments into the
// name, ie it for nested template arguments it creates "Foo" instead of
// "Foo<int>". The fully correct fix is to make `canonical_{name,path}`
// include template arguments properly.

let mut path = item.canonical_path(ctx);
let args: Vec<_> = self.args.iter().map(|a| a.canonical_name(ctx)).collect();
{
let mut last = path.last_mut().unwrap();
last.push('<');
last.push_str(&args.join(", "));
last.push('>');
}

ctx.opaque_by_name(&path)
}
}

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

Expand Down
22 changes: 13 additions & 9 deletions src/ir/ty.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use super::dot::DotAttributes;
use super::enum_ty::Enum;
use super::function::FunctionSig;
use super::int::IntKind;
use super::item::Item;
use super::item::{IsOpaque, Item};
use super::layout::{Layout, Opaque};
use super::objc::ObjCInterface;
use super::template::{AsTemplateParam, TemplateInstantiation, TemplateParameters};
Expand Down Expand Up @@ -102,14 +102,6 @@ impl Type {
}
}

/// Is this type of kind `TypeKind::Opaque`?
pub fn is_opaque(&self) -> bool {
match self.kind {
TypeKind::Opaque => true,
_ => false,
}
}

/// Is this type of kind `TypeKind::Named`?
pub fn is_named(&self) -> bool {
match self.kind {
Expand Down Expand Up @@ -374,6 +366,18 @@ impl Type {
}
}

impl IsOpaque for Type {
type Extra = Item;

fn is_opaque(&self, ctx: &BindgenContext, item: &Item) -> bool {
match self.kind {
TypeKind::Opaque => true,
TypeKind::TemplateInstantiation(ref inst) => inst.is_opaque(ctx, item),
_ => false,
}
}
}

impl AsTemplateParam for Type {
type Extra = Item;

Expand Down
9 changes: 0 additions & 9 deletions tests/expectations/tests/issue-674-1.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,4 @@ pub mod root {
impl Clone for CapturingContentInfo {
fn clone(&self) -> Self { *self }
}
#[test]
fn __bindgen_test_layout_Maybe_instantiation() {
assert_eq!(::std::mem::size_of::<u8>() , 1usize , concat ! (
"Size of template specialization: " , stringify ! ( u8 )
));
assert_eq!(::std::mem::align_of::<u8>() , 1usize , concat ! (
"Alignment of template specialization: " , stringify ! ( u8
) ));
}
}
9 changes: 0 additions & 9 deletions tests/expectations/tests/issue-674-2.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,6 @@ pub mod root {
pub _address: u8,
}
#[test]
fn __bindgen_test_layout_Rooted_instantiation() {
assert_eq!(::std::mem::size_of::<u8>() , 1usize , concat ! (
"Size of template specialization: " , stringify ! ( u8 )
));
assert_eq!(::std::mem::align_of::<u8>() , 1usize , concat ! (
"Alignment of template specialization: " , stringify ! ( u8
) ));
}
#[test]
fn __bindgen_test_layout_StaticRefPtr_instantiation() {
assert_eq!(::std::mem::size_of::<root::StaticRefPtr>() , 1usize ,
concat ! (
Expand Down
Loading

0 comments on commit 1fb0b32

Please sign in to comment.