Skip to content

Commit 5e85271

Browse files
author
bors-servo
authored
Auto merge of #608 - emilio:destructor-codegen, r=fitzgen
Destructor codegen Based on #542, and on top of #606, with a bunch more tests and fixes.
2 parents d4fce37 + 1f53966 commit 5e85271

24 files changed

+307
-30
lines changed

‎bindgen-integration/cpp/Test.cc

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,15 @@ Test::Test(double foo)
2121
, m_double(foo)
2222
{}
2323

24+
AutoRestoreBool::AutoRestoreBool(bool* ptr)
25+
: m_ptr(ptr)
26+
, m_value(*ptr)
27+
{}
28+
29+
AutoRestoreBool::~AutoRestoreBool() {
30+
*m_ptr = m_value;
31+
}
32+
2433
namespace bitfields {
2534

2635
bool
@@ -47,4 +56,4 @@ Third::assert(int first, bool second, ItemKind third)
4756
kind == third;
4857
}
4958

50-
}
59+
} // namespace bitfields

‎bindgen-integration/cpp/Test.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,3 +64,11 @@ struct Third {
6464
};
6565

6666
} // namespace bitfields
67+
68+
struct AutoRestoreBool {
69+
bool* m_ptr;
70+
bool m_value;
71+
72+
AutoRestoreBool(bool*);
73+
~AutoRestoreBool();
74+
};

‎bindgen-integration/src/lib.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,21 @@ fn test_bitfields_third() {
101101
bindings::bitfields::ItemKind::ITEM_KIND_TRES)
102102
});
103103
}
104+
105+
impl Drop for bindings::AutoRestoreBool {
106+
fn drop(&mut self) {
107+
unsafe { bindings::AutoRestoreBool::destruct(self) }
108+
}
109+
}
110+
111+
#[test]
112+
fn test_destructors() {
113+
let mut v = true;
114+
115+
{
116+
let auto_restore = unsafe { bindings::AutoRestoreBool::new(&mut v) };
117+
v = false;
118+
}
119+
120+
assert!(v, "Should've been restored when going out of scope");
121+
}

‎src/codegen/mod.rs

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1597,6 +1597,24 @@ impl CodeGenerator for CompInfo {
15971597
self);
15981598
}
15991599
}
1600+
1601+
if ctx.options().codegen_config.destructors {
1602+
if let Some((is_virtual, destructor)) = self.destructor() {
1603+
let kind = if is_virtual {
1604+
MethodKind::VirtualDestructor
1605+
} else {
1606+
MethodKind::Destructor
1607+
};
1608+
1609+
Method::new(kind, destructor, false)
1610+
.codegen_method(ctx,
1611+
&mut methods,
1612+
&mut method_names,
1613+
result,
1614+
whitelisted_items,
1615+
self);
1616+
}
1617+
}
16001618
}
16011619

16021620
// NB: We can't use to_rust_ty here since for opaque types this tries to
@@ -1693,6 +1711,7 @@ impl MethodCodegen for Method {
16931711
if self.is_virtual() {
16941712
return; // FIXME
16951713
}
1714+
16961715
// First of all, output the actual function.
16971716
let function_item = ctx.resolve_item(self.signature());
16981717
function_item.codegen(ctx, result, whitelisted_items, &());
@@ -1701,6 +1720,7 @@ impl MethodCodegen for Method {
17011720
let signature_item = ctx.resolve_item(function.signature());
17021721
let mut name = match self.kind() {
17031722
MethodKind::Constructor => "new".into(),
1723+
MethodKind::Destructor => "destruct".into(),
17041724
_ => function.name().to_owned(),
17051725
};
17061726

@@ -1801,11 +1821,7 @@ impl MethodCodegen for Method {
18011821
exprs[0] = quote_expr!(ctx.ext_cx(), &mut __bindgen_tmp);
18021822
} else if !self.is_static() {
18031823
assert!(!exprs.is_empty());
1804-
exprs[0] = if self.is_const() {
1805-
quote_expr!(ctx.ext_cx(), &*self)
1806-
} else {
1807-
quote_expr!(ctx.ext_cx(), &mut *self)
1808-
};
1824+
exprs[0] = quote_expr!(ctx.ext_cx(), self);
18091825
};
18101826

18111827
let call = aster::expr::ExprBuilder::new()

‎src/ir/comp.rs

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ pub enum MethodKind {
2626
/// A constructor. We represent it as method for convenience, to avoid code
2727
/// duplication.
2828
Constructor,
29+
/// A destructor.
30+
Destructor,
31+
/// A virtual destructor.
32+
VirtualDestructor,
2933
/// A static method.
3034
Static,
3135
/// A normal method.
@@ -61,14 +65,21 @@ impl Method {
6165
self.kind
6266
}
6367

68+
/// Is this a destructor method?
69+
pub fn is_destructor(&self) -> bool {
70+
self.kind == MethodKind::Destructor ||
71+
self.kind == MethodKind::VirtualDestructor
72+
}
73+
6474
/// Is this a constructor?
6575
pub fn is_constructor(&self) -> bool {
6676
self.kind == MethodKind::Constructor
6777
}
6878

6979
/// Is this a virtual method?
7080
pub fn is_virtual(&self) -> bool {
71-
self.kind == MethodKind::Virtual
81+
self.kind == MethodKind::Virtual ||
82+
self.kind == MethodKind::VirtualDestructor
7283
}
7384

7485
/// Is this a static method?
@@ -250,6 +261,10 @@ pub struct CompInfo {
250261
/// The different constructors this struct or class contains.
251262
constructors: Vec<ItemId>,
252263

264+
/// The destructor of this type. The bool represents whether this destructor
265+
/// is virtual.
266+
destructor: Option<(bool, ItemId)>,
267+
253268
/// Vector of classes this one inherits from.
254269
base_members: Vec<Base>,
255270

@@ -321,6 +336,7 @@ impl CompInfo {
321336
template_params: vec![],
322337
methods: vec![],
323338
constructors: vec![],
339+
destructor: None,
324340
base_members: vec![],
325341
inner_types: vec![],
326342
inner_vars: vec![],
@@ -434,6 +450,11 @@ impl CompInfo {
434450
&self.constructors
435451
}
436452

453+
/// Get this type's destructor.
454+
pub fn destructor(&self) -> Option<(bool, ItemId)> {
455+
self.destructor
456+
}
457+
437458
/// What kind of compound type is this?
438459
pub fn kind(&self) -> CompKind {
439460
self.kind
@@ -657,8 +678,9 @@ impl CompInfo {
657678
CXCursor_Constructor => {
658679
ci.constructors.push(signature);
659680
}
660-
// TODO(emilio): Bind the destructor?
661-
CXCursor_Destructor => {}
681+
CXCursor_Destructor => {
682+
ci.destructor = Some((is_virtual, signature));
683+
}
662684
CXCursor_CXXMethod => {
663685
let is_const = cur.method_is_const();
664686
let method_kind = if is_static {

‎src/ir/function.rs

Lines changed: 50 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ fn get_abi(cc: CXCallingConv) -> Option<abi::Abi> {
114114
pub fn cursor_mangling(ctx: &BindgenContext,
115115
cursor: &clang::Cursor)
116116
-> Option<String> {
117+
use clang_sys;
117118
if !ctx.options().enable_mangling {
118119
return None;
119120
}
@@ -131,10 +132,40 @@ pub fn cursor_mangling(ctx: &BindgenContext,
131132
}
132133

133134
// Try to undo backend linkage munging (prepended _, generally)
135+
//
136+
// TODO(emilio): This is wrong when the target system is not the host
137+
// system. See https://github.com/servo/rust-bindgen/issues/593
134138
if cfg!(target_os = "macos") {
135139
mangling.remove(0);
136140
}
137141

142+
if cursor.kind() == clang_sys::CXCursor_Destructor {
143+
// With old (3.8-) libclang versions, and the Itanium ABI, clang returns
144+
// the "destructor group 0" symbol, which means that it'll try to free
145+
// memory, which definitely isn't what we want.
146+
//
147+
// Explicitly force the destructor group 1 symbol.
148+
//
149+
// See http://refspecs.linuxbase.org/cxxabi-1.83.html#mangling-special
150+
// for the reference, and http://stackoverflow.com/a/6614369/1091587 for
151+
// a more friendly explanation.
152+
//
153+
// We don't need to do this for constructors since clang seems to always
154+
// have returned the C1 constructor.
155+
//
156+
// FIXME(emilio): Can a legit symbol in other ABIs end with this string?
157+
// I don't think so, but if it can this would become a linker error
158+
// anyway, not an invalid free at runtime.
159+
//
160+
// TODO(emilio, #611): Use cpp_demangle if this becomes nastier with
161+
// time.
162+
if mangling.ends_with("D0Ev") {
163+
let new_len = mangling.len() - 4;
164+
mangling.truncate(new_len);
165+
mangling.push_str("D1Ev");
166+
}
167+
}
168+
138169
Some(mangling)
139170
}
140171

@@ -220,13 +251,14 @@ impl FunctionSig {
220251

221252
let is_method = cursor.kind() == CXCursor_CXXMethod;
222253
let is_constructor = cursor.kind() == CXCursor_Constructor;
223-
if (is_constructor || is_method) &&
254+
let is_destructor = cursor.kind() == CXCursor_Destructor;
255+
if (is_constructor || is_destructor || is_method) &&
224256
cursor.lexical_parent() != cursor.semantic_parent() {
225257
// Only parse constructors once.
226258
return Err(ParseError::Continue);
227259
}
228260

229-
if is_method || is_constructor {
261+
if is_method || is_constructor || is_destructor {
230262
let is_const = is_method && cursor.method_is_const();
231263
let is_virtual = is_method && cursor.method_is_virtual();
232264
let is_static = is_method && cursor.method_is_static();
@@ -292,9 +324,9 @@ impl ClangSubItemParser for Function {
292324
-> Result<ParseResult<Self>, ParseError> {
293325
use clang_sys::*;
294326
match cursor.kind() {
295-
// FIXME(emilio): Generate destructors properly.
296327
CXCursor_FunctionDecl |
297328
CXCursor_Constructor |
329+
CXCursor_Destructor |
298330
CXCursor_CXXMethod => {}
299331
_ => return Err(ParseError::Continue),
300332
};
@@ -325,9 +357,23 @@ impl ClangSubItemParser for Function {
325357
let sig =
326358
try!(Item::from_ty(&cursor.cur_type(), cursor, None, context));
327359

328-
let name = cursor.spelling();
360+
let mut name = cursor.spelling();
329361
assert!(!name.is_empty(), "Empty function name?");
330362

363+
if cursor.kind() == CXCursor_Destructor {
364+
// Remove the leading `~`. The alternative to this is special-casing
365+
// code-generation for destructor functions, which seems less than
366+
// ideal.
367+
if name.starts_with('~') {
368+
name.remove(0);
369+
}
370+
371+
// Add a suffix to avoid colliding with constructors. This would be
372+
// technically fine (since we handle duplicated functions/methods),
373+
// but seems easy enough to handle it here.
374+
name.push_str("_destructor");
375+
}
376+
331377
let mut mangled_name = cursor_mangling(context, &cursor);
332378
if mangled_name.as_ref() == Some(&name) {
333379
mangled_name = None;

‎src/lib.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,8 @@ pub struct CodegenConfig {
109109
pub methods: bool,
110110
/// Whether to generate constructors.
111111
pub constructors: bool,
112+
/// Whether to generate destructors.
113+
pub destructors: bool,
112114
}
113115

114116
impl CodegenConfig {
@@ -120,6 +122,7 @@ impl CodegenConfig {
120122
vars: true,
121123
methods: true,
122124
constructors: true,
125+
destructors: true,
123126
}
124127
}
125128

@@ -131,6 +134,7 @@ impl CodegenConfig {
131134
vars: false,
132135
methods: false,
133136
constructors: false,
137+
destructors: false,
134138
}
135139
}
136140
}

‎src/options.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,8 @@ pub fn builder_from_flags<I>
111111
Arg::with_name("generate")
112112
.long("generate")
113113
.help("Generate a given kind of items, split by commas. \
114-
Valid values are \"functions\",\"types\", \"vars\" and \
115-
\"methods\".")
114+
Valid values are \"functions\",\"types\", \"vars\", \
115+
\"methods\", \"constructors\" and \"destructors\".")
116116
.takes_value(true),
117117
Arg::with_name("ignore-methods")
118118
.long("ignore-methods")
@@ -271,6 +271,8 @@ pub fn builder_from_flags<I>
271271
"types" => config.types = true,
272272
"vars" => config.vars = true,
273273
"methods" => config.methods = true,
274+
"constructors" => config.constructors = true,
275+
"destructors" => config.destructors = true,
274276
_ => {
275277
return Err(Error::new(ErrorKind::Other,
276278
"Unknown generate item"));

‎tests/expectations/tests/bitfield-method-same-name.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,14 @@ impl Foo {
5454
}
5555
#[inline]
5656
pub unsafe fn type_(&mut self) -> ::std::os::raw::c_schar {
57-
Foo_type(&mut *self)
57+
Foo_type(self)
5858
}
5959
#[inline]
6060
pub unsafe fn set_type_(&mut self, c: ::std::os::raw::c_schar) {
61-
Foo_set_type_(&mut *self, c)
61+
Foo_set_type_(self, c)
6262
}
6363
#[inline]
6464
pub unsafe fn set_type(&mut self, c: ::std::os::raw::c_schar) {
65-
Foo_set_type(&mut *self, c)
65+
Foo_set_type(self, c)
6666
}
6767
}

‎tests/expectations/tests/class.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -283,14 +283,14 @@ impl Clone for RealAbstractionWithTonsOfMethods {
283283
}
284284
impl RealAbstractionWithTonsOfMethods {
285285
#[inline]
286-
pub unsafe fn bar(&self) { RealAbstractionWithTonsOfMethods_bar(&*self) }
286+
pub unsafe fn bar(&self) { RealAbstractionWithTonsOfMethods_bar(self) }
287287
#[inline]
288288
pub unsafe fn bar1(&mut self) {
289-
RealAbstractionWithTonsOfMethods_bar1(&mut *self)
289+
RealAbstractionWithTonsOfMethods_bar1(self)
290290
}
291291
#[inline]
292292
pub unsafe fn bar2(&mut self, foo: ::std::os::raw::c_int) {
293-
RealAbstractionWithTonsOfMethods_bar2(&mut *self, foo)
293+
RealAbstractionWithTonsOfMethods_bar2(self, foo)
294294
}
295295
#[inline]
296296
pub unsafe fn sta() { RealAbstractionWithTonsOfMethods_sta() }

‎tests/expectations/tests/class_with_typedef.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,18 +70,18 @@ impl Default for C {
7070
}
7171
impl C {
7272
#[inline]
73-
pub unsafe fn method(&mut self, c: C_MyInt) { C_method(&mut *self, c) }
73+
pub unsafe fn method(&mut self, c: C_MyInt) { C_method(self, c) }
7474
#[inline]
7575
pub unsafe fn methodRef(&mut self, c: *mut C_MyInt) {
76-
C_methodRef(&mut *self, c)
76+
C_methodRef(self, c)
7777
}
7878
#[inline]
7979
pub unsafe fn complexMethodRef(&mut self, c: *mut C_Lookup) {
80-
C_complexMethodRef(&mut *self, c)
80+
C_complexMethodRef(self, c)
8181
}
8282
#[inline]
8383
pub unsafe fn anotherMethod(&mut self, c: AnotherInt) {
84-
C_anotherMethod(&mut *self, c)
84+
C_anotherMethod(self, c)
8585
}
8686
}
8787
#[repr(C)]

0 commit comments

Comments
 (0)