Destructor codegen #608

emilio · 2017-04-03T11:34:50Z

Based on #542, and on top of #606, with a bunch more tests and fixes.

highfive · 2017-04-03T11:34:54Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2017-04-03T11:35:01Z

emilio · 2017-04-03T12:25:32Z

Hm... So this still fails for 3.8. The reason for that is that the symbol produced is for the D0 destructor group, instead of D1. We definitely want the D1 group, so I'm going to change the symbol manually to point to the correct one.

See http://stackoverflow.com/questions/6613870/gnu-gcc-g-why-does-it-generate-multiple-dtors/6614369#6614369 for a quick description of the problem.

emilio · 2017-04-03T12:37:46Z

Added a commit that should fix that.

fitzgen

Sweet! Thanks!

Before landing, please:

add tests for when we're explicitly not generating ctors and dtors
squash to preserve bisect-ability, since tests started failing in the middle of the commit series

Thanks again!

fitzgen · 2017-04-03T16:24:49Z

src/ir/comp.rs

@@ -434,6 +445,11 @@ impl CompInfo {
        &self.constructors
    }

+    /// Get this type's destructor.
+    pub fn destructor(&self) -> &Option<ItemId> {
+        &self.destructor


I don't see any reason to avoid copies here; it'll just make using this slightly more annoying. Instead we should do:

pub fn destructor(&self) -> Option<ItemId> { self.destructor }

since Option<T> is Copy if T is Copy.

fitzgen · 2017-04-03T16:28:29Z

tests/headers/gen-constructors.hpp

@@ -0,0 +1,6 @@
+// bindgen-flags: --generate types,constructors,functions


We should also test when we are not generating constructors.

fitzgen · 2017-04-03T16:31:37Z

src/ir/comp.rs

@@ -446,8 +451,8 @@ impl CompInfo {
    }

    /// Get this type's destructor.
-    pub fn destructor(&self) -> &Option<ItemId> {
-        &self.destructor
+    pub fn destructor(&self) -> Option<(bool, ItemId)> {


That bool is a little gross. As a calller of this API, now I have to remember wait does true mean virtual or not? Why not reuse MethodKind here?

Also, the doc comment should be updated to describe the extra information exposed.

fitzgen · 2017-04-03T16:32:56Z

src/ir/function.rs

+            // 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");


We definitely don't want to allow bindings users to mix up overloaded constructors and destructors!

fitzgen · 2017-04-03T16:33:30Z

tests/headers/gen-destructors.hpp

@@ -0,0 +1,7 @@
+// bindgen-flags: --generate types,destructors,functions


Again, we should also test not generating destructors.

fitzgen · 2017-04-03T16:35:22Z

bindgen-integration/cpp/Test.cc

@@ -21,6 +21,15 @@ Test::Test(double foo)
  , m_double(foo)
 {}

+AutoRestoreBool::AutoRestoreBool(bool* ptr)


Thanks for adding an integration test!

fitzgen · 2017-04-03T16:39:08Z

src/ir/function.rs

+        //
+        // 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.


With weak symbols it could be a runtime error (I'm dealing with this in the SM bindings + mozglue and it is so frustrating), but still memory safe.

This only applies to the Itanium C++ ABI, which pretty much means not windows, so we could if !cfg!(target_os = "windows") { ... } all this stuff.

Also, we could parse the symbol (using my cpp_demangle crate) and check if it is the a deleting destructor or not.

https://docs.rs/cpp_demangle/0.2.1/cpp_demangle/ast/enum.CtorDtorName.html

I would just need to expose a getter for a parsed symbol's AST.

How formal and proper do we want to make these hacks? ;)

The problem of cfg! checks is basically #593, so I'd prefer to remove the existing ones instead of adding newer ones.

…ior.

emilio · 2017-04-03T23:11:09Z

Instead of squashing, I moved the symbol hack to the beginning of the PR (GitHub stills shows it chronologically, which sucks, but it's the parent of all-but-the-first commit). That should preserve bisectability.

I've left a note about cpp_demangle.

@bors-servo r=fitzgen

bors-servo · 2017-04-03T23:11:10Z

📌 Commit 1f53966 has been approved by fitzgen

bors-servo · 2017-04-03T23:11:12Z

⌛ Testing commit 1f53966 with merge 5e85271...

Destructor codegen Based on #542, and on top of #606, with a bunch more tests and fixes.

bors-servo · 2017-04-03T23:35:13Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 5e85271 to master...

highfive added the S-awaiting-review label Apr 3, 2017

highfive assigned fitzgen Apr 3, 2017

emilio mentioned this pull request Apr 3, 2017

Added codegen for destructors #542

Closed

options: Allow configuring the generation of constructors.

f313401

emilio force-pushed the destructor-codegen branch from 6190617 to afacb8d Compare April 3, 2017 11:44

emilio mentioned this pull request Apr 3, 2017

options: Allow configuring the generation of constructors. #606

Closed

emilio force-pushed the destructor-codegen branch from 3c8dafb to f5188a6 Compare April 3, 2017 13:00

fitzgen approved these changes Apr 3, 2017

View reviewed changes

emilio and others added 8 commits April 4, 2017 01:05

ir: Force the D1 destructor symbol even in older libclang versions.

8f1e5f5

Add codegen for destructors.

1714f94

Refactor a bit and do not generate virtual destructors.

5cf424c

options: Allow configuring destructors via CLI.

fcbd61c

codegen: Rename the destructor methods to "destruct".

ebcd369

codegen: Add integration tests for destructors.

7aed4ba

codegen: Simplify method implementations.

a4e606b

tests: Add negative tests for constructor/destructor generation behav…

0dc7bcd

…ior.

emilio force-pushed the destructor-codegen branch from f5188a6 to 0dc7bcd Compare April 3, 2017 23:05

ir: Add a note about cpp_demangle.

1f53966

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 3, 2017

bors-servo pushed a commit that referenced this pull request Apr 3, 2017

Auto merge of #608 - emilio:destructor-codegen, r=fitzgen

5e85271

Destructor codegen Based on #542, and on top of #606, with a bunch more tests and fixes.

bors-servo merged commit 1f53966 into rust-lang:master Apr 3, 2017

highfive removed the S-awaiting-merge label Apr 3, 2017

This was referenced Apr 3, 2017

Treat char as c_char #609

Merged

ir: Handle char in a more cross-platform way when possible. #610

Merged

emilio deleted the destructor-codegen branch April 3, 2017 23:49

emilio mentioned this pull request Apr 7, 2017

No bindings are being generated for destructors #529

Closed

Manishearth unassigned fitzgen Jul 25, 2017

		@@ -0,0 +1,6 @@
		// bindgen-flags: --generate types,constructors,functions

		@@ -0,0 +1,7 @@
		// bindgen-flags: --generate types,destructors,functions

Destructor codegen #608

Destructor codegen #608

Uh oh!

Conversation

emilio commented Apr 3, 2017

Uh oh!

highfive commented Apr 3, 2017

Uh oh!

emilio commented Apr 3, 2017

Uh oh!

emilio commented Apr 3, 2017

Uh oh!

emilio commented Apr 3, 2017

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Apr 3, 2017

Uh oh!

bors-servo commented Apr 3, 2017

Uh oh!

bors-servo commented Apr 3, 2017

Uh oh!

bors-servo commented Apr 3, 2017

Uh oh!

Uh oh!