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

[Michael D Kinney]

Hi Marvin,

Thank you for checking this specific structure.

I think this should be part of the evaluation when proposing the use of a
flexible array member to the UEFI/PI specs as part of the EDK II Code First
Process.  We should verify that the sizeof() and offsetof() return the same
value when using the structure in all supported compilers/CPU archs configured
for the UEFI ABI.

Thanks,

Mike

> -----Original Message-----
> From: Marvin Häuser <mhaeuser at posteo.de>
> Sent: Monday, June 28, 2021 8:43 AM
> To: 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>; Leif Lindholm <leif at nuviainc.com>; Bret Barkelew <Bret.Barkelew at microsoft.com>;
> michael.kubacki at microsoft.com
> Subject: 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>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqin12 at gmail.com>; Laszlo Ersek <lersek at redhat.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>; Kinney, Michael D <michael.d.kinney at intel.com>;
> >>>> Liming Gao <gaoliming at byosoft.com.cn>; Liu, Zhiguang
> >>>> <zhiguang.liu at intel.com>; Andrew Fish <afish at apple.com>; Leif
> >>>> Lindholm <leif at nuviainc.com>; Bret Barkelew
> >>>> <Bret.Barkelew at microsoft.com>; 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 (#77296): https://edk2.groups.io/g/devel/message/77296
Mute This Topic: https://groups.io/mt/83435921/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-