Use fix point analysis to implement can_derive_debug #824

photoszzt · 2017-07-18T19:44:43Z

It's failing about 30 tests now and most of them is related to template. I'm also not so sure about the place to call compute_can_derive_debug in gen.
Fix: #767

r? @fitzgen

highfive · 2017-07-18T19:44:47Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @fitzgen (or someone else) soon.

fitzgen

This looks right to me, will have to take a look at the errors to get a better idea of what is going wrong.

fitzgen · 2017-07-18T19:50:03Z

src/ir/can_derive_debug.rs

+        let ty_kind = ty.map(|ty| ty.kind());
+
+        match ty_kind {
+            Some(&TypeKind::Void) |


Since we only care about types here, we should exit the function early with non-types, which will get rid of these annoying Some(...)s and the unwraps:

let ty = match item.as_type() { None => return false, Some(ty) => ty }; match ty.kind() { ... }

fitzgen · 2017-07-18T19:53:43Z

src/ir/can_derive_debug.rs

@@ -0,0 +1,269 @@
+use super::analysis::MonotoneFramework;


nitpick: need a module doc comment, or the tests will fail with --features testing_only_docs:

//! Determining which types for which we can emit `#[derive(Debug)]`.

fitzgen · 2017-07-18T19:54:02Z

src/ir/can_derive_debug.rs

+use ir::comp::CompKind;
+
+#[derive(Debug, Clone)]
+pub struct CanDeriveDebugAnalysis<'ctx, 'gen>


nitpick: and a doc comment here.

See the doc comment above the UsedTemplateParameters type; the doc comment should help readers understand how this analysis works, why its domain is a finite height lattice and we will therefore definitely reach a fixed point, an overview of the constraining operations for different items/types are, etc...

fitzgen · 2017-07-18T20:00:02Z

src/ir/context.rs

@@ -1652,6 +1659,21 @@ impl<'ctx> BindgenContext<'ctx> {
    pub fn need_bindegen_complex_type(&self) -> bool {
        self.generated_bindegen_complex.get()
    }
+
+    /// compute whether we can derive debug


nitpick: please capitalize and punctuate sentences:

/// Compute whether we can derive `Debug` for each type.

fitzgen · 2017-07-18T20:00:51Z

src/ir/context.rs

+        self.can_derive_debug = Some(analyze::<CanDeriveDebugAnalysis>(self));
+    }
+
+    pub fn lookup_item_id_can_derive_debug(&self, id: ItemId) -> bool {


nitpick: this will also need a doc comment.

fitzgen · 2017-07-18T20:02:28Z

src/ir/item.rs

-                        .map_or(true, |l| l.opaque().can_derive_debug(ctx, ()))
-                } else {
-                    ty.can_derive_debug(ctx, ())
+        ctx.options().derive_debug &&


When ctx.options().derive_debug, the whole rest of the method should just be call to lookup_item_id_can_derive_debug now.

fitzgen · 2017-07-18T20:05:45Z

src/ir/can_derive_debug.rs

+            Some(&TypeKind::Pointer(inner)) => {
+                let inner_type = self.ctx.resolve_type(inner);
+                if let TypeKind::Function(ref sig) =
+                    *inner_type.canonical_type (self.ctx).kind() {


nitpick: no space between the method name and parentheses:

inner_type.canonical_type(self.ctx)

fitzgen · 2017-07-18T20:20:51Z

Here is tests/headers/inner_template_self.hpp's test expectation diff:

--- expected: "/home/travis/build/servo/rust-bindgen/tests/expectations/tests/inner_template_self.rs"
+++ generated from: "/home/travis/build/servo/rust-bindgen/tests/headers/inner_template_self.hpp"
 /* automatically generated by rust-bindgen */
 
 
 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
 
 
 #[repr(C)]
-#[derive(Debug, Copy, Clone)]
+#[derive(Copy, Clone)]
 pub struct LinkedList {
     pub next: *mut LinkedList,
     pub prev: *mut LinkedList,
 }
 impl Default for LinkedList {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[repr(C)]
-#[derive(Debug, Copy)]
+#[derive(Copy)]
 pub struct InstantiateIt {
     pub m_list: LinkedList,
 }

Pointers are always Debug, but it looks like the constrain function doesn't reflect that.

fitzgen · 2017-07-18T20:22:55Z

src/ir/can_derive_debug.rs

+                if self.can_derive_debug.contains(&inner) {
+                    self.can_derive_debug.insert(id);
+                    return true;
+                }


There should be no check whether the pointed-to inner type is in the set or not: pointers can always derive Debug (its just their address) so the non-function pointer case should simply be this:

self.can_derive_debug.insert(id); return true;

fitzgen · 2017-07-18T20:24:19Z

I think that should fix most of the test failures. If not, then let's look at which tests are still failing after fixing that.

photoszzt · 2017-07-18T21:13:15Z

After the above fix and move the consider edge a bit, it fails 15 tests now.

I'm not so sure about whether the edge I'm adding is correct or not. The next failure is

--- expected: "/Users/zzhu/Documents/rust-bindgen/tests/expectations/tests/crtp.rs"
+++ generated from: "/Users/zzhu/Documents/rust-bindgen/tests/headers/crtp.hpp"
 /* automatically generated by rust-bindgen */


 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]


 #[repr(C)]
 #[derive(Debug, Default, Copy, Clone)]
 pub struct Base {
     pub _address: u8,
 }
 #[repr(C)]
-#[derive(Debug, Copy)]
+#[derive(Copy)]
 pub struct Derived {
     pub _address: u8,
 }
 #[test]
 fn bindgen_test_layout_Derived() {
     assert_eq!(::std::mem::size_of::<Derived>() , 1usize , concat ! (
                "Size of: " , stringify ! ( Derived ) ));
     assert_eq! (::std::mem::align_of::<Derived>() , 1usize , concat ! (
                 "Alignment of " , stringify ! ( Derived ) ));
 }
 impl Clone for Derived {
     fn clone(&self) -> Self { *self }
 }
 impl Default for Derived {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[repr(C)]
 #[derive(Debug, Default)]
 pub struct BaseWithDestructor {
     pub _address: u8,
 }
 #[repr(C)]
-#[derive(Debug)]
 pub struct DerivedFromBaseWithDestructor {
     pub _address: u8,
 }
 #[test]
 fn bindgen_test_layout_DerivedFromBaseWithDestructor() {
     assert_eq!(::std::mem::size_of::<DerivedFromBaseWithDestructor>() , 1usize
                , concat ! (
                "Size of: " , stringify ! ( DerivedFromBaseWithDestructor ) ));
     assert_eq! (::std::mem::align_of::<DerivedFromBaseWithDestructor>() ,
                 1usize , concat ! (
                 "Alignment of " , stringify ! ( DerivedFromBaseWithDestructor
                 ) ));
 }
 impl Default for DerivedFromBaseWithDestructor {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[test]
 fn __bindgen_test_layout_Base_instantiation() {
     assert_eq!(::std::mem::size_of::<Base>() , 1usize , concat ! (
                "Size of template specialization: " , stringify ! ( Base ) ));
     assert_eq!(::std::mem::align_of::<Base>() , 1usize , concat ! (
                "Alignment of template specialization: " , stringify ! ( Base )
                ));
 }
 #[test]
 fn __bindgen_test_layout_BaseWithDestructor_instantiation() {
     assert_eq!(::std::mem::size_of::<BaseWithDestructor>() , 1usize , concat !
                (
                "Size of template specialization: " , stringify ! (
                BaseWithDestructor ) ));
     assert_eq!(::std::mem::align_of::<BaseWithDestructor>() , 1usize , concat
                ! (
                "Alignment of template specialization: " , stringify ! (
                BaseWithDestructor ) ));
 }

photoszzt · 2017-07-18T21:25:20Z

src/ir/can_derive_debug.rs

+                true
+            },
+            &TypeKind::Opaque => {
+                if ty.layout(self.ctx)


In the original implementation, the Opaque type is always true. Should I do that or uses the current implementation?

Let's try and translate exactly what master is doing into the constraint function.

fitzgen · 2017-07-18T22:03:49Z

src/ir/can_derive_debug.rs

+        match ty.kind() {
+            &TypeKind::Void |
+            &TypeKind::NullPtr |
+            &TypeKind::Int(..) |


To avoid all the repeated &s, we can match *ty.kind() { ... }

fitzgen · 2017-07-18T22:14:57Z

I'd suggest working on getting the smaller (and therefore usually simpler) test cases passing before working on larger ones. Often, the root cause of the issue will be the same in a larger test case, but easier to understand in a smaller setting that has less extra context, and then when you fix it for the small test case, the larger test case is also fixed.

It looks like template-param-usage-15.hpp is a good candidate to dig into next.

--- expected: "/home/travis/build/servo/rust-bindgen/tests/expectations/tests/template-param-usage-15.rs"
+++ generated from: "/home/travis/build/servo/rust-bindgen/tests/headers/template-param-usage-15.hpp"
 /* automatically generated by rust-bindgen */
 
 
 #![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
 
 
 #[repr(C)]
 #[derive(Debug, Copy, Clone)]
 pub struct BaseUsesT<T> {
     pub usage: *mut T,
     pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<T>>,
 }
 impl <T> Default for BaseUsesT<T> {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[repr(C)]
-#[derive(Debug, Copy, Clone)]
+#[derive(Copy, Clone)]
 pub struct CrtpIgnoresU {
     pub _base: BaseUsesT<CrtpIgnoresU>,
     pub y: ::std::os::raw::c_int,
 }
 impl Default for CrtpIgnoresU {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }

It looks like maybe the CrtpIgnoresU doesn't realize that its template instantiation of BaseUsesT should be able to derive debug?

fitzgen

This looks really great!

There are just a few things to change before landing, mostly nitpicks, see below.

Thanks a ton @photoszzt ! :)

fitzgen · 2017-07-19T16:08:35Z

src/ir/can_derive_debug.rs

+
+/// An analysis that finds for each IR item whether debug can be derived.
+///
+/// TODO More comments before merge


fitzgen · 2017-07-19T16:08:57Z

src/ir/can_derive_debug.rs

+
+    // The incremental result of this analysis's computation. Everything in this
+    // set can derive debug.
+    cant_derive_debug: HashSet<ItemId>,


The comment needs to be updated.

fitzgen · 2017-07-19T16:10:10Z

src/ir/can_derive_debug.rs

+            EdgeKind::TemplateDeclaration |
+            EdgeKind::TemplateParameterDefinition |
+            EdgeKind::InnerType |
+            EdgeKind::InnerVar => true,


I'm a bit surprised to see InnerType and InnerVar here. How can they affect whether a type can derive debug or not?

fitzgen · 2017-07-19T16:10:52Z

src/ir/can_derive_debug.rs

+}
+
+impl<'ctx, 'gen> CantDeriveDebugAnalysis<'ctx, 'gen> {
+    // Don't know where to call this method


It's called now, so I think this comment can be removed ;)

fitzgen · 2017-07-19T17:27:32Z

src/ir/can_derive_debug.rs

+                }
+                let consider_base = info.base_members()
+                    .iter()
+                    .all(|base| !self.cant_derive_debug.contains(&base.ty));


Each of the places where we do something like

let things_can_derive = things.all(|thing| !self.cant_derive_debug.contains(thing));

should become

let things_cant_derive = things.any(|thing| self.cant_derive_debug.contains(thing));

Its equivalent (de Morgan's) but the latter reads much more naturally.

Additionally, we should early return whenever we find something that would cause us to update the cant_derive_debug set, rather than continuing to consider (for example) fields even after we know we can't derive debug because of the bases.

That is, we should do something like this:

let bases_cant_derive = ...; if bases_cant_derive { self.cant_derive_debug.insert(id); return true; } let fields_cant_derive = ...; if fields_cant_derive { self.cant_derive_debug.insert(id); return true; } false

To avoid repeating the insert-into-the-set-and-then-return-true dance over and over, we can define a helper:

impl CantDeriveDebugAnalysis { ... fn insert(&mut self, id: ItemId) -> bool { let was_already_in = self.cant_derive_debug.insert(&id); assert!( !was_already_in, "We shouldn't try and insert twice because if it was already in the set, \ `constrain` would have exited early." ); true } }

fitzgen · 2017-07-19T17:32:09Z

src/ir/ty.rs

-                inst.can_derive_debug(ctx, self.layout(ctx))
-            }
-            _ => true,
-        }


Are there any other CanDeriveDebug impls that are now unused and we can remove? I'd try commenting all of them out other than Item and ItemId and see if any call sites start failing to compile, and then look at each of those call sites and determine whether it would be easier to just use an Item/ItemId impl or to uncomment the impl that was being used. I expect most of them are handled by Item and ItemId.

We should at minimum be able to delete the detect_can_derive_debug_cycle member from CompInfo and its CanDeriveDebug impl as well.

fitzgen · 2017-07-19T17:32:30Z

src/ir/context.rs

@@ -1652,6 +1660,23 @@ impl<'ctx> BindgenContext<'ctx> {
    pub fn need_bindegen_complex_type(&self) -> bool {
        self.generated_bindegen_complex.get()
    }
+
+    /// Compute whether we can derive debug


Nitpick: missing a period.

fitzgen · 2017-07-19T17:33:28Z

src/ir/context.rs

@@ -169,6 +170,11 @@ pub struct BindgenContext<'ctx> {

    /// Whether we need the mangling hack which removes the prefixing underscore.
    needs_mangling_hack: bool,
+
+    /// Set of ItemId that can't derive debug


Nitpick: missing a period.

fitzgen · 2017-07-19T17:33:55Z

src/ir/can_derive_debug.rs

+            TypeKind::Comp(ref info) => {
+                if info.has_non_type_template_params() {
+                    if ty.layout(self.ctx).map_or(false,
+                                                           |l| l.opaque().can_derive_debug(self.ctx, ())) {


Nitpick: funky indentation

fitzgen · 2017-07-20T16:45:05Z

@photoszzt, looks like there are compilation errors with the latest commit:

error[E0432]: unresolved import `super::can_derive_debug::CantDeriveDebugAnalysis`
  --> src/ir/context.rs:13:5
   |
13 | use super::can_derive_debug::CantDeriveDebugAnalysis;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Could not find `can_derive_debug` in `super`
error[E0432]: unresolved import `super::can_derive_debug::CantDeriveDebugAnalysis`
  --> src/ir/context.rs:13:5
   |
13 | use super::can_derive_debug::CantDeriveDebugAnalysis;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Could not find `can_derive_debug` in `super`
error: aborting due to previous error

bors-servo · 2017-07-20T17:05:49Z

☔ The latest upstream changes (presumably #823) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen

Thanks for sticking this one out!

fitzgen · 2017-07-20T19:38:57Z

@bors-servo r+

bors-servo · 2017-07-20T19:38:57Z

📌 Commit 01c609c has been approved by fitzgen

bors-servo · 2017-07-20T19:38:59Z

⌛ Testing commit 01c609c with merge 8f43c93...

@fitzgen

Use fix point analysis to implement can_derive_debug It's failing about 30 tests now and most of them is related to template. I'm also not so sure about the place to call compute_can_derive_debug in gen. Fix: #767 r? @fitzgen

bors-servo · 2017-07-20T19:58:35Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 8f43c93 to master...

…derive debug analysis. We have a special-case for them in codegen to generate a blob, that can derive debug. This is a regression from rust-lang#824, and hit stylo.

analysis: Account for template instantiations of opaque types in the derive debug analysis. We have a special-case for them in codegen to generate a blob, that can derive debug. This is a regression from #824, and hit stylo.

…derive debug analysis. We have a special-case for them in codegen to generate a blob, that can derive debug. This is a regression from rust-lang#824, and hit stylo.

highfive assigned fitzgen Jul 18, 2017

highfive added the S-awaiting-review label Jul 18, 2017

fitzgen reviewed Jul 18, 2017

View reviewed changes

photoszzt commented Jul 18, 2017

View reviewed changes

fitzgen reviewed Jul 18, 2017

View reviewed changes

photoszzt changed the title ~~WIP use fix point analysis to implement can_derive_debug~~ Use fix point analysis to implement can_derive_debug Jul 19, 2017

fitzgen reviewed Jul 19, 2017

View reviewed changes

emilio mentioned this pull request Jul 19, 2017

Implement IsOpaque for CompInfo #823

Merged

highfive added the S-needs-rebase label Jul 20, 2017

Use fix point analysis to implement the can_derive_debug

01c609c

fitzgen approved these changes Jul 20, 2017

View reviewed changes

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jul 20, 2017

bors-servo merged commit 01c609c into rust-lang:master Jul 20, 2017

highfive removed the S-awaiting-merge label Jul 20, 2017

bors-servo mentioned this pull request Jul 20, 2017

Only whitelist items for which we intend to generate bindings #827

Merged

emilio mentioned this pull request Jul 22, 2017

analysis: Account for template instantiations of opaque types in the derive debug analysis. #842

Merged

Manishearth unassigned fitzgen Jul 25, 2017

fitzgen mentioned this pull request Jul 26, 2017

Tools news rust-dev-tools/dev-tools-team#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fix point analysis to implement can_derive_debug #824

Use fix point analysis to implement can_derive_debug #824

photoszzt commented Jul 18, 2017

highfive commented Jul 18, 2017

fitzgen left a comment

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen commented Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen commented Jul 18, 2017

photoszzt commented Jul 18, 2017

photoszzt Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen Jul 18, 2017

fitzgen commented Jul 18, 2017 •

edited

fitzgen left a comment

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen Jul 19, 2017

fitzgen commented Jul 20, 2017

bors-servo commented Jul 20, 2017

fitzgen left a comment

fitzgen commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

Use fix point analysis to implement can_derive_debug #824

Use fix point analysis to implement can_derive_debug #824

Conversation

photoszzt commented Jul 18, 2017

highfive commented Jul 18, 2017

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Jul 18, 2017

Choose a reason for hiding this comment

fitzgen commented Jul 18, 2017

photoszzt commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Jul 18, 2017 • edited

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fitzgen commented Jul 20, 2017

bors-servo commented Jul 20, 2017

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

bors-servo commented Jul 20, 2017

fitzgen commented Jul 18, 2017 •

edited