From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 16 Aug 2019 09:28:37 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7973D308FBAF; Fri, 16 Aug 2019 16:28:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-63.ams2.redhat.com [10.36.116.63]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5809B84256; Fri, 16 Aug 2019 16:28:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro To: "vit9696@protonmail.com" , michael.d.kinney@intel.com Cc: "devel@edk2.groups.io" References: <20190813081644.53963-1-vit9696@protonmail.com> <20190813081644.53963-2-vit9696@protonmail.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E4D0EC7@SHSMSX104.ccr.corp.intel.com> <65C2153C-7D59-490C-8DD2-A48FF0EEA8DE@protonmail.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 16 Aug 2019 18:28:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <65C2153C-7D59-490C-8DD2-A48FF0EEA8DE@protonmail.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.43]); Fri, 16 Aug 2019 16:28:36 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 08/14/19 18:22, vit9696@protonmail.com wrote: > Michael, Liming, Laszlo, >=20 > Static assertions via _Static_assert are standard C11 functionality, thu= s any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to s= upport the second argument with the diagnostic description. > The notation without the message currently is only present in C++, not i= n C, thus the two-argument notation is the only allowed notation for _Stati= c_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 releva= nt section (6.7.10). >=20 > GCC and CLANG (including Xcode) appear to be conforming to the standard = for this section, and MSVC compiler static_assert extension also supports t= he diagnostic message argument. This is pretty much all we care about. >=20 > As for examples, I see little reason to clarify STATIC_ASSERT behaviour = outside of the standard reference in its description and actual usage in th= e source code, but can do that just fine if you think that it may help some= body. 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. >=20 > I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_AS= SERT, 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 B= ase.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 b= e 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 sta= tic assertions some time ago. However, fixing up all broken assertions is u= nlikely 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 referen= ce purposes and may help the maintainers to get a better idea when static a= ssertions 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 >=20 > Looking forward to hearing your opinions. >=20 > Best regards, > Vitaly >=20 >=20 > 6.7.10 Static assertions >=20 > Syntax > 1 static_assert-declaration: > _Static_assert ( constant-expression , string-literal ) ; >=20 > Constraints > 2 The constant expression shall compare unequal to 0. >=20 > Semantics > 3 The constant expression shall be an integer constant expression. If th= e value of the constant expression compares unequal to 0, the declaration h= as no effect. Otherwise, the constraint is violated and the implementation = shall produce a diagnostic message that includes the text of the string lit= eral, except that characters not in the basic source character set are not = required to appear in the message. > Forward references: diagnostics (7.2). >=20 > 7.2 Diagnostics >=20 > 3 The macro > static_assert > expands to _Static_assert. >=20 >=20 >> 14 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 18:47, Kinney, Michael D =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >> >> >> 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 decla= ration in >> the preprocessor if the size is incorrect. These are declared as "ext= ern" 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[(si= zeof(TYPE) =3D=3D (Size)) / (sizeof(TYPE) =3D=3D (Size))] >> >> // >> // Verify that ProcessorBind.h produced UEFI Data Types that are compli= ant 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 =3D 0xff >> } __VERIFY_UINT8_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint16EnumValue =3D 0xffff >> } __VERIFY_UINT16_ENUM_SIZE; >> >> typedef enum { >> __VerifyUint32EnumValue =3D 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) =3D=3D 1, "sizeof (BOOLEAN) does not me= et UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (UINT16) =3D=3D 2, "sizeof (UINT16) does not mee= t UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (INT32) =3D=3D 4, "sizeof (INT32) does not meet= UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (CHAR16) =3D=3D 2, "sizeof (CHAR16) does not mee= t UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) =3D=3D 4, "Size of enu= m does not meet UEFI Specification Data Type requirements") >> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) =3D=3D 4, "Size of en= um 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=3D2048 >>>> >>>> Provide a macro for compile time assertions. >>>> Equivalent to C11 static_assert macro from assert.h. >>>> >>>> Signed-off-by: Vitaly Cheptsov >>> >>>> --- >>>> 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) >>>> >>>> >>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>> 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] -=3D-=3D-=3D-=3D-=3D-=3D >>> >>> >>>=20 >> >=20