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


Kun Qin
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.

But this structure, as a generic definition, could be used for both PEI
and DXE MM communication. Thus for a system that supports PEI MM launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the current
EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due
to UINTN being used.

The suggested change is to make the MessageLength field defined with
definitive size as below:
```
typedef struct {
EFI_GUID HeaderGuid;
UINT64 MessageLength;
UINT8 Data[ANYSIZE_ARRAY];
} EFI_MM_COMMUNICATE_HEADER;
```

Patch v1 branch: https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Kun Qin (5):
EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
MmCommunicate
MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation
MdePkg: MmCommunication: Extend MessageLength field size to UINT64

MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 +++--
MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 8 +-
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 ++-
BZ3430-SpecChange.md | 88 ++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
MdePkg/Include/Protocol/MmCommunication.h | 3 +-
6 files changed, 124 insertions(+), 9 deletions(-)
create mode 100644 BZ3430-SpecChange.md

--
2.31.1.windows.1


Marvin Häuser
 

Good day,

May I ask about two small things?

1) Was there any rationale as to e.g. code compatibility with choosing UINT64 for the data length? I understand that this is the maximum of the two as of currently, but I wonder whether a message length that exceeds the UINT32 range (4 GB!) can possibly be considered sane or a good idea.

2) Is it feasible yet with the current set of supported compilers to support flexible arrays?

Thank you for your work!

Best regards,
Marvin

On 10.06.21 03:42, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.

But this structure, as a generic definition, could be used for both PEI
and DXE MM communication. Thus for a system that supports PEI MM launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the current
EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due
to UINTN being used.

The suggested change is to make the MessageLength field defined with
definitive size as below:
```
typedef struct {
EFI_GUID HeaderGuid;
UINT64 MessageLength;
UINT8 Data[ANYSIZE_ARRAY];
} EFI_MM_COMMUNICATE_HEADER;
```

Patch v1 branch: https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Kun Qin (5):
EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
MmCommunicate
MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation
MdePkg: MmCommunication: Extend MessageLength field size to UINT64

MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 +++--
MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c | 8 +-
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 ++-
BZ3430-SpecChange.md | 88 ++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf | 1 +
MdePkg/Include/Protocol/MmCommunication.h | 3 +-
6 files changed, 124 insertions(+), 9 deletions(-)
create mode 100644 BZ3430-SpecChange.md


Kun Qin
 

Hi Marvin,

Thanks for reaching out. Please see my comment inline.

Regards,
Kun

On 06/16/2021 00:02, Marvin Häuser wrote:
Good day,
May I ask about two small things?
1) Was there any rationale as to e.g. code compatibility with choosing UINT64 for the data length? I understand that this is the maximum of the two as of currently, but I wonder whether a message length that exceeds the UINT32 range (4 GB!) can possibly be considered sane or a good idea.
I agree that >4GB communication buffer may not be practical as of today. That is why we modified the SMM communication routine in PiSmmIpl to use SafeInt functions in Patch 2 of this series. With that change, at least for 32bit MM, larger than 4GB will be considered insane. But in the meantime, I do not want to rule out the >4GB communication capability that a 64bit MM currently already have.

Please let me know if I missed anything in regards to adding SafeInt functions to SmmCommunication routine.
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.
Thank you for your work!
Best regards,
Marvin
On 10.06.21 03:42, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.

But this structure, as a generic definition, could be used for both PEI
and DXE MM communication. Thus for a system that supports PEI MM launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the current
EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due
to UINTN being used.

The suggested change is to make the MessageLength field defined with
definitive size as below:
```
typedef struct {
EFI_GUID  HeaderGuid;
UINT64    MessageLength;
UINT8     Data[ANYSIZE_ARRAY];
} EFI_MM_COMMUNICATE_HEADER;
```

Patch v1 branch: https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Kun Qin (5):
   EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
   MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
     MmCommunicate
   MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
   MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation
   MdePkg: MmCommunication: Extend MessageLength field size to UINT64

MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 +++--
MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c |  8 +-
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 ++-
BZ3430-SpecChange.md | 88 ++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
MdePkg/Include/Protocol/MmCommunication.h |  3 +-
  6 files changed, 124 insertions(+), 9 deletions(-)
  create mode 100644 BZ3430-SpecChange.md


Marvin Häuser
 

Hey Kun,

On 16.06.21 22:58, Kun Qin wrote:
Hi Marvin,

Thanks for reaching out. Please see my comment inline.

Regards,
Kun

On 06/16/2021 00:02, Marvin Häuser wrote:
Good day,

May I ask about two small things?

1) Was there any rationale as to e.g. code compatibility with choosing UINT64 for the data length? I understand that this is the maximum of the two as of currently, but I wonder whether a message length that exceeds the UINT32 range (4 GB!) can possibly be considered sane or a good idea.
I agree that >4GB communication buffer may not be practical as of today. That is why we modified the SMM communication routine in PiSmmIpl to use SafeInt functions in Patch 2 of this series. With that change, at least for 32bit MM, larger than 4GB will be considered insane. But in the meantime, I do not want to rule out the >4GB communication capability that a 64bit MM currently already have.

Please let me know if I missed anything in regards to adding SafeInt functions to SmmCommunication routine.
I didn't see anything missed, but that is part of the point. If it was UINT32, no such validation would be required. Also I feel like in high-security scenarios, you would have a cap (that is well below 4 GB I imagine) anyway past which you reject transmission for looking insane. I totally understand it's a tough choice to reduce the capabilities and to go with a capacity less than what is possible, but I do not think current transmission realistically exceed a very few megabytes? This would still allow for incredible headroom.


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 feel like if the structure is modified anyway, it should probably get a trailing flexible array over the 1-sized hack. What do you think?

Best regards,
Marvin


Thank you for your work!

Best regards,
Marvin

On 10.06.21 03:42, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3430

In PI Spec v1.7 Errata A, Vol.4, Sec 5.7 MM Communication Protocol, the
MessageLength field of EFI_MM_COMMUNICATE_HEADER (also derived as
EFI_SMM_COMMUNICATE_HEADER) is currently defined as type UINTN.

But this structure, as a generic definition, could be used for both PEI
and DXE MM communication. Thus for a system that supports PEI MM launch,
but operates PEI in 32bit mode and MM foundation in 64bit, the current
EFI_MM_COMMUNICATE_HEADER definition will cause structure parse error due
to UINTN being used.

The suggested change is to make the MessageLength field defined with
definitive size as below:
```
typedef struct {
EFI_GUID  HeaderGuid;
UINT64    MessageLength;
UINT8     Data[ANYSIZE_ARRAY];
} EFI_MM_COMMUNICATE_HEADER;
```

Patch v1 branch: https://github.com/kuqin12/edk2/tree/BZ3398-MmCommunicate-Length

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>

Kun Qin (5):
   EDK2 Code First: PI Specification: EFI_MM_COMMUNICATE_HEADER Update
   MdeModulePkg: PiSmmIpl: Update MessageLength calculation for
     MmCommunicate
   MdeModulePkg: MemoryProfileInfo: Updated MessageLength calculation
   MdeModulePkg: SmiHandlerProfileInfo: Updated MessageLength calculation
   MdePkg: MmCommunication: Extend MessageLength field size to UINT64

MdeModulePkg/Application/MemoryProfileInfo/MemoryProfileInfo.c | 20 +++--
MdeModulePkg/Application/SmiHandlerProfileInfo/SmiHandlerProfileInfo.c |  8 +-
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 ++-
BZ3430-SpecChange.md | 88 ++++++++++++++++++++
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.inf |  1 +
MdePkg/Include/Protocol/MmCommunication.h |  3 +-
  6 files changed, 124 insertions(+), 9 deletions(-)
  create mode 100644 BZ3430-SpecChange.md



Laszlo Ersek
 

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.

Thanks
Laszlo

I feel
like if the structure is modified anyway, it should probably get a
trailing flexible array over the 1-sized hack. What do you think?


Marvin Häuser
 

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.

Best regards,
Marvin


Thanks
Laszlo

I feel
like if the structure is modified anyway, it should probably get a
trailing flexible array over the 1-sized hack. What do you think?


Laszlo Ersek
 

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


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


Marvin Häuser
 

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


Michael D Kinney
 

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


Kun Qin
 

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


Laszlo Ersek
 

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.

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


Marvin Häuser
 

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


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@posteo.de>
Sent: Monday, June 28, 2021 8:43 AM
To: Laszlo Ersek <lersek@redhat.com>; Kun Qin <kuqin12@gmail.com>; Kinney, Michael D <michael.d.kinney@intel.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>; 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

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