Make derive Debug configurable for specific structures

[andreeaflorescu]

Input C/C++ Header

# Taken from include/uapi/linux/virtio_net.h
struct virtio_net_ctrl_mac {
    __virtio32 entries;
    __u8 macs[][ETH_ALEN];
} __attribute__((packed));

Bindgen Invocation

# Bindgen version: 0.46.0
$  bindgen include/linux/virtio_net.h -o virtio_net.rs --with-derive-default --with-derive-partialeq

Actual Results

#[repr(C, packed)]
#[derive(Debug, Default)]
pub struct virtio_net_ctrl_mac {
    pub entries: __virtio32,
    pub macs: __IncompleteArrayField<[__u8; 6usize]>,
}

Compilation Warning:

warning: #[derive] can't be used on a #[repr(packed)] struct that does not derive Copy (error E0133)                                                                                                                                                                               
   --> virtio_gen/src/virtio_net.rs:684:10                                                                                                                                                                                                                                         
    |                                                                                                                                                                                                                                                                              
684 | #[derive(Debug, Default)]                                                                                                                                                                                                                                                    
    |          ^^^^^                                                                                                                                                                                                                                                               
    |                                                                                                                                                                                                                                                                              
    = note: #[warn(safe_packed_borrows)] on by default                                                                                                                                                                                                                             
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!                                                                                                                                              
    = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>        

Nice to have behaviour

We would like to be able to specify structures for which we want derive debug to be excluded (for example virtio_net_ctrl_mac) so we can automatically generate bindings. Our workaround right now is to manually remove the debug derive.

[emilio]

This should be possible and not too hard. We have per-item annotations, and very similar code to control whitelisting / blacklisting. I'd be happy to help writing / review a patch implementing something like this.

[emilio]

(Sorry for the lag btw! :/)

[youknowone]

Though the suggested feature can solve the problem, the issue seems to be solved in different way.

For now, deriving for any packed struct is warnings and it seems it is going to be errors in future. So than, when users are putting --with-derive-default, they just want to not to derive them for packed struct.

I suggest to ignore --with-derive-* options for packed structures.
(Not to make confusion, i think per-item annotation still will be useful too)

[chrysn]

It appears that this is now becoming more urgent; I didn't find the precise nightly this changed in (or whether it's really a nightly change, probably it is), but the warnings are now errors:

error: `#[derive]` can't be used on a `#[repr(packed)]` struct that does not derive Copy (error E0133)                                                                                                              
     --> /home/chrysn/git/crates/riot-doc-helpers/bin/nrf52840dongle/target/thumbv7em-none-eabihf/debug/build/riot-sys-933e33429038a18a/out/bindings.rs:73804:10                                                    
      |                                              
73804 | #[derive(Debug, Default)]                                                                         
      |          ^^^^^                                                                                    
      |                                                                                                                                                                                                             
      = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!                                                                             
      = note: for more information, see issue #82523 <https://github.com/rust-lang/rust/issues/82523>                                                                                                               
      = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)                                                                                      

[edit: It is already an error on 2022-04-25, and was still a warning in 2022-03-08]

[kulp]

@chrysn PR #2083 had the same error message and a similar situation, and was at least partially resolved by #2200. Could you confirm whether the latest bindgen master branch avoids this problem for you?

[chrysn]

It doesn't, but shifts them -- now I'm getting error: reference to packed field is unaligned but inside the automatically spelled out implementations of Debug that I've been requesting from bindgen. (Which, generally, are great to have, as it avoids a lot of downstream breakage for me in this case -- these things were Debug and other Debug impls rely on it, even if not all fields make sense).

impl ::core::fmt::Debug for ble_hci_cmd {
    fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
        write!(
            f,
            "ble_hci_cmd {{ opcode: {:?}, length: {:?}, data: {:?} }}",
            self.opcode, self.length, self.data
        )
    }
}

For some fields the workaround (recommended variously around the issue) of putting values in blocks helps

        write!(
            f,
            "ble_hci_cmd {{ opcode: {:?}, length: {:?}, data: {:?} }}",
            { self.opcode } , { self.length }, { self.data }
        )

but not for all, eg. (in this case) data is not Copy:

72964 |             { self.opcode } , { self.length }, { self.data }
      |                                                 ^^^^^^^^^ move occurs because `self.data` has type `__IncompleteArrayField<u8>`, which does not implement the `Copy` trait

for which the best I managed was to just leave it out (in the test where I manually modified the bindgen output, I put "...." in the place of data).

Should I open up a separate issue about the generated Derive code not working well for packed structs, or do we keep tracking this here?

[chrysn]

I can't work on a full PR right now, but in case I come back to this: all code is in impl_debug.rs, and maybe we'll need to pass extra data into the Item::impl_debug to indicate whether the item is referencable or whether it's a member of a packed struct (in which case it might, for example, look into whether ty is Copy, if so bracket itself, and if not just bail).

[kulp]

Should I open up a separate issue about the generated Derive Debug code not working well for packed structs, or do we keep tracking this here?

@chrysn It would be fine with me if you created a separate issue for that (including your helpful detail in #1491 (comment)), since the current issue is an enhancement request, and what you are describing is really a bug.

Thanks!

[kulp]

I noticed that this issue looks like the same thing as #961. Maybe one of them can be closed as a duplicate of the other.

[chrysn]

It's not quite the same. #961 can certainly be a workaround: the user notices that some Debug fails, and blocks that from being derived. But the better behavior (and also the compatible one, considering the failure to derive Debug from where it previously worked) is for bindgen to provide some Debug implementation when requested through impl_debug. Tracking now at #2221.

[kulp]

It's not quite the same.

I was not clear. I think the title of this issue ("Make derive Debug configurable for specific structures") and the original description ("We would like to be able to specify structures for which we want derive debug to be excluded...") are the same as #961 ("Allow specifying certain types we shouldn't derive/impl Debug for").

I agree with you that your particular issue is different -- thanks for filing #2221.