Good idea on use of STATIC_ASSERT().
For structures that use flexible array members, we can add a
STATIC_ASSERT() for the sizeof() and OFFSET_OF() returning the same result.
For example:
STATIC_ASSERT (sizeof (EFI_MM_COMMUNICATE_HEADER) == OFFSET_OF
(EFI_MM_COMMUNICATE_HEADER, Data));
Mike
*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 <mhaeu...@posteo.de>; Laszlo Ersek
<ler...@redhat.com>; Kun Qin <kuqi...@gmail.com>; Kinney, Michael D
<michael.d.kin...@intel.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>; Liming Gao <gaolim...@byosoft.com.cn>; Liu, Zhiguang
<zhiguang....@intel.com>; Andrew Fish <af...@apple.com>; Lindholm, Leif
<l...@nuviainc.com>; Michael Kubacki <michael.kuba...@microsoft.com>
*Subject:* Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code
First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
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
<mailto:mhaeu...@posteo.de>>
> *Sent: *Monday, June 28, 2021 8:43 AM
> *To: *Laszlo Ersek <mailto:ler...@redhat.com
<mailto:ler...@redhat.com>>; Kun Qin
> <mailto:kuqi...@gmail.com <mailto:kuqi...@gmail.com>>; Kinney, Michael D
> <mailto:michael.d.kin...@intel.com
<mailto:michael.d.kin...@intel.com>>; devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
> <mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>>
> *Cc: *Wang, Jian J <mailto:jian.j.w...@intel.com
<mailto:jian.j.w...@intel.com>>; Wu, Hao A
> <mailto:hao.a...@intel.com <mailto:hao.a...@intel.com>>; Dong, Eric
<mailto:eric.d...@intel.com <mailto:eric.d...@intel.com>>;
> Ni, Ray <mailto:ray...@intel.com <mailto:ray...@intel.com>>; Liming Gao
> <mailto:gaolim...@byosoft.com.cn <mailto:gaolim...@byosoft.com.cn>>;
Liu, Zhiguang
> <mailto:zhiguang....@intel.com <mailto:zhiguang....@intel.com>>;
Andrew Fish <mailto:af...@apple.com <mailto:af...@apple.com>>;
> Lindholm, Leif <mailto:l...@nuviainc.com <mailto:l...@nuviainc.com>>;
Bret Barkelew
> <mailto:bret.barke...@microsoft.com
<mailto:bret.barke...@microsoft.com>>; Michael Kubacki
> <mailto:michael.kuba...@microsoft.com
<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 <mailto:mhaeu...@posteo.de>>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqi...@gmail.com <mailto:kuqi...@gmail.com>>;
Laszlo Ersek <ler...@redhat.com <mailto:ler...@redhat.com>>;
> >>>> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
> >>>> Cc: Wang, Jian J <jian.j.w...@intel.com
<mailto:jian.j.w...@intel.com>>; Wu, Hao A
> >>>> <hao.a...@intel.com <mailto:hao.a...@intel.com>>; Dong, Eric
<eric.d...@intel.com <mailto:eric.d...@intel.com>>; Ni, Ray
> >>>> <ray...@intel.com <mailto:ray...@intel.com>>; Kinney, Michael D
<michael.d.kin...@intel.com <mailto:michael.d.kin...@intel.com>>;
> >>>> Liming Gao <gaolim...@byosoft.com.cn
<mailto:gaolim...@byosoft.com.cn>>; Liu, Zhiguang
> >>>> <zhiguang....@intel.com <mailto:zhiguang....@intel.com>>; Andrew
Fish <af...@apple.com <mailto:af...@apple.com>>; Leif
> >>>> Lindholm <l...@nuviainc.com <mailto:l...@nuviainc.com>>; Bret
Barkelew
> >>>> <bret.barke...@microsoft.com
<mailto:bret.barke...@microsoft.com>>; michael.kuba...@microsoft.com
<mailto: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
> >>>>>>
>
