[EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER
Kun Qin
kuqin12 at gmail.com
Wed Jun 30 07:56:38 UTC 2021
Thanks for the clarification. I will work on v-next with flexible array
as Data field.
Regards,
Kun
On 06/29/2021 18:07, Kinney, Michael D wrote:
> If it breaks in the future, then that would be due to a new compiler that
> or changes to the configuration of an existing compiler that break compatibility
> with UEFI ABI. The compiler issue must be resolved before the new compiler
> or change to existing compiler are accepted.
>
> Mike
>
>> -----Original Message-----
>> From: Kun Qin <kuqin12 at gmail.com>
>> Sent: Tuesday, June 29, 2021 4:11 PM
>> To: Kinney, Michael D <michael.d.kinney at intel.com>; devel at edk2.groups.io; bret.barkelew at microsoft.com; Marvin Häuser
>> <mhaeuser at posteo.de>; Laszlo Ersek <lersek at redhat.com>
>> Cc: Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni, Ray
>> <ray.ni at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu at intel.com>; Andrew Fish
>> <afish at apple.com>; Lindholm, Leif <leif at nuviainc.com>; Michael Kubacki <Michael.Kubacki at microsoft.com>
>> Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI Specification: Update
>> EFI_MM_COMMUNICATE_HEADER
>>
>> Hi Mike,
>>
>> Thanks for the note. I will add this check for sanity check in v-next,
>> assuming this will not fail for currently supported compilers.
>>
>> Just curious, what do we normally do if this type of check start to
>> break in the future?
>>
>> Thanks,
>> Kun
>>
>> On 06/29/2021 10:28, Kinney, Michael D wrote:
>>> 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 at edk2.groups.io <devel at edk2.groups.io> *On Behalf Of *Bret
>>> Barkelew via groups.io
>>> *Sent:* Tuesday, June 29, 2021 9:00 AM
>>> *To:* Marvin Häuser <mhaeuser at posteo.de>; Laszlo Ersek
>>> <lersek at redhat.com>; Kun Qin <kuqin12 at gmail.com>; Kinney, Michael D
>>> <michael.d.kinney at intel.com>; devel at edk2.groups.io
>>> *Cc:* Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A
>>> <hao.a.wu at intel.com>; Dong, Eric <eric.dong at intel.com>; Ni, Ray
>>> <ray.ni at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>; Liu, Zhiguang
>>> <zhiguang.liu at intel.com>; Andrew Fish <afish at apple.com>; Lindholm, Leif
>>> <leif at nuviainc.com>; Michael Kubacki <Michael.Kubacki at 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:mhaeuser at posteo.de>
>>> *Sent: *Tuesday, June 29, 2021 1:58 AM
>>> *To: *Bret Barkelew <mailto:Bret.Barkelew at microsoft.com>; Laszlo Ersek
>>> <mailto:lersek at redhat.com>; Kun Qin <mailto:kuqin12 at gmail.com>; Kinney,
>>> Michael D <mailto:michael.d.kinney at intel.com>; devel at edk2.groups.io
>>> <mailto:devel at edk2.groups.io>
>>> *Cc: *Wang, Jian J <mailto:jian.j.wang at intel.com>; Wu, Hao A
>>> <mailto:hao.a.wu at intel.com>; Dong, Eric <mailto:eric.dong at intel.com>;
>>> Ni, Ray <mailto:ray.ni at intel.com>; Liming Gao
>>> <mailto:gaoliming at byosoft.com.cn>; Liu, Zhiguang
>>> <mailto:zhiguang.liu at intel.com>; Andrew Fish <mailto:afish at apple.com>;
>>> Lindholm, Leif <mailto:leif at nuviainc.com>; Michael Kubacki
>>> <mailto:Michael.Kubacki at 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 at posteo.de
>>> <mailto:mhaeuser at posteo.de>>
>>> > *Sent: *Monday, June 28, 2021 8:43 AM
>>> > *To: *Laszlo Ersek <mailto:lersek at redhat.com
>>> <mailto:lersek at redhat.com>>; Kun Qin
>>> > <mailto:kuqin12 at gmail.com <mailto:kuqin12 at gmail.com>>; Kinney, Michael D
>>> > <mailto:michael.d.kinney at intel.com
>>> <mailto:michael.d.kinney at intel.com>>; devel at edk2.groups.io
>>> <mailto:devel at edk2.groups.io>
>>> > <mailto:devel at edk2.groups.io <mailto:devel at edk2.groups.io>>
>>> > *Cc: *Wang, Jian J <mailto:jian.j.wang at intel.com
>>> <mailto:jian.j.wang at intel.com>>; Wu, Hao A
>>> > <mailto:hao.a.wu at intel.com <mailto:hao.a.wu at intel.com>>; Dong, Eric
>>> <mailto:eric.dong at intel.com <mailto:eric.dong at intel.com>>;
>>> > Ni, Ray <mailto:ray.ni at intel.com <mailto:ray.ni at intel.com>>; Liming Gao
>>> > <mailto:gaoliming at byosoft.com.cn <mailto:gaoliming at byosoft.com.cn>>;
>>> Liu, Zhiguang
>>> > <mailto:zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>>;
>>> Andrew Fish <mailto:afish at apple.com <mailto:afish at apple.com>>;
>>> > Lindholm, Leif <mailto:leif at nuviainc.com <mailto:leif at nuviainc.com>>;
>>> Bret Barkelew
>>> > <mailto:Bret.Barkelew at microsoft.com
>>> <mailto:Bret.Barkelew at microsoft.com>>; Michael Kubacki
>>> > <mailto:Michael.Kubacki at microsoft.com
>>> <mailto:Michael.Kubacki at 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 at posteo.de <mailto:mhaeuser at posteo.de>>
>>> > >>>> Sent: Thursday, June 24, 2021 1:00 AM
>>> > >>>> To: Kun Qin <kuqin12 at gmail.com <mailto:kuqin12 at gmail.com>>;
>>> Laszlo Ersek <lersek at redhat.com <mailto:lersek at redhat.com>>;
>>> > >>>> devel at edk2.groups.io <mailto:devel at edk2.groups.io>
>>> > >>>> Cc: Wang, Jian J <jian.j.wang at intel.com
>>> <mailto:jian.j.wang at intel.com>>; Wu, Hao A
>>> > >>>> <hao.a.wu at intel.com <mailto:hao.a.wu at intel.com>>; Dong, Eric
>>> <eric.dong at intel.com <mailto:eric.dong at intel.com>>; Ni, Ray
>>> > >>>> <ray.ni at intel.com <mailto:ray.ni at intel.com>>; Kinney, Michael D
>>> <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>;
>>> > >>>> Liming Gao <gaoliming at byosoft.com.cn
>>> <mailto:gaoliming at byosoft.com.cn>>; Liu, Zhiguang
>>> > >>>> <zhiguang.liu at intel.com <mailto:zhiguang.liu at intel.com>>; Andrew
>>> Fish <afish at apple.com <mailto:afish at apple.com>>; Leif
>>> > >>>> Lindholm <leif at nuviainc.com <mailto:leif at nuviainc.com>>; Bret
>>> Barkelew
>>> > >>>> <Bret.Barkelew at microsoft.com
>>> <mailto:Bret.Barkelew at microsoft.com>>; michael.kubacki at microsoft.com
>>> <mailto:michael.kubacki at 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 (#77320): https://edk2.groups.io/g/devel/message/77320
Mute This Topic: https://groups.io/mt/83863450/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list