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

["Marvin Häuser" <mhaeuser@posteo.de>]

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