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.

Author: bors-servo

bindgen-integration/cpp/Test.cc

@@ -21,6 +21,15 @@ Test::Test(double foo)
   , m_double(foo)
 {}
 
+AutoRestoreBool::AutoRestoreBool(bool* ptr)
+  : m_ptr(ptr)
+  , m_value(*ptr)
+{}
+
+AutoRestoreBool::~AutoRestoreBool() {
+  *m_ptr = m_value;
+}
+
 namespace bitfields {
 
 bool
@@ -47,4 +56,4 @@ Third::assert(int first, bool second, ItemKind third)
         kind == third;
 }
 
-}
+} // namespace bitfields

bindgen-integration/cpp/Test.h

@@ -64,3 +64,11 @@ struct Third {
 };
 
 } // namespace bitfields
+
+struct AutoRestoreBool {
+  bool* m_ptr;
+  bool m_value;
+
+  AutoRestoreBool(bool*);
+  ~AutoRestoreBool();
+};

bindgen-integration/src/lib.rs

@@ -101,3 +101,21 @@ fn test_bitfields_third() {
                      bindings::bitfields::ItemKind::ITEM_KIND_TRES)
     });
 }
+
+impl Drop for bindings::AutoRestoreBool {
+    fn drop(&mut self) {
+        unsafe { bindings::AutoRestoreBool::destruct(self) }
+    }
+}
+
+#[test]
+fn test_destructors() {
+    let mut v = true;
+
+    {
+        let auto_restore = unsafe { bindings::AutoRestoreBool::new(&mut v) };
+        v = false;
+    }
+
+    assert!(v, "Should've been restored when going out of scope");
+}

src/codegen/mod.rs

@@ -1597,6 +1597,24 @@ impl CodeGenerator for CompInfo {
                                         self);
                 }
             }
+
+            if ctx.options().codegen_config.destructors {
+                if let Some((is_virtual, destructor)) = self.destructor() {
+                    let kind = if is_virtual {
+                        MethodKind::VirtualDestructor
+                    } else {
+                        MethodKind::Destructor
+                    };
+
+                    Method::new(kind, destructor, false)
+                        .codegen_method(ctx,
+                                        &mut methods,
+                                        &mut method_names,
+                                        result,
+                                        whitelisted_items,
+                                        self);
+                }
+            }
         }
 
         // NB: We can't use to_rust_ty here since for opaque types this tries to
@@ -1693,6 +1711,7 @@ impl MethodCodegen for Method {
         if self.is_virtual() {
             return; // FIXME
         }
+
         // First of all, output the actual function.
         let function_item = ctx.resolve_item(self.signature());
         function_item.codegen(ctx, result, whitelisted_items, &());
@@ -1701,6 +1720,7 @@ impl MethodCodegen for Method {
         let signature_item = ctx.resolve_item(function.signature());
         let mut name = match self.kind() {
             MethodKind::Constructor => "new".into(),
+            MethodKind::Destructor => "destruct".into(),
             _ => function.name().to_owned(),
         };
 
@@ -1801,11 +1821,7 @@ impl MethodCodegen for Method {
             exprs[0] = quote_expr!(ctx.ext_cx(), &mut __bindgen_tmp);
         } else if !self.is_static() {
             assert!(!exprs.is_empty());
-            exprs[0] = if self.is_const() {
-                quote_expr!(ctx.ext_cx(), &*self)
-            } else {
-                quote_expr!(ctx.ext_cx(), &mut *self)
-            };
+            exprs[0] = quote_expr!(ctx.ext_cx(), self);
         };
 
         let call = aster::expr::ExprBuilder::new()

src/ir/comp.rs

@@ -26,6 +26,10 @@ pub enum MethodKind {
     /// A constructor. We represent it as method for convenience, to avoid code
     /// duplication.
     Constructor,
+    /// A destructor.
+    Destructor,
+    /// A virtual destructor.
+    VirtualDestructor,
     /// A static method.
     Static,
     /// A normal method.
@@ -61,14 +65,21 @@ impl Method {
         self.kind
     }
 
+    /// Is this a destructor method?
+    pub fn is_destructor(&self) -> bool {
+        self.kind == MethodKind::Destructor ||
+        self.kind == MethodKind::VirtualDestructor
+    }
+
     /// Is this a constructor?
     pub fn is_constructor(&self) -> bool {
         self.kind == MethodKind::Constructor
     }
 
     /// Is this a virtual method?
     pub fn is_virtual(&self) -> bool {
-        self.kind == MethodKind::Virtual
+        self.kind == MethodKind::Virtual ||
+        self.kind == MethodKind::VirtualDestructor
     }
 
     /// Is this a static method?
@@ -250,6 +261,10 @@ pub struct CompInfo {
     /// The different constructors this struct or class contains.
     constructors: Vec<ItemId>,
 
+    /// The destructor of this type. The bool represents whether this destructor
+    /// is virtual.
+    destructor: Option<(bool, ItemId)>,
+
     /// Vector of classes this one inherits from.
     base_members: Vec<Base>,
 
@@ -321,6 +336,7 @@ impl CompInfo {
             template_params: vec![],
             methods: vec![],
             constructors: vec![],
+            destructor: None,
             base_members: vec![],
             inner_types: vec![],
             inner_vars: vec![],
@@ -434,6 +450,11 @@ impl CompInfo {
         &self.constructors
     }
 
+    /// Get this type's destructor.
+    pub fn destructor(&self) -> Option<(bool, ItemId)> {
+        self.destructor
+    }
+
     /// What kind of compound type is this?
     pub fn kind(&self) -> CompKind {
         self.kind
@@ -657,8 +678,9 @@ impl CompInfo {
                         CXCursor_Constructor => {
                             ci.constructors.push(signature);
                         }
-                        // TODO(emilio): Bind the destructor?
-                        CXCursor_Destructor => {}
+                        CXCursor_Destructor => {
+                            ci.destructor = Some((is_virtual, signature));
+                        }
                         CXCursor_CXXMethod => {
                             let is_const = cur.method_is_const();
                             let method_kind = if is_static {

src/ir/function.rs

@@ -114,6 +114,7 @@ fn get_abi(cc: CXCallingConv) -> Option<abi::Abi> {
 pub fn cursor_mangling(ctx: &BindgenContext,
                        cursor: &clang::Cursor)
                        -> Option<String> {
+    use clang_sys;
     if !ctx.options().enable_mangling {
         return None;
     }
@@ -131,10 +132,40 @@ pub fn cursor_mangling(ctx: &BindgenContext,
     }
 
     // Try to undo backend linkage munging (prepended _, generally)
+    //
+    // TODO(emilio): This is wrong when the target system is not the host
+    // system. See https://github.com/servo/rust-bindgen/issues/593
     if cfg!(target_os = "macos") {
         mangling.remove(0);
     }
 
+    if cursor.kind() == clang_sys::CXCursor_Destructor {
+        // With old (3.8-) libclang versions, and the Itanium ABI, clang returns
+        // the "destructor group 0" symbol, which means that it'll try to free
+        // memory, which definitely isn't what we want.
+        //
+        // Explicitly force the destructor group 1 symbol.
+        //
+        // See http://refspecs.linuxbase.org/cxxabi-1.83.html#mangling-special
+        // for the reference, and http://stackoverflow.com/a/6614369/1091587 for
+        // a more friendly explanation.
+        //
+        // We don't need to do this for constructors since clang seems to always
+        // have returned the C1 constructor.
+        //
+        // FIXME(emilio): Can a legit symbol in other ABIs end with this string?
+        // I don't think so, but if it can this would become a linker error
+        // anyway, not an invalid free at runtime.
+        //
+        // TODO(emilio, #611): Use cpp_demangle if this becomes nastier with
+        // time.
+        if mangling.ends_with("D0Ev") {
+            let new_len = mangling.len() - 4;
+            mangling.truncate(new_len);
+            mangling.push_str("D1Ev");
+        }
+    }
+
     Some(mangling)
 }
 
@@ -220,13 +251,14 @@ impl FunctionSig {
 
         let is_method = cursor.kind() == CXCursor_CXXMethod;
         let is_constructor = cursor.kind() == CXCursor_Constructor;
-        if (is_constructor || is_method) &&
+        let is_destructor = cursor.kind() == CXCursor_Destructor;
+        if (is_constructor || is_destructor || is_method) &&
            cursor.lexical_parent() != cursor.semantic_parent() {
             // Only parse constructors once.
             return Err(ParseError::Continue);
         }
 
-        if is_method || is_constructor {
+        if is_method || is_constructor || is_destructor {
             let is_const = is_method && cursor.method_is_const();
             let is_virtual = is_method && cursor.method_is_virtual();
             let is_static = is_method && cursor.method_is_static();
@@ -292,9 +324,9 @@ impl ClangSubItemParser for Function {
              -> Result<ParseResult<Self>, ParseError> {
         use clang_sys::*;
         match cursor.kind() {
-            // FIXME(emilio): Generate destructors properly.
             CXCursor_FunctionDecl |
             CXCursor_Constructor |
+            CXCursor_Destructor |
             CXCursor_CXXMethod => {}
             _ => return Err(ParseError::Continue),
         };
@@ -325,9 +357,23 @@ impl ClangSubItemParser for Function {
         let sig =
             try!(Item::from_ty(&cursor.cur_type(), cursor, None, context));
 
-        let name = cursor.spelling();
+        let mut name = cursor.spelling();
         assert!(!name.is_empty(), "Empty function name?");
 
+        if cursor.kind() == CXCursor_Destructor {
+            // Remove the leading `~`. The alternative to this is special-casing
+            // code-generation for destructor functions, which seems less than
+            // ideal.
+            if name.starts_with('~') {
+                name.remove(0);
+            }
+
+            // Add a suffix to avoid colliding with constructors. This would be
+            // technically fine (since we handle duplicated functions/methods),
+            // but seems easy enough to handle it here.
+            name.push_str("_destructor");
+        }
+
         let mut mangled_name = cursor_mangling(context, &cursor);
         if mangled_name.as_ref() == Some(&name) {
             mangled_name = None;

src/lib.rs

@@ -109,6 +109,8 @@ pub struct CodegenConfig {
     pub methods: bool,
     /// Whether to generate constructors.
     pub constructors: bool,
+    /// Whether to generate destructors.
+    pub destructors: bool,
 }
 
 impl CodegenConfig {
@@ -120,6 +122,7 @@ impl CodegenConfig {
             vars: true,
             methods: true,
             constructors: true,
+            destructors: true,
         }
     }
 
@@ -131,6 +134,7 @@ impl CodegenConfig {
             vars: false,
             methods: false,
             constructors: false,
+            destructors: false,
         }
     }
 }

src/options.rs

@@ -111,8 +111,8 @@ pub fn builder_from_flags<I>
             Arg::with_name("generate")
                 .long("generate")
                 .help("Generate a given kind of items, split by commas. \
-                       Valid values are \"functions\",\"types\", \"vars\" and \
-                       \"methods\".")
+                       Valid values are \"functions\",\"types\", \"vars\", \
+                       \"methods\", \"constructors\" and \"destructors\".")
                 .takes_value(true),
             Arg::with_name("ignore-methods")
                 .long("ignore-methods")
@@ -271,6 +271,8 @@ pub fn builder_from_flags<I>
                 "types" => config.types = true,
                 "vars" => config.vars = true,
                 "methods" => config.methods = true,
+                "constructors" => config.constructors = true,
+                "destructors" => config.destructors = true,
                 _ => {
                     return Err(Error::new(ErrorKind::Other,
                                           "Unknown generate item"));

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

@@ -54,14 +54,14 @@ impl Foo {
     }
     #[inline]
     pub unsafe fn type_(&mut self) -> ::std::os::raw::c_schar {
-        Foo_type(&mut *self)
+        Foo_type(self)
     }
     #[inline]
     pub unsafe fn set_type_(&mut self, c: ::std::os::raw::c_schar) {
-        Foo_set_type_(&mut *self, c)
+        Foo_set_type_(self, c)
     }
     #[inline]
     pub unsafe fn set_type(&mut self, c: ::std::os::raw::c_schar) {
-        Foo_set_type(&mut *self, c)
+        Foo_set_type(self, c)
     }
 }

tests/expectations/tests/class.rs

@@ -283,14 +283,14 @@ impl Clone for RealAbstractionWithTonsOfMethods {
 }
 impl RealAbstractionWithTonsOfMethods {
     #[inline]
-    pub unsafe fn bar(&self) { RealAbstractionWithTonsOfMethods_bar(&*self) }
+    pub unsafe fn bar(&self) { RealAbstractionWithTonsOfMethods_bar(self) }
     #[inline]
     pub unsafe fn bar1(&mut self) {
-        RealAbstractionWithTonsOfMethods_bar1(&mut *self)
+        RealAbstractionWithTonsOfMethods_bar1(self)
     }
     #[inline]
     pub unsafe fn bar2(&mut self, foo: ::std::os::raw::c_int) {
-        RealAbstractionWithTonsOfMethods_bar2(&mut *self, foo)
+        RealAbstractionWithTonsOfMethods_bar2(self, foo)
     }
     #[inline]
     pub unsafe fn sta() { RealAbstractionWithTonsOfMethods_sta() }

tests/expectations/tests/class_with_typedef.rs

@@ -70,18 +70,18 @@ impl Default for C {
 }
 impl C {
     #[inline]
-    pub unsafe fn method(&mut self, c: C_MyInt) { C_method(&mut *self, c) }
+    pub unsafe fn method(&mut self, c: C_MyInt) { C_method(self, c) }
     #[inline]
     pub unsafe fn methodRef(&mut self, c: *mut C_MyInt) {
-        C_methodRef(&mut *self, c)
+        C_methodRef(self, c)
     }
     #[inline]
     pub unsafe fn complexMethodRef(&mut self, c: *mut C_Lookup) {
-        C_complexMethodRef(&mut *self, c)
+        C_complexMethodRef(self, c)
     }
     #[inline]
     pub unsafe fn anotherMethod(&mut self, c: AnotherInt) {
-        C_anotherMethod(&mut *self, c)
+        C_anotherMethod(self, c)
     }
 }
 #[repr(C)]