Unify derive #1540

jethrogb · 2019-03-21T07:35:11Z

The logic for deriving traits is mostly the same, but it was implemented 5 times. This resulted in bugs only being fixed in one place but not the other, and in general just diverging implementations. This PR unifies the logic into a single implementation, while aiming to maintain bug-for-bug compatibility with the old implementations. Fixing the bugs found as a result of this PR may be done in the future. This PR is also a step in the right direction for fixing #1454, splitting PartialEq/PartialOrd, and many other possible improvements to deriving.

I've made one material change: deriving Default for a function pointer is now allowed. It was previously already allowed for function types, but those rarely appear in C source. This now follows the general flow that deriving for a function pointer and a function type should be treated the same. This change is the only change that affects the expected test outputs.

One thing of note is that there is no longer any special code for checking bitfields in compound types. This code was added in 95879dd, however, I verified with 843eb1c that none of the unit tests actually trigger that logic. I'm assuming this is already handled elsewhere (I think by the opaque type or union handling), since bitfields are part of the test suite.

There is one test failing, header_issue_1285_h. Bindgen suddenly decided to add a type in the middle that didn't exist before. I didn't really touch codegen, so I don't really understand what's going on here.

highfive · 2019-03-21T07:35:15Z

Warning

These commits modify unsafe code. Please review it carefully!

emilio · 2019-03-21T18:33:13Z

This looks really great overall, thanks @jethrogb! It would've been much nicer to try to put the behavior change in its own commit, but no big deal.

I can try to take a look at the failing test.

You also need to add docs for a couple things: https://travis-ci.com/rust-lang/rust-bindgen/jobs/186600231#L641

emilio · 2019-03-21T19:00:20Z

You also need to update the libclang5+ tests:

From 0857a3970f4dd3b4c6e60dc6006a53ce342c512c Mon Sep 17 00:00:00 2001   
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <[email protected]>       
Date: Thu, 21 Mar 2019 19:56:40 +0100                                    
Subject: [PATCH] Update tests with libclang 5+                           
MIME-Version: 1.0                                                        
Content-Type: text/plain; charset=UTF-8                                  
Content-Transfer-Encoding: 8bit                                          
                                                                         
Signed-off-by: Emilio Cobos Álvarez <[email protected]>                   
---                                                                      
 tests/expectations/tests/libclang-5/call-conv-field.rs | 7 +------      
 1 file changed, 1 insertion(+), 6 deletions(-)                          
                                                                         
diff --git a/tests/expectations/tests/libclang-5/call-conv-field.rs b/tests/expectations/tests/libclang-5/call-conv-field.rs
index e167e5eb..76712fe7 100644                                          
--- a/tests/expectations/tests/libclang-5/call-conv-field.rs             
+++ b/tests/expectations/tests/libclang-5/call-conv-field.rs             
@@ -9,7 +9,7 @@                                                          
 #![cfg(not(test))]                                                      
                                                                         
 #[repr(C)]                                                              
-#[derive(Copy, Clone)]                                                  
+#[derive(Default, Copy, Clone)]                                         
 pub struct JNINativeInterface_ {                                        
     pub GetVersion: ::std::option::Option<                              
         unsafe extern "stdcall" fn(env: *mut ::std::os::raw::c_void) -> ::std::os::raw::c_int,
@@ -50,11 +50,6 @@ fn bindgen_test_layout_JNINativeInterface_() {        
         )                                                               
     );                                                                  
 }                                                                       
-impl Default for JNINativeInterface_ {                                  
-    fn default() -> Self {                                              
-        unsafe { ::std::mem::zeroed() }                                 
-    }                                                                   
-}                                                                       
 extern "stdcall" {                                                      
     #[link_name = "\u{1}_bar@0"]                                        
     pub fn bar();                                                       
--                                                                       
2.21.0

emilio · 2019-03-21T19:05:12Z

So the test fails because bindgen thinks now that the inner union cannot derive Copy, even though it trivially can. That causes us to generate the old bindgen union structure, since we think we cannot represent it using union.

emilio · 2019-03-21T19:17:42Z

So you need to figure out why the a and b fields are not considered copy now. If you can't figure out I think I can give that a shot, let me know.

src/ir/analysis/derive.rs

jethrogb · 2019-03-21T23:02:57Z

@emilio thanks for the pointers.

So you need to figure out why the a and b fields are not considered copy now. If you can't figure out I think I can give that a shot, let me know.

They are not considered copy because they're not whitelisted:

(trace with my PR)

[TRACE bindgen::ir::analysis::derive] constrain: ItemId(6)
[TRACE bindgen::ir::analysis::derive]     cannot derive Copy for blacklisted type
[TRACE bindgen::ir::analysis::derive] inserting ItemId(6) can_derive<Copy>=No
[TRACE bindgen::ir::analysis::derive] constrain: ItemId(4)
[TRACE bindgen::ir::analysis::derive]     cannot derive Copy for blacklisted type
[TRACE bindgen::ir::analysis::derive] inserting ItemId(4) can_derive<Copy>=No
[TRACE bindgen::ir::analysis::derive] constrain: ItemId(2)
[TRACE bindgen::ir::analysis::derive] ty: Type { name: None, layout: Some(Layout { size: 4, align: 4, packed: false }), kind: Comp(CompInfo { kind: Union, fields: AfterComputingBitfieldUnits([DataMember(FieldData { name: Some("a"), ty: TypeId(ItemId(4)), comment: None, annotations: Annotations { opaque: false, hide: false, use_instead_of: None, disallow_copy: false, private_fields: None, accessor_kind: None, constify_enum_variant: false, derives: [] }, bitfield_width: None, mutable: false, offset: Some(0) }), DataMember(FieldData { name: Some("b"), ty: TypeId(ItemId(6)), comment: None, annotations: Annotations { opaque: false, hide: false, use_instead_of: None, disallow_copy: false, private_fields: None, accessor_kind: None, constify_enum_variant: false, derives: [] }, bitfield_width: None, mutable: false, offset: Some(0) })]), template_params: [], methods: [], constructors: [], destructor: None, base_members: [], inner_types: [], inner_vars: [], has_own_virtual_method: false, has_destructor: false, has_nonempty_base: false, has_non_type_template_params: false, packed_attr: false, found_unknown_attr: false, is_forward_declaration: false }), is_const: false }
[TRACE bindgen::ir::analysis::derive]     member ItemId(4) cannot derive Copy
[TRACE bindgen::ir::analysis::derive]     member ItemId(6) cannot derive Copy
[TRACE bindgen::ir::analysis::derive] inserting ItemId(2) can_derive<Copy>=No
[TRACE bindgen::ir::analysis::derive] constrain: ItemId(7)
[TRACE bindgen::ir::analysis::derive]     cannot derive Copy for blacklisted type
[TRACE bindgen::ir::analysis::derive] inserting ItemId(7) can_derive<Copy>=No
[TRACE bindgen::ir::analysis::derive] constrain: ItemId(2)
[TRACE bindgen::ir::analysis::derive]     already know it cannot derive Copy
[TRACE bindgen::ir::analysis::derive] constrain: ItemId(1)
[TRACE bindgen::ir::analysis::derive] ty: Type { name: Some("foo"), layout: Some(Layout { size: 4, align: 4, packed: false }), kind: Comp(CompInfo { kind: Struct, fields: AfterComputingBitfieldUnits([DataMember(FieldData { name: Some("bar"), ty: TypeId(ItemId(7)), comment: None, annotations: Annotations { opaque: false, hide: false, use_instead_of: None, disallow_copy: false, private_fields: None, accessor_kind: None, constify_enum_variant: false, derives: [] }, bitfield_width: None, mutable: false, offset: Some(0) })]), template_params: [], methods: [], constructors: [], destructor: None, base_members: [], inner_types: [TypeId(ItemId(2))], inner_vars: [], has_own_virtual_method: false, has_destructor: false, has_nonempty_base: false, has_non_type_template_params: false, packed_attr: false, found_unknown_attr: false, is_forward_declaration: false }), is_const: false }
[TRACE bindgen::ir::analysis::derive]     member ItemId(7) cannot derive Copy
[TRACE bindgen::ir::analysis::derive] inserting ItemId(1) can_derive<Copy>=No
[TRACE bindgen::ir::analysis::derive] constrain: ItemId(0)
[TRACE bindgen::ir::analysis::derive] inserting ItemId(0) can_derive<Copy>=Yes

So the test fails because bindgen thinks now that the inner union cannot derive Copy, even though it trivially can.

I don't think this is the source of the issue. The inner union is ItemId 2, which was already not considered copy:

(trace with ab5d31a)

[TRACE bindgen::ir::analysis::derive_copy] constrain: ItemId(2)
[TRACE bindgen::ir::analysis::derive_copy]     fields cannot derive Copy, so we can't either
[TRACE bindgen::ir::analysis::derive_copy] inserting ItemId(2) into the cannot_derive_copy set
[TRACE bindgen::ir::analysis::derive_copy] constrain: ItemId(1)
[TRACE bindgen::ir::analysis::derive_copy]     fields cannot derive Copy, so we can't either
[TRACE bindgen::ir::analysis::derive_copy] inserting ItemId(1) into the cannot_derive_copy set
[TRACE bindgen::ir::analysis::derive_copy] constrain: ItemId(0)
[TRACE bindgen::ir::analysis::derive_copy]     not a type; ignoring

Note that it looks like the old analysis is just completely missing some types in its analysis: it never looks at types 4, 6 and 7. This results in the following difference later in the trace:

(trace with my PR)

[DEBUG bindgen::codegen::struct_layout] pad_struct:
...
        cannot_derive_copy: Some(
            {
                ItemId(
                    2
                ),
                ItemId(
                    7
                ),
                ItemId(
                    4
                ),
                ItemId(
                    1
                ),
                ItemId(
                    6
                )
            }
        ),

(trace with ab5d31a)

[DEBUG bindgen::codegen::struct_layout] pad_struct:
...
        cannot_derive_copy: Some(
            {
                ItemId(
                    1
                ),
                ItemId(
                    2
                )
            }
        ),

Again, the only thing that's missing here are non-analyzed types. Is code later on incorrectly looking at these types that weren't analyzed?

jethrogb · 2019-03-21T23:06:02Z

I managed to split the Default function pointer change out into a separate commit.

emilio · 2019-03-21T23:16:35Z

I don't think this is the source of the issue. The inner union is ItemId 2, which was already not considered copy:

Sure, sorry, I meant the field types. To be clear, the difference between the output of your test-case and the current output is that this function:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/third_party/rust/bindgen/src/ir/comp.rs#1546

Returns a different result. Which means that the field types were considered copy, but are not anymore. I find it weird because they're trivials, even if blacklisted.

emilio · 2019-03-21T23:26:31Z

That being said I'm having a hard time figuring out where the decision that made those types copiable happens, looks like it's an accident right now that those types are being skipped, let me check.

emilio · 2019-03-21T23:37:41Z

Ok, yeah, the old code was bogus. It indeed doesn't visit any of the fields, and mistakenly conveys that it can be a rust union. I'd just check-in the new expectation for that test.

jethrogb · 2019-03-21T23:53:23Z

It does bring up an interesting question: how is whitelisting supposed to interact with anonymous types? This example is compiled with --no-recursive-whitelist. So, we're not considering the anonymous types in the derive logic because of that. However, in the resulting code, this non-whitelisted type is clearly codegened. This seems wrong to me.

emilio · 2019-03-22T14:34:11Z

Yeah, we should probably propagate the whitelisting from the parent for inner types.

emilio

Thanks so much again for this @jethrogb!

Enable logging in test suite

843eb1c

highfive added the S-awaiting-review label Mar 21, 2019

jethrogb force-pushed the unify-derive branch from ec832cd to e6d0709 Compare March 21, 2019 07:38

emilio reviewed Mar 21, 2019

View reviewed changes

jethrogb force-pushed the unify-derive branch from e6d0709 to 6647d38 Compare March 21, 2019 23:05

jethrogb force-pushed the unify-derive branch from 6647d38 to 6377127 Compare March 21, 2019 23:55

Jethro Beekman added 2 commits March 21, 2019 22:04

Unify derive logic

fbc5da7

Derive Default for function pointers

8ad6d67

jethrogb force-pushed the unify-derive branch from 6377127 to 8ad6d67 Compare March 22, 2019 05:04

emilio approved these changes Mar 22, 2019

View reviewed changes

emilio merged commit d699813 into rust-lang:master Mar 22, 2019

bors-servo mentioned this pull request Mar 22, 2019

ir: Whitelist items that don't generate code to improve derive behavior. #1492

Merged

emilio mentioned this pull request Mar 28, 2019

not clear how to produce debug output #1522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify derive #1540

Unify derive #1540

jethrogb commented Mar 21, 2019

highfive commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

jethrogb commented Mar 21, 2019

jethrogb commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

jethrogb commented Mar 21, 2019

emilio commented Mar 22, 2019

emilio left a comment

Unify derive #1540

Unify derive #1540

Conversation

jethrogb commented Mar 21, 2019

highfive commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

jethrogb commented Mar 21, 2019

jethrogb commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

emilio commented Mar 21, 2019

jethrogb commented Mar 21, 2019

emilio commented Mar 22, 2019

emilio left a comment

Choose a reason for hiding this comment