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

["Kun Qin" <kuqin12@gmail.com>]

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