Add assert for dangling references without an associated ItemId #312

impowski · 2016-12-02T20:43:13Z

So I think this is it? Fix issue #209
r? @fitzgen

fitzgen

Can you move this to its own function, that is called from right here? Additionally, we want this function to be a no-op in release builds.

Also, we can remove the NOTE comment. This doesn't need testing itself, this is more of extra testing code for the rest of bindgen.

emilio · 2016-12-03T00:43:16Z

libbindgen/src/ir/context.rs

@@ -431,6 +431,18 @@ impl<'ctx> BindgenContext<'ctx> {
        // because we remove it before the end of this function.
        self.gen_ctx = Some(unsafe { mem::transmute(&ctx) });

+        // NOTE (imp): I guess this should work but how do I test it?
+        for (id, item) in self.items() {
+            let mut sub_items = ItemSet::new();


On top of that, can we call this related_items instead? Seems more accurate given we insert the parent.

impowski · 2016-12-04T02:06:31Z

I was trying to understand how we will use BFS for this and quite confused. Because there is no possible way of traversing it without any parent_id, which is basically involving having an Item inside of BTreeMap<ItemId, Item>, and the only option then to traverse BTreeMap<ItemId, Item> and stdout a path of BTreeMap. Maybe I'm wrong in something?

fitzgen · 2016-12-08T03:03:14Z

@impowski: I haven't forgotten about this PR, just a bit busy this week at an all-hands -- will get to this end of this week / early next week. Thanks for your patience :)

impowski · 2016-12-08T04:28:06Z

@fitzgen I was little bit busy for last 2 days, I'll will try to do something right now, just need to check your example in IRC.

fitzgen

LGTM! Will merge once my final nitpick is taken care of :)

fitzgen · 2016-12-12T20:17:20Z

libbindgen/src/ir/context.rs

@@ -441,6 +443,22 @@ impl<'ctx> BindgenContext<'ctx> {
        ret
    }

+    /// This function trying to find any dangling references inside of `items`
+    fn related_items(&self) {


Can we rename this assert_no_dangling_references? Thanks!

fitzgen · 2016-12-12T20:23:04Z

Regarding the BFS stuff: let's land this now and then do that in a follow up. We talked a bit on irc, but if things are still unclear, just ping me again or comment on the issue.

fitzgen

Great start!

I've detailed how to reconstruct the shortest path to an ItemId below with a few code snippets and requested changes.

Let me know if you have questions!

fitzgen · 2016-12-13T19:04:23Z

libbindgen/src/ir/context.rs

+
+        let roots = self.items().map(|(&id, _)| id);
+
+        let seen: ItemSet = roots.collect();


Because this is marking every item as seen, the BFS is going to go nowhere: it won't traverse any sub-item.

I think you want to keep the seen set empty, and also put the self.items() iterator as a member into RelatedItemsIter.

fitzgen · 2016-12-13T19:12:22Z

libbindgen/src/ir/context.rs

+    where 'gen: 'ctx,
+{
+    ctx: &'ctx BindgenContext<'gen>,
+    seen: ItemSet,


This should be a BTreeMap<ItemId, ItemId> mapping from a seen item, to the item from which we first discovered it (or mapping to itself if we found it as a root).

That way, if/when we find a dangling ItemId reference, we can walk backwards through the path with this map, something like this:

if is_dangling(id) { let mut path = vec![]; let mut current = id; loop { let predecessor = self.seen.get(current) .expect("We know we found this item id, so it must have a predecessor"); if predecessor == current { break; } path.push(predecessor); current = predecessor; } path.reverse(); panic!("Found reference to dangling id = {:?}\n via path = {:?}", id, path); }

fitzgen · 2016-12-13T19:15:28Z

libbindgen/src/ir/context.rs

+        id.collect_types(self.ctx, &mut sub_types, &());
+
+        for id in sub_types {
+            if self.seen.insert(id) {


Map from a seen id to the id of its predecessor:

for sub_id in sub_types { if self.seen.insert(sub_id, id) { ... } }

fitzgen · 2016-12-13T19:21:15Z

libbindgen/src/ir/context.rs

+
+    fn next(&mut self) -> Option<Self::Item> {
+        let id = match self.to_iterate.pop_front() {
+            None => return None,


Instead of eagerly returning None, we should see if there is a next root:

let id = match self.to_iterate.pop_front() { None => { // We've traversed everything reachable from the previous root(s), see if // we have any more roots. // // (Note: The `self.all_items` here would be the iterator returned by `ctx.items()`.) match self.all_items.filter(|&(id, _)| !self.seen.contains_key(id)).next() { None => return None, Some(id) => { // This is a new root. self.seen.insert(id, id); id } }, Some(id) => id, };

fitzgen · 2016-12-13T19:21:58Z

libbindgen/src/ir/context.rs

+        };
+
+        debug_assert!(self.seen.contains(&id));
+        debug_assert!(self.ctx.items.contains_key(&id));


This is where the if is_dangling(id) { ... } snippet that finds the BFS path would go.

fitzgen · 2016-12-13T19:24:52Z

libbindgen/src/ir/context.rs

@@ -1066,3 +1102,38 @@ impl<'ctx, 'gen> Iterator for WhitelistedItemsIter<'ctx, 'gen>
        Some(id)
    }
 }
+
+pub struct RelatedItemsIter<'ctx, 'gen>


Since this iterator is going to be specific to finding BFS paths of dangling items, lets call it AssertNoDanglingItemIter.

fitzgen · 2016-12-13T19:27:02Z

libbindgen/src/ir/context.rs

@@ -441,6 +443,40 @@ impl<'ctx> BindgenContext<'ctx> {
        ret
    }

+    /// This function trying to find any dangling references inside of `items`
+    fn assert_no_dangling_references(&self) {
+        if cfg!(debug_assertions) {


With the changes requested below, the contents of this if branch can now simply be:

if cfg!(debug_assertions) { for _ in self.assert_no_dangling_item_traversal() { // The iterator's next method does the asserting for us. } }

fitzgen · 2016-12-13T19:27:39Z

libbindgen/src/ir/context.rs

+        }
+    }
+
+    fn related_items_traversal<'me>(&'me self)


Let's rename this method to assert_no_dangling_item_traversal()

emilio · 2016-12-13T20:03:28Z

It'd be nice to keep this behind an option, even in debug builds. On stylo we're starting to use bindgen at build time, and it's already pretty painful to use. Seems there's no solution until rust-lang/cargo#1359 is resolved.

…

On Tue, Dec 13, 2016 at 11:29:20AM -0800, Nick Fitzgerald wrote: fitzgen requested changes on this pull request. Great start! I've detailed how to reconstruct the shortest path to an ItemId below with a few code snippets and requested changes. Let me know if you have questions! > + for sub in sub_items { + assert!(self.resolve_item_fallible(sub).is_some(), + "Should not dangle: {:?} references {:?}", id, sub); + } + } + } + } + + fn related_items_traversal<'me>(&'me self) + -> RelatedItemsIter<'me, 'ctx> { + assert!(self.in_codegen_phase()); + assert!(self.current_module == self.root_module); + + let roots = self.items().map(|(&id, _)| id); + + let seen: ItemSet = roots.collect(); Because this is marking every item as seen, the BFS is going to go nowhere: it won't traverse any sub-item. > + for sub in sub_items { + assert!(self.resolve_item_fallible(sub).is_some(), + "Should not dangle: {:?} references {:?}", id, sub); + } + } + } + } + + fn related_items_traversal<'me>(&'me self) + -> RelatedItemsIter<'me, 'ctx> { + assert!(self.in_codegen_phase()); + assert!(self.current_module == self.root_module); + + let roots = self.items().map(|(&id, _)| id); + + let seen: ItemSet = roots.collect(); I think you want to keep the `seen` set empty, and also put the `self.items()` iterator as a member into `RelatedItemsIter`. > @@ -1066,3 +1102,38 @@ impl<'ctx, 'gen> Iterator for WhitelistedItemsIter<'ctx, 'gen> Some(id) } } + +pub struct RelatedItemsIter<'ctx, 'gen> + where 'gen: 'ctx, +{ + ctx: &'ctx BindgenContext<'gen>, + seen: ItemSet, This should be a `BTreeMap<ItemId, ItemId>` mapping from a seen item, to the item from which we first discovered it (or mapping to itself if we found it as a root). That way, if/when we find a dangling `ItemId` reference, we can walk backwards through the path with this map, something like this: ```rust if is_dangling(id) { let mut path = vec![]; let mut current = id; loop { let predecessor = self.seen.get(current) .expect("We know we found this item id, so it must have a predecessor"); if predecessor == current { break; } path.push(predecessor); current = predecessor; } path.reverse(); panic!("Found reference to dangling id = {:?}\n via path = {:?}", id, path); } ``` > + type Item = ItemId; + + fn next(&mut self) -> Option<Self::Item> { + let id = match self.to_iterate.pop_front() { + None => return None, + Some(id) => id, + }; + + debug_assert!(self.seen.contains(&id)); + debug_assert!(self.ctx.items.contains_key(&id)); + + let mut sub_types = ItemSet::new(); + id.collect_types(self.ctx, &mut sub_types, &()); + + for id in sub_types { + if self.seen.insert(id) { Map from a seen id to the id of its predecessor: ```rust for sub_id in sub_types { if self.seen.insert(sub_id, id) { ... } } ``` > +pub struct RelatedItemsIter<'ctx, 'gen> + where 'gen: 'ctx, +{ + ctx: &'ctx BindgenContext<'gen>, + seen: ItemSet, + to_iterate: VecDeque<ItemId>, +} + +impl<'ctx, 'gen> Iterator for RelatedItemsIter<'ctx, 'gen> + where 'gen: 'ctx, +{ + type Item = ItemId; + + fn next(&mut self) -> Option<Self::Item> { + let id = match self.to_iterate.pop_front() { + None => return None, Instead of eagerly returning `None`, we should see if there is a next root: ```rust let id = match self.to_iterate.pop_front() { None => { // We've traversed everything reachable from the previous root(s), see if // we have any more roots. // // (Note: The `self.all_items` here would be the iterator returned by `ctx.items()`.) match self.all_items.filter(|&(id, _)| !self.seen.contains_key(id)).next() { None => return None, Some(id) => { // This is a new root. self.seen.insert(id, id); id } }, Some(id) => id, }; ``` > + to_iterate: VecDeque<ItemId>, +} + +impl<'ctx, 'gen> Iterator for RelatedItemsIter<'ctx, 'gen> + where 'gen: 'ctx, +{ + type Item = ItemId; + + fn next(&mut self) -> Option<Self::Item> { + let id = match self.to_iterate.pop_front() { + None => return None, + Some(id) => id, + }; + + debug_assert!(self.seen.contains(&id)); + debug_assert!(self.ctx.items.contains_key(&id)); This is where the `if is_dangling(id) { ... }` snippet that finds the BFS path would go. > @@ -1066,3 +1102,38 @@ impl<'ctx, 'gen> Iterator for WhitelistedItemsIter<'ctx, 'gen> Some(id) } } + +pub struct RelatedItemsIter<'ctx, 'gen> Since this iterator is going to be specific to finding BFS paths of dangling items, lets call it `AssertNoDanglingItemIter`. > @@ -441,6 +443,40 @@ impl<'ctx> BindgenContext<'ctx> { ret } + /// This function trying to find any dangling references inside of `items` + fn assert_no_dangling_references(&self) { + if cfg!(debug_assertions) { With the changes requested below, the contents of this if branch can now simply be: ```rust if cfg!(debug_assertions) { for _ in self.assert_no_dangling_item_traversal() { // The iterator's next method does the asserting for us. } } ``` > + fn assert_no_dangling_references(&self) { + if cfg!(debug_assertions) { + for (id, item) in self.items() { + let mut sub_items = ItemSet::new(); + item.collect_types(self, &mut sub_items, &()); + + sub_items.insert(item.parent_id()); + for sub in sub_items { + assert!(self.resolve_item_fallible(sub).is_some(), + "Should not dangle: {:?} references {:?}", id, sub); + } + } + } + } + + fn related_items_traversal<'me>(&'me self) Let's rename this method to `assert_no_dangling_item_traversal()` -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #312 (review)

fitzgen · 2016-12-13T20:25:11Z

We could add a feature for this, that we only enable in local + CI tests.

fitzgen

Looks great! Thanks for sticking with this all the way through :)

Can you rebase + squash this branch, and then force push it here? After that, I'll merge :)

Thanks again!

fitzgen · 2016-12-15T16:03:33Z

@bors-servo r+

bors-servo · 2016-12-15T16:03:34Z

📌 Commit 989a516 has been approved by fitzgen

bors-servo · 2016-12-15T16:03:36Z

⌛ Testing commit 989a516 with merge 59a5256...

@fitzgen

Add assert for dangling references without an associated ItemId So I think this is it? Fix issue #209 r? @fitzgen

bors-servo · 2016-12-15T16:15:37Z

☀️ Test successful - status-travis

highfive assigned fitzgen Dec 2, 2016

highfive added the S-awaiting-review label Dec 2, 2016

fitzgen requested changes Dec 2, 2016

View reviewed changes

emilio requested changes Dec 3, 2016

View reviewed changes

fitzgen approved these changes Dec 12, 2016

View reviewed changes

fitzgen requested changes Dec 13, 2016

View reviewed changes

fitzgen approved these changes Dec 15, 2016

View reviewed changes

Add assertion for dangling references

989a516

highfive removed the S-awaiting-review label Dec 15, 2016

highfive added the S-awaiting-merge label Dec 15, 2016

bors-servo pushed a commit that referenced this pull request Dec 15, 2016

Auto merge of #312 - impowski:debug_dangling_references, r=fitzgen

59a5256

Add assert for dangling references without an associated ItemId So I think this is it? Fix issue #209 r? @fitzgen

bors-servo merged commit 989a516 into rust-lang:master Dec 15, 2016

highfive removed the S-awaiting-merge label Dec 15, 2016

impowski deleted the debug_dangling_references branch December 15, 2016 16:53

fitzgen mentioned this pull request Dec 15, 2016

Add a debug phase to assert against dangling ItemId references without an associated Item #209

Closed

Manishearth unassigned fitzgen Jul 25, 2017


		let roots = self.items().map(\|(&id, _)\| id);

		let seen: ItemSet = roots.collect();

Add assert for dangling references without an associated ItemId #312

Add assert for dangling references without an associated ItemId #312

Uh oh!

Conversation

impowski commented Dec 2, 2016

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

impowski commented Dec 4, 2016

Uh oh!

fitzgen commented Dec 8, 2016

Uh oh!

impowski commented Dec 8, 2016

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Dec 12, 2016

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 Dec 13, 2016 via email

Uh oh!

fitzgen commented Dec 13, 2016

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

fitzgen commented Dec 15, 2016

Uh oh!

bors-servo commented Dec 15, 2016

Uh oh!

bors-servo commented Dec 15, 2016

Uh oh!

bors-servo commented Dec 15, 2016

Uh oh!

Uh oh!