On 08/14/19 18:22, vit9...@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.kin...@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; vit9...@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
>>> <vit9...@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+ow...@edk2.groups.io
>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub
>>>> [liming....@intel.com] -=-=-=-=-=-=
>>>
>>>
>>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45827): https://edk2.groups.io/g/devel/message/45827
Mute This Topic: https://groups.io/mt/32850582/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
