From 60ffc316550fbc7209b8401abf9274155874e5fe Mon Sep 17 00:00:00 2001 From: Nick Fitzgerald Date: Fri, 7 Apr 2017 15:59:02 -0700 Subject: [PATCH] Correctly handle blacklisted items in the template analysis The template analysis operates on whitelisted items, and uses our tracing infrastructure to move between them. Usually, that means we can only reach other whitelisted items by tracing, because the set of whitelisted items is the transitive closure of all the items explicitly whitelisted. The exception is when some type is explicitly blacklisted. It could still be reachable via tracing from a whitelisted item, but is not considered whitelisted due to the blacklisting. The easy fix is to run the template analysis on the whole IR graph rather than just the whitelisted set. This is an approximately one line change in the analysis, however is not desirable due to performance concerns. The whole point of whitelisting is that there may be *many* types in a header, but only a *few* the user cares about, or there might be types that aren't explicitly needed and that are too complicated for bindgen to handle generally (often in ``). In these situations, we don't want to waste cycles or even confuse ourselves by considering such types! Instead, we keep the whitelisted item set around and check by hand whether any given item is in it during the template type parameter analysis. Additionally, we make the decision that blacklisted template definitions use all of their type parameters. This seems like a reasonable choice because the type will likely be ported to Rust manually by the bindgen user, and they will be looking at the C++ definition with all of its type parameters. They can always insert `PhantomData`s manually, so it also gives the most flexibility. Fixes #584 --- src/ir/context.rs | 10 ++- src/ir/named.rs | 56 ++++++++++--- ...issue-584-stylo-template-analysis-panic.rs | 80 +++++++++++++++++++ ...ssue-584-stylo-template-analysis-panic.hpp | 13 +++ 4 files changed, 145 insertions(+), 14 deletions(-) create mode 100644 tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs create mode 100644 tests/headers/issue-584-stylo-template-analysis-panic.hpp diff --git a/src/ir/context.rs b/src/ir/context.rs index 9b9ad8bd29..d00e489938 100644 --- a/src/ir/context.rs +++ b/src/ir/context.rs @@ -631,6 +631,14 @@ impl<'ctx> BindgenContext<'ctx> { /// This method may only be called during the codegen phase, because the /// template usage information is only computed as we enter the codegen /// phase. + /// + /// If the item is blacklisted, then we say that it always uses the template + /// parameter. This is a little subtle. The template parameter usage + /// analysis only considers whitelisted items, and if any blacklisted item + /// shows up in the generated bindings, it is the user's responsibility to + /// manually provide a definition for them. To give them the most + /// flexibility when doing that, we assume that they use every template + /// parameter and always pass template arguments through in instantiations. pub fn uses_template_parameter(&self, item: ItemId, template_param: ItemId) @@ -643,7 +651,7 @@ impl<'ctx> BindgenContext<'ctx> { .expect("should have found template parameter usage if we're in codegen") .get(&item) .map(|items_used_params| items_used_params.contains(&template_param)) - .unwrap_or(false) + .unwrap_or_else(|| self.resolve_item(item).is_hidden(self)) } // This deserves a comment. Builtin types don't get a valid declaration, so diff --git a/src/ir/named.rs b/src/ir/named.rs index 67b369155a..1af98a26ba 100644 --- a/src/ir/named.rs +++ b/src/ir/named.rs @@ -131,7 +131,7 @@ use super::item::ItemSet; use super::template::AsNamed; use super::traversal::{EdgeKind, Trace}; use super::ty::{TemplateDeclaration, TypeKind}; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt; /// An analysis in the monotone framework. @@ -263,6 +263,8 @@ pub struct UsedTemplateParameters<'ctx, 'gen> used: HashMap>, dependencies: HashMap>, + + whitelisted_items: HashSet, } impl<'ctx, 'gen> UsedTemplateParameters<'ctx, 'gen> { @@ -316,21 +318,30 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> { -> UsedTemplateParameters<'ctx, 'gen> { let mut used = HashMap::new(); let mut dependencies = HashMap::new(); + let whitelisted_items: HashSet<_> = ctx.whitelisted_items().collect(); - for item in ctx.whitelisted_items() { + for item in whitelisted_items.iter().cloned() { dependencies.entry(item).or_insert(vec![]); used.insert(item, Some(ItemSet::new())); { // We reverse our natural IR graph edges to find dependencies // between nodes. - item.trace(ctx, - &mut |sub_item, _| { - dependencies.entry(sub_item) - .or_insert(vec![]) - .push(item); - }, - &()); + item.trace(ctx, &mut |sub_item, _| { + // We won't be generating code for items that aren't + // whitelisted, so don't bother keeping track of their + // template parameters. But isn't whitelisting the + // transitive closure of reachable items from the explicitly + // whitelisted items? Usually! The exception is explicitly + // blacklisted items. + if !whitelisted_items.contains(&sub_item) { + return; + } + + dependencies.entry(sub_item) + .or_insert(vec![]) + .push(item); + }, &()); } // Additionally, whether a template instantiation's template @@ -361,6 +372,7 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> { ctx: ctx, used: used, dependencies: dependencies, + whitelisted_items: whitelisted_items, } } @@ -395,6 +407,19 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> { used_by_this_id.insert(id); } + // We say that blacklisted items use all of their template + // parameters. The blacklisted type is most likely implemented + // explicitly by the user, since it won't be in the generated + // bindings, and we don't know exactly what they'll to with template + // parameters, but we can push the issue down the line to them. + Some(&TypeKind::TemplateInstantiation(ref inst)) + if !self.whitelisted_items.contains(&inst.template_definition()) => { + let args = inst.template_arguments() + .iter() + .filter_map(|a| a.as_named(self.ctx, &())); + used_by_this_id.extend(args); + } + // A template instantiation's concrete template argument is // only used if the template declaration uses the // corresponding template parameter. @@ -404,12 +429,12 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> { let params = decl.self_template_params(self.ctx) .unwrap_or(vec![]); + for (arg, param) in args.iter().zip(params.iter()) { - let used_by_definition = self.used - [&inst.template_definition()] + let used_by_def = self.used[&inst.template_definition()] .as_ref() .unwrap(); - if used_by_definition.contains(param) { + if used_by_def.contains(param) { if let Some(named) = arg.as_named(self.ctx, &()) { used_by_this_id.insert(named); } @@ -421,7 +446,12 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> { // parameter usage. _ => { item.trace(self.ctx, &mut |sub_id, edge_kind| { - if sub_id == id || !Self::consider_edge(edge_kind) { + // Ignore ourselves, since union with ourself is a + // no-op. Ignore edges that aren't relevant to the + // analysis. Ignore edges to blacklisted items. + if sub_id == id || + !Self::consider_edge(edge_kind) || + !self.whitelisted_items.contains(&sub_id) { return; } diff --git a/tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs b/tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs new file mode 100644 index 0000000000..91b8e49a16 --- /dev/null +++ b/tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs @@ -0,0 +1,80 @@ +/* automatically generated by rust-bindgen */ + + +#![allow(non_snake_case)] + +pub type RefPtr = T; + +#[repr(C)] +#[derive(Debug, Copy)] +pub struct b { + pub _base: g, +} +#[test] +fn bindgen_test_layout_b() { + assert_eq!(::std::mem::size_of::() , 1usize , concat ! ( + "Size of: " , stringify ! ( b ) )); + assert_eq! (::std::mem::align_of::() , 1usize , concat ! ( + "Alignment of " , stringify ! ( b ) )); +} +impl Clone for b { + fn clone(&self) -> Self { *self } +} +impl Default for b { + fn default() -> Self { unsafe { ::std::mem::zeroed() } } +} +#[repr(C)] +#[derive(Debug, Default, Copy)] +pub struct A { + pub _address: u8, +} +pub type A_a = b; +#[test] +fn bindgen_test_layout_A() { + assert_eq!(::std::mem::size_of::() , 1usize , concat ! ( + "Size of: " , stringify ! ( A ) )); + assert_eq! (::std::mem::align_of::() , 1usize , concat ! ( + "Alignment of " , stringify ! ( A ) )); +} +impl Clone for A { + fn clone(&self) -> Self { *self } +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct e { + pub d: RefPtr, +} +impl Default for e { + fn default() -> Self { unsafe { ::std::mem::zeroed() } } +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct f { + pub _address: u8, +} +#[repr(C)] +#[derive(Debug, Copy)] +pub struct g { + pub h: f, +} +#[test] +fn bindgen_test_layout_g() { + assert_eq!(::std::mem::size_of::() , 1usize , concat ! ( + "Size of: " , stringify ! ( g ) )); + assert_eq! (::std::mem::align_of::() , 1usize , concat ! ( + "Alignment of " , stringify ! ( g ) )); + assert_eq! (unsafe { & ( * ( 0 as * const g ) ) . h as * const _ as usize + } , 0usize , concat ! ( + "Alignment of field: " , stringify ! ( g ) , "::" , stringify + ! ( h ) )); +} +impl Clone for g { + fn clone(&self) -> Self { *self } +} +impl Default for g { + fn default() -> Self { unsafe { ::std::mem::zeroed() } } +} +extern "C" { + #[link_name = "_Z25Servo_Element_GetSnapshotv"] + pub fn Servo_Element_GetSnapshot() -> A; +} diff --git a/tests/headers/issue-584-stylo-template-analysis-panic.hpp b/tests/headers/issue-584-stylo-template-analysis-panic.hpp new file mode 100644 index 0000000000..244e15a2aa --- /dev/null +++ b/tests/headers/issue-584-stylo-template-analysis-panic.hpp @@ -0,0 +1,13 @@ +// bindgen-flags: --blacklist-type '^RefPtr$' --whitelist-function 'Servo_.*' --raw-line 'pub type RefPtr = T;' -- -std=c++14 +template class RefPtr; +class b; +class A { + typedef b a; +}; +template class e { RefPtr d; }; +template class f {}; +class g { + f> h; +}; +class b : g {}; +A Servo_Element_GetSnapshot();