Hi Pedro, Looks like this series got set aside waiting for some additional feedback.
I would like to see this C++ Keyword issue resolved. Thanks, Mike > -----Original Message----- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Tuesday, June 6, 2023 10:57 AM > To: Pedro Falcato <pedro.falc...@gmail.com> > Cc: devel@edk2.groups.io; Gao, Liming <gaolim...@byosoft.com.cn>; Liu, > Zhiguang <zhiguang....@intel.com>; Oliver Smith-Denny > <o...@linux.microsoft.com>; Pop, Aaron <aaron...@microsoft.com>; > Kinney, Michael D <michael.d.kin...@intel.com> > Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add > Operator and Xor field names > > Hi Pedro, > > Based on this info, would you prefer we use xor_ style instead of Xor? > > I can update the PR with that change. > > Mike > > > -----Original Message----- > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > Sent: Thursday, June 1, 2023 8:02 AM > > To: Pedro Falcato <pedro.falc...@gmail.com> > > Cc: devel@edk2.groups.io; Gao, Liming <gaolim...@byosoft.com.cn>; > Liu, > > Zhiguang <zhiguang....@intel.com>; Oliver Smith-Denny > > <o...@linux.microsoft.com>; Pop, Aaron <aaron...@microsoft.com>; > Kinney, > > Michael D <michael.d.kin...@intel.com> > > Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add > Operator > > and Xor field names > > > > Hi Pedro, > > > > It appears other projects have run into this same issue with the > > TPM specifications and have changed the field names by appending '_'. > > > > https://github.com/MIPS/external- > > > tpm2/blob/d704926273cf17498c95ff0dc50b4b17e523c109/generator/structure_ > gene > > rator.py#L1183 > > > > Mike > > > > > -----Original Message----- > > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > > Sent: Wednesday, May 31, 2023 11:42 AM > > > To: Pedro Falcato <pedro.falc...@gmail.com> > > > Cc: devel@edk2.groups.io; Gao, Liming <gaolim...@byosoft.com.cn>; > Liu, > > > Zhiguang <zhiguang....@intel.com>; Oliver Smith-Denny > > > <o...@linux.microsoft.com>; Pop, Aaron <aaron...@microsoft.com>; > Kinney, > > > Michael D <michael.d.kin...@intel.com> > > > Subject: RE: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add > Operator > > > and Xor field names > > > > > > > > > > > > > -----Original Message----- > > > > From: Pedro Falcato <pedro.falc...@gmail.com> > > > > Sent: Wednesday, May 31, 2023 11:17 AM > > > > To: Kinney, Michael D <michael.d.kin...@intel.com> > > > > Cc: devel@edk2.groups.io; Gao, Liming <gaolim...@byosoft.com.cn>; > Liu, > > > > Zhiguang <zhiguang....@intel.com>; Oliver Smith-Denny > > > > <o...@linux.microsoft.com>; Pop, Aaron <aaron...@microsoft.com> > > > > Subject: Re: [Patch v2 1/3] MdePkg/Include/IndustryStandard: Add > > Operator > > > > and Xor field names > > > > > > > > On Tue, May 30, 2023 at 7:53 PM Michael D Kinney > > > > <michael.d.kin...@intel.com> wrote: > > > > > > > > > > Update Tpm12.h and Tpm20.h and not use c++ reserved keywords > > > > > operator and xor in C structures to support use of these > > > > > include files when building with a C++ compiler. > > > > > > > > > > This patch temporarily introduces an anonymous union to add > > > > > Operator and Xor fields to support migration from the current > > > > > field names to the new field names. > > > > > > > > > > Warning 4201 is disabled for VS20xx tool chains is a temporary > > > > > change to allow the use of anonymous unions. > > > > > > > > > > Cc: Liming Gao <gaolim...@byosoft.com.cn> > > > > > Cc: Zhiguang Liu <zhiguang....@intel.com> > > > > > Cc: Oliver Smith-Denny <o...@linux.microsoft.com> > > > > > Cc: Pedro Falcato <pedro.falc...@gmail.com> > > > > > Cc: Aaron Pop <aaron...@microsoft.com> > > > > > Signed-off-by: Michael D Kinney <michael.d.kin...@intel.com> > > > > > --- > > > > > MdePkg/Include/IndustryStandard/Tpm12.h | 22 > ++++++++++++++++++++-- > > > > > MdePkg/Include/IndustryStandard/Tpm20.h | 25 > > +++++++++++++++++++++++-- > > > > > 2 files changed, 43 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h > > > > b/MdePkg/Include/IndustryStandard/Tpm12.h > > > > > index 155dcc9f5f99..56e89d9d0835 100644 > > > > > --- a/MdePkg/Include/IndustryStandard/Tpm12.h > > > > > +++ b/MdePkg/Include/IndustryStandard/Tpm12.h > > > > > @@ -9,6 +9,14 @@ > > > > > #ifndef _TPM12_H_ > > > > > #define _TPM12_H_ > > > > > > > > > > +/// > > > > > +/// Temporary disable 4201 to support anonymous unions > > > > > +/// > > > > > +#if defined (_MSC_EXTENSIONS) > > > > > +#pragma warning( push ) > > > > > +#pragma warning ( disable : 4201 ) > > > > > +#endif > > > > > + > > > > > /// > > > > > /// The start of TPM return codes > > > > > /// > > > > > @@ -744,8 +752,11 @@ typedef struct tdTPM_PERMANENT_FLAGS { > > > > > BOOLEAN TPMpost; > > > > > BOOLEAN TPMpostLock; > > > > > BOOLEAN FIPS; > > > > > - BOOLEAN operator; > > > > > - BOOLEAN enableRevokeEK; > > > > > + union { > > > > > + BOOLEAN operator; > > > > > + BOOLEAN Operator; > > > > > + }; > > > > > > > > Do you know if this works cleanly for the other toolchains? i.e > > > > supported GCCs and CLANGs? > > > > I don't *think* there's a warning for anonymous unions beyond > passing > > > > -pedantic + -std=c<something>, but it'd be good to know. > > > > If so, we may need a pragma for this. > > > > > > I did not see any issues with my local testing or with EDK II CI. > > > > > > > > > > > > + BOOLEAN enableRevokeEK; > > > > > BOOLEAN nvLocked; > > > > > BOOLEAN readSRKPub; > > > > > BOOLEAN tpmEstablished; > > > > > @@ -2162,4 +2173,11 @@ typedef struct tdTPM_RSP_COMMAND_HDR { > > > > > > > > > > #pragma pack () > > > > > > > > > > +/// > > > > > +/// Temporary disable 4201 to support anonymous unions > > > > > +/// > > > > > +#if defined (_MSC_EXTENSIONS) > > > > > +#pragma warning( pop ) > > > > > +#endif > > > > > + > > > > > #endif > > > > > diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h > > > > b/MdePkg/Include/IndustryStandard/Tpm20.h > > > > > index 4440f3769f26..a602c0d9c289 100644 > > > > > --- a/MdePkg/Include/IndustryStandard/Tpm20.h > > > > > +++ b/MdePkg/Include/IndustryStandard/Tpm20.h > > > > > @@ -15,6 +15,14 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > > > > > > > > > #include <IndustryStandard/Tpm12.h> > > > > > > > > > > +/// > > > > > +/// Temporary disable 4201 to support anonymous unions > > > > > +/// > > > > > +#if defined (_MSC_EXTENSIONS) > > > > > +#pragma warning( push ) > > > > > +#pragma warning ( disable : 4201 ) > > > > > +#endif > > > > > + > > > > > #pragma pack (1) > > > > > > > > > > // Annex A Algorithm Constants > > > > > @@ -1247,7 +1255,10 @@ typedef union { > > > > > TPMI_AES_KEY_BITS aes; > > > > > TPMI_SM4_KEY_BITS SM4; > > > > > TPM_KEY_BITS sym; > > > > > - TPMI_ALG_HASH xor; > > > > > + union { > > > > > + TPMI_ALG_HASH xor; > > > > > + TPMI_ALG_HASH Xor; > > > > > + }; > > > > > } TPMU_SYM_KEY_BITS; > > > > > > > > > > // Table 123 - TPMU_SYM_MODE Union > > > > > @@ -1320,7 +1331,10 @@ typedef struct { > > > > > // Table 136 - TPMU_SCHEME_KEYEDHASH Union > > > > > typedef union { > > > > > TPMS_SCHEME_HMAC hmac; > > > > > - TPMS_SCHEME_XOR xor; > > > > > + union { > > > > > + TPMS_SCHEME_XOR xor; > > > > > + TPMS_SCHEME_XOR Xor; > > > > > + }; > > > > > } TPMU_SCHEME_KEYEDHASH; > > > > > > > > > > // Table 137 - TPMT_KEYEDHASH_SCHEME Structure > > > > > @@ -1809,4 +1823,11 @@ typedef struct { > > > > > #define HASH_ALG_SHA512 0x00000008 > > > > > #define HASH_ALG_SM3_256 0x00000010 > > > > > > > > > > +/// > > > > > +/// Temporary disable 4201 to support anonymous unions > > > > > +/// > > > > > +#if defined (_MSC_EXTENSIONS) > > > > > +#pragma warning( pop ) > > > > > +#endif > > > > > + > > > > > #endif > > > > > -- > > > > > 2.40.1.windows.1 > > > > > > > > > > > > > All in all, this looks ok to me. Although I have to say, I'm not > a > > > > huge fan of the naming style inconsistency introduced here (i.e > Xor vs > > > > hmac). > > > > What if we made all the struct members MixedCase? Or what if we > did > > > > something like calling them xor_ and operator_? > > > > > > The more we change, the greater the potential impact to downstream > use of > > > these structures. I prefer to do the smallest possible change to > address > > > the c++ reserved keyword name collisions in this patch series. > > > > > > I do not have strong opinion between 'xor_' vs 'Xor'. These files > are > > > based on the TCG TPM Specifications that typically start field > names with > > > lower case and camel case after that for multi-word field names. > > > > > > https://trustedcomputinggroup.org/resource/tpm-library- > specification/ > > > https://trustedcomputinggroup.org/wp- > > > content/uploads/TCG_TPM2_r1p59_Part2_Structures_pub.pdf > > > > > > > > > > > -- > > > > Pedro -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116064): https://edk2.groups.io/g/devel/message/116064 Mute This Topic: https://groups.io/mt/99226543/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
