Good note. Thanks! - Bret
From: Marvin Häuser<mailto:mhaeu...@posteo.de> Sent: Tuesday, June 29, 2021 1:58 AM To: Bret Barkelew<mailto:bret.barke...@microsoft.com>; Laszlo Ersek<mailto:ler...@redhat.com>; Kun Qin<mailto:kuqi...@gmail.com>; Kinney, Michael D<mailto:michael.d.kin...@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Wang, Jian J<mailto:jian.j.w...@intel.com>; Wu, Hao A<mailto:hao.a...@intel.com>; Dong, Eric<mailto:eric.d...@intel.com>; Ni, Ray<mailto:ray...@intel.com>; Liming Gao<mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang....@intel.com>; Andrew Fish<mailto:af...@apple.com>; Lindholm, Leif<mailto:l...@nuviainc.com>; Michael Kubacki<mailto:michael.kuba...@microsoft.com> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER 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 <mailto:mhaeu...@posteo.de> > *Sent: *Monday, June 28, 2021 8:43 AM > *To: *Laszlo Ersek <mailto:ler...@redhat.com>; Kun Qin > <mailto:kuqi...@gmail.com>; Kinney, Michael D > <mailto:michael.d.kin...@intel.com>; devel@edk2.groups.io > <mailto:devel@edk2.groups.io> > *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com>; Wu, Hao A > <mailto:hao.a...@intel.com>; Dong, Eric <mailto:eric.d...@intel.com>; > Ni, Ray <mailto:ray...@intel.com>; Liming Gao > <mailto:gaolim...@byosoft.com.cn>; Liu, Zhiguang > <mailto:zhiguang....@intel.com>; Andrew Fish <mailto:af...@apple.com>; > Lindholm, Leif <mailto:l...@nuviainc.com>; Bret Barkelew > <mailto:bret.barke...@microsoft.com>; Michael Kubacki > <mailto:michael.kuba...@microsoft.com> > *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 <mhaeu...@posteo.de> > >>>> Sent: Thursday, June 24, 2021 1:00 AM > >>>> To: Kun Qin <kuqi...@gmail.com>; Laszlo Ersek <ler...@redhat.com>; > >>>> devel@edk2.groups.io > >>>> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A > >>>> <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; Ni, Ray > >>>> <ray...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; > >>>> Liming Gao <gaolim...@byosoft.com.cn>; Liu, Zhiguang > >>>> <zhiguang....@intel.com>; Andrew Fish <af...@apple.com>; Leif > >>>> Lindholm <l...@nuviainc.com>; Bret Barkelew > >>>> <bret.barke...@microsoft.com>; michael.kuba...@microsoft.com > >>>> 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 > >>>>>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77281): https://edk2.groups.io/g/devel/message/77281 Mute This Topic: https://groups.io/mt/83863450/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-