From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@protonmail.com header.s=default header.b=ESnqqM1y; spf=pass (domain: protonmail.com, ip: 185.70.40.27, mailfrom: vit9696@protonmail.com) Received: from mail4.protonmail.ch (mail4.protonmail.ch [185.70.40.27]) by groups.io with SMTP; Fri, 16 Aug 2019 10:23:55 -0700 Date: Fri, 16 Aug 2019 17:23:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=default; t=1565976231; bh=oIi8ys2L/Z8ixTD7o3KW3IjxFqRrmT8fQn8/eOTdVB0=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References: Feedback-ID:From; b=ESnqqM1y69tU060HjmpI8W9Eogsy+9Jg4wnqQHIEoQhWaeBF293RfIj06JObERyOn H4KIeA5wvcrSdfAHiTYJKv8+DsRCoPZ5Nd3z6TCisgjmPBJUlSaqSujmPwPHRS28vI 164MltjXNenKE0assRcsH/A9zyZV7S0nqZzZGWVk= To: Laszlo Ersek From: "Vitaly Cheptsov" Cc: "devel@edk2.groups.io" , "leif.lindholm@linaro.org" , "afish@apple.com" Reply-To: "vit9696@protonmail.com" Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro Message-ID: In-Reply-To: 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> Feedback-ID: p9QuX-L1wMgUm6nrSvNrf8juLupNs0VSnzXGVXuYDxlEahFdWtaedWDMB9zpwGDklGt7kzs1-RBc0cqz327Gcg==:Ext:ProtonMail MIME-Version: 1.0 X-Spam-Status: No, score=-0.7 required=7.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,FREEMAIL_REPLYTO_END_DIGIT autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mail.protonmail.ch X-Groupsio-MsgNum: 45836 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------ff025916c8a246cabfe114b18a58e614"; charset=UTF-8 -----------------------ff025916c8a246cabfe114b18a58e614 Cc: "Gao, Liming" , "devel@edk2.groups.io" , "Yao, Jiewen" , "Kinney, Michael D" , "leif.lindholm@linaro.org" , "afish@apple.com" , "Cetola, Stephano" Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Date: Fri, 16 Aug 2019 20:23:40 +0300 From: "vit9696@protonmail.com" In-Reply-To: Message-Id: Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) 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> <-oYbCGlXQcvXnnsL2bvwm0_mx-AEeT6WtLeV1BSw1EaJlVhkgz2DJB-HVThO2RqNbGASsqNeIq8Di8yMxklNEw==@protonmail.conversationid> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro To: Laszlo Ersek X-Mailer: Apple Mail (2.3445.104.11) Laszlo, I have already mentioned that the documentation is sufficient as _Static_a= ssert is C standard, so I do not plan to make a V3 for this patch. The patc= h is merge ready. As for usage examples I have an opposing opinion to yours and believe it i= s based on very good reasons. Not using STATIC_ASSERT in the current releas= e will make the feature optionally available and let people test it in thei= r setups. In case they notice it does not work for them they will have 3 mo= nths grace period to report it to us and consider making a change. This wil= l also give them 3 months grace period of VERIFY_SIZE_OF macro removal in f= avour of STATIC_ASSERT. Making the change now will let people do seamless t= ransition to the new feature and will avoid obstacles you are currently try= ing to create. Thus STATIC_ASSERT usage and VERIFY_SIZE_OF removal must bot= h be separate patchsets with potentially separate BZs. Thanks for understanding, Vitaly > 16 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 19:33, Laszlo Ersek =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 >=20 > 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. >=20 > 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. >=20 > 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). >=20 > 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. >=20 > Thanks > Laszlo >=20 >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Y= ao, 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 macr= o >>=20 >> Good input. >> I think we should separate the work to convert all EDKII code to use ST= ATIC_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 VER= IFY_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 patc= h and another Bugzilla >>=20 >> Thank you >> Yao Jiewen >>=20 >> From: devel@edk2.groups.io [mailto:devel@e= dk2.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 macr= o >>=20 >>=20 >> Michael, Liming, Laszlo, >>=20 >> Static assertions via _Static_assert are standard C11 functionality, th= us any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to = support the second argument with the diagnostic description. >>=20 >> 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 _Stat= ic_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 relev= ant 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 = the 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 t= he source code, but can do that just fine if you think that it may help som= ebody. >>=20 >> I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_A= SSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed fro= m 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. >>=20 >> 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 st= atic 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 refere= nce purposes and may help the maintainers to get a better idea when static = assertions 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 t= he 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 li= teral, 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 <= michael.d.kinney@intel.com> =D0=BD=D0=B0= = =D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>>=20 >>>=20 >>> Liming, >>>=20 >>> 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. >>>=20 >>> /** >>> Verifies the storage size of a given data type. >>>=20 >>> 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. >>>=20 >>> @param TYPE The date type to determine the size of. >>> @param Size The expected size for the TYPE. >>>=20 >>> **/ >>> #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(s= izeof(TYPE) =3D=3D (Size)) / (sizeof(TYPE) =3D=3D (Size))] >>=20 >>>=20 >>> // >>> // Verify that ProcessorBind.h produced UEFI Data Types that are compl= iant 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); >>>=20 >>> // >>> // 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; >>>=20 >>> typedef enum { >>> __VerifyUint16EnumValue =3D 0xffff >>> } __VERIFY_UINT16_ENUM_SIZE; >>>=20 >>> typedef enum { >>> __VerifyUint32EnumValue =3D 0xffffffff >>> } __VERIFY_UINT32_ENUM_SIZE; >>>=20 >>> VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); >>> VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4); >>> VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); >>>=20 >>> A couple examples. Do all the compilers support the message parameter= too? >>>=20 >>> STATIC_ASSERT (sizeof (BOOLEAN) =3D=3D 1, "sizeof (BOOLEAN) does not m= eet UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (UINT16) =3D=3D 2, "sizeof (UINT16) does not me= et UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (INT32) =3D=3D 4, "sizeof (INT32) does not mee= t UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (CHAR16) =3D=3D 2, "sizeof (CHAR16) does not me= et UEFI Specification Data Type requirements") >>> STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) =3D=3D 4, "Size of en= um does not meet UEFI Specification Data Type requirements") >>=20 >>> STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) =3D=3D 4, "Size of e= num does not meet UEFI Specification Data Type requirements") >>=20 >>>=20 >>> Thanks, >>>=20 >>> Mike >>>=20 >>>> -----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@proton= mail.com >>>> Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add >>>> STATIC_ASSERT macro >>>>=20 >>>> Can you add the sample usage of new macro STATIC_ASSERT? >>>>=20 >>>> Or, give the link of static_assert or _Static_assert. >>>>=20 >>>> If so, the developer knows how to use them in source >>>> code. >>>>=20 >>>> 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 >>>>>=20 >>>>>=20 >>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2048 >>>>>=20 >>>>> Provide a macro for compile time assertions. >>>>> Equivalent to C11 static_assert macro from assert.h. >>>>>=20 >>>>> Signed-off-by: Vitaly Cheptsov >>>> > >>>>> --- >>>>> MdePkg/Include/Base.h | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>>=20 >>>>> 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 >>>>>=20 >>>>> +/// >>>>> +/// 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) >>>>>=20 >>>>>=20 >>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>>> Groups.io Links: You receive all messages sent to this >>>> group. >>>>>=20 >>>>> 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 >>=20 >>=20 >>=20 >>=20 >=20 -----------------------ff025916c8a246cabfe114b18a58e614 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJdVuadCRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xdwmCAB7J688 qluNGLxTur7TKCiqmQDLqTcF2zpaax4kNv5bl4siHPwAzoxyPwCUVPcwOaRh 8gny3ZIkifeKCfwRJ9ggwMWX4qXV4yCXHjF9MqIiEwu95tMHjYJZg5B7+qFN BxMC/19MgC2siszqmta/DD1VOLr8hOC0GYo68XzDShLuvB5OJBt/rTKBVzZF udedy/YQccyZKhzmEUBDU9n6KECbWHKC0JYP33/7FQy8zvHWWjq+Y9s7JqCp bbcOkGj4k6iSIg1vNUFHy3tdviONpun+7iAO0t+V+9XU9lS2J8eSeHRwzq4O B3Wxuqvt3UFq7Zz6sCBmnPLT2U1JV1OJntS3 =5q8A -----END PGP SIGNATURE----- -----------------------ff025916c8a246cabfe114b18a58e614--