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

Large bitfield causes compilation failures re: derive(Debug) #982

Closed
fitzgen opened this issue Sep 12, 2017 · 5 comments
Closed

Large bitfield causes compilation failures re: derive(Debug) #982

fitzgen opened this issue Sep 12, 2017 · 5 comments

Comments

@fitzgen
Copy link
Member

fitzgen commented Sep 12, 2017

Input C/C++ Header

struct {
  unsigned : 632;
} a;

Bindgen Invocation

$ bindgen input.h

Actual Results

clang-4.0: warning: treating 'c-header' input as 'c++-header' when in C++ mode, this behavior is deprecated [-Wdeprecated]
abc.h:2:12: warning: width of anonymous bit-field (632 bits) exceeds width of its type; value will be truncated to 32 bits [-Wbitfield-width]
  unsigned : 632;
           ^
1 warning generated.
abc.h:2:12: warning: width of anonymous bit-field (632 bits) exceeds width of its type; value will be truncated to 32 bits [-Wbitfield-width], err: false
ERROR:bindgen::codegen::struct_layout: Calculated wrong layout for _bindgen_ty_1, too more 48 bytes
error[E0277]: the trait bound `[u8; 128]: std::fmt::Debug` is not satisfied
 --> /tmp/bindings-UWtj35.rs:3:75
  |
3 | # [ repr ( C ) ] # [ derive ( Debug , Copy ) ] pub struct _bindgen_ty_1 { pub _bitfield_1 : [ u8 ; 128usize ] , pub __bindgen_align : [ u64 ; 0usize ] , } # [ test ] fn bindgen_test_layout__bindgen_ty_1 ( ) { assert_eq ! ( :: std :: mem :: size_of :: < _bindgen_ty_1 > ( ) , 80usize , concat ! ( "Size of: " , stringify ! ( _bindgen_ty_1 ) ) ) ; assert_eq ! ( :: std :: mem :: align_of :: < _bindgen_ty_1 > ( ) , 8usize , concat ! ( "Alignment of " , stringify ! ( _bindgen_ty_1 ) ) ) ; } impl Clone for _bindgen_ty_1 { fn clone ( & self ) -> Self { * self } } extern "C" {
  |                                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `[u8; 128]` cannot be formatted using `:?`; if it is defined in your crate, add `#[derive(Debug)]` or manually implement it
  |
  = help: the trait `std::fmt::Debug` is not implemented for `[u8; 128]`
  = note: required because of the requirements on the impl of `std::fmt::Debug` for `&[u8; 128]`
  = note: required for the cast to the object type `std::fmt::Debug`

error: aborting due to previous error

Interesting: bindgen emitted Rust code that won't compile!

Expected Results

We shouldn't derive(Debug) when a bitfield's allocation unit cannot derive debug.


Our various derive analyses need to check if any bitfield allocation units are too large to derive traits. We compute bitfield allocation units before the derive analyses, so this shouldn't be very difficult.

Here is where we keep the Layout for a bitfield allocation unit: https://github.com/rust-lang-nursery/rust-bindgen/blob/master/src/ir/comp.rs#L138

https://github.com/rust-lang-nursery/rust-bindgen/blob/master/src/ir/analysis/derive_debug.rs#L270 is where we need to check that (and similar places for the other derive analyses).

Happy to mentor anyone who wants to pick this up!

@aeleos
Copy link
Contributor

aeleos commented Sep 16, 2017

@highfive: assign me

@highfive
Copy link

Hey @aeleos! Thanks for your interest in working on this issue. It's now assigned to you!

@fitzgen
Copy link
Member Author

fitzgen commented Sep 18, 2017

@aeleos let me know if you have any questions about how the analyses work or whatever! :)

@aeleos
Copy link
Contributor

aeleos commented Sep 18, 2017

Hey @fitzgen so I think I have it working. I ended up with this code right before the line where it iterates over the bitfields() function output.

if bfu.layout().align > RUST_DERIVE_IN_ARRAY_LIMIT {
    trace!(
        "we cannot derive Debug for a bitfield larger then the limit"
    );
    return true;
}

When I was making this I ended up using the test-one.sh script to test my code, and I found that when I used the script it would fail for similar reasons. It would try to derive Default and fail because it isn't implemented larger than 32. I was able to fix it using the exact same code, just in derive_default.rs. Do you think it is worth including in the commit / pull request to fix this issue?

@fitzgen
Copy link
Member Author

fitzgen commented Sep 18, 2017

Awesome!

When I was making this I ended up using the test-one.sh script to test my code, and I found that when I used the script it would fail for similar reasons. It would try to derive Default and fail because it isn't implemented larger than 32. I was able to fix it using the exact same code, just in derive_default.rs. Do you think it is worth including in the commit / pull request to fix this issue?

Yep! I guess I didn't make myself clear enough with

(and similar places for the other derive analyses).

but we need to do this check in all of the derive_*.rs files.

(Yes, we're starting to repeat ourselves a bit across these functions, so if you have ideas on how to help de-duplicate some of this code, additional PRs are very welcome 😸)

bors-servo pushed a commit that referenced this issue Sep 20, 2017
Ensure all derive analyses check array limit on bitfields

Fixes #982

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

No branches or pull requests

3 participants