feat(sgx_types): add traits using derive

[longtomjr]

I started implementing the Debug and PartialEq traits using derive. This is to facilitate creating strategies with proptest and to allow us to easily compare results after the test.

This is WIP, but I wanted to open it up for feedback and to get an idea on if this makes sense to add.

[dingelish]

thanks man! i have a debug_types branch out there for your reference. iirc i put a proc macro to simplify the work. however, it made the sgx_types crate depends on something. this is not what i desired. so i stopped merge that. if we can implement Debug for all of the types including types with large arrays, it's really really helpful.

[longtomjr]

thanks man! i have a debug_types branch out there for your reference. iirc i put a proc macro to simplify the work. however, it made the sgx_types crate depends on something. this is not what i desired. so i stopped merge that. if we can implement Debug for all of the types including types with large arrays, it's really really helpful.

Thanks a lot for giving some pointers and feedback!

I will have a look at the branch and see if there is a way to get the proc macro to work without pulling in dependencies.

If it does pull in a dependency, will it be OK to tie it to a feature that is not included by default, so that it will be up to the crate user to decide when they want the debug types etc?

[longtomjr]

@dingelish I updated the existing macros to use derive for Debug and other implementations. This seems like the easiest way to go about this.

The main concern with updating the existing macros is that the impl_struct macro is exported from the crate, and this will change the behavior of the macro. I can create an internal macro for impl_struct_and_debug, and replace internal uses of the impl_struct macro with the new macro.

Also, I have not written macros before, so there might be other effects that I am missing here. Let me know if I should try a different angle or if there is something I can modify with this approach to make the implementation a little bit more sensible.

The reason I replaced the existing copy functionality is because of a compiler warning that derive cannot be used on repr(packed) structs that does not derive Copy. rust-lang/rust#46043 should have more info

[volcano0dr]

@longtomjr I think we can refer to libc crate and use extra_traits feature to control derived debug, PartialEq, etc.

For the repr(packed) structure, I think we can replace the original macro with the new impl_packed_struct macro

[longtomjr]

@longtomjr I think we can refer to libc crate and use extra_traits feature to control derived debug, PartialEq, etc.

For the repr(packed) structure, I think we can replace the original macro with the new impl_packed_struct macro

Thanks for the feedback. I have some other urgent work that needs to be finalized. Will have a look at getting this done as soon as I get that done.

[longtomjr]

@volcano0dr What is the reason for using custom copy and clone implementations rather than using derive?

[volcano0dr]

@volcano0dr What is the reason for using custom copy and clone implementations rather than using derive?

Because in the early rust version, large arrays (count> 32) cannot automatically derive Clone, Debug, Default..traits.

[volcano0dr]

I tried to submit code to your fork repo, but the permission was denied.
Can you add derive(Debug) to enum types in sgx_types? thanks!

[longtomjr]

@volcano0dr What is the reason for using custom copy and clone implementations rather than using derive?

Because in the early rust version, large arrays (count> 32) cannot automatically derive Clone, Debug, Default..traits.

Should I update the code to use derive instead now?

I tried to submit code to your fork repo, but the permission was denied.
Can you add derive(Debug) to enum types in sgx_types? thanks!

Will do!

[longtomjr]

For reference, These are the types that don't have derive(Debug) explicitly. I can add a conditional derive for those as well. if needed

rg -P "derive\((?!.*Debug)" -A 1
src/macros.rs
209:        #[derive($($derive:meta),*)]
210-        pub enum $name:ident {
--
216:        #[derive($($derive),*)]
217-        pub enum $name {

src/types.rs
1482:#[derive(Copy, Clone, Default)]
1483-pub struct sgx_align_key_128bit_t {
--
1489:#[derive(Copy, Clone, Default)]
1490-pub struct sgx_align_mac_128bit_t {
--
1496:#[derive(Copy, Clone, Default)]
1497-pub struct sgx_align_key_256bit_t {
--
1503:#[derive(Copy, Clone, Default)]
1504-pub struct sgx_align_mac_256bit_t {
--
1510:#[derive(Copy, Clone, Default)]
1511-pub struct sgx_align_ec256_dh_shared_t {
--
1517:#[derive(Copy, Clone, Default)]
1518-pub struct sgx_align_ec256_private_t {

[volcano0dr]

@longtomjr You can implement Debug, Eq, PartialEq trait for sgx_align_xxx.

#[cfg(feature = "extra_traits")]
impl PartialEq for sgx_align_key_128bit_t {
    #[inline]
    fn eq(&self, other: &sgx_align_key_128bit_t) -> bool {
        self.key.eq(&other.key)
    }
}

#[cfg(feature = "extra_traits")]
impl Eq for sgx_align_key_128bit_t {}

#[cfg(feature = "extra_traits")]
impl core::fmt::Debug for sgx_align_key_128bit_t {
    fn fmt(&self, fmt: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        fmt.debug_struct("sgx_align_key_128bit_t")
            .field("key", &self.key)
            .finish()
    }
}

[longtomjr]

@volcano0dr I used derive for those types as well. If you would prefer I can add a macro that generates PartialEq without checking the alignment as well.

Sorry for the delay, had some time sensitive stuff I had to get sorted.

[volcano0dr]

@volcano0dr I used derive for those types as well. If you would prefer I can add a macro that generates PartialEq without checking the alignment as well.

I think it is inappropriate to use derive for these types. The _pad field is just for alignment, so _pad should not be compared when implementing PartialEq.

[longtomjr]

@volcano0dr Make sense.
Is deriving debug fine in this case? Then I will do a manual impl for PartialEq for the related types.

[longtomjr]

Updated the derive to only be for debug, and added manual impl for Eq and PartialEq to the relevant types.

[volcano0dr]

Updated the derive to only be for debug, and added manual impl for Eq and PartialEq to the relevant types.

I think it would be more appropriate not to include _pad in debug.

[longtomjr]

Make sense, updated it to use a manual impl.

[volcano0dr]

@longtomjr Can you squash all commits before I merge the changes?

Thanks again for the contribution!

[longtomjr]

@volcano0dr Anytime, thanks for all the help and guidance! Squashed the commits