Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify derive #1540

Merged
merged 3 commits into from
Mar 22, 2019
Merged

Unify derive #1540

merged 3 commits into from
Mar 22, 2019

Conversation

jethrogb
Copy link
Contributor

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
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@emilio
Copy link
Contributor

emilio commented Mar 21, 2019

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
Copy link
Contributor

emilio commented Mar 21, 2019

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
Copy link
Contributor

emilio commented Mar 21, 2019

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
Copy link
Contributor

emilio commented Mar 21, 2019

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 Outdated Show resolved Hide resolved
src/ir/analysis/derive.rs Outdated Show resolved Hide resolved
src/ir/analysis/derive.rs Outdated Show resolved Hide resolved
src/ir/analysis/derive.rs Outdated Show resolved Hide resolved
src/ir/analysis/derive.rs Outdated Show resolved Hide resolved
@jethrogb
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@emilio
Copy link
Contributor

emilio commented Mar 21, 2019

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
Copy link
Contributor

emilio commented Mar 21, 2019

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
Copy link
Contributor

emilio commented Mar 21, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

emilio commented Mar 22, 2019

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

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much again for this @jethrogb!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants