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:33:10 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 68D4A3064FD4; Fri, 16 Aug 2019 16:33:09 +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 5E7693AF3; Fri, 16 Aug 2019 16:33:07 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro 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" 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> <74D8A39837DF1E4DA445A8C0B3885C503F75D776@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E4D1378@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 16 Aug 2019 18:33:06 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4D1378@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Fri, 16 Aug 2019 16:33:09 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 08/15/19 03:59, Gao, Liming wrote: > Vitaly: > As you know, edk2 201908 stable tag will start soft freeze tomorrow. D= ose 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 Ya= o, Jiewen > Sent: Thursday, August 15, 2019 9:05 AM > To: devel@edk2.groups.io; vit9696@protonmail.com; Kinney, Michael D > Cc: Laszlo Ersek > Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro >=20 > Good input. > I think we should separate the work to convert all EDKII code to use STA= TIC_ASSERT. > We can do that work once we add STATIC_ASSERT. >=20 > I recommend: >=20 > 1) Step 1: Add STATIS_ASSERT - this patch and this Bugzilla >=20 > 2) Step 2: Convert VERIFY_SIZE_OF to STATIS_ASSERT, and remove VERI= FY_SIZE_OF =E2=80=93 the other patch and the other Bugzilla >=20 > 3) Step 3: Scan the rest, if there is need. =E2=80=93 Another patch= and another Bugzilla >=20 > Thank you > Yao Jiewen >=20 > From: devel@edk2.groups.io [mailto:devel@ed= k2.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 >=20 >=20 > 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. >=20 > 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. >=20 > 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. >=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. >=20 > 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. >=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. >=20 > 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))] >=20 >> >> // >> // 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") >=20 >> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) =3D=3D 4, "Size of en= um does not meet UEFI Specification Data Type requirements") >=20 >> >> 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@protonm= ail.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 >=20 >=20 >=20 >=20