Re: [PATCH v2 1/1] MdePkg: Add STATIC_ASSERT macro


Laszlo Ersek
 

On 08/14/19 18:22, vit9696@... wrote:
Michael, Liming, Laszlo,

Static assertions via _Static_assert are standard C11 functionality, thus any at least C11 (ISO/IEC 9899 2011) conforming compiler is required to support the second argument with the diagnostic description.
The notation without the message currently is only present in C++, not in C, thus the two-argument notation is the only allowed notation for _Static_assert for at least C17 (ISO/IEC 9899 2018) and below.
In the bottom of this message I included a quote from C17 for the relevant section (6.7.10).

GCC and CLANG (including Xcode) appear to be conforming to the standard for this section, and MSVC compiler static_assert extension also supports the diagnostic message argument. This is pretty much all we care about.

As for examples, I see little reason to clarify STATIC_ASSERT behaviour outside of the standard reference in its description and actual usage in the source code, but can do that just fine if you think that it may help somebody.
Edk2 targets C95, to my understanding. If features from more recent C
language standards happen to work on all toolchains that edk2 supports,
then I agree we can put those language features to use -- but we should
document them, in the appropriate header file. In my opinion.


I fully agree that VERIFY_SIZE_OF usage should be converted to STATIC_ASSERT, and in fact I also suggest VERIFY_SIZE_OF to be entirely removed from Base.h. This should be fairly costless, as apparently it is only used in Base.h and MdeModulePkg/Library/ResetUtilityLib/ResetUtility.c, which I can replace in the same patch set.
I disagree with introducing a new macro to a core header file without
putting it to use at once, in at least one very commonly built
translation unit in edk2 itself. I would suggest to single out a few
core uses of ASSERT (e.g. in MdePkg or MdeModulePkg), and to convert them.

If you can replace VERIFY_SIZE_OF with STATIC_ASSERT, that could be a
perfect first use. Of course I'd suggest that the patches be separate --
first, add the new macro, second, gradually convert VERIFY_SIZE_OF. So
this intro work should be done as a small series.

I think that can belong to a single BZ.

As for select ASSERT usage switching to STATIC_ASSERT, this would also be great, as let us be honest, the use of ASSERT in EDK II codebase is very questioning. In fact, this was one of the reasons we introduced our own static assertions some time ago. However, fixing up all broken assertions is unlikely a best place to fit into this patchset, but I can surely add a few examples, in case somebody points them out. This will be useful for reference purposes and may help the maintainers to get a better idea when static assertions are to be used.
Wider ASSERT evaluation and conversion to STATIC_ASSERT should be done
later (separate BZs) if we ever have capacity for that.

Thanks
Laszlo



Looking forward to hearing your opinions.

Best regards,
Vitaly


6.7.10 Static assertions

Syntax
1 static_assert-declaration:
_Static_assert ( constant-expression , string-literal ) ;

Constraints
2 The constant expression shall compare unequal to 0.

Semantics
3 The constant expression shall be an integer constant expression. If the value of the constant expression compares unequal to 0, the declaration has no effect. Otherwise, the constraint is violated and the implementation shall produce a diagnostic message that includes the text of the string literal, except that characters not in the basic source character set are not required to appear in the message.
Forward references: diagnostics (7.2).

7.2 Diagnostics <assert. h>

3 The macro
static_assert
expands to _Static_assert.


14 авг. 2019 г., в 18:47, Kinney, Michael D <michael.d.kinney@...> написал(а):


Liming,

I think a good candidate to demonstrate this
feature are the checks made in MdePkg/Include/Base.h.
The current implementation forces a divide by 0
in the C pre-processor to break the build.
STATIC_ASSERT() would be a better way to do this.
I would also remove unused externs from the builds.

/**
Verifies the storage size of a given data type.

This macro generates a divide by zero error or a zero size array declaration in
the preprocessor if the size is incorrect. These are declared as "extern" so
the space for these arrays will not be in the modules.

@param TYPE The date type to determine the size of.
@param Size The expected size for the TYPE.

**/
#define VERIFY_SIZE_OF(TYPE, Size) extern UINT8 _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))]

//
// Verify that ProcessorBind.h produced UEFI Data Types that are compliant with
// Section 2.3.1 of the UEFI 2.3 Specification.
//
VERIFY_SIZE_OF (BOOLEAN, 1);
VERIFY_SIZE_OF (INT8, 1);
VERIFY_SIZE_OF (UINT8, 1);
VERIFY_SIZE_OF (INT16, 2);
VERIFY_SIZE_OF (UINT16, 2);
VERIFY_SIZE_OF (INT32, 4);
VERIFY_SIZE_OF (UINT32, 4);
VERIFY_SIZE_OF (INT64, 8);
VERIFY_SIZE_OF (UINT64, 8);
VERIFY_SIZE_OF (CHAR8, 1);
VERIFY_SIZE_OF (CHAR16, 2);

//
// The following three enum types are used to verify that the compiler
// configuration for enum types is compliant with Section 2.3.1 of the
// UEFI 2.3 Specification. These enum types and enum values are not
// intended to be used. A prefix of '__' is used avoid conflicts with
// other types.
//
typedef enum {
__VerifyUint8EnumValue = 0xff
} __VERIFY_UINT8_ENUM_SIZE;

typedef enum {
__VerifyUint16EnumValue = 0xffff
} __VERIFY_UINT16_ENUM_SIZE;

typedef enum {
__VerifyUint32EnumValue = 0xffffffff
} __VERIFY_UINT32_ENUM_SIZE;

VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4);
VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4);
VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);

A couple examples. Do all the compilers support the message parameter too?

STATIC_ASSERT (sizeof (BOOLEAN) == 1, "sizeof (BOOLEAN) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (UINT16) == 2, "sizeof (UINT16) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (INT32) == 4, "sizeof (INT32) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements")
STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements")

Thanks,

Mike

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
On Behalf Of Liming Gao
Sent: Wednesday, August 14, 2019 6:50 AM
To: devel@edk2.groups.io; vit9696@...
Subject: Re: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
STATIC_ASSERT macro

Can you add the sample usage of new macro STATIC_ASSERT?

Or, give the link of static_assert or _Static_assert.

If so, the developer knows how to use them in source
code.

Thanks
Liming
-----Original Message-----
From: devel@edk2.groups.io
[mailto:devel@edk2.groups.io] On Behalf Of
vit9696 via Groups.Io
Sent: Tuesday, August 13, 2019 4:17 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] [PATCH v2 1/1] MdePkg: Add
STATIC_ASSERT macro

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

Provide a macro for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.

Signed-off-by: Vitaly Cheptsov
<vit9696@...>
---
MdePkg/Include/Base.h | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/MdePkg/Include/Base.h
b/MdePkg/Include/Base.h index
ce20b5f01dce..f85f7028a262 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -843,6 +843,17 @@ typedef UINTN *BASE_LIST;
#define
OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
#endif

+///
+/// Portable definition for compile time assertions.
+/// Equivalent to C11 static_assert macro from
assert.h.
+/// Takes condtion and error message as its
arguments.
+///
+#ifdef _MSC_EXTENSIONS
+ #define STATIC_ASSERT static_assert #else
+ #define STATIC_ASSERT _Static_assert #endif
+
/**
Macro that returns a pointer to the data structure
that contains a specified field of
that data structure. This is a lightweight method
to hide
information by placing a
--
2.20.1 (Apple Git-117)


-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this
group.

View/Reply Online (#45503):
https://edk2.groups.io/g/devel/message/45503
Mute This Topic: https://groups.io/mt/32850582/1759384
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[liming.gao@...] -=-=-=-=-=-=

Join devel@edk2.groups.io to automatically receive all group messages.