<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <meta name="ProgId" content="Word.Document"> <meta name="Generator" content="Microsoft Word 15"> <meta name="Originator" content="Microsoft Word 15"> <link rel="File-List" href="cid:filelist.xml@01D76CD1.75391980"><link rel="Edit-Time-Data" href="cid:editdata.mso"><style></style> </head> <body lang="EN-US" link="blue" vlink="#954F72" style="tab-interval:.5in;word-wrap:break-word"> <div class="WordSection1"> Good idea on use of STATIC_ASSERT().<o:p></o:p> <o:p> </o:p> For structures that use flexible array members, we can add a STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.<o:p></o:p> <o:p> </o:p> For example:<o:p></o:p> <o:p> </o:p> STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF (EFI_MM_COMMUNICATE_HEADER, Data));<o:p></o:p> <o:p> </o:p> Mike<o:p></o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via groups.io Sent: Tuesday, June 29, 2021 9:00 AM To: Marvin Häuser <mhaeuser@posteo.de>; Laszlo Ersek <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Lindholm, Leif <leif@nuviainc.com>; Michael Kubacki <Michael.Kubacki@microsoft.com> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER<o:p></o:p> </div> </div> <o:p> </o:p> Good note. Thanks!<o:p></o:p> <o:p> </o:p> - Bret <o:p></o:p> <o:p> </o:p> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: <a href="mailto:mhaeuser@posteo.de">Marvin Häuser</a> Sent: Tuesday, June 29, 2021 1:58 AM To: <a href="mailto:Bret.Barkelew@microsoft.com">Bret Barkelew</a>; <a href="mailto:lersek@redhat.com"> Laszlo Ersek</a>; <a href="mailto:kuqin12@gmail.com">Kun Qin</a>; <a href="mailto:michael.d.kinney@intel.com"> Kinney, Michael D</a>; <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> Cc: <a href="mailto:jian.j.wang@intel.com">Wang, Jian J</a>; <a href="mailto:hao.a.wu@intel.com"> Wu, Hao A</a>; <a href="mailto:eric.dong@intel.com">Dong, Eric</a>; <a href="mailto:ray.ni@intel.com"> Ni, Ray</a>; <a href="mailto:gaoliming@byosoft.com.cn">Liming Gao</a>; <a href="mailto:zhiguang.liu@intel.com"> Liu, Zhiguang</a>; <a href="mailto:afish@apple.com">Andrew Fish</a>; <a href="mailto:leif@nuviainc.com"> Lindholm, Leif</a>; <a href="mailto:Michael.Kubacki@microsoft.com">Michael Kubacki</a> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER<o:p></o:p> </div> <o:p> </o:p> Generally yes, but gladly not for EDK II. Default GNU ABI uses 32-bit alignment for 64-bit integers on IA32 (which led to a (non-critical) mistake in our PE paper :( ) for example, but UEFI / EDK II (seem to) successfully dictate natural alignment consistently. Possibly we could introduce some STATIC_ASSERTs around important cases (e.g. UINT64 on 32-bit platforms) to ensure compilers keep it that way, once the ALIGNOF macro is introduced. Best regards, Marvin On 29.06.21 08:49, Bret Barkelew wrote: > > The fact that it may vary per ABI seems like a pretty big gotcha if > the SMM/MM Core was compiled at a different time or on a different > system than the module that’s invoking the communication. > > - Bret > > *From: *Marvin Häuser <<a href="mailto:mhaeuser@posteo.de">mailto:mhaeuser@posteo.de</a>> > *Sent: *Monday, June 28, 2021 8:43 AM > *To: *Laszlo Ersek <<a href="mailto:lersek@redhat.com">mailto:lersek@redhat.com</a>>; Kun Qin > <<a href="mailto:kuqin12@gmail.com">mailto:kuqin12@gmail.com</a>>; Kinney, Michael D > <<a href="mailto:michael.d.kinney@intel.com">mailto:michael.d.kinney@intel.com</a>>; <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> > <<a href="mailto:devel@edk2.groups.io">mailto:devel@edk2.groups.io</a>> > *Cc: *Wang, Jian J <<a href="mailto:jian.j.wang@intel.com">mailto:jian.j.wang@intel.com</a>>; Wu, Hao A > <<a href="mailto:hao.a.wu@intel.com">mailto:hao.a.wu@intel.com</a>>; Dong, Eric <<a href="mailto:eric.dong@intel.com">mailto:eric.dong@intel.com</a>>; > Ni, Ray <<a href="mailto:ray.ni@intel.com">mailto:ray.ni@intel.com</a>>; Liming Gao > <<a href="mailto:gaoliming@byosoft.com.cn">mailto:gaoliming@byosoft.com.cn</a>>; Liu, Zhiguang > <<a href="mailto:zhiguang.liu@intel.com">mailto:zhiguang.liu@intel.com</a>>; Andrew Fish <<a href="mailto:afish@apple.com">mailto:afish@apple.com</a>>; > Lindholm, Leif <<a href="mailto:leif@nuviainc.com">mailto:leif@nuviainc.com</a>>; Bret Barkelew > <<a href="mailto:Bret.Barkelew@microsoft.com">mailto:Bret.Barkelew@microsoft.com</a>>; Michael Kubacki > <<a href="mailto:Michael.Kubacki@microsoft.com">mailto:Michael.Kubacki@microsoft.com</a>> > *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: > PI Specification: Update EFI_MM_COMMUNICATE_HEADER > > On 28.06.21 16:57, Laszlo Ersek wrote: > > On 06/25/21 20:47, Kun Qin wrote: > >> Hi Mike, > >> > >> Thanks for the information. I can update the patch and proposed spec > >> change to use flexible array in v-next if there is no other concerns. > >> > >> After switching to flexible array, OFFSET_OF (Data) should lead to the > >> same result as sizeof. > > This may be true on specific compilers, but it is *not* guaranteed by > > the C language standard. > > Sorry, for completeness sake... :) > > I don't think it really depends on the compiler (but can vary per ABI), > but it's a conceptual issue with alignment requirements. Assuming > natural alignment and the usual stuff, for "struct s { uint64_t a; > uint32_t b; uint8_t c[]; }" the alignment requirement is 8 Bytes, where > there are 4 Bytes of padding after "b" (as "c" may as well be empty). > "c" however is guaranteed to start after b in the same fashion as if it > was declared with the maximum amount of elements that fit the memory. So > if we take 4 elements for "c", and note that "c" has an alignment > requirement of 1 Byte, c[0 .. 3] will alias the padding after "b". For > "sizeof" this means that the padding is included, whereas for "offsetof" > it is not, yielding "sizeof(struct s) == offsetof(struct s, c) + 4". > That is what I meant by "wasted space" earlier, but this could possibly > be made nicer with macros as necessary. > > As for this specific struct, the values should be identical as it is > padded. > > Best regards, > Marvin > > > > > Quoting C99 6.7.2.1 "Structure and union specifiers", paragraph 16: > > > > "In most situations, the flexible array member is ignored. In > > particular, the size of the structure is as if the flexible array member > > were omitted except that it may have more trailing padding than the > > omission would imply." > > > > Quoting footnotes 17 and 19, > > > > (17) [...] > > struct s { int n; double d[]; }; > > [...] > > > > (19) [...] > > the expressions: > > [...] > > sizeof (struct s) >= offsetof(struct s, d) > > > > are always equal to 1. > > > > Thanks > > Laszlo > > > > > > > >> While sizeof would be a preferred way to move > >> forward. > >> > >> Regards, > >> Kun > >> > >> On 06/24/2021 08:25, Kinney, Michael D wrote: > >>> Hello, > >>> > >>> Flexible array members are supported and should be used. The old > style > >>> of adding an array of size [1] at the end of a structure was used at a > >>> time > >>> flexible array members were not supported by all compilers (late > 1990's). > >>> The workarounds used to handle the array of size [1] are very > >>> confusing when > >>> reading the C code and the fact that sizeof() does not produce the > >>> expected > >>> result make it even worse. > >>> > >>> If we use flexible array members in this proposed change then there is > >>> no need to use OFFSET_OF(). Correct? > >>> > >>> Mike > >>> > >>> > >>>> -----Original Message----- > >>>> From: Marvin Häuser <<a href="mailto:mhaeuser@posteo.de">mhaeuser@posteo.de</a>> > >>>> Sent: Thursday, June 24, 2021 1:00 AM > >>>> To: Kun Qin <<a href="mailto:kuqin12@gmail.com">kuqin12@gmail.com</a>>; Laszlo Ersek <<a href="mailto:lersek@redhat.com">lersek@redhat.com</a>>; > >>>> <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> > >>>> Cc: Wang, Jian J <<a href="mailto:jian.j.wang@intel.com">jian.j.wang@intel.com</a>>; Wu, Hao A > >>>> <<a href="mailto:hao.a.wu@intel.com">hao.a.wu@intel.com</a>>; Dong, Eric <<a href="mailto:eric.dong@intel.com">eric.dong@intel.com</a>>; Ni, Ray > >>>> <<a href="mailto:ray.ni@intel.com">ray.ni@intel.com</a>>; Kinney, Michael D <<a href="mailto:michael.d.kinney@intel.com">michael.d.kinney@intel.com</a>>; > >>>> Liming Gao <<a href="mailto:gaoliming@byosoft.com.cn">gaoliming@byosoft.com.cn</a>>; Liu, Zhiguang > >>>> <<a href="mailto:zhiguang.liu@intel.com">zhiguang.liu@intel.com</a>>; Andrew Fish <<a href="mailto:afish@apple.com">afish@apple.com</a>>; Leif > >>>> Lindholm <<a href="mailto:leif@nuviainc.com">leif@nuviainc.com</a>>; Bret Barkelew > >>>> <<a href="mailto:Bret.Barkelew@microsoft.com">Bret.Barkelew@microsoft.com</a>>; <a href="mailto:michael.kubacki@microsoft.com">michael.kubacki@microsoft.com</a> > >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI > >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER > >>>> > >>>> Hey Kun, > >>>> > >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is > >>>> well-defined for GCC and Clang as it's implemented by an > intrinsic, and > >>>> while the expression for the MSVC compiler is undefined behaviour > as per > >>>> the C standard, it is well-defined for MSVC due to their own > >>>> implementation being identical. From my standpoint, all supported > >>>> compilers will yield well-defined behaviour even this way. > OFFSET_OF on > >>>> flexible arrays is not UB in any case to my knowledge. > >>>> > >>>> However, the same way as your new suggestion, you can replace > OFFSET_OF > >>>> with sizeof. While this *can* lead to wasted space with certain > >>>> structure layouts (e.g. when the flexible array overlays padding > bytes), > >>>> this is not the case here, and otherwise just loses you a few > bytes. I > >>>> think this comes down to preference. > >>>> > >>>> The pattern you mentioned arguably is less nice syntax when used > >>>> (involves address calculation and casting), but the biggest > problem here > >>>> is alignment constraints. For packed structures, you lose the > ability of > >>>> automatic unaligned accesses (irrelevant here because the > structure is > >>>> manually padded anyway). For non-packed structures, you still need to > >>>> ensure the alignment requirement of the trailing array data is met > >>>> manually. With flexible array members, the compiler takes care of > both > >>>> 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 > >>>>> compilers. Instead of using flexible arrays, is it better to remove > >>>>> the `Data` field, pack the structure and follow > >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern? > >>>>> > >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and > >>>>> read/write to `Data` will follow the range indicated by > MessageLength. > >>>>> But yes, that will enforce developers to update their platform level > >>>>> implementations accordingly. > >>>>> > >>>>> Regards, > >>>>> Kun > >>>>> > >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote: > >>>>>> On 06/23/21 08:54, Marvin Häuser wrote: > >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote: > >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote: > >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote: > >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote: > >>>>>>>>>>> 2) Is it feasible yet with the current set of supported > >>>>>>>>>>> compilers to > >>>>>>>>>>> support flexible arrays? > >>>>>>>>>> My impression is that flexible arrays are already supported (as > >>>>>>>>>> seen > >>>>>>>>>> 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 > >>>>>>>>>> this > >>>>>>>>>> change. So any input is appreciated. > >>>>>>>>> Huh, interesting. Last time I tried I was told about > >>>>>>>>> incompatibilities > >>>>>>>>> with MSVC, but I know some have been dropped since then > (2005 and > >>>>>>>>> 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 > >>>>>>>> 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 > >>>>>> ><o:p></o:p> <o:p> </o:p> <div> </o:p> </div> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/77297">View/Reply Online (#77297)</a> | | <a target="_blank" href="https://groups.io/mt/83863450/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>