From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web12.4499.1624521618990392678 for ; Thu, 24 Jun 2021 01:00:19 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=fdAv/RIk; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 9C378240100 for ; Thu, 24 Jun 2021 10:00:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1624521616; bh=q0tRkpIrbU3NGzS5JAxNzNCh8PPLt/F+Ta4IULNXai4=; h=Subject:To:Cc:From:Date:From; b=fdAv/RIkCyEzFN6s/iTYRfbtPSUfM4yZefx2D0CGf3prC301Ah2x4VPRZplKJPhZ1 Irva7n1PAZ4eakRIsKEwd/KPz4SLNFlwwy+VAdeLc3wjM8tm2vfftsZLeeTg+LlEN3 XJg0rGSkBZw+YOVsCW6H0GErSZKgt2Ttk6gEQsaHY3LCJ0eZgYIokihat7G9WOtk1/ uHxfTcLnYCKXNYy84EfjysTaX1OkDBKVAvQ4DLixkNuhhYKGbPwLBxaRF020FKLNda s0FcbVXpHYSVI9rJ5oFYIzi5VWAOzUB9Sqegq5vOWtDtdkEpotKumGvQsr0Ot5/eD+ iM1FgDeZ0KB+g== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4G9XcC0SZxz9rxV; Thu, 24 Jun 2021 10:00:10 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER To: Kun Qin , Laszlo Ersek , devel@edk2.groups.io Cc: Jian J Wang , Hao A Wu , Eric Dong , Ray Ni , Michael D Kinney , Liming Gao , Zhiguang Liu , Andrew Fish , Leif Lindholm , Bret Barkelew , michael.kubacki@microsoft.com References: <20210610014259.1151-1-kuqin12@gmail.com> <93fd191e-e62f-f02e-11d0-403173fcdf42@posteo.de> <817ab349-b7a2-528b-9b78-aa72cefcd25a@posteo.de> <40bffd17-28a6-d280-02b1-628f1b2daa09@posteo.de> <1525bdb4-abfa-d89a-d7d9-7f8745640bff@gmail.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <3f3ce50a-4ff0-d80f-c635-528751a0ab78@posteo.de> Date: Thu, 24 Jun 2021 08:00:10 +0000 MIME-Version: 1.0 In-Reply-To: <1525bdb4-abfa-d89a-d7d9-7f8745640bff@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable Hey Kun, Why would you rely on undefined behaviours? The OFFSET_OF macro is=20 well-defined for GCC and Clang as it's implemented by an intrinsic, and=20 while the expression for the MSVC compiler is undefined behaviour as per=20 the C standard, it is well-defined for MSVC due to their own=20 implementation being identical. From my standpoint, all supported=20 compilers will yield well-defined behaviour even this way. OFFSET_OF on=20 flexible arrays is not UB in any case to my knowledge. However, the same way as your new suggestion, you can replace OFFSET_OF=20 with sizeof. While this *can* lead to wasted space with certain=20 structure layouts (e.g. when the flexible array overlays padding bytes),=20 this is not the case here, and otherwise just loses you a few bytes. I=20 think this comes down to preference. The pattern you mentioned arguably is less nice syntax when used=20 (involves address calculation and casting), but the biggest problem here=20 is alignment constraints. For packed structures, you lose the ability of=20 automatic unaligned accesses (irrelevant here because the structure is=20 manually padded anyway). For non-packed structures, you still need to=20 ensure the alignment requirement of the trailing array data is met=20 manually. With flexible array members, the compiler takes care of both=20 cases automatically. Best regards, Marvin On 24.06.21 02:24, Kun Qin wrote: > Hi Marvin, > > I would prefer not to rely on undefined behaviors from different=20 > compilers. Instead of using flexible arrays, is it better to remove=20 > the `Data` field, pack the structure and follow=20 > "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern? > > In that case, OFFSET_OF will be forced to change to sizeof, and=20 > read/write to `Data` will follow the range indicated by MessageLength.=20 > But yes, that will enforce developers to update their platform level=20 > implementations accordingly. > > Regards, > Kun > > On 06/23/2021 08:26, Laszlo Ersek wrote: >> On 06/23/21 08:54, Marvin H=C3=A4user wrote: >>> On 22.06.21 17:34, Laszlo Ersek wrote: >>>> On 06/18/21 11:37, Marvin H=C3=A4user wrote: >>>>> On 16.06.21 22:58, Kun Qin wrote: >>>>>> On 06/16/2021 00:02, Marvin H=C3=A4user wrote: >>>>>>> 2) Is it feasible yet with the current set of supported=20 >>>>>>> compilers to >>>>>>> support flexible arrays? >>>>>> My impression is that flexible arrays are already supported (as se= en >>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h). >>>>>> Please correct me if I am wrong. >>>>>> >>>>>> Would you mind letting me know why this is applicable here? We are >>>>>> trying to seek ideas on how to catch developer mistakes caused by=20 >>>>>> this >>>>>> change. So any input is appreciated. >>>>> Huh, interesting. Last time I tried I was told about=20 >>>>> incompatibilities >>>>> with MSVC, but I know some have been dropped since then (2005 and=20 >>>>> 2008 >>>>> if I recall correctly?), so that'd be great to allow globally. >>>> I too am surprised to see >>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The >>>> flexible array member is a C99 feature, and I didn't even know that = we >>>> disallowed it for the sake of particular VS toolchains -- I thought = we >>>> had a more general reason than just "not supported by VS versions X >>>> and Y". >>>> >>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF() >>>> macro definition for non-gcc / non-clang: >>>> >>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field)) >>>> >>>> borders on undefined behavior as far as I can tell, so its behavior = is >>>> totally up to the compiler. It works thus far okay on Visual=20 >>>> Studio, but >>>> I couldn't say if it extended correctly to flexible array members. >>> >>> Yes, it's UB by the standard, but this is actually how MS implements >>> them (or used to anyway?). I don't see why it'd cause issues with >>> flexible arrays, as only the start of the array is relevant (which is >>> constant for all instances of the structure no matter the amount of >>> elements actually stored). Any specific concern? If so, they could be >>> addressed by appropriate STATIC_ASSERTs. >> >> No specific concern; my point was that two aspects of the same "class" >> of undefined behavior didn't need to be consistent with each other. >> >> Thanks >> Laszlo >>