Skip to content

Commit dc253cb

Browse files
author
bors-servo
authored
Auto merge of #932 - bd339:has_destructor_fp, r=fitzgen
Rewrite `has destructor` analysis as a fixed-point analysis Fixes #927 . Note that this creates a dependency between the "cannot derive copy" and "has destructor" analysis, i.e. the "has destructor" analysis must run before the "cannot derive copy" analysis, because "cannot derive copy" needs the results of "has destructor".
2 parents d22e636 + cd41e5c commit dc253cb

File tree

7 files changed

+213
-73
lines changed

7 files changed

+213
-73
lines changed

src/ir/analysis/derive_copy.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ impl<'ctx, 'gen> MonotoneFramework for CannotDeriveCopy<'ctx, 'gen> {
208208
// NOTE: Take into account that while unions in C and C++ are copied by
209209
// default, the may have an explicit destructor in C++, so we can't
210210
// defer this check just for the union case.
211-
if info.has_destructor(self.ctx) {
211+
if self.ctx.lookup_item_id_has_destructor(&id) {
212212
trace!(" comp has destructor which cannot derive copy");
213213
return self.insert(id);
214214
}

src/ir/analysis/has_destructor.rs

+179
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
//! Determining which types have destructors
2+
3+
use super::{ConstrainResult, MonotoneFramework, generate_dependencies};
4+
use ir::context::{BindgenContext, ItemId};
5+
use ir::traversal::EdgeKind;
6+
use ir::comp::{CompKind, Field, FieldMethods};
7+
use ir::ty::TypeKind;
8+
use std::collections::HashMap;
9+
use std::collections::HashSet;
10+
11+
/// An analysis that finds for each IR item whether it has a destructor or not
12+
///
13+
/// We use the monotone function `has destructor`, defined as follows:
14+
///
15+
/// * If T is a type alias, a templated alias, or an indirection to another type,
16+
/// T has a destructor if the type T refers to has a destructor.
17+
/// * If T is a compound type, T has a destructor if we saw a destructor when parsing it,
18+
/// or if it's a struct, T has a destructor if any of its base members has a destructor,
19+
/// or if any of its fields have a destructor.
20+
/// * If T is an instantiation of an abstract template definition, T has
21+
/// a destructor if its template definition has a destructor,
22+
/// or if any of the template arguments has a destructor.
23+
/// * If T is the type of a field, that field has a destructor if it's not a bitfield,
24+
/// and if T has a destructor.
25+
#[derive(Debug, Clone)]
26+
pub struct HasDestructorAnalysis<'ctx, 'gen>
27+
where
28+
'gen: 'ctx,
29+
{
30+
ctx: &'ctx BindgenContext<'gen>,
31+
32+
// The incremental result of this analysis's computation. Everything in this
33+
// set definitely has a destructor.
34+
have_destructor: HashSet<ItemId>,
35+
36+
// Dependencies saying that if a key ItemId has been inserted into the
37+
// `have_destructor` set, then each of the ids in Vec<ItemId> need to be
38+
// considered again.
39+
//
40+
// This is a subset of the natural IR graph with reversed edges, where we
41+
// only include the edges from the IR graph that can affect whether a type
42+
// has a destructor or not.
43+
dependencies: HashMap<ItemId, Vec<ItemId>>,
44+
}
45+
46+
impl<'ctx, 'gen> HasDestructorAnalysis<'ctx, 'gen> {
47+
fn consider_edge(kind: EdgeKind) -> bool {
48+
match kind {
49+
// These are the only edges that can affect whether a type has a
50+
// destructor or not.
51+
EdgeKind::TypeReference |
52+
EdgeKind::BaseMember |
53+
EdgeKind::Field |
54+
EdgeKind::TemplateArgument |
55+
EdgeKind::TemplateDeclaration => true,
56+
_ => false,
57+
}
58+
}
59+
60+
fn insert(&mut self, id: ItemId) -> ConstrainResult {
61+
let was_not_already_in_set = self.have_destructor.insert(id);
62+
assert!(
63+
was_not_already_in_set,
64+
"We shouldn't try and insert {:?} twice because if it was \
65+
already in the set, `constrain` should have exited early.",
66+
id
67+
);
68+
ConstrainResult::Changed
69+
}
70+
}
71+
72+
impl<'ctx, 'gen> MonotoneFramework for HasDestructorAnalysis<'ctx, 'gen> {
73+
type Node = ItemId;
74+
type Extra = &'ctx BindgenContext<'gen>;
75+
type Output = HashSet<ItemId>;
76+
77+
fn new(ctx: &'ctx BindgenContext<'gen>) -> Self {
78+
let have_destructor = HashSet::new();
79+
let dependencies = generate_dependencies(ctx, Self::consider_edge);
80+
81+
HasDestructorAnalysis {
82+
ctx,
83+
have_destructor,
84+
dependencies,
85+
}
86+
}
87+
88+
fn initial_worklist(&self) -> Vec<ItemId> {
89+
self.ctx.whitelisted_items().iter().cloned().collect()
90+
}
91+
92+
fn constrain(&mut self, id: ItemId) -> ConstrainResult {
93+
if self.have_destructor.contains(&id) {
94+
// We've already computed that this type has a destructor and that can't
95+
// change.
96+
return ConstrainResult::Same;
97+
}
98+
99+
let item = self.ctx.resolve_item(id);
100+
let ty = match item.as_type() {
101+
None => return ConstrainResult::Same,
102+
Some(ty) => ty,
103+
};
104+
105+
match *ty.kind() {
106+
TypeKind::TemplateAlias(t, _) |
107+
TypeKind::Alias(t) |
108+
TypeKind::ResolvedTypeRef(t) => {
109+
if self.have_destructor.contains(&t) {
110+
self.insert(id)
111+
} else {
112+
ConstrainResult::Same
113+
}
114+
}
115+
116+
TypeKind::Comp(ref info) => {
117+
if info.has_own_destructor() {
118+
return self.insert(id);
119+
}
120+
121+
match info.kind() {
122+
CompKind::Union => ConstrainResult::Same,
123+
CompKind::Struct => {
124+
let base_or_field_destructor =
125+
info.base_members().iter().any(|base| {
126+
self.have_destructor.contains(&base.ty)
127+
}) ||
128+
info.fields().iter().any(|field| {
129+
match *field {
130+
Field::DataMember(ref data) =>
131+
self.have_destructor.contains(&data.ty()),
132+
Field::Bitfields(_) => false
133+
}
134+
});
135+
if base_or_field_destructor {
136+
self.insert(id)
137+
} else {
138+
ConstrainResult::Same
139+
}
140+
}
141+
}
142+
}
143+
144+
TypeKind::TemplateInstantiation(ref inst) => {
145+
let definition_or_arg_destructor =
146+
self.have_destructor.contains(&inst.template_definition())
147+
||
148+
inst.template_arguments().iter().any(|arg| {
149+
self.have_destructor.contains(arg)
150+
});
151+
if definition_or_arg_destructor {
152+
self.insert(id)
153+
} else {
154+
ConstrainResult::Same
155+
}
156+
}
157+
158+
_ => ConstrainResult::Same,
159+
}
160+
}
161+
162+
fn each_depending_on<F>(&self, id: ItemId, mut f: F)
163+
where
164+
F: FnMut(ItemId),
165+
{
166+
if let Some(edges) = self.dependencies.get(&id) {
167+
for item in edges {
168+
trace!("enqueue {:?} into worklist", item);
169+
f(*item);
170+
}
171+
}
172+
}
173+
}
174+
175+
impl<'ctx, 'gen> From<HasDestructorAnalysis<'ctx, 'gen>> for HashSet<ItemId> {
176+
fn from(analysis: HasDestructorAnalysis<'ctx, 'gen>) -> Self {
177+
analysis.have_destructor
178+
}
179+
}

src/ir/analysis/mod.rs

+2
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ pub use self::derive_debug::CannotDeriveDebug;
4545
mod has_vtable;
4646
pub use self::has_vtable::HasVtable;
4747
pub use self::has_vtable::HasVtableAnalysis;
48+
mod has_destructor;
49+
pub use self::has_destructor::HasDestructorAnalysis;
4850
mod derive_default;
4951
pub use self::derive_default::CannotDeriveDefault;
5052
mod derive_copy;

src/ir/comp.rs

+5-46
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use codegen::struct_layout::{align_to, bytes_from_bits_pow2};
1313
use ir::derive::CanDeriveCopy;
1414
use parse::{ClangItemParser, ParseError};
1515
use peeking_take_while::PeekableExt;
16-
use std::cell::Cell;
1716
use std::cmp;
1817
use std::io;
1918
use std::mem;
@@ -170,18 +169,6 @@ pub enum Field {
170169
}
171170

172171
impl Field {
173-
fn has_destructor(&self, ctx: &BindgenContext) -> bool {
174-
match *self {
175-
Field::DataMember(ref data) => {
176-
ctx.resolve_type(data.ty).has_destructor(ctx)
177-
}
178-
// Bitfields may not be of a type that has a destructor.
179-
Field::Bitfields(BitfieldUnit {
180-
..
181-
}) => false,
182-
}
183-
}
184-
185172
/// Get this field's layout.
186173
pub fn layout(&self, ctx: &BindgenContext) -> Option<Layout> {
187174
match *self {
@@ -865,10 +852,6 @@ pub struct CompInfo {
865852
/// and pray, or behave as an opaque type.
866853
found_unknown_attr: bool,
867854

868-
/// Used to detect if we've run in a has_destructor cycle while cycling
869-
/// around the template arguments.
870-
detect_has_destructor_cycle: Cell<bool>,
871-
872855
/// Used to indicate when a struct has been forward declared. Usually used
873856
/// in headers so that APIs can't modify them directly.
874857
is_forward_declaration: bool,
@@ -893,7 +876,6 @@ impl CompInfo {
893876
has_non_type_template_params: false,
894877
packed: false,
895878
found_unknown_attr: false,
896-
detect_has_destructor_cycle: Cell::new(false),
897879
is_forward_declaration: false,
898880
}
899881
}
@@ -909,34 +891,6 @@ impl CompInfo {
909891
})
910892
}
911893

912-
/// Does this compound type have a destructor?
913-
pub fn has_destructor(&self, ctx: &BindgenContext) -> bool {
914-
if self.detect_has_destructor_cycle.get() {
915-
warn!("Cycle detected looking for destructors");
916-
// Assume no destructor, since we don't have an explicit one.
917-
return false;
918-
}
919-
920-
self.detect_has_destructor_cycle.set(true);
921-
922-
let has_destructor = self.has_destructor ||
923-
match self.kind {
924-
CompKind::Union => false,
925-
CompKind::Struct => {
926-
self.base_members.iter().any(|base| {
927-
ctx.resolve_type(base.ty).has_destructor(ctx)
928-
}) ||
929-
self.fields().iter().any(
930-
|field| field.has_destructor(ctx),
931-
)
932-
}
933-
};
934-
935-
self.detect_has_destructor_cycle.set(false);
936-
937-
has_destructor
938-
}
939-
940894
/// Compute the layout of this type.
941895
///
942896
/// This is called as a fallback under some circumstances where LLVM doesn't
@@ -989,6 +943,11 @@ impl CompInfo {
989943
return self.has_own_virtual_method;
990944
}
991945

946+
/// Did we see a destructor when parsing this type?
947+
pub fn has_own_destructor(&self) -> bool {
948+
self.has_destructor
949+
}
950+
992951
/// Get this type's set of methods.
993952
pub fn methods(&self) -> &[Method] {
994953
&self.methods

src/ir/context.rs

+26-2
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
use super::analysis::{CannotDeriveCopy, CannotDeriveDebug,
44
CannotDeriveDefault, CannotDeriveHash,
55
CannotDerivePartialEq, HasTypeParameterInArray,
6-
HasVtableAnalysis, UsedTemplateParameters, HasFloat,
7-
analyze};
6+
HasVtableAnalysis, HasDestructorAnalysis, UsedTemplateParameters,
7+
HasFloat, analyze};
88
use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault,
99
CanDeriveHash, CanDerivePartialEq, CanDeriveEq};
1010
use super::int::IntKind;
@@ -239,6 +239,12 @@ pub struct BindgenContext<'ctx> {
239239
/// before that and `Some` after.
240240
have_vtable: Option<HashSet<ItemId>>,
241241

242+
/// The set of (`ItemId's of`) types that has destructor.
243+
///
244+
/// Populated when we enter codegen by `compute_has_destructor`; always `None`
245+
/// before that and `Some` after.
246+
have_destructor: Option<HashSet<ItemId>>,
247+
242248
/// The set of (`ItemId's of`) types that has array.
243249
///
244250
/// Populated when we enter codegen by `compute_has_type_param_in_array`; always `None`
@@ -390,6 +396,7 @@ impl<'ctx> BindgenContext<'ctx> {
390396
cannot_derive_hash: None,
391397
cannot_derive_partialeq: None,
392398
have_vtable: None,
399+
have_destructor: None,
393400
has_type_param_in_array: None,
394401
has_float: None,
395402
};
@@ -901,6 +908,7 @@ impl<'ctx> BindgenContext<'ctx> {
901908
self.assert_every_item_in_a_module();
902909

903910
self.compute_has_vtable();
911+
self.compute_has_destructor();
904912
self.find_used_template_parameters();
905913
self.compute_cannot_derive_debug();
906914
self.compute_cannot_derive_default();
@@ -1001,6 +1009,22 @@ impl<'ctx> BindgenContext<'ctx> {
10011009
self.have_vtable.as_ref().unwrap().contains(id)
10021010
}
10031011

1012+
/// Compute whether the type has a destructor.
1013+
fn compute_has_destructor(&mut self) {
1014+
assert!(self.have_destructor.is_none());
1015+
self.have_destructor = Some(analyze::<HasDestructorAnalysis>(self));
1016+
}
1017+
1018+
/// Look up whether the item with `id` has a destructor.
1019+
pub fn lookup_item_id_has_destructor(&self, id: &ItemId) -> bool {
1020+
assert!(
1021+
self.in_codegen_phase(),
1022+
"We only compute destructors when we enter codegen"
1023+
);
1024+
1025+
self.have_destructor.as_ref().unwrap().contains(id)
1026+
}
1027+
10041028
fn find_used_template_parameters(&mut self) {
10051029
if self.options.whitelist_recursively {
10061030
let used_params = analyze::<UsedTemplateParameters>(self);

src/ir/template.rs

-8
Original file line numberDiff line numberDiff line change
@@ -309,14 +309,6 @@ impl TemplateInstantiation {
309309
template_args,
310310
))
311311
}
312-
313-
/// Does this instantiation have a destructor?
314-
pub fn has_destructor(&self, ctx: &BindgenContext) -> bool {
315-
ctx.resolve_type(self.definition).has_destructor(ctx) ||
316-
self.args.iter().any(|arg| {
317-
ctx.resolve_type(*arg).has_destructor(ctx)
318-
})
319-
}
320312
}
321313

322314
impl IsOpaque for TemplateInstantiation {

src/ir/ty.rs

-16
Original file line numberDiff line numberDiff line change
@@ -237,22 +237,6 @@ impl Type {
237237
})
238238
}
239239

240-
/// Returns whether this type has a destructor.
241-
pub fn has_destructor(&self, ctx: &BindgenContext) -> bool {
242-
match self.kind {
243-
TypeKind::TemplateAlias(t, _) |
244-
TypeKind::Alias(t) |
245-
TypeKind::ResolvedTypeRef(t) => {
246-
ctx.resolve_type(t).has_destructor(ctx)
247-
}
248-
TypeKind::TemplateInstantiation(ref inst) => {
249-
inst.has_destructor(ctx)
250-
}
251-
TypeKind::Comp(ref info) => info.has_destructor(ctx),
252-
_ => false,
253-
}
254-
}
255-
256240
/// Whether this named type is an invalid C++ identifier. This is done to
257241
/// avoid generating invalid code with some cases we can't handle, see:
258242
///

0 commit comments

Comments
 (0)