Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

["Bret Barkelew" <bret.barkelew@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 11411 bytes --]

Good note. Thanks!

- Bret

From: Marvin Häuser<mailto:mhaeuser@posteo.de>
Sent: Tuesday, June 29, 2021 1:58 AM
To: Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Kun Qin<mailto:kuqin12@gmail.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; Dong, Eric<mailto:eric.dong@intel.com>; Ni, Ray<mailto:ray.ni@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; Andrew Fish<mailto:afish@apple.com>; Lindholm, Leif<mailto:leif@nuviainc.com>; Michael Kubacki<mailto:Michael.Kubacki@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:mhaeuser@posteo.de>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:lersek@redhat.com>; Kun Qin
> <mailto:kuqin12@gmail.com>; Kinney, Michael D
> <mailto:michael.d.kinney@intel.com>; devel@edk2.groups.io
> <mailto:devel@edk2.groups.io>
> *Cc: *Wang, Jian J <mailto:jian.j.wang@intel.com>; Wu, Hao A
> <mailto:hao.a.wu@intel.com>; Dong, Eric <mailto:eric.dong@intel.com>;
> Ni, Ray <mailto:ray.ni@intel.com>; Liming Gao
> <mailto:gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <mailto:zhiguang.liu@intel.com>; Andrew Fish <mailto:afish@apple.com>;
> Lindholm, Leif <mailto:leif@nuviainc.com>; Bret Barkelew
> <mailto:Bret.Barkelew@microsoft.com>; Michael Kubacki
> <mailto:Michael.Kubacki@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 <mhaeuser@posteo.de>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12@gmail.com>; Laszlo Ersek <lersek@redhat.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>; Kinney, Michael D <michael.d.kinney@intel.com>;
> >>>> Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> >>>> <zhiguang.liu@intel.com>; Andrew Fish <afish@apple.com>; Leif
> >>>> Lindholm <leif@nuviainc.com>; Bret Barkelew
> >>>> <Bret.Barkelew@microsoft.com>; michael.kubacki@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
> >>>>>>
>


[-- Attachment #2: Type: text/html, Size: 18862 bytes --]