[PATCH V2 2/3] MdePkg/Base.h: Introduce various alignment-related macros


Marvin Häuser <mhaeuser@...>
 

Considering all my branches are badly broken because of the Uncrustify changes, there have been many changes to the existing PE/COFF libraries in the mean time (and more external dependencies incoming with PMR), and even this non-invasive patch has been mostly ignored (thanks for the review, Ray!), can I now safely assume there is no (and frankly never was) any actual interest in the reworked PE/COFF library? Things are at a point where I cannot update all my changes to latest master on my own, and if nobody provides any active support, you may best mark all related BZs as WONTFIX to reflect the roadmap.

Best regards,
Marvin

On 23.11.21 11:12, Marvin Häuser wrote:
Ping? :)

On 16.08.21 15:10, Marvin Häuser wrote:
Hey Ray,

On 16/08/2021 11:42, Ni, Ray wrote:
Marvin,
So lucky to have you in the edk2 project looking into these fundamentals!
Thank you. :)

+  #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)

1. Does struct{} inside a macro conform to C standard? How is the compatibility with different compilers?
This should work, yes. The C standard defines offsetof as such:

"The macros are [...]

        offsetof(type, member-designator)

which expands to an integer constant expression that has type size_t, the value of
which is the offset in bytes, to the structure member (designated by member-designator),
from the beginning of its structure (designated by type). The type and member designator
shall be such that given

        static type t;

then the expression &(t.member-designator) evaluates to an address constant. [...]" [1]

If we plug in t:

        static struct { CHAR8 C; TYPE A; } t;

we get a valid static storage duration variable declaration that satisfies the the last condition because:

"An address constant is [...], a pointer to an lvalue designating an object of static
storage duration, or [...]" [2]

It worked with all compilers I tinkered with at https://godbolt.org/
I sadly do not have access to any of the compilers where this may be used effectively (RVCT, EBC).

+#define IS_POW2(Value)  ((Value) != 0U && ((Value) & ((Value) - 1U)) ==
+0U)

2. Good to me. I learned this trick when implementing the MtrrLib.

+#define ALIGN_VALUE_ADDEND(Value, Alignment)  (((Alignment) - (Value))
+& ((Alignment) - 1U))

3. Is any other open source project using the same macro for the addend?
This is actually a general question to all new macros.
I would like the macros look familiar to developers from other open source projects.
Good question, I never really saw it. I only came up with it because for the new PE loader, we may align the PE memory within an underaligned buffer, and for that we need the addend. I initially used to align up and then subtract, but I saw this could be simplified with ALIGN_VALUE_ADDEND, which was used in ALIGN_VALUE anyway. If you have a better name, I'll change it.

Best regards,
Marvin


[1] ISO/IEC 9899:2011, 7.19, 3.

[2] ISO/IEC 9899:2011, 6.6, 9.









mjsbeaton@...
 

Can I tentatively suggest ALIGN_VALUE_OFFSET as a better name? (Speaking as a native English speaker, and with a relatively high level of command including technical language, addend is not at all a common word!)

While I'm here additional ping and +1 for this to be merged, pls!


Marvin Häuser <mhaeuser@...>
 

Hey Mike,

Thanks! I agree using "offset" may make it more readable, but I haven't seen it being used much outside of memory terminology (the macro also applies to plain integers). Any feedback from the maintainers for preferences? Thanks!

Best regards,
Marvin

On 08.12.21 10:10, mjsbeaton@gmail.com wrote:
Can I tentatively suggest ALIGN_VALUE_OFFSET as a better name? (Speaking as a native English speaker, and with a relatively high level of command including technical language, addend is not at all a common word!) While I'm here additional ping and +1 for this to be merged, pls!


Marvin Häuser <mhaeuser@...>
 

Ping? :)

On 16.08.21 15:10, Marvin Häuser wrote:
Hey Ray,

On 16/08/2021 11:42, Ni, Ray wrote:
Marvin,
So lucky to have you in the edk2 project looking into these fundamentals!
Thank you. :)

+  #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)

1. Does struct{} inside a macro conform to C standard? How is the compatibility with different compilers?
This should work, yes. The C standard defines offsetof as such:

"The macros are [...]

        offsetof(type, member-designator)

which expands to an integer constant expression that has type size_t, the value of
which is the offset in bytes, to the structure member (designated by member-designator),
from the beginning of its structure (designated by type). The type and member designator
shall be such that given

        static type t;

then the expression &(t.member-designator) evaluates to an address constant. [...]" [1]

If we plug in t:

        static struct { CHAR8 C; TYPE A; } t;

we get a valid static storage duration variable declaration that satisfies the the last condition because:

"An address constant is [...], a pointer to an lvalue designating an object of static
storage duration, or [...]" [2]

It worked with all compilers I tinkered with at https://godbolt.org/
I sadly do not have access to any of the compilers where this may be used effectively (RVCT, EBC).

+#define IS_POW2(Value)  ((Value) != 0U && ((Value) & ((Value) - 1U)) ==
+0U)

2. Good to me. I learned this trick when implementing the MtrrLib.

+#define ALIGN_VALUE_ADDEND(Value, Alignment)  (((Alignment) - (Value))
+& ((Alignment) - 1U))

3. Is any other open source project using the same macro for the addend?
This is actually a general question to all new macros.
I would like the macros look familiar to developers from other open source projects.
Good question, I never really saw it. I only came up with it because for the new PE loader, we may align the PE memory within an underaligned buffer, and for that we need the addend. I initially used to align up and then subtract, but I saw this could be simplified with ALIGN_VALUE_ADDEND, which was used in ALIGN_VALUE anyway. If you have a better name, I'll change it.

Best regards,
Marvin


[1] ISO/IEC 9899:2011, 7.19, 3.

[2] ISO/IEC 9899:2011, 6.6, 9.





Ni, Ray
 

I don't have better names.

Reviewed-by: Ray Ni <ray.ni@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Marvin Häuser
Sent: Monday, August 16, 2021 9:10 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
<zhiguang.liu@intel.com>; Vitaly Cheptsov <vit9696@protonmail.com>
Subject: Re: [edk2-devel] [PATCH V2 2/3] MdePkg/Base.h: Introduce various alignment-related macros

Hey Ray,

On 16/08/2021 11:42, Ni, Ray wrote:
Marvin,
So lucky to have you in the edk2 project looking into these fundamentals!
Thank you. :)

+ #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)

1. Does struct{} inside a macro conform to C standard? How is the compatibility with different compilers?
This should work, yes. The C standard defines offsetof as such:

"The macros are [...]

        offsetof(type, member-designator)

which expands to an integer constant expression that has type size_t,
the value of
which is the offset in bytes, to the structure member (designated by
member-designator),
from the beginning of its structure (designated by type). The type and
member designator
shall be such that given

        static type t;

then the expression &(t.member-designator) evaluates to an address
constant. [...]" [1]

If we plug in t:

        static struct { CHAR8 C; TYPE A; } t;

we get a valid static storage duration variable declaration that
satisfies the the last condition because:

"An address constant is [...], a pointer to an lvalue designating an
object of static
storage duration, or [...]" [2]

It worked with all compilers I tinkered with at https://godbolt.org/
I sadly do not have access to any of the compilers where this may be
used effectively (RVCT, EBC).

+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) ==
+0U)

2. Good to me. I learned this trick when implementing the MtrrLib.

+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value))
+& ((Alignment) - 1U))

3. Is any other open source project using the same macro for the addend?
This is actually a general question to all new macros.
I would like the macros look familiar to developers from other open source projects.
Good question, I never really saw it. I only came up with it because for
the new PE loader, we may align the PE memory within an underaligned
buffer, and for that we need the addend. I initially used to align up
and then subtract, but I saw this could be simplified with
ALIGN_VALUE_ADDEND, which was used in ALIGN_VALUE anyway. If you have a
better name, I'll change it.

Best regards,
Marvin


[1] ISO/IEC 9899:2011, 7.19, 3.

[2] ISO/IEC 9899:2011, 6.6, 9.









Marvin Häuser <mhaeuser@...>
 

Hey Ray,

On 16/08/2021 11:42, Ni, Ray wrote:
Marvin,
So lucky to have you in the edk2 project looking into these fundamentals!
Thank you. :)

+ #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)

1. Does struct{} inside a macro conform to C standard? How is the compatibility with different compilers?
This should work, yes. The C standard defines offsetof as such:

"The macros are [...]

        offsetof(type, member-designator)

which expands to an integer constant expression that has type size_t, the value of
which is the offset in bytes, to the structure member (designated by member-designator),
from the beginning of its structure (designated by type). The type and member designator
shall be such that given

        static type t;

then the expression &(t.member-designator) evaluates to an address constant. [...]" [1]

If we plug in t:

        static struct { CHAR8 C; TYPE A; } t;

we get a valid static storage duration variable declaration that satisfies the the last condition because:

"An address constant is [...], a pointer to an lvalue designating an object of static
storage duration, or [...]" [2]

It worked with all compilers I tinkered with at https://godbolt.org/
I sadly do not have access to any of the compilers where this may be used effectively (RVCT, EBC).

+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) ==
+0U)

2. Good to me. I learned this trick when implementing the MtrrLib.

+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value))
+& ((Alignment) - 1U))

3. Is any other open source project using the same macro for the addend?
This is actually a general question to all new macros.
I would like the macros look familiar to developers from other open source projects.
Good question, I never really saw it. I only came up with it because for the new PE loader, we may align the PE memory within an underaligned buffer, and for that we need the addend. I initially used to align up and then subtract, but I saw this could be simplified with ALIGN_VALUE_ADDEND, which was used in ALIGN_VALUE anyway. If you have a better name, I'll change it.

Best regards,
Marvin


[1] ISO/IEC 9899:2011, 7.19, 3.

[2] ISO/IEC 9899:2011, 6.6, 9.





Ni, Ray
 

Marvin,
So lucky to have you in the edk2 project looking into these fundamentals!

+ #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)

1. Does struct{} inside a macro conform to C standard? How is the compatibility with different compilers?

+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) ==
+0U)

2. Good to me. I learned this trick when implementing the MtrrLib.

+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value))
+& ((Alignment) - 1U))

3. Is any other open source project using the same macro for the addend?
This is actually a general question to all new macros.
I would like the macros look familiar to developers from other open source projects.


Marvin Häuser <mhaeuser@...>
 

ALIGNOF: Determining the alignment requirement of data types is
crucial to ensure safe memory accesses when parsing untrusted data.

IS_POW2: Determining whether a value is a power of two is important
to verify whether untrusted values are valid alignment values.

IS_ALIGNED: In combination with ALIGNOF data offsets can be verified.
A more general version of the IS_ALIGNED macro previously defined by several modules.

ADDRESS_IS_ALIGNED: Variant of IS_ALIGNED for pointers and addresses.
Replaces module-specific definitions throughout the codebase.

ALIGN_VALUE_ADDEND: The addend to align up can be used to directly
determine the required offset for data alignment.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Vitaly Cheptsov <vit9696@protonmail.com>
Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
---
MdePkg/Include/Base.h | 90 +++++++++++++++++++-
1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 2da08b0c787f..32d0e512e05f 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -789,6 +789,35 @@ typedef UINTN *BASE_LIST;
#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
#endif

+/**
+ Returns the alignment requirement of a type.
+
+ @param TYPE The name of the type to retrieve the alignment requirement of.
+
+ @return Alignment requirement, in Bytes, of TYPE.
+**/
+#if defined(__GNUC__) || defined(__clang__) || (defined(_MSC_VER) && _MSC_VER >= 1900)
+ //
+ // All supported versions of GCC and Clang, as well as MSVC 2015 and later,
+ // support the standard operator _Alignof.
+ //
+ #define ALIGNOF(TYPE) _Alignof (TYPE)
+#elif defined(_MSC_VER)
+ //
+ // Earlier versions of MSVC, at least MSVC 2008 and later, support the
+ // vendor-extension __alignof.
+ //
+ #define ALIGNOF(TYPE) __alignof (TYPE)
+#else
+ //
+ // For compilers that do not support inbuilt alignof operators, use OFFSET_OF.
+ // CHAR8 is known to have both a size and an alignment requirement of 1 Byte.
+ // As such, A must be located exactly at the offset equal to its alignment
+ // requirement.
+ //
+ #define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)
+#endif
+
/**
Portable definition for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.
@@ -824,6 +853,21 @@ STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specif
STATIC_ASSERT (sizeof (L'A') == 2, "sizeof (L'A') does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (L"A") == 4, "sizeof (L\"A\") does not meet UEFI Specification Data Type requirements");

+STATIC_ASSERT (ALIGNOF (BOOLEAN) == sizeof (BOOLEAN), "Alignment of BOOLEAN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT8) == sizeof (INT8), "Alignment of INT8 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT8) == sizeof (UINT8), "Alignment of INT16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT16) == sizeof (INT16), "Alignment of INT16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT16) == sizeof (UINT16), "Alignment of UINT16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT32) == sizeof (INT32), "Alignment of INT32 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT32) == sizeof (UINT32), "Alignment of UINT32 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INT64) == sizeof (INT64), "Alignment of INT64 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT64) == sizeof (UINT64), "Alignment of UINT64 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (CHAR8) == sizeof (CHAR8), "Alignment of CHAR8 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (CHAR16) == sizeof (CHAR16), "Alignment of CHAR16 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (INTN) == sizeof (INTN), "Alignment of INTN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINTN) == sizeof (UINTN), "Alignment of UINTN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (VOID *) == sizeof (VOID *), "Alignment of VOID * does not meet UEFI Specification Data Type requirements");
+
//
// 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
@@ -847,6 +891,10 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not me
STATIC_ASSERT (sizeof (__VERIFY_UINT16_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");

+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT8_ENUM_SIZE) == sizeof (__VERIFY_UINT8_ENUM_SIZE), "Alignment of enum does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT16_ENUM_SIZE) == sizeof (__VERIFY_UINT16_ENUM_SIZE), "Alignment of enum does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT32_ENUM_SIZE) == sizeof (__VERIFY_UINT32_ENUM_SIZE), "Alignment of enum does not meet UEFI Specification Data Type requirements");
+
/**
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
@@ -868,6 +916,46 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
**/
#define BASE_CR(Record, TYPE, Field) ((TYPE *) ((CHAR8 *) (Record) - OFFSET_OF (TYPE, Field)))

+/**
+ Checks whether a value is a power of two.
+
+ @param Value The value to check.
+
+ @return Whether Value is a power of two.
+**/
+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) == 0U)
+
+/**
+ Checks whether a value is aligned by a specified alignment.
+
+ @param Value The value to check.
+ @param Alignment The alignment boundary used to check against.
+
+ @return Whether Value is aligned by Alignment.
+**/
+#define IS_ALIGNED(Value, Alignment) (((Value) & ((Alignment) - 1U)) == 0U)
+
+/**
+ Checks whether a pointer or address is aligned by a specified alignment.
+
+ @param Address The pointer or address to check.
+ @param Alignment The alignment boundary used to check against.
+
+ @return Whether Address is aligned by Alignment.
+**/
+#define ADDRESS_IS_ALIGNED(Address, Alignment) IS_ALIGNED ((UINTN) (Address), Alignment)
+
+/**
+ Determines the addend to add to a value to round it up to the next boundary of
+ a specified alignment.
+
+ @param Value The value to round up.
+ @param Alignment The alignment boundary used to return the addend.
+
+ @return Addend to round Value up to alignment boundary Alignment.
+**/
+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value)) & ((Alignment) - 1U))
+
/**
Rounds a value up to the next boundary using a specified alignment.

@@ -880,7 +968,7 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
@return A value up to the next boundary.

**/
-#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1)))
+#define ALIGN_VALUE(Value, Alignment) ((Value) + ALIGN_VALUE_ADDEND (Value, Alignment))

/**
Adjust a pointer by adding the minimum offset required for it to be aligned on
--
2.31.1