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 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; vit9...@protonmail.com; Kinney, Michael D > > Cc: Laszlo Ersek > 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 > Cc: devel@edk2.groups.io; Laszlo Ersek > 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 > > 3 The macro > static_assert > expands to _Static_assert. > >> 14авг. 2019 г., в 18:47, 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 >> i
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
3 The macro
static_assert
expands to _Static_assert.
> 14 авг. 2019 г., в 18:47, 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 = 0x
> } __VERIFY_UINT16_ENUM_SIZE;
>
> typedef enum {
> __VerifyUint32EnumValue = 0x
> } __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")
>
