Ensure that every item is in some module's children list #770

fitzgen · 2017-06-19T21:11:04Z

Previously, if an item's parent was not a module (eg a nested class definition whose parent it the outer class definition) and the parent was not whitelisted but the item was transitively whitelisted, then we could generate uses of the item without emitting any definition for it. This could happen because we were relying on the outer type calling for code generation on its inner types, but that relies on us doing code generation for the outer type, which won't happen if the outer type is not whitelisted.

This commit avoids this gotcha by ensuring that all items end up in a module's children list, and so will be code generated even if their parent is not whitelisted.

This does have the downside of changing the relative order of some of the emitted code, and so this has a big diff (as will the next bindgen update for downstream dependencies) but I actually think the newer order makes more sense, for what that is worth.

Fixes #769

r? @emilio

highfive · 2017-06-19T21:11:09Z

Warning

These commits modify unsafe code. Please review it carefully!

fitzgen · 2017-06-19T23:18:10Z

Looks like this uncovered a bug with static variables whose type is a template type parameter. Coming up with a fix for that too.

fitzgen · 2017-06-19T23:59:23Z

Huh, now I'm getting tripped up by the recent naming scheme change for the layout tests for template instantiations. It appears it isn't as unique as we'd hoped.

fitzgen · 2017-06-20T00:05:09Z

Going to use the overloaded function counter instead.

emilio

Looks sensible, r=me

emilio · 2017-06-20T08:31:29Z

src/ir/context.rs

-        // Be sure to track all the generated children under namespace, even
-        // those generated after resolving typerefs, etc.
+        // Ensure that every item (other than the root module) is in a module's
+        // children list.


Maybe point to the issue this is fixing or something? it may be slightly counter-intuitive otherwise.

emilio · 2017-06-20T08:31:42Z

src/ir/context.rs

                }
            }
+
+            if !added_as_child {
+                if let Some(mut current_module) = self.items.get_mut(&self.current_module) {


You can probably unwrap here instead of asserting below?

emilio · 2017-06-20T08:35:42Z

This seems to make the order of the generated bindings not match across libclang versions though, and thus CI is red.

fitzgen · 2017-06-20T16:35:42Z

Thanks for review! (Sorry it's such a huge diff)

This seems to make the order of the generated bindings not match across libclang versions though, and thus CI is red.

I'll make the children an ItemSet instead of a Vec. This has two advantages:

An ItemSet is ordered, so we should get deterministic iteration of a module's children (and therefore emit them in that deterministic order during codegen)
It disallows duplicate entries (which should be harmless, but its so easy to prevent that it seems crazy not to prevent it).

Before this commit, test-one.sh was unusable with tests/headers/template.hpp because there were too many things with "template.hpp" as a suffix. This allows us to specify "/template.hpp" to run the test.

fitzgen · 2017-06-20T17:41:34Z

I expect CI will still be red, since I need to get the diffs for libclang 3.8 and 3.9

fitzgen · 2017-06-20T21:49:11Z

This might need some more updates for other libclang versions, but I'm not 100%.

In any case, it probably deserves a second glance, since a bunch of code changed from the version you reviewed before, @emilio.

r? @emilio

Previously, if an item's parent was not a module (eg a nested class definition whose parent it the outer class definition) and the parent was not whitelisted but the item was transitively whitelisted, then we could generate uses of the item without emitting any definition for it. This could happen because we were relying on the outer type calling for code generation on its inner types, but that relies on us doing code generation for the outer type, which won't happen if the outer type is not whitelisted. This commit avoids this gotcha by ensuring that all items end up in a module's children list, and so will be code generated even if their parent is not whitelisted. Fixes rust-lang#769

There's a lot of these edges so it helps to make them un-bold.

This commit adds assertions that run when the "testing_only_extra_assertions" feature is enabled, which make sure that every single item we parse is a child of some ancestor module.

fitzgen · 2017-06-20T22:35:54Z

Ok, tests for other libclang versions should be good to go.

emilio

Looks sensible, some questions

emilio · 2017-06-20T23:14:53Z

tests/expectations/tests/enum_and_vtable_mangling.rs

@@ -34,3 +34,7 @@ impl Clone for C {
 impl Default for C {
    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
+extern "C" {
+    #[link_name = "_ZN1C5matchEv"]
+    pub fn C_match(this: *mut ::std::os::raw::c_void);


So, these are somewhat surprising, because AIUI they aren't referenced from anywhere... Perhaps it's not a big deal...

These get generated because the functions are no longer defined inline in the header, just declared -- maybe I am misunderstanding you?

Err, my fault, for some reason I thought this was a destructor... nevermind.

emilio · 2017-06-20T23:17:25Z

src/ir/context.rs

@@ -1038,13 +1105,14 @@ impl<'ctx> BindgenContext<'ctx> {
                        let sub_item = Item::new(sub_id,
                                                 None,
                                                 None,
-                                                 template_decl_id,
+                                                 self.current_module,


So we'll instantiate templates against the current module? Even if they're nested? That looks suspicious. The parentage relationship for...

struct Foo { template<typename T> struct Bar { T baz; }; Bar<int> m_member; };

So, if I'm reading this correctly, we'll scope Bar<int> to the gloal scope, not to Foo, right? Can you justify that? If it doesn't matter (which I think it could be true). Could you add a comment?

I just changed from one well-known-but-sort-of-bogus parent to another well-known-but-sort-of-bogus parent.

They used to always be parented by the template definition, which doesn't really make sense either, just happened to be handy when the "real" parent wasn't.

I can definitely add a comment.

Comment added.

emilio · 2017-06-20T23:49:09Z

@bors-servo r+

bors-servo · 2017-06-20T23:49:10Z

📌 Commit 5230565 has been approved by emilio

bors-servo · 2017-06-20T23:49:12Z

⌛ Testing commit 5230565 with merge 232a21e...

@emilio

Ensure that every item is in some module's children list Previously, if an item's parent was not a module (eg a nested class definition whose parent it the outer class definition) and the parent was not whitelisted but the item was transitively whitelisted, then we could generate uses of the item without emitting any definition for it. This could happen because we were relying on the outer type calling for code generation on its inner types, but that relies on us doing code generation for the outer type, which won't happen if the outer type is not whitelisted. This commit avoids this gotcha by ensuring that all items end up in a module's children list, and so will be code generated even if their parent is not whitelisted. This does have the downside of changing the relative order of some of the emitted code, and so this has a big diff (as will the next bindgen update for downstream dependencies) but I actually think the newer order makes more sense, for what that is worth. Fixes #769 r? @emilio

fitzgen · 2017-06-20T23:57:26Z

Thanks!

bors-servo · 2017-06-21T00:15:56Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing 232a21e to master...

highfive assigned emilio Jun 19, 2017

highfive added the S-awaiting-review label Jun 19, 2017

fitzgen mentioned this pull request Jun 19, 2017

WIP smup servo/mozjs#119

Closed

fitzgen force-pushed the issue-769-bad-instantiation-test branch 3 times, most recently from fcfc166 to 6a92092 Compare June 19, 2017 23:48

fitzgen force-pushed the issue-769-bad-instantiation-test branch from 6a92092 to 7519eb9 Compare June 20, 2017 00:06

emilio approved these changes Jun 20, 2017

View reviewed changes

Allow path separators in test-one.sh

26094ea

Before this commit, test-one.sh was unusable with tests/headers/template.hpp because there were too many things with "template.hpp" as a suffix. This allows us to specify "/template.hpp" to run the test.

fitzgen force-pushed the issue-769-bad-instantiation-test branch from 7519eb9 to 8536a6b Compare June 20, 2017 17:40

fitzgen force-pushed the issue-769-bad-instantiation-test branch from 8536a6b to c79a0a2 Compare June 20, 2017 21:48

fitzgen force-pushed the issue-769-bad-instantiation-test branch from c79a0a2 to 06a96bb Compare June 20, 2017 22:04

fitzgen added 3 commits June 20, 2017 15:35

Add dotted lines from modules to children in IR visualization

33272cd

There's a lot of these edges so it helps to make them un-bold.

Assert that every item is a child of an ancestor module

c736faa

This commit adds assertions that run when the "testing_only_extra_assertions" feature is enabled, which make sure that every single item we parse is a child of some ancestor module.

fitzgen force-pushed the issue-769-bad-instantiation-test branch from 06a96bb to c736faa Compare June 20, 2017 22:35

emilio reviewed Jun 20, 2017

View reviewed changes

Add a comment about template instantiation parentage

5230565

highfive removed the S-awaiting-review label Jun 20, 2017

highfive added the S-awaiting-merge label Jun 20, 2017

bors-servo merged commit 5230565 into rust-lang:master Jun 21, 2017

highfive removed the S-awaiting-merge label Jun 21, 2017

fitzgen deleted the issue-769-bad-instantiation-test branch June 21, 2017 00:16

emilio mentioned this pull request Jul 18, 2017

Bindgen tries to generate bindings for methods even with --ignore-methods #826

Closed

Ensure that every item is in some module's children list #770

Ensure that every item is in some module's children list #770

Uh oh!

Conversation

fitzgen commented Jun 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 19, 2017

Uh oh!

fitzgen commented Jun 19, 2017

Uh oh!

fitzgen commented Jun 19, 2017

Uh oh!

fitzgen commented Jun 20, 2017

Uh oh!

emilio 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!

emilio commented Jun 20, 2017

Uh oh!

fitzgen commented Jun 20, 2017

Uh oh!

fitzgen commented Jun 20, 2017

Uh oh!

fitzgen commented Jun 20, 2017

Uh oh!

fitzgen commented Jun 20, 2017

Uh oh!

emilio 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!

emilio commented Jun 20, 2017

Uh oh!

bors-servo commented Jun 20, 2017

Uh oh!

bors-servo commented Jun 20, 2017

Uh oh!

fitzgen commented Jun 20, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

Uh oh!

fitzgen commented Jun 19, 2017 •

edited

Loading