* [PATCH v2 0/1] MdePkg: Add STATIC_ASSERT macro @ 2019-08-13 8:16 vit9696 2019-08-13 8:16 ` [PATCH v2 1/1] " vit9696 0 siblings, 1 reply; 26+ messages in thread From: vit9696 @ 2019-08-13 8:16 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 542 bytes --] Implements https://bugzilla.tianocore.org/show_bug.cgi?id=2048. Things to note: - _Static_assert is a standard C11 keyword and thus is available on every modern compiler (including Apple Clang, Clang, and GCC). - static_assert is a hack to support MSVC, which implements static assertions with this vendor-specific keyword starting from at least VS 2010 to date. Vitaly Cheptsov (1): MdePkg: Add STATIC_ASSERT macro MdePkg/Include/Base.h | 11 +++++++++++ 1 file changed, 11 insertions(+) -- 2.20.1 (Apple Git-117) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-13 8:16 [PATCH v2 0/1] MdePkg: Add STATIC_ASSERT macro vit9696 @ 2019-08-13 8:16 ` vit9696 2019-08-14 13:50 ` [edk2-devel] " Liming Gao 2019-08-15 16:08 ` Michael D Kinney 0 siblings, 2 replies; 26+ messages in thread From: vit9696 @ 2019-08-13 8:16 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 1110 bytes --] REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 Provide a macro for compile time assertions. Equivalent to C11 static_assert macro from assert.h. Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> --- MdePkg/Include/Base.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index ce20b5f01dce..f85f7028a262 100644 --- a/MdePkg/Include/Base.h +++ b/MdePkg/Include/Base.h @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) #endif +/// +/// Portable definition for compile time assertions. +/// Equivalent to C11 static_assert macro from assert.h. +/// Takes condtion and error message as its arguments. +/// +#ifdef _MSC_EXTENSIONS + #define STATIC_ASSERT static_assert +#else + #define STATIC_ASSERT _Static_assert +#endif + /** Macro that returns a pointer to the data structure that contains a specified field of that data structure. This is a lightweight method to hide information by placing a -- 2.20.1 (Apple Git-117) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-13 8:16 ` [PATCH v2 1/1] " vit9696 @ 2019-08-14 13:50 ` Liming Gao 2019-08-14 15:47 ` Michael D Kinney 2019-08-15 16:08 ` Michael D Kinney 1 sibling, 1 reply; 26+ messages in thread From: Liming Gao @ 2019-08-14 13:50 UTC (permalink / raw) To: devel@edk2.groups.io, vit9696@protonmail.com Can you add the sample usage of new macro STATIC_ASSERT? Or, give the link of static_assert or _Static_assert. If so, the developer knows how to use them in source code. Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of vit9696 via Groups.Io > Sent: Tuesday, August 13, 2019 4:17 PM > To: devel@edk2.groups.io > Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 > > Provide a macro for compile time assertions. > Equivalent to C11 static_assert macro from assert.h. > > Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> > --- > MdePkg/Include/Base.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > index ce20b5f01dce..f85f7028a262 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; > #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) > #endif > > +/// > +/// Portable definition for compile time assertions. > +/// Equivalent to C11 static_assert macro from assert.h. > +/// Takes condtion and error message as its arguments. > +/// > +#ifdef _MSC_EXTENSIONS > + #define STATIC_ASSERT static_assert > +#else > + #define STATIC_ASSERT _Static_assert > +#endif > + > /** > Macro that returns a pointer to the data structure that contains a specified field of > that data structure. This is a lightweight method to hide information by placing a > -- > 2.20.1 (Apple Git-117) > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#45503): https://edk2.groups.io/g/devel/message/45503 > Mute This Topic: https://groups.io/mt/32850582/1759384 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [liming.gao@intel.com] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-14 13:50 ` [edk2-devel] " Liming Gao @ 2019-08-14 15:47 ` Michael D Kinney 2019-08-14 16:22 ` Vitaly Cheptosv 0 siblings, 1 reply; 26+ messages in thread From: Michael D Kinney @ 2019-08-14 15:47 UTC (permalink / raw) To: devel@edk2.groups.io, Gao, Liming, vit9696@protonmail.com, Kinney, Michael D Liming, I think a good candidate to demonstrate this feature are the checks made in MdePkg/Include/Base.h. The current implementation forces a divide by 0 in the C pre-processor to break the build. STATIC_ASSERT() would be a better way to do this. I would also remove unused externs from the builds. /** Verifies the storage size of a given data type. This macro generates a divide by zero error or a zero size array declaration in the preprocessor if the size is incorrect. These are declared as "extern" so the space for these arrays will not be in the modules. @param TYPE The date type to determine the size of. @param Size The expected size for the TYPE. **/ #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] // // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with // Section 2.3.1 of the UEFI 2.3 Specification. // VERIFY_SIZE_OF (BOOLEAN, 1); VERIFY_SIZE_OF (INT8, 1); VERIFY_SIZE_OF (UINT8, 1); VERIFY_SIZE_OF (INT16, 2); VERIFY_SIZE_OF (UINT16, 2); VERIFY_SIZE_OF (INT32, 4); VERIFY_SIZE_OF (UINT32, 4); VERIFY_SIZE_OF (INT64, 8); VERIFY_SIZE_OF (UINT64, 8); VERIFY_SIZE_OF (CHAR8, 1); VERIFY_SIZE_OF (CHAR16, 2); // // The following three enum types are used to verify that the compiler // configuration for enum types is compliant with Section 2.3.1 of the // UEFI 2.3 Specification. These enum types and enum values are not // intended to be used. A prefix of '__' is used avoid conflicts with // other types. // typedef enum { __VerifyUint8EnumValue = 0xff } __VERIFY_UINT8_ENUM_SIZE; typedef enum { __VerifyUint16EnumValue = 0xffff } __VERIFY_UINT16_ENUM_SIZE; typedef enum { __VerifyUint32EnumValue = 0xffffffff } __VERIFY_UINT32_ENUM_SIZE; VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); A couple examples. Do all the compilers support the message parameter too? STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") Thanks, Mike > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] > On Behalf Of Liming Gao > Sent: Wednesday, August 14, 2019 6:50 AM > To: devel@edk2.groups.io; vit9696@protonmail.com > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add > STATIC_ASSERT macro > > Can you add the sample usage of new macro STATIC_ASSERT? > > Or, give the link of static_assert or _Static_assert. > > If so, the developer knows how to use them in source > code. > > Thanks > Liming > > -----Original Message----- > > From: devel@edk2.groups.io > [mailto:devel@edk2.groups.io] On Behalf Of > > vit9696 via Groups.Io > > Sent: Tuesday, August 13, 2019 4:17 PM > > To: devel@edk2.groups.io > > Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add > STATIC_ASSERT macro > > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 > > > > Provide a macro for compile time assertions. > > Equivalent to C11 static_assert macro from assert.h. > > > > Signed-off-by: Vitaly Cheptsov > <vit9696@protonmail.com> > > --- > > MdePkg/Include/Base.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/MdePkg/Include/Base.h > b/MdePkg/Include/Base.h index > > ce20b5f01dce..f85f7028a262 100644 > > --- a/MdePkg/Include/Base.h > > +++ b/MdePkg/Include/Base.h > > @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; > #define > > OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) > #endif > > > > +/// > > +/// Portable definition for compile time assertions. > > +/// Equivalent to C11 static_assert macro from > assert.h. > > +/// Takes condtion and error message as its > arguments. > > +/// > > +#ifdef _MSC_EXTENSIONS > > + #define STATIC_ASSERT static_assert #else > > + #define STATIC_ASSERT _Static_assert #endif > > + > > /** > > Macro that returns a pointer to the data structure > that contains a specified field of > > that data structure. This is a lightweight method > to hide > > information by placing a > > -- > > 2.20.1 (Apple Git-117) > > > > > > -=-=-=-=-=-= > > Groups.io Links: You receive all messages sent to this > group. > > > > View/Reply Online (#45503): > > https://edk2.groups.io/g/devel/message/45503 > > Mute This Topic: https://groups.io/mt/32850582/1759384 > > Group Owner: devel+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub > > [liming.gao@intel.com] -=-=-=-=-=-= > > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-14 15:47 ` Michael D Kinney @ 2019-08-14 16:22 ` Vitaly Cheptosv 2019-08-15 1:05 ` Yao, Jiewen 2019-08-16 16:28 ` Laszlo Ersek 0 siblings, 2 replies; 26+ messages in thread From: Vitaly Cheptosv @ 2019-08-14 16:22 UTC (permalink / raw) To: michael.d.kinney; +Cc: devel@edk2.groups.io, Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 8237 bytes --] Michael, Liming, Laszlo, Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. Looking forward to hearing your opinions. Best regards, Vitaly 6.7.10 Static assertions Syntax 1 static_assert-declaration: _Static_assert ( constant-expression , string-literal ) ; Constraints 2 The constant expression shall compare unequal to 0. Semantics 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. Forward references: diagnostics (7.2). 7.2 Diagnostics <assert. h> 3 The macro static_assert expands to _Static_assert. > 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com> написал(а): > > > Liming, > > I think a good candidate to demonstrate this > feature are the checks made in MdePkg/Include/Base.h. > The current implementation forces a divide by 0 > in the C pre-processor to break the build. > STATIC_ASSERT() would be a better way to do this. > I would also remove unused externs from the builds. > > /** > Verifies the storage size of a given data type. > > This macro generates a divide by zero error or a zero size array declaration in > the preprocessor if the size is incorrect. These are declared as "extern" so > the space for these arrays will not be in the modules. > > @param TYPE The date type to determine the size of. > @param Size The expected size for the TYPE. > > **/ > #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] > > // > // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with > // Section 2.3.1 of the UEFI 2.3 Specification. > // > VERIFY_SIZE_OF (BOOLEAN, 1); > VERIFY_SIZE_OF (INT8, 1); > VERIFY_SIZE_OF (UINT8, 1); > VERIFY_SIZE_OF (INT16, 2); > VERIFY_SIZE_OF (UINT16, 2); > VERIFY_SIZE_OF (INT32, 4); > VERIFY_SIZE_OF (UINT32, 4); > VERIFY_SIZE_OF (INT64, 8); > VERIFY_SIZE_OF (UINT64, 8); > VERIFY_SIZE_OF (CHAR8, 1); > VERIFY_SIZE_OF (CHAR16, 2); > > // > // The following three enum types are used to verify that the compiler > // configuration for enum types is compliant with Section 2.3.1 of the > // UEFI 2.3 Specification. These enum types and enum values are not > // intended to be used. A prefix of '__' is used avoid conflicts with > // other types. > // > typedef enum { > __VerifyUint8EnumValue = 0xff > } __VERIFY_UINT8_ENUM_SIZE; > > typedef enum { > __VerifyUint16EnumValue = 0xffff > } __VERIFY_UINT16_ENUM_SIZE; > > typedef enum { > __VerifyUint32EnumValue = 0xffffffff > } __VERIFY_UINT32_ENUM_SIZE; > > VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); > > A couple examples. Do all the compilers support the message parameter too? > > STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > > Thanks, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] >> On Behalf Of Liming Gao >> Sent: Wednesday, August 14, 2019 6:50 AM >> To: devel@edk2.groups.io; vit9696@protonmail.com >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >> >> Can you add the sample usage of new macro STATIC_ASSERT? >> >> Or, give the link of static_assert or _Static_assert. >> >> If so, the developer knows how to use them in source >> code. >> >> Thanks >> Liming >>> -----Original Message----- >>> From: devel@edk2.groups.io >> [mailto:devel@edk2.groups.io] On Behalf Of >>> vit9696 via Groups.Io >>> Sent: Tuesday, August 13, 2019 4:17 PM >>> To: devel@edk2.groups.io >>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >>> >>> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>> >>> Provide a macro for compile time assertions. >>> Equivalent to C11 static_assert macro from assert.h. >>> >>> Signed-off-by: Vitaly Cheptsov >> <vit9696@protonmail.com> >>> --- >>> MdePkg/Include/Base.h | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/MdePkg/Include/Base.h >> b/MdePkg/Include/Base.h index >>> ce20b5f01dce..f85f7028a262 100644 >>> --- a/MdePkg/Include/Base.h >>> +++ b/MdePkg/Include/Base.h >>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >> #define >>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >> #endif >>> >>> +/// >>> +/// Portable definition for compile time assertions. >>> +/// Equivalent to C11 static_assert macro from >> assert.h. >>> +/// Takes condtion and error message as its >> arguments. >>> +/// >>> +#ifdef _MSC_EXTENSIONS >>> + #define STATIC_ASSERT static_assert #else >>> + #define STATIC_ASSERT _Static_assert #endif >>> + >>> /** >>> Macro that returns a pointer to the data structure >> that contains a specified field of >>> that data structure. This is a lightweight method >> to hide >>> information by placing a >>> -- >>> 2.20.1 (Apple Git-117) >>> >>> >>> -=-=-=-=-=-= >>> Groups.io Links: You receive all messages sent to this >> group. >>> >>> View/Reply Online (#45503): >>> https://edk2.groups.io/g/devel/message/45503 >>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>> Group Owner: devel+owner@edk2.groups.io >>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>> [liming.gao@intel.com] -=-=-=-=-=-= >> >> >> > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-14 16:22 ` Vitaly Cheptosv @ 2019-08-15 1:05 ` Yao, Jiewen 2019-08-15 1:59 ` Liming Gao 2019-08-16 16:28 ` Laszlo Ersek 1 sibling, 1 reply; 26+ messages in thread From: Yao, Jiewen @ 2019-08-15 1:05 UTC (permalink / raw) To: devel@edk2.groups.io, vit9696@protonmail.com, Kinney, Michael D Cc: Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 9279 bytes --] Good input. I think we should separate the work to convert all EDKII code to use STATIC_ASSERT. We can do that work once we add STATIC_ASSERT. I recommend: 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla 3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla Thank you Yao Jiewen From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io Sent: Thursday, August 15, 2019 12:23 AM To: Kinney, Michael D <michael.d.kinney@intel.com> Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Michael, Liming, Laszlo, Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. Looking forward to hearing your opinions. Best regards, Vitaly 6.7.10 Static assertions Syntax 1 static_assert-declaration: _Static_assert ( constant-expression , string-literal ) ; Constraints 2 The constant expression shall compare unequal to 0. Semantics 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. Forward references: diagnostics (7.2). 7.2 Diagnostics <assert. h> 3 The macro static_assert expands to _Static_assert. > 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а): > > > Liming, > > I think a good candidate to demonstrate this > feature are the checks made in MdePkg/Include/Base.h. > The current implementation forces a divide by 0 > in the C pre-processor to break the build. > STATIC_ASSERT() would be a better way to do this. > I would also remove unused externs from the builds. > > /** > Verifies the storage size of a given data type. > > This macro generates a divide by zero error or a zero size array declaration in > the preprocessor if the size is incorrect. These are declared as "extern" so > the space for these arrays will not be in the modules. > > @param TYPE The date type to determine the size of. > @param Size The expected size for the TYPE. > > **/ > #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] > > // > // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with > // Section 2.3.1 of the UEFI 2.3 Specification. > // > VERIFY_SIZE_OF (BOOLEAN, 1); > VERIFY_SIZE_OF (INT8, 1); > VERIFY_SIZE_OF (UINT8, 1); > VERIFY_SIZE_OF (INT16, 2); > VERIFY_SIZE_OF (UINT16, 2); > VERIFY_SIZE_OF (INT32, 4); > VERIFY_SIZE_OF (UINT32, 4); > VERIFY_SIZE_OF (INT64, 8); > VERIFY_SIZE_OF (UINT64, 8); > VERIFY_SIZE_OF (CHAR8, 1); > VERIFY_SIZE_OF (CHAR16, 2); > > // > // The following three enum types are used to verify that the compiler > // configuration for enum types is compliant with Section 2.3.1 of the > // UEFI 2.3 Specification. These enum types and enum values are not > // intended to be used. A prefix of '__' is used avoid conflicts with > // other types. > // > typedef enum { > __VerifyUint8EnumValue = 0xff > } __VERIFY_UINT8_ENUM_SIZE; > > typedef enum { > __VerifyUint16EnumValue = 0xffff > } __VERIFY_UINT16_ENUM_SIZE; > > typedef enum { > __VerifyUint32EnumValue = 0xffffffff > } __VERIFY_UINT32_ENUM_SIZE; > > VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); > > A couple examples. Do all the compilers support the message parameter too? > > STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > > Thanks, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] >> On Behalf Of Liming Gao >> Sent: Wednesday, August 14, 2019 6:50 AM >> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com> >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >> >> Can you add the sample usage of new macro STATIC_ASSERT? >> >> Or, give the link of static_assert or _Static_assert. >> >> If so, the developer knows how to use them in source >> code. >> >> Thanks >> Liming >>> -----Original Message----- >>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >> [mailto:devel@edk2.groups.io] On Behalf Of >>> vit9696 via Groups.Io >>> Sent: Tuesday, August 13, 2019 4:17 PM >>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >>> >>> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>> >>> Provide a macro for compile time assertions. >>> Equivalent to C11 static_assert macro from assert.h. >>> >>> Signed-off-by: Vitaly Cheptsov >> <vit9696@protonmail.com<mailto:vit9696@protonmail.com>> >>> --- >>> MdePkg/Include/Base.h | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/MdePkg/Include/Base.h >> b/MdePkg/Include/Base.h index >>> ce20b5f01dce..f85f7028a262 100644 >>> --- a/MdePkg/Include/Base.h >>> +++ b/MdePkg/Include/Base.h >>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >> #define >>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >> #endif >>> >>> +/// >>> +/// Portable definition for compile time assertions. >>> +/// Equivalent to C11 static_assert macro from >> assert.h. >>> +/// Takes condtion and error message as its >> arguments. >>> +/// >>> +#ifdef _MSC_EXTENSIONS >>> + #define STATIC_ASSERT static_assert #else >>> + #define STATIC_ASSERT _Static_assert #endif >>> + >>> /** >>> Macro that returns a pointer to the data structure >> that contains a specified field of >>> that data structure. This is a lightweight method >> to hide >>> information by placing a >>> -- >>> 2.20.1 (Apple Git-117) >>> >>> >>> -=-=-=-=-=-= >>> Groups.io Links: You receive all messages sent to this >> group. >>> >>> View/Reply Online (#45503): >>> https://edk2.groups.io/g/devel/message/45503 >>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> >>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>> [liming.gao@intel.com] -=-=-=-=-=-= >> >> >> > [-- Attachment #2: Type: text/html, Size: 38542 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-15 1:05 ` Yao, Jiewen @ 2019-08-15 1:59 ` Liming Gao 2019-08-15 2:22 ` Vitaly Cheptosv 2019-08-16 16:33 ` Laszlo Ersek 0 siblings, 2 replies; 26+ messages in thread From: Liming Gao @ 2019-08-15 1:59 UTC (permalink / raw) To: devel@edk2.groups.io, Yao, Jiewen, vit9696@protonmail.com, Kinney, Michael D Cc: Laszlo Ersek, leif.lindholm@linaro.org, afish@apple.com, Cetola, Stephano [-- Attachment #1: Type: text/plain, Size: 9979 bytes --] Vitaly: As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose this patch plan to catch this stable tag? If yes, please ask the feedback from Tianocore Stewards. I have cc this patch to all Stewards. Thanks Liming From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Yao, Jiewen Sent: Thursday, August 15, 2019 9:05 AM To: devel@edk2.groups.io; vit9696@protonmail.com; Kinney, Michael D <michael.d.kinney@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Good input. I think we should separate the work to convert all EDKII code to use STATIC_ASSERT. We can do that work once we add STATIC_ASSERT. I recommend: 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla 3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla Thank you Yao Jiewen From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io Sent: Thursday, August 15, 2019 12:23 AM To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Michael, Liming, Laszlo, Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. Looking forward to hearing your opinions. Best regards, Vitaly 6.7.10 Static assertions Syntax 1 static_assert-declaration: _Static_assert ( constant-expression , string-literal ) ; Constraints 2 The constant expression shall compare unequal to 0. Semantics 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. Forward references: diagnostics (7.2). 7.2 Diagnostics <assert. h> 3 The macro static_assert expands to _Static_assert. > 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а): > > > Liming, > > I think a good candidate to demonstrate this > feature are the checks made in MdePkg/Include/Base.h. > The current implementation forces a divide by 0 > in the C pre-processor to break the build. > STATIC_ASSERT() would be a better way to do this. > I would also remove unused externs from the builds. > > /** > Verifies the storage size of a given data type. > > This macro generates a divide by zero error or a zero size array declaration in > the preprocessor if the size is incorrect. These are declared as "extern" so > the space for these arrays will not be in the modules. > > @param TYPE The date type to determine the size of. > @param Size The expected size for the TYPE. > > **/ > #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] > > // > // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with > // Section 2.3.1 of the UEFI 2.3 Specification. > // > VERIFY_SIZE_OF (BOOLEAN, 1); > VERIFY_SIZE_OF (INT8, 1); > VERIFY_SIZE_OF (UINT8, 1); > VERIFY_SIZE_OF (INT16, 2); > VERIFY_SIZE_OF (UINT16, 2); > VERIFY_SIZE_OF (INT32, 4); > VERIFY_SIZE_OF (UINT32, 4); > VERIFY_SIZE_OF (INT64, 8); > VERIFY_SIZE_OF (UINT64, 8); > VERIFY_SIZE_OF (CHAR8, 1); > VERIFY_SIZE_OF (CHAR16, 2); > > // > // The following three enum types are used to verify that the compiler > // configuration for enum types is compliant with Section 2.3.1 of the > // UEFI 2.3 Specification. These enum types and enum values are not > // intended to be used. A prefix of '__' is used avoid conflicts with > // other types. > // > typedef enum { > __VerifyUint8EnumValue = 0xff > } __VERIFY_UINT8_ENUM_SIZE; > > typedef enum { > __VerifyUint16EnumValue = 0xffff > } __VERIFY_UINT16_ENUM_SIZE; > > typedef enum { > __VerifyUint32EnumValue = 0xffffffff > } __VERIFY_UINT32_ENUM_SIZE; > > VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); > > A couple examples. Do all the compilers support the message parameter too? > > STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > > Thanks, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] >> On Behalf Of Liming Gao >> Sent: Wednesday, August 14, 2019 6:50 AM >> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com> >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >> >> Can you add the sample usage of new macro STATIC_ASSERT? >> >> Or, give the link of static_assert or _Static_assert. >> >> If so, the developer knows how to use them in source >> code. >> >> Thanks >> Liming >>> -----Original Message----- >>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >> [mailto:devel@edk2.groups.io] On Behalf Of >>> vit9696 via Groups.Io >>> Sent: Tuesday, August 13, 2019 4:17 PM >>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >>> >>> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>> >>> Provide a macro for compile time assertions. >>> Equivalent to C11 static_assert macro from assert.h. >>> >>> Signed-off-by: Vitaly Cheptsov >> <vit9696@protonmail.com<mailto:vit9696@protonmail.com>> >>> --- >>> MdePkg/Include/Base.h | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/MdePkg/Include/Base.h >> b/MdePkg/Include/Base.h index >>> ce20b5f01dce..f85f7028a262 100644 >>> --- a/MdePkg/Include/Base.h >>> +++ b/MdePkg/Include/Base.h >>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >> #define >>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >> #endif >>> >>> +/// >>> +/// Portable definition for compile time assertions. >>> +/// Equivalent to C11 static_assert macro from >> assert.h. >>> +/// Takes condtion and error message as its >> arguments. >>> +/// >>> +#ifdef _MSC_EXTENSIONS >>> + #define STATIC_ASSERT static_assert #else >>> + #define STATIC_ASSERT _Static_assert #endif >>> + >>> /** >>> Macro that returns a pointer to the data structure >> that contains a specified field of >>> that data structure. This is a lightweight method >> to hide >>> information by placing a >>> -- >>> 2.20.1 (Apple Git-117) >>> >>> >>> -=-=-=-=-=-= >>> Groups.io Links: You receive all messages sent to this >> group. >>> >>> View/Reply Online (#45503): >>> https://edk2.groups.io/g/devel/message/45503 >>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> >>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>> [liming.gao@intel.com] -=-=-=-=-=-= >> >> >> > [-- Attachment #2: Type: text/html, Size: 41013 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-15 1:59 ` Liming Gao @ 2019-08-15 2:22 ` Vitaly Cheptosv 2019-08-15 7:36 ` Yao, Jiewen 2019-08-16 16:33 ` Laszlo Ersek 1 sibling, 1 reply; 26+ messages in thread From: Vitaly Cheptosv @ 2019-08-15 2:22 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io, Yao, Jiewen, Kinney, Michael D Cc: Laszlo Ersek, leif.lindholm@linaro.org, afish@apple.com, Cetola, Stephano [-- Attachment #1: Type: text/plain, Size: 9895 bytes --] Liming, Thank you for adding everyone to the CC list. Yes, I would like this to be merged into the next EDK II stable release. Best regards, Vitaly On чт, авг. 15, 2019 at 04:59, Gao, Liming <liming.gao@intel.com> wrote: > Vitaly: > > As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose this patch plan to catch this stable tag? > > If yes, please ask the feedback from Tianocore Stewards. I have cc this patch to all Stewards. > > Thanks > > Liming[] > > []From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Yao, Jiewen > Sent: Thursday, August 15, 2019 9:05 AM > To: devel@edk2.groups.io; vit9696@protonmail.com; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro > > Good input. > > I think we should separate the work to convert all EDKII code to use STATIC_ASSERT. > > We can do that work once we add STATIC_ASSERT. > > I recommend: > > 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla > > 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla > > 3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla > > Thank you > > Yao Jiewen > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io > Sent: Thursday, August 15, 2019 12:23 AM > To: Kinney, Michael D <michael.d.kinney@intel.com> > Cc: devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.com> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro > > Michael, Liming, Laszlo, > > Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. > > The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. > > In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). > > GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. > > As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. > > I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. > > As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. > > Looking forward to hearing your opinions. > > Best regards, > Vitaly > > 6.7.10 Static assertions > > Syntax > 1 static_assert-declaration: > _Static_assert ( constant-expression , string-literal ) ; > > Constraints > 2 The constant expression shall compare unequal to 0. > > Semantics > 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. > > Forward references: diagnostics (7.2). > > 7.2 Diagnostics <assert. h> > > 3 The macro > static_assert > expands to _Static_assert. > >> 14авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com> написал(а): >> >> >> Liming, >> >> I think a good candidate to demonstrate this >> feature are the checks made in MdePkg/Include/Base.h. >> The current implementation forces a divide by 0 >> in the C pre-processor to break the build. >> STATIC_ASSERT() would be a better way to do this. >> I would also remove unused externs from the builds. >> >> /** >> Verifies the storage size of a given data type. >> >> This macro generates a divide by zero error or a zero size array declaration in >> the preprocessor if the size is incorrect. These are declared as "extern" so >> the space for these arrays will not be in the modules. >> >> @param TYPE The date type to determine the size of. >> @param Size The expected size for the TYPE. >> >> **/ >> #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] > >> >> // >> // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with >> // Section 2.3.1 of the UEFI 2.3 Specification. >> // >> VERIFY_SIZE_OF (BOOLEAN, 1); >> VERIFY_SIZE_OF (INT8, 1); >> VERIFY_SIZE_OF (UINT8, 1); >> VERIFY_SIZE_OF (INT16, 2); >> VERIFY_SIZE_OF (UINT16, 2); >> VERIFY_SIZE_OF (INT32, 4); >> VERIFY_SIZE_OF (UINT32, 4); >> VERIFY_SIZE_OF (INT64, 8); >> VERIFY_SIZE_OF (UINT64, 8); >> VERIFY_SIZE_OF (CHAR8, 1); >> VERIFY_SIZE_OF (CHAR16, 2); >> >> // >> // The following three enum types are used to verify that the compiler >> // configuration for enum types is compliant with Section 2.3.1 of the >> // UEFI 2.3 Specification. These enum types and enum values are not >> // intended to be used. A prefix of '__' is used avoid conflicts with >> // other types. >> // >> typedef enum { >> __VerifyUint8EnumValue = 0xff >> } __VERIFY_UINT8_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint16EnumValue = 0xffff >> } __VERIFY_UINT16_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint32EnumValue = 0xffffffff >> } __VERIFY_UINT32_ENUM_SIZE; >> >> VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); >> VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); >> VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); >> >> A couple examples. Do all the compilers support the message parameter too? >> >> STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > >> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > >> >> Thanks, >> >> Mike >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] >>> On Behalf Of Liming Gao >>> Sent: Wednesday, August 14, 2019 6:50 AM >>> To: devel@edk2.groups.io; vit9696@protonmail.com >>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> STATIC_ASSERT macro >>> >>> Can you add the sample usage of new macro STATIC_ASSERT? >>> >>> Or, give the link of static_assert or _Static_assert. >>> >>> If so, the developer knows how to use them in source >>> code. >>> >>> Thanks >>> Liming >>>> -----Original Message----- >>>> From: devel@edk2.groups.io >>> [mailto:devel@edk2.groups.io] On Behalf Of >>>> vit9696 via Groups.Io >>>> Sent: Tuesday, August 13, 2019 4:17 PM >>>> To: devel@edk2.groups.io >>>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> STATIC_ASSERT macro >>>> >>>> >>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>>> >>>> Provide a macro for compile time assertions. >>>> Equivalent to C11 static_assert macro from assert.h. >>>> >>>> Signed-off-by: Vitaly Cheptsov >>> <vit9696@protonmail.com> >>>> --- >>>> MdePkg/Include/Base.h | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/MdePkg/Include/Base.h >>> b/MdePkg/Include/Base.h index >>>> ce20b5f01dce..f85f7028a262 100644 >>>> --- a/MdePkg/Include/Base.h >>>> +++ b/MdePkg/Include/Base.h >>>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >>> #define >>>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >>> #endif >>>> >>>> +/// >>>> +/// Portable definition for compile time assertions. >>>> +/// Equivalent to C11 static_assert macro from >>> assert.h. >>>> +/// Takes condtion and error message as its >>> arguments. >>>> +/// >>>> +#ifdef _MSC_EXTENSIONS >>>> + #define STATIC_ASSERT static_assert #else >>>> + #define STATIC_ASSERT _Static_assert #endif >>>> + >>>> /** >>>> Macro that returns a pointer to the data structure >>> that contains a specified field of >>>> that data structure. This is a lightweight method >>> to hide >>>> information by placing a >>>> -- >>>> 2.20.1 (Apple Git-117) >>>> >>>> >>>> -=-=-=-=-=-= >>>> Groups.io Links: You receive all messages sent to this >>> group. >>>> >>>> View/Reply Online (#45503): >>>> https://edk2.groups.io/g/devel/message/45503 >>>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>>> Group Owner: devel+owner@edk2.groups.io >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>> [liming.gao@intel.com] -=-=-=-=-=-= >>> >>> >>> >> > > [-- Attachment #2: Type: text/html, Size: 36533 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-15 2:22 ` Vitaly Cheptosv @ 2019-08-15 7:36 ` Yao, Jiewen 0 siblings, 0 replies; 26+ messages in thread From: Yao, Jiewen @ 2019-08-15 7:36 UTC (permalink / raw) To: devel@edk2.groups.io, vit9696@protonmail.com, Gao, Liming, Kinney, Michael D Cc: Laszlo Ersek, leif.lindholm@linaro.org, afish@apple.com, Cetola, Stephano [-- Attachment #1: Type: text/plain, Size: 10986 bytes --] Acked-by: Jiewen Yao <Jiewen.yao@intel.com> Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io Sent: Thursday, August 15, 2019 10:22 AM To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Cc: Laszlo Ersek <lersek@redhat.com>; leif.lindholm@linaro.org; afish@apple.com; Cetola, Stephano <stephano.cetola@intel.com> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Liming, Thank you for adding everyone to the CC list. Yes, I would like this to be merged into the next EDK II stable release. Best regards, Vitaly On чт, авг. 15, 2019 at 04:59, Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> wrote: Vitaly: As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose this patch plan to catch this stable tag? If yes, please ask the feedback from Tianocore Stewards. I have cc this patch to all Stewards. Thanks Liming From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Yao, Jiewen Sent: Thursday, August 15, 2019 9:05 AM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Good input. I think we should separate the work to convert all EDKII code to use STATIC_ASSERT. We can do that work once we add STATIC_ASSERT. I recommend: 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla 3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla Thank you Yao Jiewen From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io Sent: Thursday, August 15, 2019 12:23 AM To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Michael, Liming, Laszlo, Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. Looking forward to hearing your opinions. Best regards, Vitaly 6.7.10 Static assertions Syntax 1 static_assert-declaration: _Static_assert ( constant-expression , string-literal ) ; Constraints 2 The constant expression shall compare unequal to 0. Semantics 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. Forward references: diagnostics (7.2). 7.2 Diagnostics <assert. h> 3 The macro static_assert expands to _Static_assert. > 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а): > > > Liming, > > I think a good candidate to demonstrate this > feature are the checks made in MdePkg/Include/Base.h. > The current implementation forces a divide by 0 > in the C pre-processor to break the build. > STATIC_ASSERT() would be a better way to do this. > I would also remove unused externs from the builds. > > /** > Verifies the storage size of a given data type. > > This macro generates a divide by zero error or a zero size array declaration in > the preprocessor if the size is incorrect. These are declared as "extern" so > the space for these arrays will not be in the modules. > > @param TYPE The date type to determine the size of. > @param Size The expected size for the TYPE. > > **/ > #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] > > // > // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with > // Section 2.3.1 of the UEFI 2.3 Specification. > // > VERIFY_SIZE_OF (BOOLEAN, 1); > VERIFY_SIZE_OF (INT8, 1); > VERIFY_SIZE_OF (UINT8, 1); > VERIFY_SIZE_OF (INT16, 2); > VERIFY_SIZE_OF (UINT16, 2); > VERIFY_SIZE_OF (INT32, 4); > VERIFY_SIZE_OF (UINT32, 4); > VERIFY_SIZE_OF (INT64, 8); > VERIFY_SIZE_OF (UINT64, 8); > VERIFY_SIZE_OF (CHAR8, 1); > VERIFY_SIZE_OF (CHAR16, 2); > > // > // The following three enum types are used to verify that the compiler > // configuration for enum types is compliant with Section 2.3.1 of the > // UEFI 2.3 Specification. These enum types and enum values are not > // intended to be used. A prefix of '__' is used avoid conflicts with > // other types. > // > typedef enum { > __VerifyUint8EnumValue = 0xff > } __VERIFY_UINT8_ENUM_SIZE; > > typedef enum { > __VerifyUint16EnumValue = 0xffff > } __VERIFY_UINT16_ENUM_SIZE; > > typedef enum { > __VerifyUint32EnumValue = 0xffffffff > } __VERIFY_UINT32_ENUM_SIZE; > > VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); > VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); > > A couple examples. Do all the compilers support the message parameter too? > > STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > > Thanks, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] >> On Behalf Of Liming Gao >> Sent: Wednesday, August 14, 2019 6:50 AM >> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com> >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >> >> Can you add the sample usage of new macro STATIC_ASSERT? >> >> Or, give the link of static_assert or _Static_assert. >> >> If so, the developer knows how to use them in source >> code. >> >> Thanks >> Liming >>> -----Original Message----- >>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >> [mailto:devel@edk2.groups.io] On Behalf Of >>> vit9696 via Groups.Io >>> Sent: Tuesday, August 13, 2019 4:17 PM >>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >>> >>> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>> >>> Provide a macro for compile time assertions. >>> Equivalent to C11 static_assert macro from assert.h. >>> >>> Signed-off-by: Vitaly Cheptsov >> <vit9696@protonmail.com<mailto:vit9696@protonmail.com>> >>> --- >>> MdePkg/Include/Base.h | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/MdePkg/Include/Base.h >> b/MdePkg/Include/Base.h index >>> ce20b5f01dce..f85f7028a262 100644 >>> --- a/MdePkg/Include/Base.h >>> +++ b/MdePkg/Include/Base.h >>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >> #define >>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >> #endif >>> >>> +/// >>> +/// Portable definition for compile time assertions. >>> +/// Equivalent to C11 static_assert macro from >> assert.h. >>> +/// Takes condtion and error message as its >> arguments. >>> +/// >>> +#ifdef _MSC_EXTENSIONS >>> + #define STATIC_ASSERT static_assert #else >>> + #define STATIC_ASSERT _Static_assert #endif >>> + >>> /** >>> Macro that returns a pointer to the data structure >> that contains a specified field of >>> that data structure. This is a lightweight method >> to hide >>> information by placing a >>> -- >>> 2.20.1 (Apple Git-117) >>> >>> >>> -=-=-=-=-=-= >>> Groups.io Links: You receive all messages sent to this >> group. >>> >>> View/Reply Online (#45503): >>> https://edk2.groups.io/g/devel/message/45503 >>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> >>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>> [liming.gao@intel.com] -=-=-=-=-=-= >> >> >> > [-- Attachment #2: Type: text/html, Size: 43942 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-15 1:59 ` Liming Gao 2019-08-15 2:22 ` Vitaly Cheptosv @ 2019-08-16 16:33 ` Laszlo Ersek 2019-08-16 17:23 ` Vitaly Cheptsov 1 sibling, 1 reply; 26+ messages in thread From: Laszlo Ersek @ 2019-08-16 16:33 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io, Yao, Jiewen, vit9696@protonmail.com, Kinney, Michael D Cc: leif.lindholm@linaro.org, afish@apple.com, Cetola, Stephano On 08/15/19 03:59, Gao, Liming wrote: > Vitaly: > As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose this patch plan to catch this stable tag? > If yes, please ask the feedback from Tianocore Stewards. I have cc this patch to all Stewards. If a feature patch (or series) is fully reviewed before the soft feature freeze (by the respective package maintainers), it can be merged during the soft feature freeze. However, I don't think this patch is mature enough for that. As I've just said up-thread, I'd like to see STATIC_ASSERT being put to use at once (in the same series, not in the same patch). In addition, the documentation should be improved (the (constant-expression , string-literal) parameter list seems absent, or at least insufficiently documented). In turn, I doubt a v3 posting could be reviewed with enough care before we enter the soft feature freeze. I'd suggest to submit the v3 series as soon as we start the next development cycle. Thanks Laszlo > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Yao, Jiewen > Sent: Thursday, August 15, 2019 9:05 AM > To: devel@edk2.groups.io; vit9696@protonmail.com; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro > > Good input. > I think we should separate the work to convert all EDKII code to use STATIC_ASSERT. > We can do that work once we add STATIC_ASSERT. > > I recommend: > > 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla > > 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla > > 3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla > > Thank you > Yao Jiewen > > From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io > Sent: Thursday, August 15, 2019 12:23 AM > To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> > Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro > > > Michael, Liming, Laszlo, > > Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. > > The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. > > In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). > > GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. > > As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. > > I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. > > As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. > > Looking forward to hearing your opinions. > > Best regards, > Vitaly > > > 6.7.10 Static assertions > > Syntax > 1 static_assert-declaration: > _Static_assert ( constant-expression , string-literal ) ; > > Constraints > 2 The constant expression shall compare unequal to 0. > > Semantics > 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. > > Forward references: diagnostics (7.2). > > 7.2 Diagnostics <assert. h> > > 3 The macro > static_assert > expands to _Static_assert. > > >> 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а): >> >> >> Liming, >> >> I think a good candidate to demonstrate this >> feature are the checks made in MdePkg/Include/Base.h. >> The current implementation forces a divide by 0 >> in the C pre-processor to break the build. >> STATIC_ASSERT() would be a better way to do this. >> I would also remove unused externs from the builds. >> >> /** >> Verifies the storage size of a given data type. >> >> This macro generates a divide by zero error or a zero size array declaration in >> the preprocessor if the size is incorrect. These are declared as "extern" so >> the space for these arrays will not be in the modules. >> >> @param TYPE The date type to determine the size of. >> @param Size The expected size for the TYPE. >> >> **/ >> #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] > >> >> // >> // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with >> // Section 2.3.1 of the UEFI 2.3 Specification. >> // >> VERIFY_SIZE_OF (BOOLEAN, 1); >> VERIFY_SIZE_OF (INT8, 1); >> VERIFY_SIZE_OF (UINT8, 1); >> VERIFY_SIZE_OF (INT16, 2); >> VERIFY_SIZE_OF (UINT16, 2); >> VERIFY_SIZE_OF (INT32, 4); >> VERIFY_SIZE_OF (UINT32, 4); >> VERIFY_SIZE_OF (INT64, 8); >> VERIFY_SIZE_OF (UINT64, 8); >> VERIFY_SIZE_OF (CHAR8, 1); >> VERIFY_SIZE_OF (CHAR16, 2); >> >> // >> // The following three enum types are used to verify that the compiler >> // configuration for enum types is compliant with Section 2.3.1 of the >> // UEFI 2.3 Specification. These enum types and enum values are not >> // intended to be used. A prefix of '__' is used avoid conflicts with >> // other types. >> // >> typedef enum { >> __VerifyUint8EnumValue = 0xff >> } __VERIFY_UINT8_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint16EnumValue = 0xffff >> } __VERIFY_UINT16_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint32EnumValue = 0xffffffff >> } __VERIFY_UINT32_ENUM_SIZE; >> >> VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); >> VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); >> VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); >> >> A couple examples. Do all the compilers support the message parameter too? >> >> STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > >> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") > >> >> Thanks, >> >> Mike >> >>> -----Original Message----- >>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] >>> On Behalf Of Liming Gao >>> Sent: Wednesday, August 14, 2019 6:50 AM >>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com> >>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> STATIC_ASSERT macro >>> >>> Can you add the sample usage of new macro STATIC_ASSERT? >>> >>> Or, give the link of static_assert or _Static_assert. >>> >>> If so, the developer knows how to use them in source >>> code. >>> >>> Thanks >>> Liming >>>> -----Original Message----- >>>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>> [mailto:devel@edk2.groups.io] On Behalf Of >>>> vit9696 via Groups.Io >>>> Sent: Tuesday, August 13, 2019 4:17 PM >>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> STATIC_ASSERT macro >>>> >>>> >>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>>> >>>> Provide a macro for compile time assertions. >>>> Equivalent to C11 static_assert macro from assert.h. >>>> >>>> Signed-off-by: Vitaly Cheptsov >>> <vit9696@protonmail.com<mailto:vit9696@protonmail.com>> >>>> --- >>>> MdePkg/Include/Base.h | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/MdePkg/Include/Base.h >>> b/MdePkg/Include/Base.h index >>>> ce20b5f01dce..f85f7028a262 100644 >>>> --- a/MdePkg/Include/Base.h >>>> +++ b/MdePkg/Include/Base.h >>>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >>> #define >>>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >>> #endif >>>> >>>> +/// >>>> +/// Portable definition for compile time assertions. >>>> +/// Equivalent to C11 static_assert macro from >>> assert.h. >>>> +/// Takes condtion and error message as its >>> arguments. >>>> +/// >>>> +#ifdef _MSC_EXTENSIONS >>>> + #define STATIC_ASSERT static_assert #else >>>> + #define STATIC_ASSERT _Static_assert #endif >>>> + >>>> /** >>>> Macro that returns a pointer to the data structure >>> that contains a specified field of >>>> that data structure. This is a lightweight method >>> to hide >>>> information by placing a >>>> -- >>>> 2.20.1 (Apple Git-117) >>>> >>>> >>>> -=-=-=-=-=-= >>>> Groups.io Links: You receive all messages sent to this >>> group. >>>> >>>> View/Reply Online (#45503): >>>> https://edk2.groups.io/g/devel/message/45503 >>>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>>> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>> [liming.gao@intel.com] -=-=-=-=-=-= >>> >>> >>> >> > > > > > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 16:33 ` Laszlo Ersek @ 2019-08-16 17:23 ` Vitaly Cheptsov 2019-08-16 19:38 ` Laszlo Ersek 0 siblings, 1 reply; 26+ messages in thread From: Vitaly Cheptsov @ 2019-08-16 17:23 UTC (permalink / raw) To: Laszlo Ersek Cc: devel@edk2.groups.io, leif.lindholm@linaro.org, afish@apple.com [-- Attachment #1: Type: text/plain, Size: 12434 bytes --] Laszlo, I have already mentioned that the documentation is sufficient as _Static_assert is C standard, so I do not plan to make a V3 for this patch. The patch is merge ready. As for usage examples I have an opposing opinion to yours and believe it is based on very good reasons. Not using STATIC_ASSERT in the current release will make the feature optionally available and let people test it in their setups. In case they notice it does not work for them they will have 3 months grace period to report it to us and consider making a change. This will also give them 3 months grace period of VERIFY_SIZE_OF macro removal in favour of STATIC_ASSERT. Making the change now will let people do seamless transition to the new feature and will avoid obstacles you are currently trying to create. Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be separate patchsets with potentially separate BZs. Thanks for understanding, Vitaly > 16 авг. 2019 г., в 19:33, Laszlo Ersek <lersek@redhat.com> написал(а): > > > On 08/15/19 03:59, Gao, Liming wrote: >> Vitaly: >> As you know, edk2 201908 stable tag will start soft freeze tomorrow. Dose this patch plan to catch this stable tag? >> If yes, please ask the feedback from Tianocore Stewards. I have cc this patch to all Stewards. > > If a feature patch (or series) is fully reviewed before the soft feature > freeze (by the respective package maintainers), it can be merged during > the soft feature freeze. > > However, I don't think this patch is mature enough for that. As I've > just said up-thread, I'd like to see STATIC_ASSERT being put to use at > once (in the same series, not in the same patch). In addition, the > documentation should be improved (the (constant-expression , > string-literal) parameter list seems absent, or at least insufficiently > documented). > > In turn, I doubt a v3 posting could be reviewed with enough care before > we enter the soft feature freeze. I'd suggest to submit the v3 series as > soon as we start the next development cycle. > > Thanks > Laszlo > >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Yao, Jiewen >> Sent: Thursday, August 15, 2019 9:05 AM >> To: devel@edk2.groups.io; vit9696@protonmail.com; Kinney, Michael D <michael.d.kinney@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro >> >> Good input. >> I think we should separate the work to convert all EDKII code to use STATIC_ASSERT. >> We can do that work once we add STATIC_ASSERT. >> >> I recommend: >> >> 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla >> >> 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERIFY_SIZE_OF – the other patch and the other Bugzilla >> >> 3) Step 3: Scan the rest, if there is need. – Another patch and another Bugzilla >> >> Thank you >> Yao Jiewen >> >> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptosv via Groups.Io >> Sent: Thursday, August 15, 2019 12:23 AM >> To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> >> Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro >> >> >> Michael, Liming, Laszlo, >> >> Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. >> >> The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. >> >> In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). >> >> GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. >> >> As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. >> >> I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. >> >> As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. >> >> Looking forward to hearing your opinions. >> >> Best regards, >> Vitaly >> >> >> 6.7.10 Static assertions >> >> Syntax >> 1 static_assert-declaration: >> _Static_assert ( constant-expression , string-literal ) ; >> >> Constraints >> 2 The constant expression shall compare unequal to 0. >> >> Semantics >> 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. >> >> Forward references: diagnostics (7.2). >> >> 7.2 Diagnostics <assert. h> >> >> 3 The macro >> static_assert >> expands to _Static_assert. >> >> >>> 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> написал(а): >>> >>> >>> Liming, >>> >>> I think a good candidate to demonstrate this >>> feature are the checks made in MdePkg/Include/Base.h. >>> The current implementation forces a divide by 0 >>> in the C pre-processor to break the build. >>> STATIC_ASSERT() would be a better way to do this. >>> I would also remove unused externs from the builds. >>> >>> /** >>> Verifies the storage size of a given data type. >>> >>> This macro generates a divide by zero error or a zero size array declaration in >>> the preprocessor if the size is incorrect. These are declared as "extern" so >>> the space for these arrays will not be in the modules. >>> >>> @param TYPE The date type to determine the size of. >>> @param Size The expected size for the TYPE. >>> >>> **/ >>> #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] >> >>> >>> // >>> // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with >>> // Section 2.3.1 of the UEFI 2.3 Specification. >>> // >>> VERIFY_SIZE_OF (BOOLEAN, 1); >>> VERIFY_SIZE_OF (INT8, 1); >>> VERIFY_SIZE_OF (UINT8, 1); >>> VERIFY_SIZE_OF (INT16, 2); >>> VERIFY_SIZE_OF (UINT16, 2); >>> VERIFY_SIZE_OF (INT32, 4); >>> VERIFY_SIZE_OF (UINT32, 4); >>> VERIFY_SIZE_OF (INT64, 8); >>> VERIFY_SIZE_OF (UINT64, 8); >>> VERIFY_SIZE_OF (CHAR8, 1); >>> VERIFY_SIZE_OF (CHAR16, 2); >>> >>> // >>> // The following three enum types are used to verify that the compiler >>> // configuration for enum types is compliant with Section 2.3.1 of the >>> // UEFI 2.3 Specification. These enum types and enum values are not >>> // intended to be used. A prefix of '__' is used avoid conflicts with >>> // other types. >>> // >>> typedef enum { >>> __VerifyUint8EnumValue = 0xff >>> } __VERIFY_UINT8_ENUM_SIZE; >>> >>> typedef enum { >>> __VerifyUint16EnumValue = 0xffff >>> } __VERIFY_UINT16_ENUM_SIZE; >>> >>> typedef enum { >>> __VerifyUint32EnumValue = 0xffffffff >>> } __VERIFY_UINT32_ENUM_SIZE; >>> >>> VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); >>> VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); >>> VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); >>> >>> A couple examples. Do all the compilers support the message parameter too? >>> >>> STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") >> >>> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") >> >>> >>> Thanks, >>> >>> Mike >>> >>>> -----Original Message----- >>>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> [mailto:devel@edk2.groups.io] >>>> On Behalf Of Liming Gao >>>> Sent: Wednesday, August 14, 2019 6:50 AM >>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; vit9696@protonmail.com<mailto:vit9696@protonmail.com> >>>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>>> STATIC_ASSERT macro >>>> >>>> Can you add the sample usage of new macro STATIC_ASSERT? >>>> >>>> Or, give the link of static_assert or _Static_assert. >>>> >>>> If so, the developer knows how to use them in source >>>> code. >>>> >>>> Thanks >>>> Liming >>>>> -----Original Message----- >>>>> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>>> [mailto:devel@edk2.groups.io] On Behalf Of >>>>> vit9696 via Groups.Io >>>>> Sent: Tuesday, August 13, 2019 4:17 PM >>>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> >>>>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>>> STATIC_ASSERT macro >>>>> >>>>> >>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>>>> >>>>> Provide a macro for compile time assertions. >>>>> Equivalent to C11 static_assert macro from assert.h. >>>>> >>>>> Signed-off-by: Vitaly Cheptsov >>>> <vit9696@protonmail.com<mailto:vit9696@protonmail.com>> >>>>> --- >>>>> MdePkg/Include/Base.h | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> diff --git a/MdePkg/Include/Base.h >>>> b/MdePkg/Include/Base.h index >>>>> ce20b5f01dce..f85f7028a262 100644 >>>>> --- a/MdePkg/Include/Base.h >>>>> +++ b/MdePkg/Include/Base.h >>>>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >>>> #define >>>>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >>>> #endif >>>>> >>>>> +/// >>>>> +/// Portable definition for compile time assertions. >>>>> +/// Equivalent to C11 static_assert macro from >>>> assert.h. >>>>> +/// Takes condtion and error message as its >>>> arguments. >>>>> +/// >>>>> +#ifdef _MSC_EXTENSIONS >>>>> + #define STATIC_ASSERT static_assert #else >>>>> + #define STATIC_ASSERT _Static_assert #endif >>>>> + >>>>> /** >>>>> Macro that returns a pointer to the data structure >>>> that contains a specified field of >>>>> that data structure. This is a lightweight method >>>> to hide >>>>> information by placing a >>>>> -- >>>>> 2.20.1 (Apple Git-117) >>>>> >>>>> >>>>> -=-=-=-=-=-= >>>>> Groups.io Links: You receive all messages sent to this >>>> group. >>>>> >>>>> View/Reply Online (#45503): >>>>> https://edk2.groups.io/g/devel/message/45503 >>>>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>>>> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>>> [liming.gao@intel.com] -=-=-=-=-=-= >>>> >>>> >>>> >>> >> >> >> >> >> >> > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 17:23 ` Vitaly Cheptsov @ 2019-08-16 19:38 ` Laszlo Ersek 2019-08-16 20:19 ` Laszlo Ersek ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Laszlo Ersek @ 2019-08-16 19:38 UTC (permalink / raw) To: vit9696@protonmail.com Cc: devel@edk2.groups.io, leif.lindholm@linaro.org, afish@apple.com On 08/16/19 19:23, vit9696@protonmail.com wrote: > Laszlo, > > I have already mentioned that the documentation is sufficient as > _Static_assert is C standard Yes, in a release of the ISO C standard that edk2 does not target. In addition, edk2 already has several restrictions in place against standards-conformant code. (Such as bit-shifting of UINT64 values, initialization of structures with automatic storage duration, structure assignment, maybe more.) > so I do not plan to make a V3 for this patch. I find that regrettable. > The patch is merge ready. Such statements are usually made when people that comment on a patch arrive at a consensus. The patch may be merge-ready from your perspective and from Mike's. It is not merge-ready from my perspective. I hope I'm allowed to comment (constructively) on patches that aren't strictly aimed at the subsystems I co-maintain. > As for usage examples I have an opposing opinion to yours and believe > it is based on very good reasons. Not using STATIC_ASSERT in the > current release will make the feature optionally available and let > people test it in their setups. Not using STATIC_ASSERT in the current stable release makes the STATIC_ASSERT macro definition *dead code* in edk2 proper. I understand that edk2 is a "kit", and quite explicitly caters to out-of-tree platforms. That's not a positive trait of edk2 however; it's a negative one, in my judgement. Whatever we add to the core of edk2, we should exercise as diligently as we can *inside* of edk2. > In case they notice it does not work for them they will have 3 months > grace period to report it to us and consider making a change. That is what the feature freezes are for. The feature is reviewed before the soft feature freeze, merged (at the latest) during the soft feature freeze, and bugs can still be fixed during the hard feature freeze. The community is expected to test diligently during the hard feature freeze. Perhaps we should extend the hard feature freeze. My problem is not that the change is not "in your face". I'm all for avoiding regressions. My problem is that the code is dead and untestable without platform changes, even though it could be put to great use in core code at once. If you think that's too risky, this close to the stable tag, then one solution is to resubmit at the beginning of the next development cycle (again with additional patches that utilize the STATIC_ASSERT macro at once). Developers will then have close to three months to report and fix issues. Another solution would be to conditionally keep VERIFY_SIZE_OF, vs. using STATIC_ASSERT, for expressing the build-time invariants. The default would be STATIC_ASSERT. Should it break, people could immediately switch back to VERIFY_SIZE_OF, without disruption to their workflows. We've done similar things in OvmfPkg in the past. For example: - USE_LEGACY_ISA_STACK (commit a06810229618 / commit 562688707145), - USE_OLD_BDS (commit 79c098b6d25d / commit dd43486577b3), - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit cef83a3050e5). > This will also give them 3 months grace period of VERIFY_SIZE_OF macro > removal in favour of STATIC_ASSERT. Making the change now will let > people do seamless transition to the new feature and will avoid > obstacles you are currently trying to create. Please stop making claims in bad faith. I'm not trying to "create obstacles". I'm a fan of STATIC_ASSERT. I'm not a fan of dead code. > Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be > separate patchsets with potentially separate BZs. > > Thanks for understanding, Why are you presenting this as a done deal? The v2 patch was submitted three days ago, IIUC. Also, I wish we could have this discussion without condescension. Thanks, Laszlo ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 19:38 ` Laszlo Ersek @ 2019-08-16 20:19 ` Laszlo Ersek 2019-08-16 21:04 ` Michael D Kinney 2019-08-16 21:32 ` Vitaly Cheptsov 2 siblings, 0 replies; 26+ messages in thread From: Laszlo Ersek @ 2019-08-16 20:19 UTC (permalink / raw) To: vit9696@protonmail.com Cc: devel@edk2.groups.io, leif.lindholm@linaro.org, afish@apple.com On 08/16/19 21:38, Laszlo Ersek wrote: > I understand that edk2 is a "kit", and quite explicitly caters to > out-of-tree platforms. That's not a positive trait of edk2 however; > it's a negative one, in my judgement. To clarify... I didn't mean that edk2 should willfully ignore dependent platforms. Harmony between universal edk2 code and dependent platforms is important. I meant that more platform code should live inside the edk2 project. Again, this is a personal opinion. Laszlo ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 19:38 ` Laszlo Ersek 2019-08-16 20:19 ` Laszlo Ersek @ 2019-08-16 21:04 ` Michael D Kinney 2019-08-16 21:40 ` Vitaly Cheptsov 2019-08-16 21:32 ` Vitaly Cheptsov 2 siblings, 1 reply; 26+ messages in thread From: Michael D Kinney @ 2019-08-16 21:04 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, vit9696@protonmail.com, Kinney, Michael D Cc: leif.lindholm@linaro.org, afish@apple.com Laszlo, I agree that better comments/documentation of STATIC_ASSERT() for EDK II usages is required. For example, EDK II defines the ASSERT() macro which is based on the standard C function assert(), but we still document it fully for EDK II usage. /** Macro that calls DebugAssert() if an expression evaluates to FALSE. If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean expression specified by Expression. If Expression evaluates to FALSE, then DebugAssert() is called passing in the source filename, source line number, and Expression. @param Expression Boolean expression. **/ #if !defined(MDEPKG_NDEBUG) #define ASSERT(Expression) \ do { \ if (DebugAssertEnabled ()) { \ if (!(Expression)) { \ _ASSERT (Expression); \ ANALYZER_UNREACHABLE (); \ } \ } \ } while (FALSE) #else #define ASSERT(Expression) #endif I would like to see the macro documentation for STATIC_ASSERT() with the Doxygen style description of the parameters. The fact I asked if STATIC_ASSERT() supported the 2nd message parameter should have been a trigger for me to ask for the more complete macro comment block. The fact that this macro can be directly mapped to built in compiler name makes the implementation simple, but other implementations are possible for compilers that do not support this feature directly. This is why the complete description of the EDK II version is required. I would like to see a V3 with the complete description. In general, I agree it is better if there is code that uses a new feature in the code base, so the feature can be tested. A second option we can consider going forward is for unit test code to be submitted with a new feature, so even if there are no consumers from the EDK II repos, the feature can still be tested as the EDK II repos are maintained. A third option is for community members to provide Tested-by responses to the feature along with statements in the Bugzilla that clearly documents how the the feature was tested. Best regards, Mike > -----Original Message----- > From: devel@edk2.groups.io > [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > Sent: Friday, August 16, 2019 12:39 PM > To: vit9696@protonmail.com > Cc: devel@edk2.groups.io; leif.lindholm@linaro.org; > afish@apple.com > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add > STATIC_ASSERT macro > > On 08/16/19 19:23, vit9696@protonmail.com wrote: > > Laszlo, > > > > I have already mentioned that the documentation is > sufficient as > > _Static_assert is C standard > > Yes, in a release of the ISO C standard that edk2 does > not target. > > In addition, edk2 already has several restrictions in > place against standards-conformant code. (Such as bit- > shifting of UINT64 values, initialization of structures > with automatic storage duration, structure assignment, > maybe more.) > > > so I do not plan to make a V3 for this patch. > > I find that regrettable. > > > The patch is merge ready. > > Such statements are usually made when people that > comment on a patch arrive at a consensus. The patch may > be merge-ready from your perspective and from Mike's. > It is not merge-ready from my perspective. > I hope I'm allowed to comment (constructively) on > patches that aren't strictly aimed at the subsystems I > co-maintain. > > > As for usage examples I have an opposing opinion to > yours and believe > > it is based on very good reasons. Not using > STATIC_ASSERT in the > > current release will make the feature optionally > available and let > > people test it in their setups. > > Not using STATIC_ASSERT in the current stable release > makes the STATIC_ASSERT macro definition *dead code* in > edk2 proper. I understand that edk2 is a "kit", and > quite explicitly caters to out-of-tree platforms. > That's not a positive trait of edk2 however; it's a > negative one, in my judgement. Whatever we add to the > core of edk2, we should exercise as diligently as we > can *inside* of edk2. > > > In case they notice it does not work for them they > will have 3 months > > grace period to report it to us and consider making a > change. > > That is what the feature freezes are for. The feature > is reviewed before the soft feature freeze, merged (at > the latest) during the soft feature freeze, and bugs > can still be fixed during the hard feature freeze. The > community is expected to test diligently during the > hard feature freeze. > Perhaps we should extend the hard feature freeze. > > My problem is not that the change is not "in your > face". I'm all for avoiding regressions. My problem is > that the code is dead and untestable without platform > changes, even though it could be put to great use in > core code at once. If you think that's too risky, this > close to the stable tag, then one solution is to > resubmit at the beginning of the next development cycle > (again with additional patches that utilize the > STATIC_ASSERT macro at once). Developers will then have > close to three months to report and fix issues. > > Another solution would be to conditionally keep > VERIFY_SIZE_OF, vs. > using STATIC_ASSERT, for expressing the build-time > invariants. The default would be STATIC_ASSERT. Should > it break, people could immediately switch back to > VERIFY_SIZE_OF, without disruption to their workflows. > > We've done similar things in OvmfPkg in the past. For > example: > - USE_LEGACY_ISA_STACK (commit a06810229618 / commit > 562688707145), > - USE_OLD_BDS (commit 79c098b6d25d / commit > dd43486577b3), > - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit > cef83a3050e5). > > > This will also give them 3 months grace period of > VERIFY_SIZE_OF macro > > removal in favour of STATIC_ASSERT. Making the change > now will let > > people do seamless transition to the new feature and > will avoid > > obstacles you are currently trying to create. > > Please stop making claims in bad faith. I'm not trying > to "create obstacles". I'm a fan of STATIC_ASSERT. I'm > not a fan of dead code. > > > Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal > must both be > > separate patchsets with potentially separate BZs. > > > > Thanks for understanding, > > Why are you presenting this as a done deal? The v2 > patch was submitted three days ago, IIUC. > > Also, I wish we could have this discussion without > condescension. > > Thanks, > Laszlo > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 21:04 ` Michael D Kinney @ 2019-08-16 21:40 ` Vitaly Cheptsov 2019-08-16 22:23 ` rebecca 2019-08-16 22:58 ` Andrew Fish 0 siblings, 2 replies; 26+ messages in thread From: Vitaly Cheptsov @ 2019-08-16 21:40 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, lersek@redhat.com Cc: leif.lindholm@linaro.org, afish@apple.com [-- Attachment #1: Type: text/plain, Size: 7345 bytes --] Mike, I missed your message while writing mine, but I am afraid I disagree with the functional macro usage for this feature. I explicitly quoted C standard static_assert definition in one of my previous messages, and I want EDK II to be as close to standard C as possible. This will avoid a lot of confusion for newcomers and will let us later adopt a more flexible single and double argument interface when it gets standardised. For these reasons altogether, I am not positive the macro could get a doxygen documentation as it is not designed to have any arguments in the first place. Best wishes, Vitaly On сб, авг. 17, 2019 at 00:04, Kinney, Michael D <michael.d.kinney@intel.com> wrote: > Laszlo, > > I agree that better comments/documentation of STATIC_ASSERT() > for EDK II usages is required. For example, EDK II defines > the ASSERT() macro which is based on the standard C function > assert(), but we still document it fully for EDK II usage. > > /** > Macro that calls DebugAssert() if an expression evaluates to FALSE. > > If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED > bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean > expression specified by Expression. If Expression evaluates to FALSE, then > DebugAssert() is called passing in the source filename, source line number, > and Expression. > > @param Expression Boolean expression. > > **/ > #if !defined(MDEPKG_NDEBUG) > #define ASSERT(Expression) \ > do { \ > if (DebugAssertEnabled ()) { \ > if (!(Expression)) { \ > _ASSERT (Expression); \ > ANALYZER_UNREACHABLE (); \ > } \ > } \ > } while (FALSE) > #else > #define ASSERT(Expression) > #endif > > I would like to see the macro documentation for > STATIC_ASSERT() with the Doxygen style description of the > parameters. The fact I asked if STATIC_ASSERT() supported > the 2nd message parameter should have been a trigger > for me to ask for the more complete macro comment block. > The fact that this macro can be directly mapped to > built in compiler name makes the implementation simple, > but other implementations are possible for compilers > that do not support this feature directly. This is why > the complete description of the EDK II version is required. > > I would like to see a V3 with the complete description. > > In general, I agree it is better if there is code that > uses a new feature in the code base, so the feature can > be tested. A second option we can consider going forward > is for unit test code to be submitted with a new feature, > so even if there are no consumers from the EDK II repos, > the feature can still be tested as the EDK II repos are > maintained. A third option is for community members to > provide Tested-by responses to the feature along with > statements in the Bugzilla that clearly documents how the > the feature was tested. > > Best regards, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io >> [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek >> Sent: Friday, August 16, 2019 12:39 PM >> To: vit9696@protonmail.com >> Cc: devel@edk2.groups.io; leif.lindholm@linaro.org; >> afish@apple.com >> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >> >> On 08/16/19 19:23, vit9696@protonmail.com wrote: >> > Laszlo, >> > >> > I have already mentioned that the documentation is >> sufficient as >> > _Static_assert is C standard >> >> Yes, in a release of the ISO C standard that edk2 does >> not target. >> >> In addition, edk2 already has several restrictions in >> place against standards-conformant code. (Such as bit- >> shifting of UINT64 values, initialization of structures >> with automatic storage duration, structure assignment, >> maybe more.) >> >> > so I do not plan to make a V3 for this patch. >> >> I find that regrettable. >> >> > The patch is merge ready. >> >> Such statements are usually made when people that >> comment on a patch arrive at a consensus. The patch may >> be merge-ready from your perspective and from Mike's. >> It is not merge-ready from my perspective. >> I hope I'm allowed to comment (constructively) on >> patches that aren't strictly aimed at the subsystems I >> co-maintain. >> >> > As for usage examples I have an opposing opinion to >> yours and believe >> > it is based on very good reasons. Not using >> STATIC_ASSERT in the >> > current release will make the feature optionally >> available and let >> > people test it in their setups. >> >> Not using STATIC_ASSERT in the current stable release >> makes the STATIC_ASSERT macro definition *dead code* in >> edk2 proper. I understand that edk2 is a "kit", and >> quite explicitly caters to out-of-tree platforms. >> That's not a positive trait of edk2 however; it's a >> negative one, in my judgement. Whatever we add to the >> core of edk2, we should exercise as diligently as we >> can *inside* of edk2. >> >> > In case they notice it does not work for them they >> will have 3 months >> > grace period to report it to us and consider making a >> change. >> >> That is what the feature freezes are for. The feature >> is reviewed before the soft feature freeze, merged (at >> the latest) during the soft feature freeze, and bugs >> can still be fixed during the hard feature freeze. The >> community is expected to test diligently during the >> hard feature freeze. >> Perhaps we should extend the hard feature freeze. >> >> My problem is not that the change is not "in your >> face". I'm all for avoiding regressions. My problem is >> that the code is dead and untestable without platform >> changes, even though it could be put to great use in >> core code at once. If you think that's too risky, this >> close to the stable tag, then one solution is to >> resubmit at the beginning of the next development cycle >> (again with additional patches that utilize the >> STATIC_ASSERT macro at once). Developers will then have >> close to three months to report and fix issues. >> >> Another solution would be to conditionally keep >> VERIFY_SIZE_OF, vs. >> using STATIC_ASSERT, for expressing the build-time >> invariants. The default would be STATIC_ASSERT. Should >> it break, people could immediately switch back to >> VERIFY_SIZE_OF, without disruption to their workflows. >> >> We've done similar things in OvmfPkg in the past. For >> example: >> - USE_LEGACY_ISA_STACK (commit a06810229618 / commit >> 562688707145), >> - USE_OLD_BDS (commit 79c098b6d25d / commit >> dd43486577b3), >> - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit >> cef83a3050e5). >> >> > This will also give them 3 months grace period of >> VERIFY_SIZE_OF macro >> > removal in favour of STATIC_ASSERT. Making the change >> now will let >> > people do seamless transition to the new feature and >> will avoid >> > obstacles you are currently trying to create. >> >> Please stop making claims in bad faith. I'm not trying >> to "create obstacles". I'm a fan of STATIC_ASSERT. I'm >> not a fan of dead code. >> >> > Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal >> must both be >> > separate patchsets with potentially separate BZs. >> > >> > Thanks for understanding, >> >> Why are you presenting this as a done deal? The v2 >> patch was submitted three days ago, IIUC. >> >> Also, I wish we could have this discussion without >> condescension. >> >> Thanks, >> Laszlo >> >> [-- Attachment #2: Type: text/html, Size: 8618 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 21:40 ` Vitaly Cheptsov @ 2019-08-16 22:23 ` rebecca 2019-08-16 22:58 ` Andrew Fish 1 sibling, 0 replies; 26+ messages in thread From: rebecca @ 2019-08-16 22:23 UTC (permalink / raw) To: devel, vit9696, Kinney, Michael D, lersek@redhat.com Cc: leif.lindholm@linaro.org, afish@apple.com On 2019-08-16 15:40, Vitaly Cheptsov via Groups.Io wrote: > I missed your message while writing mine, but I am afraid I disagree > with the functional macro usage for this feature. > > I explicitly quoted C standard static_assert definition in one of my > previous messages, and I want EDK II to be as close to standard C as > possible. > Choosing a random message in this thread to comment. We could also migrate the existing uses of __STATIC_ASSERT in BaseTools to the new STATIC_ASSERT: BaseTools/Source/C/Common/PcdValueCommon.h:22:#define __STATIC_ASSERT static_assert BaseTools/Source/C/Common/PcdValueCommon.h:24:#define __STATIC_ASSERT _Static_assert Binary file BaseTools/Source/Python/Workspace/DscBuildData.pyc matches BaseTools/Source/Python/Workspace/DscBuildData.py:2037: CApp = CApp + '__STATIC_ASSERT(sizeof(%s_%s_INIT_Value) < %d * sizeof(%s), "Pcd %s.%s Value in Dec exceed the array capability %s"); // From %s Line %s \n ' % (Pcd.TokenSpaceGuidCName, Pcd.TokenCName,pcdarraysize,Pcd.BaseDatumType,Pcd.TokenSpaceGuidCName, Pcd.TokenCName,Pcd.DatumType,Pcd.DefaultValueFromDecInfo[0],Pcd.DefaultValueFromDecInfo[1]) BaseTools/Source/Python/Workspace/DscBuildData.py:2042: CApp = CApp + '__STATIC_ASSERT(%d < %d * sizeof(%s), "Pcd %s.%s Value in Dec exceed the array capability %s"); // From %s Line %s \n' % (ValueSize,pcdarraysize,Pcd.BaseDatumType,Pcd.TokenSpaceGuidCName, Pcd.TokenCName,Pcd.DatumType,Pcd.DefaultValueFromDecInfo[0],Pcd.DefaultValueFromDecInfo[1]) .... -- Rebecca Cran ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 21:40 ` Vitaly Cheptsov 2019-08-16 22:23 ` rebecca @ 2019-08-16 22:58 ` Andrew Fish 2019-08-16 23:34 ` Vitaly Cheptsov 2019-08-17 0:01 ` rebecca 1 sibling, 2 replies; 26+ messages in thread From: Andrew Fish @ 2019-08-16 22:58 UTC (permalink / raw) To: devel, vit9696; +Cc: Mike Kinney, lersek@redhat.com, leif.lindholm@linaro.org [-- Attachment #1: Type: text/plain, Size: 9158 bytes --] > On Aug 16, 2019, at 2:40 PM, Vitaly Cheptsov via Groups.Io <vit9696=protonmail.com@groups.io> wrote: > > Mike, > > I missed your message while writing mine, but I am afraid I disagree with the functional macro usage for this feature. > > I explicitly quoted C standard static_assert definition in one of my previous messages, and I want EDK II to be as close to standard C as possible. > > This will avoid a lot of confusion for newcomers and will let us later adopt a more flexible single and double argument interface when it gets standardised. > > For these reasons altogether, I am not positive the macro could get a doxygen documentation as it is not designed to have any arguments in the first place. > Vitaly, I don't understand your logic? It is always possible to write a comment in C code? In terms of the C standard and the EFI type system we kind of have a long history of how the code ended up like it is. The (U)EFI spec defined its own type system (and ABI) as a way of specifying interoperability so the code got built on top of that. 20 years ago we shied away from having and EFI code base produce definitions of standard C things as we wanted to make it easier to import chunks of code in OS loaders that needed to get ported to EFI from lots of different vendors. Also one of the early compilers that we standardized on was VC2003 and that does not even fully support C99. For some reason it seems VC2008 was also a popular target for some time. I don't think VC++ got around to C99 until 2013/2015. So that is kind how the edk2 ended up with its own type system. I'm all for modernization of the C code as long we are thoughtful about compatibility. For example I still see that VS2008 is a supported BaseTools/Conf/tools_def.template. Thanks, Andrew Fish > Best wishes, > Vitaly > > On сб, авг. 17, 2019 at 00:04, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> wrote: >> >> Laszlo, >> >> I agree that better comments/documentation of STATIC_ASSERT() >> for EDK II usages is required. For example, EDK II defines >> the ASSERT() macro which is based on the standard C function >> assert(), but we still document it fully for EDK II usage. >> >> /** >> Macro that calls DebugAssert() if an expression evaluates to FALSE. >> >> If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED >> bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean >> expression specified by Expression. If Expression evaluates to FALSE, then >> DebugAssert() is called passing in the source filename, source line number, >> and Expression. >> >> @param Expression Boolean expression. >> >> **/ >> #if !defined(MDEPKG_NDEBUG) >> #define ASSERT(Expression) \ >> do { \ >> if (DebugAssertEnabled ()) { \ >> if (!(Expression)) { \ >> _ASSERT (Expression); \ >> ANALYZER_UNREACHABLE (); \ >> } \ >> } \ >> } while (FALSE) >> #else >> #define ASSERT(Expression) >> #endif >> >> I would like to see the macro documentation for >> STATIC_ASSERT() with the Doxygen style description of the >> parameters. The fact I asked if STATIC_ASSERT() supported >> the 2nd message parameter should have been a trigger >> for me to ask for the more complete macro comment block. >> The fact that this macro can be directly mapped to >> built in compiler name makes the implementation simple, >> but other implementations are possible for compilers >> that do not support this feature directly. This is why >> the complete description of the EDK II version is required. >> >> I would like to see a V3 with the complete description. >> >> In general, I agree it is better if there is code that >> uses a new feature in the code base, so the feature can >> be tested. A second option we can consider going forward >> is for unit test code to be submitted with a new feature, >> so even if there are no consumers from the EDK II repos, >> the feature can still be tested as the EDK II repos are >> maintained. A third option is for community members to >> provide Tested-by responses to the feature along with >> statements in the Bugzilla that clearly documents how the >> the feature was tested. >> >> Best regards, >> >> Mike >> >> > -----Original Message----- >> > From: devel@edk2.groups.io >> > [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek >> > Sent: Friday, August 16, 2019 12:39 PM >> > To: vit9696@protonmail.com >> > Cc: devel@edk2.groups.io; leif.lindholm@linaro.org; >> > afish@apple.com >> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> > STATIC_ASSERT macro >> > >> > On 08/16/19 19:23, vit9696@protonmail.com wrote: >> > > Laszlo, >> > > >> > > I have already mentioned that the documentation is >> > sufficient as >> > > _Static_assert is C standard >> > >> > Yes, in a release of the ISO C standard that edk2 does >> > not target. >> > >> > In addition, edk2 already has several restrictions in >> > place against standards-conformant code. (Such as bit- >> > shifting of UINT64 values, initialization of structures >> > with automatic storage duration, structure assignment, >> > maybe more.) >> > >> > > so I do not plan to make a V3 for this patch. >> > >> > I find that regrettable. >> > >> > > The patch is merge ready. >> > >> > Such statements are usually made when people that >> > comment on a patch arrive at a consensus. The patch may >> > be merge-ready from your perspective and from Mike's. >> > It is not merge-ready from my perspective. >> > I hope I'm allowed to comment (constructively) on >> > patches that aren't strictly aimed at the subsystems I >> > co-maintain. >> > >> > > As for usage examples I have an opposing opinion to >> > yours and believe >> > > it is based on very good reasons. Not using >> > STATIC_ASSERT in the >> > > current release will make the feature optionally >> > available and let >> > > people test it in their setups. >> > >> > Not using STATIC_ASSERT in the current stable release >> > makes the STATIC_ASSERT macro definition *dead code* in >> > edk2 proper. I understand that edk2 is a "kit", and >> > quite explicitly caters to out-of-tree platforms. >> > That's not a positive trait of edk2 however; it's a >> > negative one, in my judgement. Whatever we add to the >> > core of edk2, we should exercise as diligently as we >> > can *inside* of edk2. >> > >> > > In case they notice it does not work for them they >> > will have 3 months >> > > grace period to report it to us and consider making a >> > change. >> > >> > That is what the feature freezes are for. The feature >> > is reviewed before the soft feature freeze, merged (at >> > the latest) during the soft feature freeze, and bugs >> > can still be fixed during the hard feature freeze. The >> > community is expected to test diligently during the >> > hard feature freeze. >> > Perhaps we should extend the hard feature freeze. >> > >> > My problem is not that the change is not "in your >> > face". I'm all for avoiding regressions. My problem is >> > that the code is dead and untestable without platform >> > changes, even though it could be put to great use in >> > core code at once. If you think that's too risky, this >> > close to the stable tag, then one solution is to >> > resubmit at the beginning of the next development cycle >> > (again with additional patches that utilize the >> > STATIC_ASSERT macro at once). Developers will then have >> > close to three months to report and fix issues. >> > >> > Another solution would be to conditionally keep >> > VERIFY_SIZE_OF, vs. >> > using STATIC_ASSERT, for expressing the build-time >> > invariants. The default would be STATIC_ASSERT. Should >> > it break, people could immediately switch back to >> > VERIFY_SIZE_OF, without disruption to their workflows. >> > >> > We've done similar things in OvmfPkg in the past. For >> > example: >> > - USE_LEGACY_ISA_STACK (commit a06810229618 / commit >> > 562688707145), >> > - USE_OLD_BDS (commit 79c098b6d25d / commit >> > dd43486577b3), >> > - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit >> > cef83a3050e5). >> > >> > > This will also give them 3 months grace period of >> > VERIFY_SIZE_OF macro >> > > removal in favour of STATIC_ASSERT. Making the change >> > now will let >> > > people do seamless transition to the new feature and >> > will avoid >> > > obstacles you are currently trying to create. >> > >> > Please stop making claims in bad faith. I'm not trying >> > to "create obstacles". I'm a fan of STATIC_ASSERT. I'm >> > not a fan of dead code. >> > >> > > Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal >> > must both be >> > > separate patchsets with potentially separate BZs. >> > > >> > > Thanks for understanding, >> > >> > Why are you presenting this as a done deal? The v2 >> > patch was submitted three days ago, IIUC. >> > >> > Also, I wish we could have this discussion without >> > condescension. >> > >> > Thanks, >> > Laszlo >> > >> > >> > > > [-- Attachment #2: Type: text/html, Size: 18959 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 22:58 ` Andrew Fish @ 2019-08-16 23:34 ` Vitaly Cheptsov 2019-08-17 0:01 ` rebecca 1 sibling, 0 replies; 26+ messages in thread From: Vitaly Cheptsov @ 2019-08-16 23:34 UTC (permalink / raw) To: Andrew Fish Cc: devel, Mike Kinney, lersek@redhat.com, leif.lindholm@linaro.org [-- Attachment #1.1: Type: text/plain, Size: 10487 bytes --] Andrew, Mike, Actually thanks for making me recheck it. I have just installed doxygen, and can confirm that it can generate parameters for non-functional macros. This was my main reason for going with /// comment style, which is used for all other plain macros. I have just submitted V3 version with this fixed. With C language modernisation I fully agree that it needs to be done reasonably, and that is why I also try to be very realistic on what people actually use. For instance, one of the issues that caused several problems is the use of 1-sized arrays instead of flexible array members. A couple years ago that was still there just because there still existed some compilers that did not support [] syntax. Best wishes, Vitaly > 17 авг. 2019 г., в 1:58, Andrew Fish <afish@apple.com> написал(а): > > > >> On Aug 16, 2019, at 2:40 PM, Vitaly Cheptsov via Groups.Io <vit9696=protonmail.com@groups.io <mailto:vit9696=protonmail.com@groups.io>> wrote: >> >> Mike, >> >> I missed your message while writing mine, but I am afraid I disagree with the functional macro usage for this feature. >> >> I explicitly quoted C standard static_assert definition in one of my previous messages, and I want EDK II to be as close to standard C as possible. >> >> This will avoid a lot of confusion for newcomers and will let us later adopt a more flexible single and double argument interface when it gets standardised. >> >> For these reasons altogether, I am not positive the macro could get a doxygen documentation as it is not designed to have any arguments in the first place. >> > > Vitaly, > > I don't understand your logic? It is always possible to write a comment in C code? > > In terms of the C standard and the EFI type system we kind of have a long history of how the code ended up like it is. The (U)EFI spec defined its own type system (and ABI) as a way of specifying interoperability so the code got built on top of that. 20 years ago we shied away from having and EFI code base produce definitions of standard C things as we wanted to make it easier to import chunks of code in OS loaders that needed to get ported to EFI from lots of different vendors. Also one of the early compilers that we standardized on was VC2003 and that does not even fully support C99. For some reason it seems VC2008 was also a popular target for some time. I don't think VC++ got around to C99 until 2013/2015. So that is kind how the edk2 ended up with its own type system. > > I'm all for modernization of the C code as long we are thoughtful about compatibility. For example I still see that VS2008 is a supported BaseTools/Conf/tools_def.template. > > Thanks, > > Andrew Fish > > >> Best wishes, >> Vitaly >> >> On сб, авг. 17, 2019 at 00:04, Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>> wrote: >>> >>> Laszlo, >>> >>> I agree that better comments/documentation of STATIC_ASSERT() >>> for EDK II usages is required. For example, EDK II defines >>> the ASSERT() macro which is based on the standard C function >>> assert(), but we still document it fully for EDK II usage. >>> >>> /** >>> Macro that calls DebugAssert() if an expression evaluates to FALSE. >>> >>> If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED >>> bit of PcdDebugProperyMask is set, then this macro evaluates the Boolean >>> expression specified by Expression. If Expression evaluates to FALSE, then >>> DebugAssert() is called passing in the source filename, source line number, >>> and Expression. >>> >>> @param Expression Boolean expression. >>> >>> **/ >>> #if !defined(MDEPKG_NDEBUG) >>> #define ASSERT(Expression) \ >>> do { \ >>> if (DebugAssertEnabled ()) { \ >>> if (!(Expression)) { \ >>> _ASSERT (Expression); \ >>> ANALYZER_UNREACHABLE (); \ >>> } \ >>> } \ >>> } while (FALSE) >>> #else >>> #define ASSERT(Expression) >>> #endif >>> >>> I would like to see the macro documentation for >>> STATIC_ASSERT() with the Doxygen style description of the >>> parameters. The fact I asked if STATIC_ASSERT() supported >>> the 2nd message parameter should have been a trigger >>> for me to ask for the more complete macro comment block. >>> The fact that this macro can be directly mapped to >>> built in compiler name makes the implementation simple, >>> but other implementations are possible for compilers >>> that do not support this feature directly. This is why >>> the complete description of the EDK II version is required. >>> >>> I would like to see a V3 with the complete description. >>> >>> In general, I agree it is better if there is code that >>> uses a new feature in the code base, so the feature can >>> be tested. A second option we can consider going forward >>> is for unit test code to be submitted with a new feature, >>> so even if there are no consumers from the EDK II repos, >>> the feature can still be tested as the EDK II repos are >>> maintained. A third option is for community members to >>> provide Tested-by responses to the feature along with >>> statements in the Bugzilla that clearly documents how the >>> the feature was tested. >>> >>> Best regards, >>> >>> Mike >>> >>> > -----Original Message----- >>> > From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> >>> > [mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>] On Behalf Of Laszlo Ersek >>> > Sent: Friday, August 16, 2019 12:39 PM >>> > To: vit9696@protonmail.com <mailto:vit9696@protonmail.com> >>> > Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; leif.lindholm@linaro.org <mailto:leif.lindholm@linaro.org>; >>> > afish@apple.com <mailto:afish@apple.com> >>> > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> > STATIC_ASSERT macro >>> > >>> > On 08/16/19 19:23, vit9696@protonmail.com <mailto:vit9696@protonmail.com> wrote: >>> > > Laszlo, >>> > > >>> > > I have already mentioned that the documentation is >>> > sufficient as >>> > > _Static_assert is C standard >>> > >>> > Yes, in a release of the ISO C standard that edk2 does >>> > not target. >>> > >>> > In addition, edk2 already has several restrictions in >>> > place against standards-conformant code. (Such as bit- >>> > shifting of UINT64 values, initialization of structures >>> > with automatic storage duration, structure assignment, >>> > maybe more.) >>> > >>> > > so I do not plan to make a V3 for this patch. >>> > >>> > I find that regrettable. >>> > >>> > > The patch is merge ready. >>> > >>> > Such statements are usually made when people that >>> > comment on a patch arrive at a consensus. The patch may >>> > be merge-ready from your perspective and from Mike's. >>> > It is not merge-ready from my perspective. >>> > I hope I'm allowed to comment (constructively) on >>> > patches that aren't strictly aimed at the subsystems I >>> > co-maintain. >>> > >>> > > As for usage examples I have an opposing opinion to >>> > yours and believe >>> > > it is based on very good reasons. Not using >>> > STATIC_ASSERT in the >>> > > current release will make the feature optionally >>> > available and let >>> > > people test it in their setups. >>> > >>> > Not using STATIC_ASSERT in the current stable release >>> > makes the STATIC_ASSERT macro definition *dead code* in >>> > edk2 proper. I understand that edk2 is a "kit", and >>> > quite explicitly caters to out-of-tree platforms. >>> > That's not a positive trait of edk2 however; it's a >>> > negative one, in my judgement. Whatever we add to the >>> > core of edk2, we should exercise as diligently as we >>> > can *inside* of edk2. >>> > >>> > > In case they notice it does not work for them they >>> > will have 3 months >>> > > grace period to report it to us and consider making a >>> > change. >>> > >>> > That is what the feature freezes are for. The feature >>> > is reviewed before the soft feature freeze, merged (at >>> > the latest) during the soft feature freeze, and bugs >>> > can still be fixed during the hard feature freeze. The >>> > community is expected to test diligently during the >>> > hard feature freeze. >>> > Perhaps we should extend the hard feature freeze. >>> > >>> > My problem is not that the change is not "in your >>> > face". I'm all for avoiding regressions. My problem is >>> > that the code is dead and untestable without platform >>> > changes, even though it could be put to great use in >>> > core code at once. If you think that's too risky, this >>> > close to the stable tag, then one solution is to >>> > resubmit at the beginning of the next development cycle >>> > (again with additional patches that utilize the >>> > STATIC_ASSERT macro at once). Developers will then have >>> > close to three months to report and fix issues. >>> > >>> > Another solution would be to conditionally keep >>> > VERIFY_SIZE_OF, vs. >>> > using STATIC_ASSERT, for expressing the build-time >>> > invariants. The default would be STATIC_ASSERT. Should >>> > it break, people could immediately switch back to >>> > VERIFY_SIZE_OF, without disruption to their workflows. >>> > >>> > We've done similar things in OvmfPkg in the past. For >>> > example: >>> > - USE_LEGACY_ISA_STACK (commit a06810229618 / commit >>> > 562688707145), >>> > - USE_OLD_BDS (commit 79c098b6d25d / commit >>> > dd43486577b3), >>> > - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit >>> > cef83a3050e5). >>> > >>> > > This will also give them 3 months grace period of >>> > VERIFY_SIZE_OF macro >>> > > removal in favour of STATIC_ASSERT. Making the change >>> > now will let >>> > > people do seamless transition to the new feature and >>> > will avoid >>> > > obstacles you are currently trying to create. >>> > >>> > Please stop making claims in bad faith. I'm not trying >>> > to "create obstacles". I'm a fan of STATIC_ASSERT. I'm >>> > not a fan of dead code. >>> > >>> > > Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal >>> > must both be >>> > > separate patchsets with potentially separate BZs. >>> > > >>> > > Thanks for understanding, >>> > >>> > Why are you presenting this as a done deal? The v2 >>> > patch was submitted three days ago, IIUC. >>> > >>> > Also, I wish we could have this discussion without >>> > condescension. >>> > >>> > Thanks, >>> > Laszlo >>> > >>> > >>> >> >> >> [-- Attachment #1.2: Type: text/html, Size: 26054 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 489 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 22:58 ` Andrew Fish 2019-08-16 23:34 ` Vitaly Cheptsov @ 2019-08-17 0:01 ` rebecca 2019-08-17 0:03 ` Andrew Fish 1 sibling, 1 reply; 26+ messages in thread From: rebecca @ 2019-08-17 0:01 UTC (permalink / raw) To: devel, afish, vit9696 Cc: Mike Kinney, lersek@redhat.com, leif.lindholm@linaro.org On 2019-08-16 16:58, Andrew Fish via Groups.Io wrote: > > I'm all for modernization of the C code as long we are thoughtful > about compatibility. For example I still see that VS2008 is a > supported BaseTools/Conf/tools_def.template. It would be interesting to see who's using the older toolchains against current/newer revisions of the edk2. For example at work we build a UEFI driver using VS2012, but it's against the UDK2014.SP1 branch, and being for a legacy product we'll never migrate to a newer UDK release or master tag. -- Rebecca Cran ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-17 0:01 ` rebecca @ 2019-08-17 0:03 ` Andrew Fish 2019-08-17 0:09 ` rebecca [not found] ` <15BB8D3E51450F1C.5853@groups.io> 0 siblings, 2 replies; 26+ messages in thread From: Andrew Fish @ 2019-08-17 0:03 UTC (permalink / raw) To: Rebecca Cran Cc: devel, vit9696, Mike Kinney, lersek@redhat.com, leif.lindholm@linaro.org > On Aug 16, 2019, at 5:01 PM, Rebecca Cran <rebecca@bsdio.com> wrote: > > On 2019-08-16 16:58, Andrew Fish via Groups.Io wrote: >> >> I'm all for modernization of the C code as long we are thoughtful >> about compatibility. For example I still see that VS2008 is a >> supported BaseTools/Conf/tools_def.template. > > > It would be interesting to see who's using the older toolchains against > current/newer revisions of the edk2. > > For example at work we build a UEFI driver using VS2012, but it's > against the UDK2014.SP1 branch, and being for a legacy product we'll > never migrate to a newer UDK release or master tag. > Rebecca, It is also interesting in the context of our CI as we could pick oldest and newest version from every compiler family for the CI build test. Thanks, Andrew Fish > > -- > Rebecca Cran > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-17 0:03 ` Andrew Fish @ 2019-08-17 0:09 ` rebecca [not found] ` <15BB8D3E51450F1C.5853@groups.io> 1 sibling, 0 replies; 26+ messages in thread From: rebecca @ 2019-08-17 0:09 UTC (permalink / raw) To: Andrew Fish Cc: devel, vit9696, Mike Kinney, lersek@redhat.com, leif.lindholm@linaro.org On 2019-08-16 18:03, Andrew Fish wrote: > It is also interesting in the context of our CI as we could pick oldest and newest version from every compiler family for the CI build test. Yes, that's going to be important. Given https://docs.microsoft.com/en-us/cpp/porting/visual-cpp-change-history-2003-2015?view=vs-2019, I suspect support for VS2008 might already have been broken, since it reports that "static_assert" was introduced in Visual Studio 2010 - and we use it in BaseTools/Source/C/Common/PcdValueCommon.h (the _STATIC_ASSERT was added in September 2018): #if defined(_MSC_EXTENSIONS) #define __STATIC_ASSERT static_assert #else #define __STATIC_ASSERT _Static_assert #endif -- Rebecca Cran ^ permalink raw reply [flat|nested] 26+ messages in thread
[parent not found: <15BB8D3E51450F1C.5853@groups.io>]
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro [not found] ` <15BB8D3E51450F1C.5853@groups.io> @ 2019-08-21 21:41 ` rebecca 0 siblings, 0 replies; 26+ messages in thread From: rebecca @ 2019-08-21 21:41 UTC (permalink / raw) To: Andrew Fish Cc: devel, vit9696, Mike Kinney, lersek@redhat.com, leif.lindholm@linaro.org On 2019-08-16 18:09, rebecca@bsdio.com wrote: > Yes, that's going to be important. Given > https://docs.microsoft.com/en-us/cpp/porting/visual-cpp-change-history-2003-2015?view=vs-2019, > I suspect support for VS2008 might already have been broken, since it > reports that "static_assert" was introduced in Visual Studio 2010 - and > we use it in BaseTools/Source/C/Common/PcdValueCommon.h (the > _STATIC_ASSERT was added in September 2018): I just checked, and VS2008 SP1 _can_ build OVMF, though it looks like support has been removed for building _BaseTools_ with anything older than VS2012. From "edksetup.bat -h" ... ForceRebuild Force a full rebuild of BaseTools binaries. VS2012 Set the env for VS2012 build. VS2013 Set the env for VS2013 build. VS2015 Set the env for VS2015 build. VS2017 Set the env for VS2017 build. -- Rebecca Cran ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-16 19:38 ` Laszlo Ersek 2019-08-16 20:19 ` Laszlo Ersek 2019-08-16 21:04 ` Michael D Kinney @ 2019-08-16 21:32 ` Vitaly Cheptsov 2 siblings, 0 replies; 26+ messages in thread From: Vitaly Cheptsov @ 2019-08-16 21:32 UTC (permalink / raw) To: Laszlo Ersek Cc: devel@edk2.groups.io, leif.lindholm@linaro.org, afish@apple.com [-- Attachment #1: Type: text/plain, Size: 6162 bytes --] Laszlo, I am very glad to you for expressing a different opinion as this lets me view the situation from different angles. I understand your concerns, and believe that most of them should actually be addressed in a way you explain. In fact, I plan to submit more patches myself for everyone's benefit. The exact situation with static assertions is that they are not coming too early, but actually too late. We have been using static assertions in UEFI code for quite some time already, and I believe we are not alone. All of us will benefit from legacy code removal once this patch lands upstream. For your claim that this code is not well tested I should mention that the patch is based on one of the open-source projects I maintain, which everyone can track, and which I believe have gotten reasonable attention from different people with different compilers. For dead code I believe that in EDK II we do not have a good definition for that term as normally done in serious industrial projects like aerospace or military that have no dead code requirement in their SDL. Primarily because EDK II is a library for others to rely on, it is not a self contained system where dead code term is usually defined, standardised and verified against. Whether it is liked or not, the fact EDK II gets continual development is only because different companies, academia, and individuals use its code. I feel bad for these people having to fork, and believe that most value in EDK is what it gives to the outside, not the inside. So supporting a new interface a number of projects use and need makes most sense to me. I do not want to make more changes to core code for multiple reasons as you see above. One of them indeed being some necessary discussion for the use inside EDK II. But I do not believe this a good stopper from giving a working interface to others, which unlike EDK II, actually have defined compilers, infrastructure, and requirements. Hopefully I pointed out to enough reasons to leave you with some doubts and permit this patch to land in as an exception from your personal standpoint. Thank you for understanding and being constructive. Cheers, Vitaly On пт, авг. 16, 2019 at 22:38, Laszlo Ersek <lersek@redhat.com> wrote: > On 08/16/19 19:23, vit9696@protonmail.com wrote: >> Laszlo, >> >> I have already mentioned that the documentation is sufficient as >> _Static_assert is C standard > > Yes, in a release of the ISO C standard that edk2 does not target. > > In addition, edk2 already has several restrictions in place against > standards-conformant code. (Such as bit-shifting of UINT64 values, > initialization of structures with automatic storage duration, structure > assignment, maybe more.) > >> so I do not plan to make a V3 for this patch. > > I find that regrettable. > >> The patch is merge ready. > > Such statements are usually made when people that comment on a patch > arrive at a consensus. The patch may be merge-ready from your > perspective and from Mike's. It is not merge-ready from my perspective. > I hope I'm allowed to comment (constructively) on patches that aren't > strictly aimed at the subsystems I co-maintain. > >> As for usage examples I have an opposing opinion to yours and believe >> it is based on very good reasons. Not using STATIC_ASSERT in the >> current release will make the feature optionally available and let >> people test it in their setups. > > Not using STATIC_ASSERT in the current stable release makes the > STATIC_ASSERT macro definition *dead code* in edk2 proper. I understand > that edk2 is a "kit", and quite explicitly caters to out-of-tree > platforms. That's not a positive trait of edk2 however; it's a negative > one, in my judgement. Whatever we add to the core of edk2, we should > exercise as diligently as we can *inside* of edk2. > >> In case they notice it does not work for them they will have 3 months >> grace period to report it to us and consider making a change. > > That is what the feature freezes are for. The feature is reviewed before > the soft feature freeze, merged (at the latest) during the soft feature > freeze, and bugs can still be fixed during the hard feature freeze. The > community is expected to test diligently during the hard feature freeze. > Perhaps we should extend the hard feature freeze. > > My problem is not that the change is not "in your face". I'm all for > avoiding regressions. My problem is that the code is dead and untestable > without platform changes, even though it could be put to great use in > core code at once. If you think that's too risky, this close to the > stable tag, then one solution is to resubmit at the beginning of the > next development cycle (again with additional patches that utilize the > STATIC_ASSERT macro at once). Developers will then have close to three > months to report and fix issues. > > Another solution would be to conditionally keep VERIFY_SIZE_OF, vs. > using STATIC_ASSERT, for expressing the build-time invariants. The > default would be STATIC_ASSERT. Should it break, people could > immediately switch back to VERIFY_SIZE_OF, without disruption to their > workflows. > > We've done similar things in OvmfPkg in the past. For example: > - USE_LEGACY_ISA_STACK (commit a06810229618 / commit 562688707145), > - USE_OLD_BDS (commit 79c098b6d25d / commit dd43486577b3), > - USE_OLD_PCI_HOST (commit 4014885ffdfa / commit cef83a3050e5). > >> This will also give them 3 months grace period of VERIFY_SIZE_OF macro >> removal in favour of STATIC_ASSERT. Making the change now will let >> people do seamless transition to the new feature and will avoid >> obstacles you are currently trying to create. > > Please stop making claims in bad faith. I'm not trying to "create > obstacles". I'm a fan of STATIC_ASSERT. I'm not a fan of dead code. > >> Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must both be >> separate patchsets with potentially separate BZs. >> >> Thanks for understanding, > > Why are you presenting this as a done deal? The v2 patch was submitted > three days ago, IIUC. > > Also, I wish we could have this discussion without condescension. > > Thanks, > Laszlo [-- Attachment #2: Type: text/html, Size: 6987 bytes --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-14 16:22 ` Vitaly Cheptosv 2019-08-15 1:05 ` Yao, Jiewen @ 2019-08-16 16:28 ` Laszlo Ersek 1 sibling, 0 replies; 26+ messages in thread From: Laszlo Ersek @ 2019-08-16 16:28 UTC (permalink / raw) To: vit9696@protonmail.com, michael.d.kinney; +Cc: devel@edk2.groups.io On 08/14/19 18:22, vit9696@protonmail.com wrote: > Michael, Liming, Laszlo, > > Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description. > The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below. > In the bottom of this message I included a quote from C17 for the relevant section (6.7.10). > > GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about. > > As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody. Edk2 targets C95, to my understanding. If features from more recent C language standards happen to work on all toolchains that edk2 supports, then I agree we can put those language features to use -- but we should document them, in the appropriate header file. In my opinion. > > I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set. I disagree with introducing a new macro to a core header file without putting it to use at once, in at least one very commonly built translation unit in edk2 itself. I would suggest to single out a few core uses of ASSERT (e.g. in MdePkg or MdeModulePkg), and to convert them. If you can replace VERIFY_SIZE_OF with STATIC_ASSERT, that could be a perfect first use. Of course I'd suggest that the patches be separate -- first, add the new macro, second, gradually convert VERIFY_SIZE_OF. So this intro work should be done as a small series. I think that can belong to a single BZ. > As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used. Wider ASSERT evaluation and conversion to STATIC_ASSERT should be done later (separate BZs) if we ever have capacity for that. Thanks Laszlo > > Looking forward to hearing your opinions. > > Best regards, > Vitaly > > > 6.7.10 Static assertions > > Syntax > 1 static_assert-declaration: > _Static_assert ( constant-expression , string-literal ) ; > > Constraints > 2 The constant expression shall compare unequal to 0. > > Semantics > 3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message. > Forward references: diagnostics (7.2). > > 7.2 Diagnostics <assert. h> > > 3 The macro > static_assert > expands to _Static_assert. > > >> 14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@intel.com> написал(а): >> >> >> Liming, >> >> I think a good candidate to demonstrate this >> feature are the checks made in MdePkg/Include/Base.h. >> The current implementation forces a divide by 0 >> in the C pre-processor to break the build. >> STATIC_ASSERT() would be a better way to do this. >> I would also remove unused externs from the builds. >> >> /** >> Verifies the storage size of a given data type. >> >> This macro generates a divide by zero error or a zero size array declaration in >> the preprocessor if the size is incorrect. These are declared as "extern" so >> the space for these arrays will not be in the modules. >> >> @param TYPE The date type to determine the size of. >> @param Size The expected size for the TYPE. >> >> **/ >> #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))] >> >> // >> // Verify that ProcessorBind.h produced UEFI Data Types that are compliant with >> // Section 2.3.1 of the UEFI 2.3 Specification. >> // >> VERIFY_SIZE_OF (BOOLEAN, 1); >> VERIFY_SIZE_OF (INT8, 1); >> VERIFY_SIZE_OF (UINT8, 1); >> VERIFY_SIZE_OF (INT16, 2); >> VERIFY_SIZE_OF (UINT16, 2); >> VERIFY_SIZE_OF (INT32, 4); >> VERIFY_SIZE_OF (UINT32, 4); >> VERIFY_SIZE_OF (INT64, 8); >> VERIFY_SIZE_OF (UINT64, 8); >> VERIFY_SIZE_OF (CHAR8, 1); >> VERIFY_SIZE_OF (CHAR16, 2); >> >> // >> // The following three enum types are used to verify that the compiler >> // configuration for enum types is compliant with Section 2.3.1 of the >> // UEFI 2.3 Specification. These enum types and enum values are not >> // intended to be used. A prefix of '__' is used avoid conflicts with >> // other types. >> // >> typedef enum { >> __VerifyUint8EnumValue = 0xff >> } __VERIFY_UINT8_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint16EnumValue = 0xffff >> } __VERIFY_UINT16_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint32EnumValue = 0xffffffff >> } __VERIFY_UINT32_ENUM_SIZE; >> >> VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); >> VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); >> VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); >> >> A couple examples. Do all the compilers support the message parameter too? >> >> STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements") >> >> Thanks, >> >> Mike >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] >>> On Behalf Of Liming Gao >>> Sent: Wednesday, August 14, 2019 6:50 AM >>> To: devel@edk2.groups.io; vit9696@protonmail.com >>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> STATIC_ASSERT macro >>> >>> Can you add the sample usage of new macro STATIC_ASSERT? >>> >>> Or, give the link of static_assert or _Static_assert. >>> >>> If so, the developer knows how to use them in source >>> code. >>> >>> Thanks >>> Liming >>>> -----Original Message----- >>>> From: devel@edk2.groups.io >>> [mailto:devel@edk2.groups.io] On Behalf Of >>>> vit9696 via Groups.Io >>>> Sent: Tuesday, August 13, 2019 4:17 PM >>>> To: devel@edk2.groups.io >>>> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>> STATIC_ASSERT macro >>>> >>>> >>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >>>> >>>> Provide a macro for compile time assertions. >>>> Equivalent to C11 static_assert macro from assert.h. >>>> >>>> Signed-off-by: Vitaly Cheptsov >>> <vit9696@protonmail.com> >>>> --- >>>> MdePkg/Include/Base.h | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/MdePkg/Include/Base.h >>> b/MdePkg/Include/Base.h index >>>> ce20b5f01dce..f85f7028a262 100644 >>>> --- a/MdePkg/Include/Base.h >>>> +++ b/MdePkg/Include/Base.h >>>> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >>> #define >>>> OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >>> #endif >>>> >>>> +/// >>>> +/// Portable definition for compile time assertions. >>>> +/// Equivalent to C11 static_assert macro from >>> assert.h. >>>> +/// Takes condtion and error message as its >>> arguments. >>>> +/// >>>> +#ifdef _MSC_EXTENSIONS >>>> + #define STATIC_ASSERT static_assert #else >>>> + #define STATIC_ASSERT _Static_assert #endif >>>> + >>>> /** >>>> Macro that returns a pointer to the data structure >>> that contains a specified field of >>>> that data structure. This is a lightweight method >>> to hide >>>> information by placing a >>>> -- >>>> 2.20.1 (Apple Git-117) >>>> >>>> >>>> -=-=-=-=-=-= >>>> Groups.io Links: You receive all messages sent to this >>> group. >>>> >>>> View/Reply Online (#45503): >>>> https://edk2.groups.io/g/devel/message/45503 >>>> Mute This Topic: https://groups.io/mt/32850582/1759384 >>>> Group Owner: devel+owner@edk2.groups.io >>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub >>>> [liming.gao@intel.com] -=-=-=-=-=-= >>> >>> >>> >> > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-13 8:16 ` [PATCH v2 1/1] " vit9696 2019-08-14 13:50 ` [edk2-devel] " Liming Gao @ 2019-08-15 16:08 ` Michael D Kinney 2019-08-16 19:40 ` Laszlo Ersek 1 sibling, 1 reply; 26+ messages in thread From: Michael D Kinney @ 2019-08-15 16:08 UTC (permalink / raw) To: devel@edk2.groups.io, vit9696@protonmail.com, Kinney, Michael D, Gao, Liming Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> Mike > -----Original Message----- > From: devel@edk2.groups.io > [mailto:devel@edk2.groups.io] On Behalf Of vit9696 via > Groups.Io > Sent: Tuesday, August 13, 2019 1:17 AM > To: devel@edk2.groups.io > Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add > STATIC_ASSERT macro > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 > > Provide a macro for compile time assertions. > Equivalent to C11 static_assert macro from assert.h. > > Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> > --- > MdePkg/Include/Base.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/MdePkg/Include/Base.h > b/MdePkg/Include/Base.h index > ce20b5f01dce..f85f7028a262 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; > #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)- > >Field)) #endif > > +/// > +/// Portable definition for compile time assertions. > +/// Equivalent to C11 static_assert macro from > assert.h. > +/// Takes condtion and error message as its arguments. > +/// > +#ifdef _MSC_EXTENSIONS > + #define STATIC_ASSERT static_assert > +#else > + #define STATIC_ASSERT _Static_assert > +#endif > + > /** > Macro that returns a pointer to the data structure > that contains a specified field of > that data structure. This is a lightweight method > to hide information by placing a > -- > 2.20.1 (Apple Git-117) > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this > group. > > View/Reply Online (#45503): > https://edk2.groups.io/g/devel/message/45503 > Mute This Topic: https://groups.io/mt/32850582/1643496 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub > [michael.d.kinney@intel.com] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 2019-08-15 16:08 ` Michael D Kinney @ 2019-08-16 19:40 ` Laszlo Ersek 0 siblings, 0 replies; 26+ messages in thread From: Laszlo Ersek @ 2019-08-16 19:40 UTC (permalink / raw) To: devel, michael.d.kinney, vit9696@protonmail.com, Gao, Liming On 08/15/19 18:08, Michael D Kinney wrote: > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com> To summarize: personally, I disgree, but I can accept if the patch goes in with Mike's R-b. Thanks, Laszlo >> -----Original Message----- >> From: devel@edk2.groups.io >> [mailto:devel@edk2.groups.io] On Behalf Of vit9696 via >> Groups.Io >> Sent: Tuesday, August 13, 2019 1:17 AM >> To: devel@edk2.groups.io >> Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >> STATIC_ASSERT macro >> >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2048 >> >> Provide a macro for compile time assertions. >> Equivalent to C11 static_assert macro from assert.h. >> >> Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com> >> --- >> MdePkg/Include/Base.h | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/MdePkg/Include/Base.h >> b/MdePkg/Include/Base.h index >> ce20b5f01dce..f85f7028a262 100644 >> --- a/MdePkg/Include/Base.h >> +++ b/MdePkg/Include/Base.h >> @@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST; >> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)- >>> Field)) #endif >> >> +/// >> +/// Portable definition for compile time assertions. >> +/// Equivalent to C11 static_assert macro from >> assert.h. >> +/// Takes condtion and error message as its arguments. >> +/// >> +#ifdef _MSC_EXTENSIONS >> + #define STATIC_ASSERT static_assert >> +#else >> + #define STATIC_ASSERT _Static_assert >> +#endif >> + >> /** >> Macro that returns a pointer to the data structure >> that contains a specified field of >> that data structure. This is a lightweight method >> to hide information by placing a >> -- >> 2.20.1 (Apple Git-117) >> >> >> -=-=-=-=-=-= >> Groups.io Links: You receive all messages sent to this >> group. >> >> View/Reply Online (#45503): >> https://edk2.groups.io/g/devel/message/45503 >> Mute This Topic: https://groups.io/mt/32850582/1643496 >> Group Owner: devel+owner@edk2.groups.io >> Unsubscribe: https://edk2.groups.io/g/devel/unsub >> [michael.d.kinney@intel.com] >> -=-=-=-=-=-= > > > > ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-08-21 21:41 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-13 8:16 [PATCH v2 0/1] MdePkg: Add STATIC_ASSERT macro vit9696
2019-08-13 8:16 ` [PATCH v2 1/1] " vit9696
2019-08-14 13:50 ` [edk2-devel] " Liming Gao
2019-08-14 15:47 ` Michael D Kinney
2019-08-14 16:22 ` Vitaly Cheptosv
2019-08-15 1:05 ` Yao, Jiewen
2019-08-15 1:59 ` Liming Gao
2019-08-15 2:22 ` Vitaly Cheptosv
2019-08-15 7:36 ` Yao, Jiewen
2019-08-16 16:33 ` Laszlo Ersek
2019-08-16 17:23 ` Vitaly Cheptsov
2019-08-16 19:38 ` Laszlo Ersek
2019-08-16 20:19 ` Laszlo Ersek
2019-08-16 21:04 ` Michael D Kinney
2019-08-16 21:40 ` Vitaly Cheptsov
2019-08-16 22:23 ` rebecca
2019-08-16 22:58 ` Andrew Fish
2019-08-16 23:34 ` Vitaly Cheptsov
2019-08-17 0:01 ` rebecca
2019-08-17 0:03 ` Andrew Fish
2019-08-17 0:09 ` rebecca
[not found] ` <15BB8D3E51450F1C.5853@groups.io>
2019-08-21 21:41 ` rebecca
2019-08-16 21:32 ` Vitaly Cheptsov
2019-08-16 16:28 ` Laszlo Ersek
2019-08-15 16:08 ` Michael D Kinney
2019-08-16 19:40 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox