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


Bret Barkelew
 

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
Sent: Monday, June 28, 2021 8:43 AM
To: Laszlo Ersek; Kun Qin; Kinney, Michael D; devel@edk2.groups.io
Cc: Wang, Jian J; Wu, Hao A; Dong, Eric; Ni, Ray; Liming Gao; Liu, Zhiguang; Andrew Fish; Lindholm, Leif; Bret Barkelew; Michael Kubacki
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@...>
>>>> Sent: Thursday, June 24, 2021 1:00 AM
>>>> To: Kun Qin <kuqin12@...>; Laszlo Ersek <lersek@...>;
>>>> devel@edk2.groups.io
>>>> Cc: Wang, Jian J <jian.j.wang@...>; Wu, Hao A
>>>> <hao.a.wu@...>; Dong, Eric <eric.dong@...>; Ni, Ray
>>>> <ray.ni@...>; Kinney, Michael D <michael.d.kinney@...>;
>>>> Liming Gao <gaoliming@...>; Liu, Zhiguang
>>>> <zhiguang.liu@...>; Andrew Fish <afish@...>; Leif
>>>> Lindholm <leif@...>; Bret Barkelew
>>>> <Bret.Barkelew@...>; michael.kubacki@...
>>>> 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
>>>>>>

 


Marvin Häuser
 

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


Bret Barkelew
 

Good note. Thanks!

 

- Bret

 

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

 


Michael D Kinney
 

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

 


Kun Qin
 

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


Michael D Kinney
 

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@gmail.com>
Sent: Tuesday, June 29, 2021 4:11 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; bret.barkelew@microsoft.com; Marvin Häuser
<mhaeuser@posteo.de>; Laszlo Ersek <lersek@redhat.com>
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

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@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

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



Kun Qin
 

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@gmail.com>
Sent: Tuesday, June 29, 2021 4:11 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; bret.barkelew@microsoft.com; Marvin Häuser
<mhaeuser@posteo.de>; Laszlo Ersek <lersek@redhat.com>
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

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@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

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