1. posterid:5949180
  2. [PATCH v1 0/5] EDK2 Code First: PI Specification: Update EFI_MM_COMMUNICATE_HEADER

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

Kun Qin
 

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
Join devel@edk2.groups.io to automatically receive all group messages.