You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@teaclave.apache.org by Herman <no...@github.com.INVALID> on 2021/03/11 14:35:40 UTC
[apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add traits
using derive (#325)
I started implementing the Debug and PartialEq traits using derive. This is to facilitate creating strategies with [proptest](https://github.com/AltSysrq/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.
You can view, comment on, or merge this pull request online at:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325
-- Commit Summary --
* feat(sgx_types): add traits using derive
-- File Changes --
M sgx_types/src/types.rs (6)
-- Patch Links --
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325.patch
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
@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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-821341883
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
@longtomjr Can you squash all commits before I merge the changes?
Thanks again for the contribution!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-822917182
Re: [apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add
traits using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
@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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-800260645
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
@volcano0dr Make sense.
Is deriving debug fine in this case? Then I will do a manual impl for PartialEq for the related types.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-822236582
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
@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()
}
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-817531809
Re: [apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add
traits using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
@longtomjr I think we can refer to libc crate and use extra_traits feature to control derived debug, PartialEq, etc.
For the repr structure, I think we can replace the original macro with the new `impl_packed_struct` macro
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-800986385
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
@volcano0dr Anytime, thanks for all the help and guidance! Squashed the commits
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-823055867
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
> > @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?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-815005422
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
Merged #325 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#event-4622429104
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
> 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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-822272790
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
Make sense, updated it to use a manual impl.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-822654411
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
Updated the derive to only be for debug, and added manual impl for Eq and PartialEq to the relevant types.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-822265789
Re: [apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add
traits using derive (#325)
Posted by Yu Ding <no...@github.com.INVALID>.
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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-797802125
Re: [apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add
traits using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
> 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?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-799492692
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
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
```sh
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 {
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-815038088
Re: [apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add
traits using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
@volcano0dr What is the reason for using custom copy and clone implementations rather than using derive?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-809177750
Re: [apache/incubator-teaclave-sgx-sdk] WIP - feat(sgx_types): add
traits using derive (#325)
Posted by Herman <no...@github.com.INVALID>.
> @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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-805808882
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
> @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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-822229666
Re: [apache/incubator-teaclave-sgx-sdk] feat(sgx_types): add traits
using derive (#325)
Posted by volcano <no...@github.com.INVALID>.
> @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.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/apache/incubator-teaclave-sgx-sdk/pull/325#issuecomment-811801036