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 <mhaeu...@posteo.de>
> Sent: Monday, June 28, 2021 8:43 AM
> To: Laszlo Ersek <ler...@redhat.com>; Kun Qin <kuqi...@gmail.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>;
> devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>;
> Dong, Eric <eric.d...@intel.com>; Ni, Ray
> <ray...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang....@intel.com>; Andrew Fish
> <af...@apple.com>; Leif Lindholm <l...@nuviainc.com>; Bret Barkelew
> <bret.barke...@microsoft.com>;
> michael.kuba...@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 <mhaeu...@posteo.de>
> >>>> Sent: Thursday, June 24, 2021 1:00 AM
> >>>> To: Kun Qin <kuqi...@gmail.com>; Laszlo Ersek <ler...@redhat.com>;
> >>>> devel@edk2.groups.io
> >>>> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
> >>>> <hao.a...@intel.com>; Dong, Eric <eric.d...@intel.com>; Ni, Ray
> >>>> <ray...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> >>>> Liming Gao <gaolim...@byosoft.com.cn>; Liu, Zhiguang
> >>>> <zhiguang....@intel.com>; Andrew Fish <af...@apple.com>; Leif
> >>>> Lindholm <l...@nuviainc.com>; Bret Barkelew
> >>>> <bret.barke...@microsoft.com>; michael.kuba...@microsoft.com
> >>>> Subject: Re: [edk2-devel] [PATCH v1 0/5] EDK2 Code First: PI
> >>>> Specification: Update EFI_MM_COMMUNICATE_HEADER
> >>>>
> >>>> Hey Kun,
> >>>>
> >>>> Why would you rely on undefined behaviours? The OFFSET_OF macro is
> >>>> well-defined for GCC and Clang as it's implemented by an intrinsic, and
> >>>> while the expression for the MSVC compiler is undefined behaviour as per
> >>>> the C standard, it is well-defined for MSVC due to their own
> >>>> implementation being identical. From my standpoint, all supported
> >>>> compilers will yield well-defined behaviour even this way. OFFSET_OF on
> >>>> flexible arrays is not UB in any case to my knowledge.
> >>>>
> >>>> However, the same way as your new suggestion, you can replace OFFSET_OF
> >>>> with sizeof. While this *can* lead to wasted space with certain
> >>>> structure layouts (e.g. when the flexible array overlays padding bytes),
> >>>> this is not the case here, and otherwise just loses you a few bytes. I
> >>>> think this comes down to preference.
> >>>>
> >>>> The pattern you mentioned arguably is less nice syntax when used
> >>>> (involves address calculation and casting), but the biggest problem here
> >>>> is alignment constraints. For packed structures, you lose the ability of
> >>>> automatic unaligned accesses (irrelevant here because the structure is
> >>>> manually padded anyway). For non-packed structures, you still need to
> >>>> ensure the alignment requirement of the trailing array data is met
> >>>> manually. With flexible array members, the compiler takes care of both
> >>>> cases automatically.
> >>>>
> >>>> Best regards,
> >>>> Marvin
> >>>>
> >>>> On 24.06.21 02:24, Kun Qin wrote:
> >>>>> Hi Marvin,
> >>>>>
> >>>>> I would prefer not to rely on undefined behaviors from different
> >>>>> compilers. Instead of using flexible arrays, is it better to remove
> >>>>> the `Data` field, pack the structure and follow
> >>>>> "VARIABLE_LOCK_ON_VAR_STATE_POLICY" pattern?
> >>>>>
> >>>>> In that case, OFFSET_OF will be forced to change to sizeof, and
> >>>>> read/write to `Data` will follow the range indicated by MessageLength.
> >>>>> But yes, that will enforce developers to update their platform level
> >>>>> implementations accordingly.
> >>>>>
> >>>>> Regards,
> >>>>> Kun
> >>>>>
> >>>>> On 06/23/2021 08:26, Laszlo Ersek wrote:
> >>>>>> On 06/23/21 08:54, Marvin Häuser wrote:
> >>>>>>> On 22.06.21 17:34, Laszlo Ersek wrote:
> >>>>>>>> On 06/18/21 11:37, Marvin Häuser wrote:
> >>>>>>>>> On 16.06.21 22:58, Kun Qin wrote:
> >>>>>>>>>> On 06/16/2021 00:02, Marvin Häuser wrote:
> >>>>>>>>>>> 2) Is it feasible yet with the current set of supported
> >>>>>>>>>>> compilers to
> >>>>>>>>>>> support flexible arrays?
> >>>>>>>>>> My impression is that flexible arrays are already supported (as
> >>>>>>>>>> seen
> >>>>>>>>>> in UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h).
> >>>>>>>>>> Please correct me if I am wrong.
> >>>>>>>>>>
> >>>>>>>>>> Would you mind letting me know why this is applicable here? We are
> >>>>>>>>>> trying to seek ideas on how to catch developer mistakes caused by
> >>>>>>>>>> this
> >>>>>>>>>> change. So any input is appreciated.
> >>>>>>>>> Huh, interesting. Last time I tried I was told about
> >>>>>>>>> incompatibilities
> >>>>>>>>> with MSVC, but I know some have been dropped since then (2005 and
> >>>>>>>>> 2008
> >>>>>>>>> if I recall correctly?), so that'd be great to allow globally.
> >>>>>>>> I too am surprised to see
> >>>>>>>> "UnitTestFrameworkPkg/PrivateInclude/UnitTestFrameworkTypes.h". The
> >>>>>>>> flexible array member is a C99 feature, and I didn't even know
> >>>>>>>> that we
> >>>>>>>> disallowed it for the sake of particular VS toolchains -- I
> >>>>>>>> thought we
> >>>>>>>> had a more general reason than just "not supported by VS versions X
> >>>>>>>> and Y".
> >>>>>>>>
> >>>>>>>> The behavior of OFFSET_OF() would be interesting -- the OFFSET_OF()
> >>>>>>>> macro definition for non-gcc / non-clang:
> >>>>>>>>
> >>>>>>>> #define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
> >>>>>>>>
> >>>>>>>> borders on undefined behavior as far as I can tell, so its
> >>>>>>>> behavior is
> >>>>>>>> totally up to the compiler. It works thus far okay on Visual
> >>>>>>>> Studio, but
> >>>>>>>> I couldn't say if it extended correctly to flexible array members.
> >>>>>>> Yes, it's UB by the standard, but this is actually how MS implements
> >>>>>>> them (or used to anyway?). I don't see why it'd cause issues with
> >>>>>>> flexible arrays, as only the start of the array is relevant (which is
> >>>>>>> constant for all instances of the structure no matter the amount of
> >>>>>>> elements actually stored). Any specific concern? If so, they could be
> >>>>>>> addressed by appropriate STATIC_ASSERTs.
> >>>>>> No specific concern; my point was that two aspects of the same "class"
> >>>>>> of undefined behavior didn't need to be consistent with each other.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Laszlo
> >>>>>>
-=-=-=-=-=-=-=-=-=-=-=-
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
