* [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. @ 2018-02-27 16:47 Marvin Häuser 2018-02-27 19:54 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-02-27 16:47 UTC (permalink / raw) To: edk2-devel@lists.01.org; +Cc: michael.d.kinney@intel.com, liming.gao@intel.com As per the C standard, bit-level operations on signed integers are either undefined or implementation-defined. Hence, mark all BIT defines and shifts as unsigned to safely allow such operations. For the SIGNATURE macros, add several casts to account for int promotions, which might be signed. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> --- MdePkg/Include/Base.h | 160 ++++++++++---------- 1 file changed, 80 insertions(+), 80 deletions(-) diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index a94182f08886..f108ed92eb0b 100644 --- a/MdePkg/Include/Base.h +++ b/MdePkg/Include/Base.h @@ -404,38 +404,38 @@ struct _LIST_ENTRY { #define MIN_INT32 (((INT32) -2147483647) - 1) #define MIN_INT64 (((INT64) -9223372036854775807LL) - 1) -#define BIT0 0x00000001 -#define BIT1 0x00000002 -#define BIT2 0x00000004 -#define BIT3 0x00000008 -#define BIT4 0x00000010 -#define BIT5 0x00000020 -#define BIT6 0x00000040 -#define BIT7 0x00000080 -#define BIT8 0x00000100 -#define BIT9 0x00000200 -#define BIT10 0x00000400 -#define BIT11 0x00000800 -#define BIT12 0x00001000 -#define BIT13 0x00002000 -#define BIT14 0x00004000 -#define BIT15 0x00008000 -#define BIT16 0x00010000 -#define BIT17 0x00020000 -#define BIT18 0x00040000 -#define BIT19 0x00080000 -#define BIT20 0x00100000 -#define BIT21 0x00200000 -#define BIT22 0x00400000 -#define BIT23 0x00800000 -#define BIT24 0x01000000 -#define BIT25 0x02000000 -#define BIT26 0x04000000 -#define BIT27 0x08000000 -#define BIT28 0x10000000 -#define BIT29 0x20000000 -#define BIT30 0x40000000 -#define BIT31 0x80000000 +#define BIT0 0x00000001U +#define BIT1 0x00000002U +#define BIT2 0x00000004U +#define BIT3 0x00000008U +#define BIT4 0x00000010U +#define BIT5 0x00000020U +#define BIT6 0x00000040U +#define BIT7 0x00000080U +#define BIT8 0x00000100U +#define BIT9 0x00000200U +#define BIT10 0x00000400U +#define BIT11 0x00000800U +#define BIT12 0x00001000U +#define BIT13 0x00002000U +#define BIT14 0x00004000U +#define BIT15 0x00008000U +#define BIT16 0x00010000U +#define BIT17 0x00020000U +#define BIT18 0x00040000U +#define BIT19 0x00080000U +#define BIT20 0x00100000U +#define BIT21 0x00200000U +#define BIT22 0x00400000U +#define BIT23 0x00800000U +#define BIT24 0x01000000U +#define BIT25 0x02000000U +#define BIT26 0x04000000U +#define BIT27 0x08000000U +#define BIT28 0x10000000U +#define BIT29 0x20000000U +#define BIT30 0x40000000U +#define BIT31 0x80000000U #define BIT32 0x0000000100000000ULL #define BIT33 0x0000000200000000ULL #define BIT34 0x0000000400000000ULL @@ -469,28 +469,28 @@ struct _LIST_ENTRY { #define BIT62 0x4000000000000000ULL #define BIT63 0x8000000000000000ULL -#define SIZE_1KB 0x00000400 -#define SIZE_2KB 0x00000800 -#define SIZE_4KB 0x00001000 -#define SIZE_8KB 0x00002000 -#define SIZE_16KB 0x00004000 -#define SIZE_32KB 0x00008000 -#define SIZE_64KB 0x00010000 -#define SIZE_128KB 0x00020000 -#define SIZE_256KB 0x00040000 -#define SIZE_512KB 0x00080000 -#define SIZE_1MB 0x00100000 -#define SIZE_2MB 0x00200000 -#define SIZE_4MB 0x00400000 -#define SIZE_8MB 0x00800000 -#define SIZE_16MB 0x01000000 -#define SIZE_32MB 0x02000000 -#define SIZE_64MB 0x04000000 -#define SIZE_128MB 0x08000000 -#define SIZE_256MB 0x10000000 -#define SIZE_512MB 0x20000000 -#define SIZE_1GB 0x40000000 -#define SIZE_2GB 0x80000000 +#define SIZE_1KB 0x00000400U +#define SIZE_2KB 0x00000800U +#define SIZE_4KB 0x00001000U +#define SIZE_8KB 0x00002000U +#define SIZE_16KB 0x00004000U +#define SIZE_32KB 0x00008000U +#define SIZE_64KB 0x00010000U +#define SIZE_128KB 0x00020000U +#define SIZE_256KB 0x00040000U +#define SIZE_512KB 0x00080000U +#define SIZE_1MB 0x00100000U +#define SIZE_2MB 0x00200000U +#define SIZE_4MB 0x00400000U +#define SIZE_8MB 0x00800000U +#define SIZE_16MB 0x01000000U +#define SIZE_32MB 0x02000000U +#define SIZE_64MB 0x04000000U +#define SIZE_128MB 0x08000000U +#define SIZE_256MB 0x10000000U +#define SIZE_512MB 0x20000000U +#define SIZE_1GB 0x40000000U +#define SIZE_2GB 0x80000000U #define SIZE_4GB 0x0000000100000000ULL #define SIZE_8GB 0x0000000200000000ULL #define SIZE_16GB 0x0000000400000000ULL @@ -524,28 +524,28 @@ struct _LIST_ENTRY { #define SIZE_4EB 0x4000000000000000ULL #define SIZE_8EB 0x8000000000000000ULL -#define BASE_1KB 0x00000400 -#define BASE_2KB 0x00000800 -#define BASE_4KB 0x00001000 -#define BASE_8KB 0x00002000 -#define BASE_16KB 0x00004000 -#define BASE_32KB 0x00008000 -#define BASE_64KB 0x00010000 -#define BASE_128KB 0x00020000 -#define BASE_256KB 0x00040000 -#define BASE_512KB 0x00080000 -#define BASE_1MB 0x00100000 -#define BASE_2MB 0x00200000 -#define BASE_4MB 0x00400000 -#define BASE_8MB 0x00800000 -#define BASE_16MB 0x01000000 -#define BASE_32MB 0x02000000 -#define BASE_64MB 0x04000000 -#define BASE_128MB 0x08000000 -#define BASE_256MB 0x10000000 -#define BASE_512MB 0x20000000 -#define BASE_1GB 0x40000000 -#define BASE_2GB 0x80000000 +#define BASE_1KB 0x00000400U +#define BASE_2KB 0x00000800U +#define BASE_4KB 0x00001000U +#define BASE_8KB 0x00002000U +#define BASE_16KB 0x00004000U +#define BASE_32KB 0x00008000U +#define BASE_64KB 0x00010000U +#define BASE_128KB 0x00020000U +#define BASE_256KB 0x00040000U +#define BASE_512KB 0x00080000U +#define BASE_1MB 0x00100000U +#define BASE_2MB 0x00200000U +#define BASE_4MB 0x00400000U +#define BASE_8MB 0x00800000U +#define BASE_16MB 0x01000000U +#define BASE_32MB 0x02000000U +#define BASE_64MB 0x04000000U +#define BASE_128MB 0x08000000U +#define BASE_256MB 0x10000000U +#define BASE_512MB 0x20000000U +#define BASE_1GB 0x40000000U +#define BASE_2GB 0x80000000U #define BASE_4GB 0x0000000100000000ULL #define BASE_8GB 0x0000000200000000ULL #define BASE_16GB 0x0000000400000000ULL @@ -974,7 +974,7 @@ typedef UINTN RETURN_STATUS; @return The value specified by StatusCode with the highest bit set. **/ -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | (StatusCode))) +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | (StatusCode##ULL))) /** Produces a RETURN_STATUS code with the highest bit clear. @@ -1221,7 +1221,7 @@ typedef UINTN RETURN_STATUS; @return A 16-bit value built from the two ASCII characters specified by A and B. **/ -#define SIGNATURE_16(A, B) ((A) | (B << 8)) +#define SIGNATURE_16(A, B) ((UINT16)(A) | (UINT16)((UINT16)(B) << 8U)) /** Returns a 32-bit signature built from 4 ASCII characters. @@ -1238,7 +1238,7 @@ typedef UINTN RETURN_STATUS; C and D. **/ -#define SIGNATURE_32(A, B, C, D) (SIGNATURE_16 (A, B) | (SIGNATURE_16 (C, D) << 16)) +#define SIGNATURE_32(A, B, C, D) ((UINT32)SIGNATURE_16 (A, B) | (UINT32)((UINT32)SIGNATURE_16 (C, D) << 16U)) /** Returns a 64-bit signature built from 8 ASCII characters. @@ -1260,7 +1260,7 @@ typedef UINTN RETURN_STATUS; **/ #define SIGNATURE_64(A, B, C, D, E, F, G, H) \ - (SIGNATURE_32 (A, B, C, D) | ((UINT64) (SIGNATURE_32 (E, F, G, H)) << 32)) + ((UINT64)SIGNATURE_32 (A, B, C, D) | ((UINT64) ((UINT64)SIGNATURE_32 (E, F, G, H)) << 32U)) #if defined(_MSC_EXTENSIONS) && !defined (__INTEL_COMPILER) && !defined (MDE_CPU_EBC) void * _ReturnAddress(void); -- 2.16.0.windows.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-27 16:47 [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations Marvin Häuser @ 2018-02-27 19:54 ` Laszlo Ersek 2018-02-27 20:31 ` Marvin Häuser 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2018-02-27 19:54 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: michael.d.kinney@intel.com, liming.gao@intel.com On 02/27/18 17:47, Marvin Häuser wrote: > As per the C standard, bit-level operations on signed integers are > either undefined or implementation-defined. Hence, mark all BIT > defines and shifts as unsigned to safely allow such operations. Sigh, this is why threading is important in patch sets. :/ This patch comes first, and it sort of invalidates my review for patch #2 in this series. I think this change is a good idea *in theory* (arguably the macros should have been introduced like this in the first place). However, at this point, quite a bit of code could silently see its meaning changed. Let me give you a pathologic example (it *does* occur in the wild, although maybe not in edk2): { UINT64 MyMask; MyMask = ~BIT0; } Before your patch, MyMask is assigned the value 0xFFFF_FFFF_FFFF_FFFE. This is because: - BIT0 has type "int" and value 1 - Due to our (implementation-defined) representation of negative integers, namely two's complement, ~BIT0 (also having type "int") stands for value (-2). - When this value is converted to UINT64 ("unsigned long long"), we get 18446744073709551615 + 1 + (-2) == 18446744073709551614. That's 0xFFFF_FFFF_FFFF_FFFE, which is the mask value that we intended. (An alternative way people sometimes put this is that ~BIT0 is "sign extended". I strongly dislike this term when speaking about C. It is appropriate for (x86?) assembly language, but not for C, in my opinion.) In the above example, the "clever" programmer consciously exploited the two's complement representation for the bit-neg, and the conversion of the negative value to "unsigned long long" (as dictated by the standard). Now, if you change BIT0 to have type "unsigned int" (aka UINT32), then ~BIT0 will have value 4294967294 (0xFFFF_FFFE), and type "unsigned int". When this ~BIT0 is converted to UINT64, the value will be preserved, and MyMask will end up with 0x0000_0000_FFFF_FFFE. Likely not the mask that the clever programmer intended. (An alternative way to state this is that ~BIT0 is "zero extended". Again, I think this is inappropriate for C language discussion; it's fine for (x86?) assembly.) My point is that such innocent-looking (and otherwise really good!) changes for *central* macros require a large audit effort. More comments below: > For the SIGNATURE macros, add several casts to account for int > promotions, which might be signed. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > --- > MdePkg/Include/Base.h | 160 ++++++++++---------- > 1 file changed, 80 insertions(+), 80 deletions(-) > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h > index a94182f08886..f108ed92eb0b 100644 > --- a/MdePkg/Include/Base.h > +++ b/MdePkg/Include/Base.h > @@ -404,38 +404,38 @@ struct _LIST_ENTRY { > #define MIN_INT32 (((INT32) -2147483647) - 1) > #define MIN_INT64 (((INT64) -9223372036854775807LL) - 1) > > -#define BIT0 0x00000001 > -#define BIT1 0x00000002 > -#define BIT2 0x00000004 > -#define BIT3 0x00000008 > -#define BIT4 0x00000010 > -#define BIT5 0x00000020 > -#define BIT6 0x00000040 > -#define BIT7 0x00000080 > -#define BIT8 0x00000100 > -#define BIT9 0x00000200 > -#define BIT10 0x00000400 > -#define BIT11 0x00000800 > -#define BIT12 0x00001000 > -#define BIT13 0x00002000 > -#define BIT14 0x00004000 > -#define BIT15 0x00008000 > -#define BIT16 0x00010000 > -#define BIT17 0x00020000 > -#define BIT18 0x00040000 > -#define BIT19 0x00080000 > -#define BIT20 0x00100000 > -#define BIT21 0x00200000 > -#define BIT22 0x00400000 > -#define BIT23 0x00800000 > -#define BIT24 0x01000000 > -#define BIT25 0x02000000 > -#define BIT26 0x04000000 > -#define BIT27 0x08000000 > -#define BIT28 0x10000000 > -#define BIT29 0x20000000 > -#define BIT30 0x40000000 > -#define BIT31 0x80000000 > +#define BIT0 0x00000001U > +#define BIT1 0x00000002U > +#define BIT2 0x00000004U > +#define BIT3 0x00000008U > +#define BIT4 0x00000010U > +#define BIT5 0x00000020U > +#define BIT6 0x00000040U > +#define BIT7 0x00000080U > +#define BIT8 0x00000100U > +#define BIT9 0x00000200U > +#define BIT10 0x00000400U > +#define BIT11 0x00000800U > +#define BIT12 0x00001000U > +#define BIT13 0x00002000U > +#define BIT14 0x00004000U > +#define BIT15 0x00008000U > +#define BIT16 0x00010000U > +#define BIT17 0x00020000U > +#define BIT18 0x00040000U > +#define BIT19 0x00080000U > +#define BIT20 0x00100000U > +#define BIT21 0x00200000U > +#define BIT22 0x00400000U > +#define BIT23 0x00800000U > +#define BIT24 0x01000000U > +#define BIT25 0x02000000U > +#define BIT26 0x04000000U > +#define BIT27 0x08000000U > +#define BIT28 0x10000000U > +#define BIT29 0x20000000U > +#define BIT30 0x40000000U > +#define BIT31 0x80000000U > #define BIT32 0x0000000100000000ULL > #define BIT33 0x0000000200000000ULL > #define BIT34 0x0000000400000000ULL > @@ -469,28 +469,28 @@ struct _LIST_ENTRY { > #define BIT62 0x4000000000000000ULL > #define BIT63 0x8000000000000000ULL > > -#define SIZE_1KB 0x00000400 > -#define SIZE_2KB 0x00000800 > -#define SIZE_4KB 0x00001000 > -#define SIZE_8KB 0x00002000 > -#define SIZE_16KB 0x00004000 > -#define SIZE_32KB 0x00008000 > -#define SIZE_64KB 0x00010000 > -#define SIZE_128KB 0x00020000 > -#define SIZE_256KB 0x00040000 > -#define SIZE_512KB 0x00080000 > -#define SIZE_1MB 0x00100000 > -#define SIZE_2MB 0x00200000 > -#define SIZE_4MB 0x00400000 > -#define SIZE_8MB 0x00800000 > -#define SIZE_16MB 0x01000000 > -#define SIZE_32MB 0x02000000 > -#define SIZE_64MB 0x04000000 > -#define SIZE_128MB 0x08000000 > -#define SIZE_256MB 0x10000000 > -#define SIZE_512MB 0x20000000 > -#define SIZE_1GB 0x40000000 > -#define SIZE_2GB 0x80000000 > +#define SIZE_1KB 0x00000400U > +#define SIZE_2KB 0x00000800U > +#define SIZE_4KB 0x00001000U > +#define SIZE_8KB 0x00002000U > +#define SIZE_16KB 0x00004000U > +#define SIZE_32KB 0x00008000U > +#define SIZE_64KB 0x00010000U > +#define SIZE_128KB 0x00020000U > +#define SIZE_256KB 0x00040000U > +#define SIZE_512KB 0x00080000U > +#define SIZE_1MB 0x00100000U > +#define SIZE_2MB 0x00200000U > +#define SIZE_4MB 0x00400000U > +#define SIZE_8MB 0x00800000U > +#define SIZE_16MB 0x01000000U > +#define SIZE_32MB 0x02000000U > +#define SIZE_64MB 0x04000000U > +#define SIZE_128MB 0x08000000U > +#define SIZE_256MB 0x10000000U > +#define SIZE_512MB 0x20000000U > +#define SIZE_1GB 0x40000000U > +#define SIZE_2GB 0x80000000U > #define SIZE_4GB 0x0000000100000000ULL > #define SIZE_8GB 0x0000000200000000ULL > #define SIZE_16GB 0x0000000400000000ULL > @@ -524,28 +524,28 @@ struct _LIST_ENTRY { > #define SIZE_4EB 0x4000000000000000ULL > #define SIZE_8EB 0x8000000000000000ULL > > -#define BASE_1KB 0x00000400 > -#define BASE_2KB 0x00000800 > -#define BASE_4KB 0x00001000 > -#define BASE_8KB 0x00002000 > -#define BASE_16KB 0x00004000 > -#define BASE_32KB 0x00008000 > -#define BASE_64KB 0x00010000 > -#define BASE_128KB 0x00020000 > -#define BASE_256KB 0x00040000 > -#define BASE_512KB 0x00080000 > -#define BASE_1MB 0x00100000 > -#define BASE_2MB 0x00200000 > -#define BASE_4MB 0x00400000 > -#define BASE_8MB 0x00800000 > -#define BASE_16MB 0x01000000 > -#define BASE_32MB 0x02000000 > -#define BASE_64MB 0x04000000 > -#define BASE_128MB 0x08000000 > -#define BASE_256MB 0x10000000 > -#define BASE_512MB 0x20000000 > -#define BASE_1GB 0x40000000 > -#define BASE_2GB 0x80000000 > +#define BASE_1KB 0x00000400U > +#define BASE_2KB 0x00000800U > +#define BASE_4KB 0x00001000U > +#define BASE_8KB 0x00002000U > +#define BASE_16KB 0x00004000U > +#define BASE_32KB 0x00008000U > +#define BASE_64KB 0x00010000U > +#define BASE_128KB 0x00020000U > +#define BASE_256KB 0x00040000U > +#define BASE_512KB 0x00080000U > +#define BASE_1MB 0x00100000U > +#define BASE_2MB 0x00200000U > +#define BASE_4MB 0x00400000U > +#define BASE_8MB 0x00800000U > +#define BASE_16MB 0x01000000U > +#define BASE_32MB 0x02000000U > +#define BASE_64MB 0x04000000U > +#define BASE_128MB 0x08000000U > +#define BASE_256MB 0x10000000U > +#define BASE_512MB 0x20000000U > +#define BASE_1GB 0x40000000U > +#define BASE_2GB 0x80000000U > #define BASE_4GB 0x0000000100000000ULL > #define BASE_8GB 0x0000000200000000ULL > #define BASE_16GB 0x0000000400000000ULL > @@ -974,7 +974,7 @@ typedef UINTN RETURN_STATUS; > @return The value specified by StatusCode with the highest bit set. > > **/ > -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | (StatusCode))) > +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | (StatusCode##ULL))) > MAX_BIT always has type UINTN (UINT64 aka "unsigned long long" on 64-bit platforms, and UINT32 aka "unsigned int" on 32-bit platforms). This means that ENCODE_ERROR results in a UINTN, right now. After the change, it would always result in a UINT64. I think that could be a problem for 32-bit platforms, especially if you push open-coded RETURN_xxx status codes to the stack, for example for printing with "%r". Pushing a UINT64 arg instead of a UINT32 arg through the ellipsis (...) might throw off the rest of the argument processing. So, I wouldn't recommend this change. What is the exact motivation for it? > /** > Produces a RETURN_STATUS code with the highest bit clear. > @@ -1221,7 +1221,7 @@ typedef UINTN RETURN_STATUS; > @return A 16-bit value built from the two ASCII characters specified by A and B. > > **/ > -#define SIGNATURE_16(A, B) ((A) | (B << 8)) > +#define SIGNATURE_16(A, B) ((UINT16)(A) | (UINT16)((UINT16)(B) << 8U)) This might not have the intended effect. Due to "int" (INT32) being capable of representing the entire domain of "unsigned short" (UINT16), both operands of the bitwise OR operator would be promoted to "int". The result would have type "int". Then, even if you cast the entire expression to UINT16 in the replacement text, it would still be promoted back to "int" in almost every context. (One exception would be the operand of the "sizeof" operator -- the expression operand of the "sizeof" operator is only evaluated if it has variable length array (VLA) type). I don't think this change is helpful. > > /** > Returns a 32-bit signature built from 4 ASCII characters. > @@ -1238,7 +1238,7 @@ typedef UINTN RETURN_STATUS; > C and D. > > **/ > -#define SIGNATURE_32(A, B, C, D) (SIGNATURE_16 (A, B) | (SIGNATURE_16 (C, D) << 16)) > +#define SIGNATURE_32(A, B, C, D) ((UINT32)SIGNATURE_16 (A, B) | (UINT32)((UINT32)SIGNATURE_16 (C, D) << 16U)) This looks better; the original sub-expression SIGNATURE_16 (C, D) << 16 invokes undefined behavior *if* (SIGNATURE_16 (C, D)) has bit#15 set. In practice that should never happen though: the documentation says that the macro arguments have to be "ASCII characters", and bit#7 is clear in those. Anyway, if we want to be pedantic, the fix can be expressed more simply, like this: (SIGNATURE_16 (A, B) | ((UINT32)SIGNATURE_16 (C, D) << 16)) ^^^^^^^^ Then the UINT32 type ("unsigned int") will "cascade" through the entire expression -- (SIGNATURE_16 (A, B)) will be converted from "int" to "unsigned int" for the bitwise OR, and the result will also have type UINT32: (INT32_VAL | (UINT32)INT32_VAL << 16) (INT32_VAL | UINT32_VAL) (UINT32_VAL | UINT32_VAL) (UINT32_VAL) > > /** > Returns a 64-bit signature built from 8 ASCII characters. > @@ -1260,7 +1260,7 @@ typedef UINTN RETURN_STATUS; > > **/ > #define SIGNATURE_64(A, B, C, D, E, F, G, H) \ > - (SIGNATURE_32 (A, B, C, D) | ((UINT64) (SIGNATURE_32 (E, F, G, H)) << 32)) > + ((UINT64)SIGNATURE_32 (A, B, C, D) | ((UINT64) ((UINT64)SIGNATURE_32 (E, F, G, H)) << 32U)) > > #if defined(_MSC_EXTENSIONS) && !defined (__INTEL_COMPILER) && !defined (MDE_CPU_EBC) > void * _ReturnAddress(void); > Assuming that the SIGNATURE_32 is *either* fixed (so that it produces a UINT32 value) *or* used in accordance with the documentation (ie., only ASCII characters as arguments), the replacement text for SIGNATURE_64 is already correct, and this hunk is unneeded. In the former case (SIGNATURE_32 fixed), we have: (UINT32_VAL | (UINT64)UINT32_VAL << 32) (UINT32_VAL | UINT64_VAL) (UINT64_VAL | UINT64_VAL) (UINT64_VAL) In the latter case -- SIGNATURE_32 left unchanged, but call sites only use ASCII char args --, we have: (INT32_VAL | (UINT64)INT32_VAL << 32) (INT32_VAL | UINT64_VAL) (UINT64_VAL | UINT64_VAL) (UINT64_VAL) which does what we want, because both INT32 --> UINT64 conversions start on non-negative values (i.e., bit#15 is clear in both SIGNATURE_32() expansions). So, I think we don't need to touch SIGNATURE_64(). Thanks, Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-27 19:54 ` Laszlo Ersek @ 2018-02-27 20:31 ` Marvin Häuser 2018-02-28 11:00 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-02-27 20:31 UTC (permalink / raw) To: edk2-devel@lists.01.org Cc: Laszlo Ersek, michael.d.kinney@intel.com, liming.gao@intel.com Hey Laszlo, Thank you very much for your review, again. Comments are inline. Regards, Marvin. > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Tuesday, February 27, 2018 8:54 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > On 02/27/18 17:47, Marvin Häuser wrote: > > As per the C standard, bit-level operations on signed integers are > > either undefined or implementation-defined. Hence, mark all BIT > > defines and shifts as unsigned to safely allow such operations. > > Sigh, this is why threading is important in patch sets. :/ This patch comes first, > and it sort of invalidates my review for patch #2 in this series. > > I think this change is a good idea *in theory* (arguably the macros should > have been introduced like this in the first place). However, at this point, quite > a bit of code could silently see its meaning changed. You are right that likely at least some vendor has code that relies on the defines being signed. However, you seem to agree that bit definitions are naturally assumed to be unsigned and the same as this change could break code, not making the change could lead to non-functioning code that looks valid at first sight. Regarding your comment on the 'truncating patches', saying the code relies on the definitions being signed: I honestly doubt the code was intentionally written with that in mind, but rather intuitively and it happened to work. I might be wrong of course. > > Let me give you a pathologic example (it *does* occur in the wild, although > maybe not in edk2): > > { > UINT64 MyMask; > > MyMask = ~BIT0; > } > > Before your patch, MyMask is assigned the value 0xFFFF_FFFF_FFFF_FFFE. > > This is because: > > - BIT0 has type "int" and value 1 > > - Due to our (implementation-defined) representation of negative integers, > namely two's complement, ~BIT0 (also having type "int") stands for value (- > 2). There is pretty much where my issue is. I don't think triggering implementation-defined behavior is a good idea, not to speak about relying on it. EDK no longer targets solely x86, there is even a new architecture (Arch-V or something?) on the way, so I consider relying on compiler specifics very dangerous. > > - When this value is converted to UINT64 ("unsigned long long"), we get > 18446744073709551615 + 1 + (-2) == 18446744073709551614. That's > 0xFFFF_FFFF_FFFF_FFFE, which is the mask value that we intended. > > (An alternative way people sometimes put this is that ~BIT0 is "sign > extended". I strongly dislike this term when speaking about C. It is > appropriate for (x86?) assembly language, but not for C, in my opinion.) > > > In the above example, the "clever" programmer consciously exploited the > two's complement representation for the bit-neg, and the conversion of the > negative value to "unsigned long long" (as dictated by the standard). > > > Now, if you change BIT0 to have type "unsigned int" (aka UINT32), then > ~BIT0 will have value 4294967294 (0xFFFF_FFFE), and type "unsigned int". > > When this ~BIT0 is converted to UINT64, the value will be preserved, and > MyMask will end up with 0x0000_0000_FFFF_FFFE. Likely not the mask that > the clever programmer intended. > > (An alternative way to state this is that ~BIT0 is "zero extended". > Again, I think this is inappropriate for C language discussion; it's fine for (x86?) > assembly.) > > My point is that such innocent-looking (and otherwise really good!) changes > for *central* macros require a large audit effort. Indeed that audit effort is required. How would that be handled for non-edk2 code anyway? Might there be a note in the UDK changelog, if accepted? > > More comments below: > > > For the SIGNATURE macros, add several casts to account for int > > promotions, which might be signed. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com> > > --- > > MdePkg/Include/Base.h | 160 ++++++++++---------- > > 1 file changed, 80 insertions(+), 80 deletions(-) > > > > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index > > a94182f08886..f108ed92eb0b 100644 > > --- a/MdePkg/Include/Base.h > > +++ b/MdePkg/Include/Base.h > > @@ -404,38 +404,38 @@ struct _LIST_ENTRY { #define MIN_INT32 > > (((INT32) -2147483647) - 1) #define MIN_INT64 (((INT64) > > -9223372036854775807LL) - 1) > > > > -#define BIT0 0x00000001 > > -#define BIT1 0x00000002 > > -#define BIT2 0x00000004 > > -#define BIT3 0x00000008 > > -#define BIT4 0x00000010 > > -#define BIT5 0x00000020 > > -#define BIT6 0x00000040 > > -#define BIT7 0x00000080 > > -#define BIT8 0x00000100 > > -#define BIT9 0x00000200 > > -#define BIT10 0x00000400 > > -#define BIT11 0x00000800 > > -#define BIT12 0x00001000 > > -#define BIT13 0x00002000 > > -#define BIT14 0x00004000 > > -#define BIT15 0x00008000 > > -#define BIT16 0x00010000 > > -#define BIT17 0x00020000 > > -#define BIT18 0x00040000 > > -#define BIT19 0x00080000 > > -#define BIT20 0x00100000 > > -#define BIT21 0x00200000 > > -#define BIT22 0x00400000 > > -#define BIT23 0x00800000 > > -#define BIT24 0x01000000 > > -#define BIT25 0x02000000 > > -#define BIT26 0x04000000 > > -#define BIT27 0x08000000 > > -#define BIT28 0x10000000 > > -#define BIT29 0x20000000 > > -#define BIT30 0x40000000 > > -#define BIT31 0x80000000 > > +#define BIT0 0x00000001U > > +#define BIT1 0x00000002U > > +#define BIT2 0x00000004U > > +#define BIT3 0x00000008U > > +#define BIT4 0x00000010U > > +#define BIT5 0x00000020U > > +#define BIT6 0x00000040U > > +#define BIT7 0x00000080U > > +#define BIT8 0x00000100U > > +#define BIT9 0x00000200U > > +#define BIT10 0x00000400U > > +#define BIT11 0x00000800U > > +#define BIT12 0x00001000U > > +#define BIT13 0x00002000U > > +#define BIT14 0x00004000U > > +#define BIT15 0x00008000U > > +#define BIT16 0x00010000U > > +#define BIT17 0x00020000U > > +#define BIT18 0x00040000U > > +#define BIT19 0x00080000U > > +#define BIT20 0x00100000U > > +#define BIT21 0x00200000U > > +#define BIT22 0x00400000U > > +#define BIT23 0x00800000U > > +#define BIT24 0x01000000U > > +#define BIT25 0x02000000U > > +#define BIT26 0x04000000U > > +#define BIT27 0x08000000U > > +#define BIT28 0x10000000U > > +#define BIT29 0x20000000U > > +#define BIT30 0x40000000U > > +#define BIT31 0x80000000U > > #define BIT32 0x0000000100000000ULL > > #define BIT33 0x0000000200000000ULL > > #define BIT34 0x0000000400000000ULL > > @@ -469,28 +469,28 @@ struct _LIST_ENTRY { > > #define BIT62 0x4000000000000000ULL > > #define BIT63 0x8000000000000000ULL > > > > -#define SIZE_1KB 0x00000400 > > -#define SIZE_2KB 0x00000800 > > -#define SIZE_4KB 0x00001000 > > -#define SIZE_8KB 0x00002000 > > -#define SIZE_16KB 0x00004000 > > -#define SIZE_32KB 0x00008000 > > -#define SIZE_64KB 0x00010000 > > -#define SIZE_128KB 0x00020000 > > -#define SIZE_256KB 0x00040000 > > -#define SIZE_512KB 0x00080000 > > -#define SIZE_1MB 0x00100000 > > -#define SIZE_2MB 0x00200000 > > -#define SIZE_4MB 0x00400000 > > -#define SIZE_8MB 0x00800000 > > -#define SIZE_16MB 0x01000000 > > -#define SIZE_32MB 0x02000000 > > -#define SIZE_64MB 0x04000000 > > -#define SIZE_128MB 0x08000000 > > -#define SIZE_256MB 0x10000000 > > -#define SIZE_512MB 0x20000000 > > -#define SIZE_1GB 0x40000000 > > -#define SIZE_2GB 0x80000000 > > +#define SIZE_1KB 0x00000400U > > +#define SIZE_2KB 0x00000800U > > +#define SIZE_4KB 0x00001000U > > +#define SIZE_8KB 0x00002000U > > +#define SIZE_16KB 0x00004000U > > +#define SIZE_32KB 0x00008000U > > +#define SIZE_64KB 0x00010000U > > +#define SIZE_128KB 0x00020000U > > +#define SIZE_256KB 0x00040000U > > +#define SIZE_512KB 0x00080000U > > +#define SIZE_1MB 0x00100000U > > +#define SIZE_2MB 0x00200000U > > +#define SIZE_4MB 0x00400000U > > +#define SIZE_8MB 0x00800000U > > +#define SIZE_16MB 0x01000000U > > +#define SIZE_32MB 0x02000000U > > +#define SIZE_64MB 0x04000000U > > +#define SIZE_128MB 0x08000000U > > +#define SIZE_256MB 0x10000000U > > +#define SIZE_512MB 0x20000000U > > +#define SIZE_1GB 0x40000000U > > +#define SIZE_2GB 0x80000000U > > #define SIZE_4GB 0x0000000100000000ULL > > #define SIZE_8GB 0x0000000200000000ULL > > #define SIZE_16GB 0x0000000400000000ULL > > @@ -524,28 +524,28 @@ struct _LIST_ENTRY { > > #define SIZE_4EB 0x4000000000000000ULL > > #define SIZE_8EB 0x8000000000000000ULL > > > > -#define BASE_1KB 0x00000400 > > -#define BASE_2KB 0x00000800 > > -#define BASE_4KB 0x00001000 > > -#define BASE_8KB 0x00002000 > > -#define BASE_16KB 0x00004000 > > -#define BASE_32KB 0x00008000 > > -#define BASE_64KB 0x00010000 > > -#define BASE_128KB 0x00020000 > > -#define BASE_256KB 0x00040000 > > -#define BASE_512KB 0x00080000 > > -#define BASE_1MB 0x00100000 > > -#define BASE_2MB 0x00200000 > > -#define BASE_4MB 0x00400000 > > -#define BASE_8MB 0x00800000 > > -#define BASE_16MB 0x01000000 > > -#define BASE_32MB 0x02000000 > > -#define BASE_64MB 0x04000000 > > -#define BASE_128MB 0x08000000 > > -#define BASE_256MB 0x10000000 > > -#define BASE_512MB 0x20000000 > > -#define BASE_1GB 0x40000000 > > -#define BASE_2GB 0x80000000 > > +#define BASE_1KB 0x00000400U > > +#define BASE_2KB 0x00000800U > > +#define BASE_4KB 0x00001000U > > +#define BASE_8KB 0x00002000U > > +#define BASE_16KB 0x00004000U > > +#define BASE_32KB 0x00008000U > > +#define BASE_64KB 0x00010000U > > +#define BASE_128KB 0x00020000U > > +#define BASE_256KB 0x00040000U > > +#define BASE_512KB 0x00080000U > > +#define BASE_1MB 0x00100000U > > +#define BASE_2MB 0x00200000U > > +#define BASE_4MB 0x00400000U > > +#define BASE_8MB 0x00800000U > > +#define BASE_16MB 0x01000000U > > +#define BASE_32MB 0x02000000U > > +#define BASE_64MB 0x04000000U > > +#define BASE_128MB 0x08000000U > > +#define BASE_256MB 0x10000000U > > +#define BASE_512MB 0x20000000U > > +#define BASE_1GB 0x40000000U > > +#define BASE_2GB 0x80000000U > > #define BASE_4GB 0x0000000100000000ULL > > #define BASE_8GB 0x0000000200000000ULL > > #define BASE_16GB 0x0000000400000000ULL > > @@ -974,7 +974,7 @@ typedef UINTN RETURN_STATUS; > > @return The value specified by StatusCode with the highest bit set. > > > > **/ > > -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | > (StatusCode))) > > +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | > (StatusCode##ULL))) > > > > MAX_BIT always has type UINTN (UINT64 aka "unsigned long long" on 64-bit > platforms, and UINT32 aka "unsigned int" on 32-bit platforms). This means > that ENCODE_ERROR results in a UINTN, right now. Good point, I didn't keep that in mind. It probably should be solely 'U'. > > After the change, it would always result in a UINT64. I think that could be a > problem for 32-bit platforms, especially if you push open-coded RETURN_xxx > status codes to the stack, for example for printing with "%r". Pushing a > UINT64 arg instead of a UINT32 arg through the ellipsis > (...) might throw off the rest of the argument processing. Actually the cast to RETURN_STATUS is covering the entire operation, so in the end it should be UINTN again, no? > > So, I wouldn't recommend this change. What is the exact motivation for it? If I am not terribly mistaken, the operation results in implementation-defined behavior as-is, which the patch as a whole intends to avoid. > > > /** > > Produces a RETURN_STATUS code with the highest bit clear. > > @@ -1221,7 +1221,7 @@ typedef UINTN RETURN_STATUS; > > @return A 16-bit value built from the two ASCII characters specified by A > and B. > > > > **/ > > -#define SIGNATURE_16(A, B) ((A) | (B << 8)) > > +#define SIGNATURE_16(A, B) ((UINT16)(A) | (UINT16)((UINT16)(B) << > 8U)) > > This might not have the intended effect. Due to "int" (INT32) being capable > of representing the entire domain of "unsigned short" (UINT16), both > operands of the bitwise OR operator would be promoted to "int". The result > would have type "int". To me, what type the result has was less important than making sure both operands are unsigned to ensure a well-defined operation result. > > Then, even if you cast the entire expression to UINT16 in the replacement > text, it would still be promoted back to "int" in almost every context. (One > exception would be the operand of the "sizeof" > operator -- the expression operand of the "sizeof" operator is only evaluated > if it has variable length array (VLA) type). > > I don't think this change is helpful. > > > > > /** > > Returns a 32-bit signature built from 4 ASCII characters. > > @@ -1238,7 +1238,7 @@ typedef UINTN RETURN_STATUS; > > C and D. > > > > **/ > > -#define SIGNATURE_32(A, B, C, D) (SIGNATURE_16 (A, B) | > > (SIGNATURE_16 (C, D) << 16)) > > +#define SIGNATURE_32(A, B, C, D) ((UINT32)SIGNATURE_16 (A, B) | > > +(UINT32)((UINT32)SIGNATURE_16 (C, D) << 16U)) > > This looks better; the original sub-expression > > SIGNATURE_16 (C, D) << 16 > > invokes undefined behavior *if* (SIGNATURE_16 (C, D)) has bit#15 set. > > In practice that should never happen though: the documentation says that > the macro arguments have to be "ASCII characters", and bit#7 is clear in > those. Thanks for this hint! While this is true of course, I usually prefer to show 'clear intentions' instead of relying on potentially unknown premises. > > Anyway, if we want to be pedantic, the fix can be expressed more simply, > like this: > > (SIGNATURE_16 (A, B) | ((UINT32)SIGNATURE_16 (C, D) << 16)) > ^^^^^^^^ > > Then the UINT32 type ("unsigned int") will "cascade" through the entire > expression -- (SIGNATURE_16 (A, B)) will be converted from "int" to > "unsigned int" for the bitwise OR, and the result will also have type > UINT32: > > (INT32_VAL | (UINT32)INT32_VAL << 16) > (INT32_VAL | UINT32_VAL) > (UINT32_VAL | UINT32_VAL) > (UINT32_VAL) This is only true if int <= INT32, isn't it? > > > > > /** > > Returns a 64-bit signature built from 8 ASCII characters. > > @@ -1260,7 +1260,7 @@ typedef UINTN RETURN_STATUS; > > > > **/ > > #define SIGNATURE_64(A, B, C, D, E, F, G, H) \ > > - (SIGNATURE_32 (A, B, C, D) | ((UINT64) (SIGNATURE_32 (E, F, G, H)) << > 32)) > > + ((UINT64)SIGNATURE_32 (A, B, C, D) | ((UINT64) > > + ((UINT64)SIGNATURE_32 (E, F, G, H)) << 32U)) > > > > #if defined(_MSC_EXTENSIONS) && !defined (__INTEL_COMPILER) && > !defined (MDE_CPU_EBC) > > void * _ReturnAddress(void); > > > > Assuming that the SIGNATURE_32 is *either* fixed (so that it produces a > UINT32 value) *or* used in accordance with the documentation (ie., only > ASCII characters as arguments), the replacement text for SIGNATURE_64 is > already correct, and this hunk is unneeded. I think this is also only true if int <= INT64. > > In the former case (SIGNATURE_32 fixed), we have: > > (UINT32_VAL | (UINT64)UINT32_VAL << 32) > (UINT32_VAL | UINT64_VAL) > (UINT64_VAL | UINT64_VAL) > (UINT64_VAL) > > In the latter case -- SIGNATURE_32 left unchanged, but call sites only use > ASCII char args --, we have: > > (INT32_VAL | (UINT64)INT32_VAL << 32) > (INT32_VAL | UINT64_VAL) > (UINT64_VAL | UINT64_VAL) > (UINT64_VAL) > > which does what we want, because both INT32 --> UINT64 conversions start > on non-negative values (i.e., bit#15 is clear in both SIGNATURE_32() > expansions). > > So, I think we don't need to touch SIGNATURE_64(). > > Thanks, > Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-27 20:31 ` Marvin Häuser @ 2018-02-28 11:00 ` Laszlo Ersek 2018-02-28 11:43 ` Marvin Häuser 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2018-02-28 11:00 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: michael.d.kinney@intel.com, liming.gao@intel.com On 02/27/18 21:31, Marvin Häuser wrote: >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Tuesday, February 27, 2018 8:54 PM >> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- >> devel@lists.01.org >> Cc: michael.d.kinney@intel.com; liming.gao@intel.com >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >> operations. >> >> On 02/27/18 17:47, Marvin Häuser wrote: [...] >> I think this change is a good idea *in theory* (arguably the macros >> should have been introduced like this in the first place). However, >> at this point, quite a bit of code could silently see its meaning >> changed. > > You are right that likely at least some vendor has code that relies on > the defines being signed. However, you seem to agree that bit > definitions are naturally assumed to be unsigned and the same as this > change could break code, not making the change could lead to > non-functioning code that looks valid at first sight. This is exactly how I feel, yes. My concern is that making the change now runs the risk of tricky regressions that we might not even prevent with a detailed audit. Anyway, I don't want to spread FUD about this -- I think the goal of these changes is good. It's up to the MdePkg maintainers to evaluate the risks, I just wanted us to be aware of them. Once we reach an end state where all the BIT, SIZE and BASE macros are unsigned, and no regressions are found (or none remain), that's for the best! > Regarding your comment on the 'truncating patches', saying the code > relies on the definitions being signed: I honestly doubt the code was > intentionally written with that in mind, but rather intuitively and it > happened to work. I might be wrong of course. I think I agree with your assessment, considering the usual application of these macros in source code. I distinguish three kinds of uses: (1) The trusting / maybe "naive" client code: silently expects the BIT macros to be unsigned, resultant code happens to work; when it doesn't, it is fixed up with individual patches. (2) The paranoid code: is aware that BIT macros are not (all) unsigned, believes that they should be unsigned, casts BIT macro applications to unsigned types locally, all further manipulation is done with unsigned types. (3) The clever code: is aware that BIT macros are (mostly) signed, and exploits that fact for various ends. I agree that most of the edk2 codebase should fall under (1). As you may expect, I personally write (2), and code like (2) should not worry about BIT / SIZE / BASE becoming unsigned. My concern is (3). There have been examples in core edk2 modules that explicitly relied on undefined behavior, such as left shifts of negative integers and such. We've only slowly fixed them up after compilers / analyzers started flagging them. If we don't think (3) is a real risk, I'm fine with the approach of these patches. (I don't think I'll be able to send a real R-b for them, because that would require me to evaluate all uses, and that's a Herculean task I just can't take on; apologies.) >> Let me give you a pathologic example (it *does* occur in the wild, >> although maybe not in edk2): >> >> { >> UINT64 MyMask; >> >> MyMask = ~BIT0; >> } >> >> Before your patch, MyMask is assigned the value >> 0xFFFF_FFFF_FFFF_FFFE. >> >> This is because: >> >> - BIT0 has type "int" and value 1 >> >> - Due to our (implementation-defined) representation of negative >> integers, namely two's complement, ~BIT0 (also having type "int") >> stands for value (- 2). > > There is pretty much where my issue is. I don't think triggering > implementation-defined behavior is a good idea, not to speak about > relying on it. EDK no longer targets solely x86, there is even a new > architecture (Arch-V or something?) on the way, so I consider relying > on compiler specifics very dangerous. Oh I totally agree about that! As I said, the goal of the patches is great, I absolutely agree about it; my concern is, will we find tricks like the above by source code audit, or by a series of functional regressions, drawn out over time? [...] >> My point is that such innocent-looking (and otherwise really good!) >> changes for *central* macros require a large audit effort. > > Indeed that audit effort is required. How would that be handled for > non-edk2 code anyway? Might there be a note in the UDK changelog, if > accepted? Very good question, and I don't have the slightest idea. Considering the Linux kernel as an example, if your stuff is "out of tree", you are on your own, when incompatible changes are committed. (If you are "in-tree", then whoever does the incompat change will fix up your code too.) Obviously this doesn't (or may not) fly for edk2, which is a "Development Kit". I don't know what the right procedure is here. I do remember a problem from the past, when the IP4Config protocol was replaced with the IP4Config2 protocol. It was well documented in UDK release notes (as I recall), yet it broke a bunch of (proprietary) downstreams of edk2, and some annoyed messages were posted to edk2-devel. Again, IMO something for the MdePkg maintainers to judge. [...] >>> -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | >> (StatusCode))) >>> +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | >> (StatusCode##ULL))) >>> >> >> MAX_BIT always has type UINTN (UINT64 aka "unsigned long long" on >> 64-bit platforms, and UINT32 aka "unsigned int" on 32-bit platforms). >> This means that ENCODE_ERROR results in a UINTN, right now. > > Good point, I didn't keep that in mind. It probably should be solely > 'U'. In fact there's no need even for that: because MAX_BIT has type UINTN ("unsigned long long" or "unsigned int"), the StatusCode macro argument, which is expected to be a small nonnegative "int", will be converted to UINTN, for the bitwise OR operator. Now, of course the suggested U suffix doesn't hurt, no question about that. If it improves readability, then it's a good thing. In that case though, how about updating the constants in the invocations of ENCODE_ERROR(), instead? Because, if you use ##U within the replacement text, then ENCODE_ERROR(4U) will become invalid (which I think is unexpected). >> After the change, it would always result in a UINT64. I think that >> could be a problem for 32-bit platforms, especially if you push >> open-coded RETURN_xxx status codes to the stack, for example for >> printing with "%r". Pushing a UINT64 arg instead of a UINT32 arg >> through the ellipsis (...) might throw off the rest of the argument >> processing. > > Actually the cast to RETURN_STATUS is covering the entire operation, > so in the end it should be UINTN again, no? True; sorry, I lost track of that. >> So, I wouldn't recommend this change. What is the exact motivation >> for it? > > If I am not terribly mistaken, the operation results in > implementation-defined behavior as-is, which the patch as a whole > intends to avoid. What is implementation-defined in the current definition of ENCODE_ERROR()? Given the current definition, if StatusCode is a signed integer, then one way or another, it will be converted to (not reinterpreted as) an unsigned integer type. This is due to the rules on the bitwise OR operator and the outermost cast (thank you for reminding me of that). Such conversions are fully defined in the C standard, they are not implementation-defined. For example, if we are on a 32-bit platform (i.e. UINTN means UINT32), and StatusCode is (for some reason) an INT64 value, then MAX_BIT will first be converted to INT64 (because the latter has higher conversion rank, and can represent all values of UINT32), the bit-OR is performed in INT64, and the result is converted back to UINT32 (RETURN_STATUS). I don't see any way for the sign bit to be set (or otherwise toggled) in a signed integer here. Anyway, the edk2 coding style spec does advise against intricate C code that requires deep knowledge of the standard / language rules; so if we can improve readability without limiting use, I'm fine with that. [...] >>> **/ >>> -#define SIGNATURE_16(A, B) ((A) | (B << 8)) >>> +#define SIGNATURE_16(A, B) ((UINT16)(A) | (UINT16)((UINT16)(B) << >> 8U)) >> >> This might not have the intended effect. Due to "int" (INT32) being >> capable of representing the entire domain of "unsigned short" >> (UINT16), both operands of the bitwise OR operator would be promoted >> to "int". The result would have type "int". > > To me, what type the result has was less important than making sure > both operands are unsigned to ensure a well-defined operation result. This is a good point -- and my comment was that this well-defined nature was already ensured by simply following the documented interface contract, namely using ASCII characters as macro arguments. I don't insist though. [...] >> Anyway, if we want to be pedantic, the fix can be expressed more >> simply, like this: >> >> (SIGNATURE_16 (A, B) | ((UINT32)SIGNATURE_16 (C, D) << 16)) >> ^^^^^^^^ >> >> Then the UINT32 type ("unsigned int") will "cascade" through the >> entire expression -- (SIGNATURE_16 (A, B)) will be converted from >> "int" to "unsigned int" for the bitwise OR, and the result will also >> have type UINT32: >> >> (INT32_VAL | (UINT32)INT32_VAL << 16) >> (INT32_VAL | UINT32_VAL) >> (UINT32_VAL | UINT32_VAL) >> (UINT32_VAL) > > This is only true if int <= INT32, isn't it? That's correct. If UINT32 mapped to "unsigned short" and INT64 mapped to "int", then ((UINT32)SIGNATURE_16 (C, D)) would be promoted to INT64 ("int"), and the rest of the operations would be carried out in INT64 ("int"). Is your point that we shouldn't even assume INT32=="int" and UINT32=="unsigned int"? I think that would be a good point for a highly portable C codebase that was written from the ground up without type assumptions. But I don't think it applies to edk2. If UINT32 mapped to "unsigned short" and INT64 mapped to "int", edk2 would break in a thousand places immediately. [...] >>> @@ -1260,7 +1260,7 @@ typedef UINTN RETURN_STATUS; >>> >>> **/ >>> #define SIGNATURE_64(A, B, C, D, E, F, G, H) \ >>> - (SIGNATURE_32 (A, B, C, D) | ((UINT64) (SIGNATURE_32 (E, F, G, H)) << >> 32)) >>> + ((UINT64)SIGNATURE_32 (A, B, C, D) | ((UINT64) >>> + ((UINT64)SIGNATURE_32 (E, F, G, H)) << 32U)) >>> >>> #if defined(_MSC_EXTENSIONS) && !defined (__INTEL_COMPILER) && >> !defined (MDE_CPU_EBC) >>> void * _ReturnAddress(void); >>> >> >> Assuming that the SIGNATURE_32 is *either* fixed (so that it produces >> a UINT32 value) *or* used in accordance with the documentation (ie., >> only ASCII characters as arguments), the replacement text for >> SIGNATURE_64 is already correct, and this hunk is unneeded. > > I think this is also only true if int <= INT64. Sure. And I think if "int" corresponds to INT128, then this macro is the smallest problem that the edk2 code base runs into. :) I do get your point now. I believe you are trying to eliminate all (or most) implementation-defined traits from these macros, because "that's how portable C code should be written". Personally I think that's a laudable goal, and I agree that all portable C code should be written like that. I also think however that there's a large disconnect between that goal / style, and how edk2 (and even the UEFI spec) exist today. In my opinion / experience, edk2 was never meant to be portable to unknown, future platforms. For one, it is tied to little endian byte order on the spec level. Anyway, if you submit a v2 of these series (please make them threaded!), I'll try to review those with this in mind. Thanks! Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 11:00 ` Laszlo Ersek @ 2018-02-28 11:43 ` Marvin Häuser 2018-02-28 13:57 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-02-28 11:43 UTC (permalink / raw) To: edk2-devel@lists.01.org, Laszlo Ersek Cc: michael.d.kinney@intel.com, liming.gao@intel.com Thanks. Comments are inline. Best regards, Marvin. > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, February 28, 2018 12:00 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > On 02/27/18 21:31, Marvin Häuser wrote: > >> -----Original Message----- > >> From: Laszlo Ersek <lersek@redhat.com> > >> Sent: Tuesday, February 27, 2018 8:54 PM > >> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > >> devel@lists.01.org > >> Cc: michael.d.kinney@intel.com; liming.gao@intel.com > >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > >> operations. > >> [...] > > This is exactly how I feel, yes. My concern is that making the change now > runs the risk of tricky regressions that we might not even prevent with a > detailed audit. Actually, your explanations and the rest of the bit defines made me wonder, whether marking all BIT defines and bit masks of any kind, anywhere, as ULL, might be the best solution. +1) Inversion can be used for any integer size uncasted, as edk2 does not support vendor extensions such as __int128 anyway. +2) Binary operations (AND, OR, ...) should not raise any problems at all (except for our fellow using VS2005x86 :) ) +3) Assignments and passing-by-arguments will result in compiler-time errors when not done properly in the sense of this patch, which eliminates the need to audit all usages manually, but just compile the entire codebase and fix the errors one-by-one (which is of course still not a quick task, but if the package authors agree with the proposal, I might attempt it). -1) The 'truncating constant value' warning would probably need to be disabled globally, however I don't understand how an explicit cast is a problem anyway. Did I overlook anything contra regarding that? > > Anyway, I don't want to spread FUD about this -- I think the goal of these > changes is good. It's up to the MdePkg maintainers to evaluate the risks, I > just wanted us to be aware of them. Once we reach an end state where all > the BIT, SIZE and BASE macros are unsigned, and no regressions are found (or > none remain), that's for the best! Nah, your comments are great, thanks! [...] > I think I agree with your assessment, considering the usual application of > these macros in source code. I distinguish three kinds of uses: > [...] > > (3) The clever code: is aware that BIT macros are (mostly) signed, and > exploits that fact for various ends. > > I agree that most of the edk2 codebase should fall under (1). > > As you may expect, I personally write (2), and code like (2) should not worry > about BIT / SIZE / BASE becoming unsigned. > > My concern is (3). There have been examples in core edk2 modules that > explicitly relied on undefined behavior, such as left shifts of negative integers > and such. We've only slowly fixed them up after compilers / analyzers started > flagging them. > > If we don't think (3) is a real risk, I'm fine with the approach of these patches. > (I don't think I'll be able to send a real R-b for them, because that would > require me to evaluate all uses, and that's a Herculean task I just can't take > on; apologies.) I hope that the 'all ULL' proposal uncovers them all. Would there be cases, where no error would be raised? [...] > > >>> -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | > >> (StatusCode))) > >>> +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT > | > >> (StatusCode##ULL))) > >>> > >> > >> MAX_BIT always has type UINTN (UINT64 aka "unsigned long long" on > >> 64-bit platforms, and UINT32 aka "unsigned int" on 32-bit platforms). > >> This means that ENCODE_ERROR results in a UINTN, right now. > > > > Good point, I didn't keep that in mind. It probably should be solely > > 'U'. > > In fact there's no need even for that: because MAX_BIT has type UINTN > ("unsigned long long" or "unsigned int"), the StatusCode macro argument, > which is expected to be a small nonnegative "int", will be converted to > UINTN, for the bitwise OR operator. Oh, right, thanks. I would still prefer it to be explicit, but I'll wait for a maintainer's comment. [...] > > What is implementation-defined in the current definition of > ENCODE_ERROR()? That was a result of the misunderstanding above, scrap that. > > Given the current definition, if StatusCode is a signed integer, then one way > or another, it will be converted to (not reinterpreted as) an unsigned integer > type. This is due to the rules on the bitwise OR operator and the outermost > cast (thank you for reminding me of that). > Such conversions are fully defined in the C standard, they are not > implementation-defined. Thanks for noting. Would you still append the U-prefix for readability / making the intention clear? [...] > > Is your point that we shouldn't even assume INT32=="int" and > UINT32=="unsigned int"? Yes, pretty much. I don't think it does hurt either. While it is certainly a very ugly definition, that's my preferred use for macros anyway - hide ugly code behind a lovely name. > > I think that would be a good point for a highly portable C codebase that was > written from the ground up without type assumptions. But I don't think it > applies to edk2. If UINT32 mapped to "unsigned short" and INT64 mapped to > "int", edk2 would break in a thousand places immediately. Of course it would, however I imagined a header file that is included in every file for every architecture to be as close to flawless as possible. While of course I would also fix such assumptions in executable code if I found, and hope the maintainers care enough to review and merge, I consider header files a greater danger as that 'flawed' (in an idealistic sense) macro is expanded into external code, which might cause trouble while debugging, as the consumer assumes the mistake is within his code. Headers >>> library code > module code, in my opinion, especially for the most central one. [...] > > I do get your point now. I believe you are trying to eliminate all (or > most) implementation-defined traits from these macros, because "that's > how portable C code should be written". Exactly. :) > > Personally I think that's a laudable goal, and I agree that all portable C code > should be written like that. I also think however that there's a large > disconnect between that goal / style, and how edk2 (and even the UEFI > spec) exist today. In my opinion / experience, edk2 was never meant to be > portable to unknown, future platforms. For one, it is tied to little endian byte > order on the spec level. Personally, I am fine to assert anything that the specification demands (that's what it's here for after all), however we are not aware of the compiler-specifics at spec-level. > > Anyway, if you submit a v2 of these series (please make them threaded!), I'll > try to review those with this in mind. > > Thanks! > Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 11:43 ` Marvin Häuser @ 2018-02-28 13:57 ` Laszlo Ersek 2018-02-28 14:01 ` Laszlo Ersek 2018-02-28 14:21 ` Marvin Häuser 0 siblings, 2 replies; 18+ messages in thread From: Laszlo Ersek @ 2018-02-28 13:57 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: michael.d.kinney@intel.com, liming.gao@intel.com On 02/28/18 12:43, Marvin Häuser wrote: > Actually, your explanations and the rest of the bit defines made me > wonder, whether marking all BIT defines and bit masks of any kind, > anywhere, as ULL, might be the best solution. For a new project just being started, that could be one of the safest / wisest choices. :) For edk2, about the only counter-argument I'd have right now is that many assignments and function arguments could suddenly require down-casts. > +1) Inversion can be used for any integer size uncasted, Right, you could just use ~BITn, and truncate it to whatever unsigned type is needed. > as edk2 does not support vendor extensions such as __int128 anyway. Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table 5, "Common UEFI Data Types". I believe those typedefs may have been added for RISC-V. > +2) Binary operations (AND, OR, ...) should not raise any problems at > all (except for our fellow using VS2005x86 :) ) Haha :) In earnest though, you are right. > +3) Assignments and passing-by-arguments will result in compiler-time > errors when not done properly in the sense of this patch, which > eliminates the need to audit all usages manually, but just compile the > entire codebase and fix the errors one-by-one (which is of course > still not a quick task, but if the package authors agree with the > proposal, I might attempt it). Good point; it didn't occur to me that a large number of the "client sites" would be flagged by the compiler at once. ... I've just remembered another possible problem: I think some EBC compiler cannot use 64-bit integers as case labels in switch statements. See commit ada385843b94 ("MdeModulePkg/PciBus: Change switch-case to if-else to fix EBC build", 2018-01-10). Some case labels currently built of BIT[0..31] macros could break in EBC builds. OTOH, the (EBC) compiler would catch those as well; no silent breakage. > -1) The 'truncating constant value' warning would probably need to be > disabled globally, however I don't understand how an explicit cast is > a problem anyway. > > Did I overlook anything contra regarding that? Hmmm... Do you think it could have a performance impact on 32-bit platforms? (I don't think so, at least not in optimized / RELEASE builds.) >> (3) The clever code: is aware that BIT macros are (mostly) signed, >> and exploits that fact for various ends. >> If we don't think (3) is a real risk, I'm fine with the approach of >> these patches. (I don't think I'll be able to send a real R-b for >> them, because that would require me to evaluate all uses, and that's >> a Herculean task I just can't take on; apologies.) > > I hope that the 'all ULL' proposal uncovers them all. Would there be > cases, where no error would be raised? Dunno :) I quite like the idea! >>>>> -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | >>>> (StatusCode))) >>>>> +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT >> | >>>> (StatusCode##ULL))) >> Given the current definition, if StatusCode is a signed integer, then >> one way or another, it will be converted to (not reinterpreted as) an >> unsigned integer type. This is due to the rules on the bitwise OR >> operator and the outermost cast (thank you for reminding me of that). >> Such conversions are fully defined in the C standard, they are not >> implementation-defined. > > Thanks for noting. Would you still append the U-prefix for readability > / making the intention clear? I think I'd document the requirement that StatusCode have an unsigned integer type, and then I'd update the ENCODE_ERROR() macro invocations to conform. >> Is your point that we shouldn't even assume INT32=="int" and >> UINT32=="unsigned int"? > > Yes, pretty much. I don't think it does hurt either. While it is > certainly a very ugly definition, that's my preferred use for macros > anyway - hide ugly code behind a lovely name. > >> >> I think that would be a good point for a highly portable C codebase >> that was written from the ground up without type assumptions. But I >> don't think it applies to edk2. If UINT32 mapped to "unsigned short" >> and INT64 mapped to "int", edk2 would break in a thousand places >> immediately. > > Of course it would, however I imagined a header file that is included > in every file for every architecture to be as close to flawless as > possible. > While of course I would also fix such assumptions in executable code > if I found, and hope the maintainers care enough to review and merge, > I consider header files a greater danger as that 'flawed' (in an > idealistic sense) macro is expanded into external code, which might > cause trouble while debugging, as the consumer assumes the mistake is > within his code. > Headers >>> library code > module code, in my opinion, especially for > the most central one. I'll defer to the MdePkg maintainers on this. >> Personally I think that's a laudable goal, and I agree that all >> portable C code should be written like that. I also think however >> that there's a large disconnect between that goal / style, and how >> edk2 (and even the UEFI spec) exist today. In my opinion / >> experience, edk2 was never meant to be portable to unknown, future >> platforms. For one, it is tied to little endian byte order on the >> spec level. > > Personally, I am fine to assert anything that the specification > demands (that's what it's here for after all), however we are not > aware of the compiler-specifics at spec-level. It is interesting to contrast: - the use of the standard (optional!) uint32_t type in portable, hosted C projects - with the use of UINT32 in edk2. * From C99: 7.18.1.1 Exact-width integer types 1 [...] 2 The typedef name uintN_t designates an unsigned integer type with width N. Thus, uint24_t denotes an unsigned integer type with a width of exactly 24 bits. 3 These types are optional. However, if an implementation provides integer types with widths of 8, 16, 32, or 64 bits, no padding bits, and (for the signed types) that have a two'\x19s complement representation, it shall define the corresponding typedef names. In my experience, portable, hosted C code uses the standard types by default (int, unsigned int, size_t, etc) and uses uint32_t and friends only in contexts where the representation matters -- mostly for implementing binary formats (files, network, ...). And going from one set of types to the other is usually strongly protected with range checks; there is usually some kind of (de)serialization that very consciously handles such transitions. This practice is very good because the integer promotions and the usual arithmetic conversions, as defined by the standard, i.e. in terms of standard integer types, are relatively easy to evalute for most of the code. Even bit-fiddling can be done in just "unsigned", "unsigned long" or "unsigned long long", because <limits.h> guarantees minimum magnitudes for those types. * In edk2 however (and in the UEFI spec), we have: - exact-width integer types that are hard to map to standard integer types -- hence the promotions and conversions are hard to evaluate in a portability mindset (they are not hard to evaluate if you just take UINT32=="unsigned int" for guaranteed, but that's what you'd like to avoid), - UINTN / INTN, which is defined as "native width". That might make some sense in assembly, but in C, "native width" makes no sense. The promotion and conversion rules can be interpreted for UINTN only very indirectly, by making some assumptions about UINT32 and UINT64. (And, about UINT128, theoretically :) ) Combine this with the fact that in edk2 source code, we're supposed to *avoid* using the standard C types! This is the exact opposite of the practice that I described for those portable, hosted C projects (i.e., stick with the standard types by default, go "exact width" only when unavoidable). When I first encountered edk2, I was utterly confused by this practice. I had seen it nowhere else. Reasoning about promotions and conversions looked plain impossible. I still think it was a bad choice (I don't know where it comes from, maybe Windows?), but I've gotten used to it by now -- I do equate UINT32 to "unsigned int", "UINT64" to "unsigned long long", and this way I can actually reason about the ISO C promotions and conversions. If you take that away, I'm not sure what we'll be left with :) I just think that the entire ISO C portability mindset, albeit *absolutely* desirable, is entirely foreign to the edk2 codebase (and the UEFI spec too). I agree we should avoid even implementation-defined behavior, as much as we can -- but not more than we can. If we start doubting UINT32 maps to "unsigned int", the entire code base falls apart in my eyes. Now, I do understand why UEFI (the spec) defines such exact-width types, and declares protocols in terms of those types. That's because UEFI is an ABI spec, not an API spec. That makes sense. However: - rather than introducing UINT32 etc, the spec should have used the standard C types uint32_t etc, - regardless of the UEFI interfaces being declared with exact-width integer types, edk2 code should be allowed (or even *prefer*) to use standard C types, such as "int", "unsigned int" etc, unless necessitated otherwise. Sorry about this long rant; I'm trying to explain why I felt that the goal of your patches didn't match edk2 very well. It doesn't mean that your patches are "wrong". Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 13:57 ` Laszlo Ersek @ 2018-02-28 14:01 ` Laszlo Ersek 2018-02-28 14:21 ` Marvin Häuser 1 sibling, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2018-02-28 14:01 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: michael.d.kinney@intel.com, liming.gao@intel.com On 02/28/18 14:57, Laszlo Ersek wrote: > On 02/28/18 12:43, Marvin Häuser wrote: >> +2) Binary operations (AND, OR, ...) should not raise any problems at >> all (except for our fellow using VS2005x86 :) ) > > Haha :) In earnest though, you are right. Hm... It should not be a frequent pattern, but: (BIT5 | BIT3) << 2 might no longer build; we might have to use LShiftU64() instead. Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 13:57 ` Laszlo Ersek 2018-02-28 14:01 ` Laszlo Ersek @ 2018-02-28 14:21 ` Marvin Häuser 2018-02-28 18:37 ` Kinney, Michael D 2018-02-28 18:45 ` Marvin Häuser 1 sibling, 2 replies; 18+ messages in thread From: Marvin Häuser @ 2018-02-28 14:21 UTC (permalink / raw) To: edk2-devel@lists.01.org, Laszlo Ersek Cc: michael.d.kinney@intel.com, liming.gao@intel.com Hey Laszlo, I cut your rant because it is not strictly related to this patch. However, thank you for composing it nevertheless because it was an interesting read! Comments are inline. Michael, Liming, Do you have any comments regarding the discussion? Thanks in advance. Best regards, Marvin. > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, February 28, 2018 2:57 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > On 02/28/18 12:43, Marvin Häuser wrote: [...] > > as edk2 does not support vendor extensions such as __int128 anyway. > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table 5, "Common > UEFI Data Types". I believe those typedefs may have been added for RISC-V. Oh yikes, I have not noticed that before. Besides that I wonder how that will be implemented by edk2 for non-RISC-V platforms, maybe that should be considered? As ridiculous as it sounds, maybe some kind of UINT_MAX type (now UINT64, later UINT128) should be introduced and any BIT or bitmask definition being explicitly casted to that? Are BIT definitions or masks occasionally used in preprocessor operations? That might break after all. Anyway, if that idea would be approved, there really would have to be a note regarding this design in some of the EDK2 specifications, probably C Code Style. [...] > > > -1) The 'truncating constant value' warning would probably need to be > > disabled globally, however I don't understand how an explicit cast is > > a problem anyway. > > > > Did I overlook anything contra regarding that? > > Hmmm... Do you think it could have a performance impact on 32-bit > platforms? (I don't think so, at least not in optimized / RELEASE > builds.) I don't think any proper optimizer would not optimize this. After all, it can not only evaluate the value directly and notice that the value does not reach into the 'long long range', but also consider the type of the other operand. [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 14:21 ` Marvin Häuser @ 2018-02-28 18:37 ` Kinney, Michael D 2018-02-28 18:52 ` Marvin Häuser 2018-02-28 18:45 ` Marvin Häuser 1 sibling, 1 reply; 18+ messages in thread From: Kinney, Michael D @ 2018-02-28 18:37 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org, Laszlo Ersek, Kinney, Michael D Cc: Gao, Liming Hi Marvin, I do not think add 'u' to the BITxx defines does not seem to be a complete solution. Code can use integer constants in lots of places including other #defines or inline in expressions. If we follow your suggestion wouldn’t we need to add 'u' to every constant that does not start with a '-' and might potentially be used with a bit operation? Compilers are doing a good job of finding undefined behavior. Isn’t that sufficient to fix the issues identified? Mike > -----Original Message----- > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] > Sent: Wednesday, February 28, 2018 6:21 AM > To: edk2-devel@lists.01.org; Laszlo Ersek > <lersek@redhat.com> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; > Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise operations. > > Hey Laszlo, > > I cut your rant because it is not strictly related to > this patch. However, thank you for composing it > nevertheless because it was an interesting read! > Comments are inline. > > Michael, Liming, > Do you have any comments regarding the discussion? > Thanks in advance. > > Best regards, > Marvin. > > > -----Original Message----- > > From: Laszlo Ersek <lersek@redhat.com> > > Sent: Wednesday, February 28, 2018 2:57 PM > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > devel@lists.01.org > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise > > operations. > > > > On 02/28/18 12:43, Marvin Häuser wrote: > [...] > > > as edk2 does not support vendor extensions such as > __int128 anyway. > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / > INT128, in table 5, "Common > > UEFI Data Types". I believe those typedefs may have > been added for RISC-V. > > Oh yikes, I have not noticed that before. Besides that > I wonder how that will be implemented by edk2 for non- > RISC-V platforms, maybe that should be considered? > As ridiculous as it sounds, maybe some kind of UINT_MAX > type (now UINT64, later UINT128) should be introduced > and any BIT or bitmask definition being explicitly > casted to that? > Are BIT definitions or masks occasionally used in > preprocessor operations? That might break after all. > Anyway, if that idea would be approved, there really > would have to be a note regarding this design in some > of the EDK2 specifications, probably C Code Style. > > [...] > > > > > -1) The 'truncating constant value' warning would > probably need to be > > > disabled globally, however I don't understand how > an explicit cast is > > > a problem anyway. > > > > > > Did I overlook anything contra regarding that? > > > > Hmmm... Do you think it could have a performance > impact on 32-bit > > platforms? (I don't think so, at least not in > optimized / RELEASE > > builds.) > > I don't think any proper optimizer would not optimize > this. After all, it can not only evaluate the value > directly and notice that the value does not reach into > the 'long long range', but also consider the type of > the other operand. > > [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 18:37 ` Kinney, Michael D @ 2018-02-28 18:52 ` Marvin Häuser 2018-03-01 1:41 ` Kinney, Michael D 0 siblings, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-02-28 18:52 UTC (permalink / raw) To: edk2-devel@lists.01.org, michael.d.kinney@intel.com Cc: lersek@redhat.com, liming.gao@intel.com Hey Mike, You are right, the patch was premature because I did not consider any 'incorrect' or 'clever' usages of these definitions. The problem is not primarily undefined behavior, but implementation-defined behavior. Any bitwise operation to a signed integer results in implementation-defined behavior, which compilers usually do not warn about, while well-defined behavior is desirable. Have you read Laszlo's comments? They are quite good at showing up what logics might be and are relied on, which however are not guaranteed to be the case for non-x86 architectures, or even for x86 in case a development team decides to change this behavior some day or a new toolchain not having adopted them in the first place should be added. Furthermore, I don't think inconsistency between the definitions generally is desirable. Thanks, Marvin. > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Wednesday, February 28, 2018 7:37 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Hi Marvin, > > I do not think add 'u' to the BITxx defines does not seem to be a complete > solution. Code can use integer constants in lots of places including other > #defines or inline in expressions. > > If we follow your suggestion wouldn’t we need to add 'u' to every constant > that does not start with a '-' > and might potentially be used with a bit operation? > > Compilers are doing a good job of finding undefined behavior. Isn’t that > sufficient to fix the issues identified? > > Mike > > > -----Original Message----- > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] > > Sent: Wednesday, February 28, 2018 6:21 AM > > To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming > > <liming.gao@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > Hey Laszlo, > > > > I cut your rant because it is not strictly related to this patch. > > However, thank you for composing it nevertheless because it was an > > interesting read! > > Comments are inline. > > > > Michael, Liming, > > Do you have any comments regarding the discussion? > > Thanks in advance. > > > > Best regards, > > Marvin. > > > > > -----Original Message----- > > > From: Laszlo Ersek <lersek@redhat.com> > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > devel@lists.01.org > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > > safe bitwise > > > operations. > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > [...] > > > > as edk2 does not support vendor extensions such as > > __int128 anyway. > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / > > INT128, in table 5, "Common > > > UEFI Data Types". I believe those typedefs may have > > been added for RISC-V. > > > > Oh yikes, I have not noticed that before. Besides that I wonder how > > that will be implemented by edk2 for non- RISC-V platforms, maybe that > > should be considered? > > As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > > UINT64, later UINT128) should be introduced and any BIT or bitmask > > definition being explicitly casted to that? > > Are BIT definitions or masks occasionally used in preprocessor > > operations? That might break after all. > > Anyway, if that idea would be approved, there really would have to be > > a note regarding this design in some of the EDK2 specifications, > > probably C Code Style. > > > > [...] > > > > > > > -1) The 'truncating constant value' warning would > > probably need to be > > > > disabled globally, however I don't understand how > > an explicit cast is > > > > a problem anyway. > > > > > > > > Did I overlook anything contra regarding that? > > > > > > Hmmm... Do you think it could have a performance > > impact on 32-bit > > > platforms? (I don't think so, at least not in > > optimized / RELEASE > > > builds.) > > > > I don't think any proper optimizer would not optimize this. After all, > > it can not only evaluate the value directly and notice that the value > > does not reach into the 'long long range', but also consider the type > > of the other operand. > > > > [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 18:52 ` Marvin Häuser @ 2018-03-01 1:41 ` Kinney, Michael D 2018-03-01 11:10 ` Marvin Häuser 0 siblings, 1 reply; 18+ messages in thread From: Kinney, Michael D @ 2018-03-01 1:41 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org, Kinney, Michael D Cc: lersek@redhat.com, Gao, Liming Hi Marvin, Yes. I have been reading the thread. Lots of really good analysis. However, it does not make sense to me to add 'u' to the smaller BITxx, BASExx, and SIZExx macros if we have constants in other places that do not add the 'u'. I think this patch may increase the inconsistency of the whole tree. If we don’t make this change, what types of issues do we need to fix and what would the fix entail? Mike > -----Original Message----- > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] > Sent: Wednesday, February 28, 2018 10:52 AM > To: edk2-devel@lists.01.org; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: lersek@redhat.com; Gao, Liming > <liming.gao@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise operations. > > Hey Mike, > > You are right, the patch was premature because I did > not consider any 'incorrect' or 'clever' usages of > these definitions. > The problem is not primarily undefined behavior, but > implementation-defined behavior. > Any bitwise operation to a signed integer results in > implementation-defined behavior, which compilers > usually do not warn about, while well-defined behavior > is desirable. > > Have you read Laszlo's comments? They are quite good at > showing up what logics might be and are relied on, > which however are not guaranteed to be the case for > non-x86 architectures, > or even for x86 in case a development team decides to > change this behavior some day or a new toolchain not > having adopted them in the first place should be added. > > Furthermore, I don't think inconsistency between the > definitions generally is desirable. > > Thanks, > Marvin. > > > -----Original Message----- > > From: Kinney, Michael D <michael.d.kinney@intel.com> > > Sent: Wednesday, February 28, 2018 7:37 PM > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; > Kinney, Michael D > > <michael.d.kinney@intel.com> > > Cc: Gao, Liming <liming.gao@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise > > operations. > > > > Hi Marvin, > > > > I do not think add 'u' to the BITxx defines does not > seem to be a complete > > solution. Code can use integer constants in lots of > places including other > > #defines or inline in expressions. > > > > If we follow your suggestion wouldn’t we need to add > 'u' to every constant > > that does not start with a '-' > > and might potentially be used with a bit operation? > > > > Compilers are doing a good job of finding undefined > behavior. Isn’t that > > sufficient to fix the issues identified? > > > > Mike > > > > > -----Original Message----- > > > From: Marvin Häuser > [mailto:Marvin.Haeuser@outlook.com] > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > To: edk2-devel@lists.01.org; Laszlo Ersek > <lersek@redhat.com> > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; > Gao, Liming > > > <liming.gao@intel.com> > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > Ensure safe bitwise > > > operations. > > > > > > Hey Laszlo, > > > > > > I cut your rant because it is not strictly related > to this patch. > > > However, thank you for composing it nevertheless > because it was an > > > interesting read! > > > Comments are inline. > > > > > > Michael, Liming, > > > Do you have any comments regarding the discussion? > > > Thanks in advance. > > > > > > Best regards, > > > Marvin. > > > > > > > -----Original Message----- > > > > From: Laszlo Ersek <lersek@redhat.com> > > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; > edk2- > > > > devel@lists.01.org > > > > Cc: michael.d.kinney@intel.com; > liming.gao@intel.com > > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: > Ensure > > > safe bitwise > > > > operations. > > > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > > [...] > > > > > as edk2 does not support vendor extensions such > as > > > __int128 anyway. > > > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 > / > > > INT128, in table 5, "Common > > > > UEFI Data Types". I believe those typedefs may > have > > > been added for RISC-V. > > > > > > Oh yikes, I have not noticed that before. Besides > that I wonder how > > > that will be implemented by edk2 for non- RISC-V > platforms, maybe that > > > should be considered? > > > As ridiculous as it sounds, maybe some kind of > UINT_MAX type (now > > > UINT64, later UINT128) should be introduced and any > BIT or bitmask > > > definition being explicitly casted to that? > > > Are BIT definitions or masks occasionally used in > preprocessor > > > operations? That might break after all. > > > Anyway, if that idea would be approved, there > really would have to be > > > a note regarding this design in some of the EDK2 > specifications, > > > probably C Code Style. > > > > > > [...] > > > > > > > > > -1) The 'truncating constant value' warning > would > > > probably need to be > > > > > disabled globally, however I don't understand > how > > > an explicit cast is > > > > > a problem anyway. > > > > > > > > > > Did I overlook anything contra regarding that? > > > > > > > > Hmmm... Do you think it could have a performance > > > impact on 32-bit > > > > platforms? (I don't think so, at least not in > > > optimized / RELEASE > > > > builds.) > > > > > > I don't think any proper optimizer would not > optimize this. After all, > > > it can not only evaluate the value directly and > notice that the value > > > does not reach into the 'long long range', but also > consider the type > > > of the other operand. > > > > > > [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-03-01 1:41 ` Kinney, Michael D @ 2018-03-01 11:10 ` Marvin Häuser 2018-03-01 17:18 ` Kinney, Michael D 0 siblings, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-03-01 11:10 UTC (permalink / raw) To: edk2-devel@lists.01.org, Kinney, Michael D Cc: liming.gao@intel.com, lersek@redhat.com > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Thursday, March 1, 2018 2:42 AM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: lersek@redhat.com; Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Hi Marvin, > > Yes. I have been reading the thread. > > Lots of really good analysis. > > However, it does not make sense to me to add 'u' to the smaller BITxx, > BASExx, and SIZExx macros if we have constants in other places that do not > add the 'u'. I think this patch may increase the inconsistency of the whole > tree. Basically applying this to the BIT definitions first was to see whether the concept is percepted as a whole. Of course this should be applied to all definitions that are at some point used as a mask, which I continued to do locally. > > If we don’t make this change, what types of issues do we need to fix and > what would the fix entail? To be honest, actual issues are very unlikely to happen as all architectures supported by the specification use two-complements for negative values. Furthermore, all currently supported compilers implement bitwise operations for signed integers seemingly the same way as for unsigned types. However, if either will change in the future, code will silently break as in many mask operations will return values not intended by the code. If you are not interested in the solution concepts previously discussed, I propose as least a Unit Test to verify the operations used in praxis work out fine. Thanks, Marvin. > > Mike > > > -----Original Message----- > > From: Marvin Häuser [mailto:Marvin.Haeuser@outlook.com] > > Sent: Wednesday, February 28, 2018 10:52 AM > > To: edk2-devel@lists.01.org; Kinney, Michael D > > <michael.d.kinney@intel.com> > > Cc: lersek@redhat.com; Gao, Liming > > <liming.gao@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > Hey Mike, > > > > You are right, the patch was premature because I did not consider any > > 'incorrect' or 'clever' usages of these definitions. > > The problem is not primarily undefined behavior, but > > implementation-defined behavior. > > Any bitwise operation to a signed integer results in > > implementation-defined behavior, which compilers usually do not warn > > about, while well-defined behavior is desirable. > > > > Have you read Laszlo's comments? They are quite good at showing up > > what logics might be and are relied on, which however are not > > guaranteed to be the case for > > non-x86 architectures, > > or even for x86 in case a development team decides to change this > > behavior some day or a new toolchain not having adopted them in the > > first place should be added. > > > > Furthermore, I don't think inconsistency between the definitions > > generally is desirable. > > > > Thanks, > > Marvin. > > > > > -----Original Message----- > > > From: Kinney, Michael D <michael.d.kinney@intel.com> > > > Sent: Wednesday, February 28, 2018 7:37 PM > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>; > > Kinney, Michael D > > > <michael.d.kinney@intel.com> > > > Cc: Gao, Liming <liming.gao@intel.com> > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > > safe bitwise > > > operations. > > > > > > Hi Marvin, > > > > > > I do not think add 'u' to the BITxx defines does not > > seem to be a complete > > > solution. Code can use integer constants in lots of > > places including other > > > #defines or inline in expressions. > > > > > > If we follow your suggestion wouldn’t we need to add > > 'u' to every constant > > > that does not start with a '-' > > > and might potentially be used with a bit operation? > > > > > > Compilers are doing a good job of finding undefined > > behavior. Isn’t that > > > sufficient to fix the issues identified? > > > > > > Mike > > > > > > > -----Original Message----- > > > > From: Marvin Häuser > > [mailto:Marvin.Haeuser@outlook.com] > > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > > To: edk2-devel@lists.01.org; Laszlo Ersek > > <lersek@redhat.com> > > > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; > > Gao, Liming > > > > <liming.gao@intel.com> > > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > > Ensure safe bitwise > > > > operations. > > > > > > > > Hey Laszlo, > > > > > > > > I cut your rant because it is not strictly related > > to this patch. > > > > However, thank you for composing it nevertheless > > because it was an > > > > interesting read! > > > > Comments are inline. > > > > > > > > Michael, Liming, > > > > Do you have any comments regarding the discussion? > > > > Thanks in advance. > > > > > > > > Best regards, > > > > Marvin. > > > > > > > > > -----Original Message----- > > > > > From: Laszlo Ersek <lersek@redhat.com> > > > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; > > edk2- > > > > > devel@lists.01.org > > > > > Cc: michael.d.kinney@intel.com; > > liming.gao@intel.com > > > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: > > Ensure > > > > safe bitwise > > > > > operations. > > > > > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > > > [...] > > > > > > as edk2 does not support vendor extensions such > > as > > > > __int128 anyway. > > > > > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 > > / > > > > INT128, in table 5, "Common > > > > > UEFI Data Types". I believe those typedefs may > > have > > > > been added for RISC-V. > > > > > > > > Oh yikes, I have not noticed that before. Besides > > that I wonder how > > > > that will be implemented by edk2 for non- RISC-V > > platforms, maybe that > > > > should be considered? > > > > As ridiculous as it sounds, maybe some kind of > > UINT_MAX type (now > > > > UINT64, later UINT128) should be introduced and any > > BIT or bitmask > > > > definition being explicitly casted to that? > > > > Are BIT definitions or masks occasionally used in > > preprocessor > > > > operations? That might break after all. > > > > Anyway, if that idea would be approved, there > > really would have to be > > > > a note regarding this design in some of the EDK2 > > specifications, > > > > probably C Code Style. > > > > > > > > [...] > > > > > > > > > > > -1) The 'truncating constant value' warning > > would > > > > probably need to be > > > > > > disabled globally, however I don't understand > > how > > > > an explicit cast is > > > > > > a problem anyway. > > > > > > > > > > > > Did I overlook anything contra regarding that? > > > > > > > > > > Hmmm... Do you think it could have a performance > > > > impact on 32-bit > > > > > platforms? (I don't think so, at least not in > > > > optimized / RELEASE > > > > > builds.) > > > > > > > > I don't think any proper optimizer would not > > optimize this. After all, > > > > it can not only evaluate the value directly and > > notice that the value > > > > does not reach into the 'long long range', but also > > consider the type > > > > of the other operand. > > > > > > > > [...] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-03-01 11:10 ` Marvin Häuser @ 2018-03-01 17:18 ` Kinney, Michael D 2018-03-01 17:28 ` Marvin Häuser 0 siblings, 1 reply; 18+ messages in thread From: Kinney, Michael D @ 2018-03-01 17:18 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org, Kinney, Michael D Cc: lersek@redhat.com, Gao, Liming Marvin, Thanks. I agree that there may be some compiler behavior assumptions. I would prefer we add to Base.h tests for the expected behavior assumptions and break the build if the compiler does not adhere to those assumptions. We have already added these to verify the size of base types and the size of enums. /** 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); Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel- > bounces@lists.01.org] On Behalf Of Marvin Häuser > Sent: Thursday, March 1, 2018 3:11 AM > To: edk2-devel@lists.01.org; Kinney, Michael D > <michael.d.kinney@intel.com> > Cc: lersek@redhat.com; Gao, Liming > <liming.gao@intel.com> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise operations. > > > > -----Original Message----- > > From: Kinney, Michael D <michael.d.kinney@intel.com> > > Sent: Thursday, March 1, 2018 2:42 AM > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > devel@lists.01.org; Kinney, Michael D > <michael.d.kinney@intel.com> > > Cc: lersek@redhat.com; Gao, Liming > <liming.gao@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise > > operations. > > > > Hi Marvin, > > > > Yes. I have been reading the thread. > > > > Lots of really good analysis. > > > > However, it does not make sense to me to add 'u' to > the smaller BITxx, > > BASExx, and SIZExx macros if we have constants in > other places that do not > > add the 'u'. I think this patch may increase the > inconsistency of the whole > > tree. > > Basically applying this to the BIT definitions first > was to see whether the concept is percepted as a whole. > Of course this should be applied to all definitions > that are at some point used as a mask, which I > continued to do locally. > > > > > If we don’t make this change, what types of issues do > we need to fix and > > what would the fix entail? > > To be honest, actual issues are very unlikely to happen > as all architectures supported by the specification use > two-complements for negative values. > Furthermore, all currently supported compilers > implement bitwise operations for signed integers > seemingly the same way as for unsigned types. > However, if either will change in the future, code will > silently break as in many mask operations will return > values not intended by the code. > > If you are not interested in the solution concepts > previously discussed, I propose as least a Unit Test to > verify the operations used in praxis work out fine. > > Thanks, > Marvin. > > > > > Mike > > > > > -----Original Message----- > > > From: Marvin Häuser > [mailto:Marvin.Haeuser@outlook.com] > > > Sent: Wednesday, February 28, 2018 10:52 AM > > > To: edk2-devel@lists.01.org; Kinney, Michael D > > > <michael.d.kinney@intel.com> > > > Cc: lersek@redhat.com; Gao, Liming > > > <liming.gao@intel.com> > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > Ensure safe bitwise > > > operations. > > > > > > Hey Mike, > > > > > > You are right, the patch was premature because I > did not consider any > > > 'incorrect' or 'clever' usages of these > definitions. > > > The problem is not primarily undefined behavior, > but > > > implementation-defined behavior. > > > Any bitwise operation to a signed integer results > in > > > implementation-defined behavior, which compilers > usually do not warn > > > about, while well-defined behavior is desirable. > > > > > > Have you read Laszlo's comments? They are quite > good at showing up > > > what logics might be and are relied on, which > however are not > > > guaranteed to be the case for > > > non-x86 architectures, > > > or even for x86 in case a development team decides > to change this > > > behavior some day or a new toolchain not having > adopted them in the > > > first place should be added. > > > > > > Furthermore, I don't think inconsistency between > the definitions > > > generally is desirable. > > > > > > Thanks, > > > Marvin. > > > > > > > -----Original Message----- > > > > From: Kinney, Michael D > <michael.d.kinney@intel.com> > > > > Sent: Wednesday, February 28, 2018 7:37 PM > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; > edk2- > > > > devel@lists.01.org; Laszlo Ersek > <lersek@redhat.com>; > > > Kinney, Michael D > > > > <michael.d.kinney@intel.com> > > > > Cc: Gao, Liming <liming.gao@intel.com> > > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > Ensure > > > safe bitwise > > > > operations. > > > > > > > > Hi Marvin, > > > > > > > > I do not think add 'u' to the BITxx defines does > not > > > seem to be a complete > > > > solution. Code can use integer constants in lots > of > > > places including other > > > > #defines or inline in expressions. > > > > > > > > If we follow your suggestion wouldn’t we need to > add > > > 'u' to every constant > > > > that does not start with a '-' > > > > and might potentially be used with a bit > operation? > > > > > > > > Compilers are doing a good job of finding > undefined > > > behavior. Isn’t that > > > > sufficient to fix the issues identified? > > > > > > > > Mike > > > > > > > > > -----Original Message----- > > > > > From: Marvin Häuser > > > [mailto:Marvin.Haeuser@outlook.com] > > > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > > > To: edk2-devel@lists.01.org; Laszlo Ersek > > > <lersek@redhat.com> > > > > > Cc: Kinney, Michael D > <michael.d.kinney@intel.com>; > > > Gao, Liming > > > > > <liming.gao@intel.com> > > > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > > > Ensure safe bitwise > > > > > operations. > > > > > > > > > > Hey Laszlo, > > > > > > > > > > I cut your rant because it is not strictly > related > > > to this patch. > > > > > However, thank you for composing it > nevertheless > > > because it was an > > > > > interesting read! > > > > > Comments are inline. > > > > > > > > > > Michael, Liming, > > > > > Do you have any comments regarding the > discussion? > > > > > Thanks in advance. > > > > > > > > > > Best regards, > > > > > Marvin. > > > > > > > > > > > -----Original Message----- > > > > > > From: Laszlo Ersek <lersek@redhat.com> > > > > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > > > > To: Marvin Häuser > <Marvin.Haeuser@outlook.com>; > > > edk2- > > > > > > devel@lists.01.org > > > > > > Cc: michael.d.kinney@intel.com; > > > liming.gao@intel.com > > > > > > Subject: Re: [edk2] [PATCH 1/2] > MdePkg/Base.h: > > > Ensure > > > > > safe bitwise > > > > > > operations. > > > > > > > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > > > > [...] > > > > > > > as edk2 does not support vendor extensions > such > > > as > > > > > __int128 anyway. > > > > > > > > > > > > Not *yet*, I guess :) UEFI 2.7 does list > UINT128 > > > / > > > > > INT128, in table 5, "Common > > > > > > UEFI Data Types". I believe those typedefs > may > > > have > > > > > been added for RISC-V. > > > > > > > > > > Oh yikes, I have not noticed that before. > Besides > > > that I wonder how > > > > > that will be implemented by edk2 for non- RISC- > V > > > platforms, maybe that > > > > > should be considered? > > > > > As ridiculous as it sounds, maybe some kind of > > > UINT_MAX type (now > > > > > UINT64, later UINT128) should be introduced and > any > > > BIT or bitmask > > > > > definition being explicitly casted to that? > > > > > Are BIT definitions or masks occasionally used > in > > > preprocessor > > > > > operations? That might break after all. > > > > > Anyway, if that idea would be approved, there > > > really would have to be > > > > > a note regarding this design in some of the > EDK2 > > > specifications, > > > > > probably C Code Style. > > > > > > > > > > [...] > > > > > > > > > > > > > -1) The 'truncating constant value' warning > > > would > > > > > probably need to be > > > > > > > disabled globally, however I don't > understand > > > how > > > > > an explicit cast is > > > > > > > a problem anyway. > > > > > > > > > > > > > > Did I overlook anything contra regarding > that? > > > > > > > > > > > > Hmmm... Do you think it could have a > performance > > > > > impact on 32-bit > > > > > > platforms? (I don't think so, at least not in > > > > > optimized / RELEASE > > > > > > builds.) > > > > > > > > > > I don't think any proper optimizer would not > > > optimize this. After all, > > > > > it can not only evaluate the value directly and > > > notice that the value > > > > > does not reach into the 'long long range', but > also > > > consider the type > > > > > of the other operand. > > > > > > > > > > [...] > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-03-01 17:18 ` Kinney, Michael D @ 2018-03-01 17:28 ` Marvin Häuser 0 siblings, 0 replies; 18+ messages in thread From: Marvin Häuser @ 2018-03-01 17:28 UTC (permalink / raw) To: edk2-devel@lists.01.org, Kinney, Michael D Cc: liming.gao@intel.com, lersek@redhat.com Hey Mike, Thanks for your reply. Under these circumstances I will not submit a V2. I hope you find a decent set of tests to be performed. Regards, Marvin. > -----Original Message----- > From: Kinney, Michael D <michael.d.kinney@intel.com> > Sent: Thursday, March 1, 2018 6:19 PM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > Cc: lersek@redhat.com; Gao, Liming <liming.gao@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Marvin, > > Thanks. I agree that there may be some compiler behavior assumptions. > > I would prefer we add to Base.h tests for the expected behavior > assumptions and break the build if the compiler does not adhere to those > assumptions. We have already added these to verify the size of base types > and the size of enums. > > /** > 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); > > > Mike > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel- > > bounces@lists.01.org] On Behalf Of Marvin Häuser > > Sent: Thursday, March 1, 2018 3:11 AM > > To: edk2-devel@lists.01.org; Kinney, Michael D > > <michael.d.kinney@intel.com> > > Cc: lersek@redhat.com; Gao, Liming > > <liming.gao@intel.com> > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > > > > -----Original Message----- > > > From: Kinney, Michael D <michael.d.kinney@intel.com> > > > Sent: Thursday, March 1, 2018 2:42 AM > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > devel@lists.01.org; Kinney, Michael D > > <michael.d.kinney@intel.com> > > > Cc: lersek@redhat.com; Gao, Liming > > <liming.gao@intel.com> > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > > safe bitwise > > > operations. > > > > > > Hi Marvin, > > > > > > Yes. I have been reading the thread. > > > > > > Lots of really good analysis. > > > > > > However, it does not make sense to me to add 'u' to > > the smaller BITxx, > > > BASExx, and SIZExx macros if we have constants in > > other places that do not > > > add the 'u'. I think this patch may increase the > > inconsistency of the whole > > > tree. > > > > Basically applying this to the BIT definitions first was to see > > whether the concept is percepted as a whole. > > Of course this should be applied to all definitions that are at some > > point used as a mask, which I continued to do locally. > > > > > > > > If we don’t make this change, what types of issues do > > we need to fix and > > > what would the fix entail? > > > > To be honest, actual issues are very unlikely to happen as all > > architectures supported by the specification use two-complements for > > negative values. > > Furthermore, all currently supported compilers implement bitwise > > operations for signed integers seemingly the same way as for unsigned > > types. > > However, if either will change in the future, code will silently break > > as in many mask operations will return values not intended by the > > code. > > > > If you are not interested in the solution concepts previously > > discussed, I propose as least a Unit Test to verify the operations > > used in praxis work out fine. > > > > Thanks, > > Marvin. > > > > > > > > Mike > > > > > > > -----Original Message----- > > > > From: Marvin Häuser > > [mailto:Marvin.Haeuser@outlook.com] > > > > Sent: Wednesday, February 28, 2018 10:52 AM > > > > To: edk2-devel@lists.01.org; Kinney, Michael D > > > > <michael.d.kinney@intel.com> > > > > Cc: lersek@redhat.com; Gao, Liming <liming.gao@intel.com> > > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > > Ensure safe bitwise > > > > operations. > > > > > > > > Hey Mike, > > > > > > > > You are right, the patch was premature because I > > did not consider any > > > > 'incorrect' or 'clever' usages of these > > definitions. > > > > The problem is not primarily undefined behavior, > > but > > > > implementation-defined behavior. > > > > Any bitwise operation to a signed integer results > > in > > > > implementation-defined behavior, which compilers > > usually do not warn > > > > about, while well-defined behavior is desirable. > > > > > > > > Have you read Laszlo's comments? They are quite > > good at showing up > > > > what logics might be and are relied on, which > > however are not > > > > guaranteed to be the case for > > > > non-x86 architectures, > > > > or even for x86 in case a development team decides > > to change this > > > > behavior some day or a new toolchain not having > > adopted them in the > > > > first place should be added. > > > > > > > > Furthermore, I don't think inconsistency between > > the definitions > > > > generally is desirable. > > > > > > > > Thanks, > > > > Marvin. > > > > > > > > > -----Original Message----- > > > > > From: Kinney, Michael D > > <michael.d.kinney@intel.com> > > > > > Sent: Wednesday, February 28, 2018 7:37 PM > > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; > > edk2- > > > > > devel@lists.01.org; Laszlo Ersek > > <lersek@redhat.com>; > > > > Kinney, Michael D > > > > > <michael.d.kinney@intel.com> > > > > > Cc: Gao, Liming <liming.gao@intel.com> > > > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > > Ensure > > > > safe bitwise > > > > > operations. > > > > > > > > > > Hi Marvin, > > > > > > > > > > I do not think add 'u' to the BITxx defines does > > not > > > > seem to be a complete > > > > > solution. Code can use integer constants in lots > > of > > > > places including other > > > > > #defines or inline in expressions. > > > > > > > > > > If we follow your suggestion wouldn’t we need to > > add > > > > 'u' to every constant > > > > > that does not start with a '-' > > > > > and might potentially be used with a bit > > operation? > > > > > > > > > > Compilers are doing a good job of finding > > undefined > > > > behavior. Isn’t that > > > > > sufficient to fix the issues identified? > > > > > > > > > > Mike > > > > > > > > > > > -----Original Message----- > > > > > > From: Marvin Häuser > > > > [mailto:Marvin.Haeuser@outlook.com] > > > > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > > > > To: edk2-devel@lists.01.org; Laszlo Ersek > > > > <lersek@redhat.com> > > > > > > Cc: Kinney, Michael D > > <michael.d.kinney@intel.com>; > > > > Gao, Liming > > > > > > <liming.gao@intel.com> > > > > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > > > > Ensure safe bitwise > > > > > > operations. > > > > > > > > > > > > Hey Laszlo, > > > > > > > > > > > > I cut your rant because it is not strictly > > related > > > > to this patch. > > > > > > However, thank you for composing it > > nevertheless > > > > because it was an > > > > > > interesting read! > > > > > > Comments are inline. > > > > > > > > > > > > Michael, Liming, > > > > > > Do you have any comments regarding the > > discussion? > > > > > > Thanks in advance. > > > > > > > > > > > > Best regards, > > > > > > Marvin. > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Laszlo Ersek <lersek@redhat.com> > > > > > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > > > > > To: Marvin Häuser > > <Marvin.Haeuser@outlook.com>; > > > > edk2- > > > > > > > devel@lists.01.org > > > > > > > Cc: michael.d.kinney@intel.com; > > > > liming.gao@intel.com > > > > > > > Subject: Re: [edk2] [PATCH 1/2] > > MdePkg/Base.h: > > > > Ensure > > > > > > safe bitwise > > > > > > > operations. > > > > > > > > > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > > > > > [...] > > > > > > > > as edk2 does not support vendor extensions > > such > > > > as > > > > > > __int128 anyway. > > > > > > > > > > > > > > Not *yet*, I guess :) UEFI 2.7 does list > > UINT128 > > > > / > > > > > > INT128, in table 5, "Common > > > > > > > UEFI Data Types". I believe those typedefs > > may > > > > have > > > > > > been added for RISC-V. > > > > > > > > > > > > Oh yikes, I have not noticed that before. > > Besides > > > > that I wonder how > > > > > > that will be implemented by edk2 for non- RISC- > > V > > > > platforms, maybe that > > > > > > should be considered? > > > > > > As ridiculous as it sounds, maybe some kind of > > > > UINT_MAX type (now > > > > > > UINT64, later UINT128) should be introduced and > > any > > > > BIT or bitmask > > > > > > definition being explicitly casted to that? > > > > > > Are BIT definitions or masks occasionally used > > in > > > > preprocessor > > > > > > operations? That might break after all. > > > > > > Anyway, if that idea would be approved, there > > > > really would have to be > > > > > > a note regarding this design in some of the > > EDK2 > > > > specifications, > > > > > > probably C Code Style. > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > -1) The 'truncating constant value' warning > > > > would > > > > > > probably need to be > > > > > > > > disabled globally, however I don't > > understand > > > > how > > > > > > an explicit cast is > > > > > > > > a problem anyway. > > > > > > > > > > > > > > > > Did I overlook anything contra regarding > > that? > > > > > > > > > > > > > > Hmmm... Do you think it could have a > > performance > > > > > > impact on 32-bit > > > > > > > platforms? (I don't think so, at least not in > > > > > > optimized / RELEASE > > > > > > > builds.) > > > > > > > > > > > > I don't think any proper optimizer would not > > > > optimize this. After all, > > > > > > it can not only evaluate the value directly and > > > > notice that the value > > > > > > does not reach into the 'long long range', but > > also > > > > consider the type > > > > > > of the other operand. > > > > > > > > > > > > [...] > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 14:21 ` Marvin Häuser 2018-02-28 18:37 ` Kinney, Michael D @ 2018-02-28 18:45 ` Marvin Häuser 2018-02-28 21:07 ` Marvin Häuser 1 sibling, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-02-28 18:45 UTC (permalink / raw) To: edk2-devel@lists.01.org, Laszlo Ersek Cc: michael.d.kinney@intel.com, liming.gao@intel.com I have just locally updated all BIT defines to use the ULL prefix and added casts to defines using them. I did that to ensure that 1) inversions always produce the correct value and 2) assignments never result in implicit casts to a smaller int, which would raise a warning. After I was done doing it for MdePkg, a build showed that (N)ASM files consumed these definitions. I only see a bunch of possible solutions to that: * Prohibit the usage of such defines in assembly code (which I would strongly dislike). * Introduce a "DEFINE_BIT" macro which produces one definition for C code and one for assembly. * Rely on 'ULL' always producing the biggest possible value (including the 128-bit range new to the spec) or documenting an exception for it, and insist on the caller casting (which I would find quite ugly). * Scrap the patch and continue to rely on compiler-/architecture-specific behavior, which could cause issues seemingly randomly. Thanks, Marvin. > -----Original Message----- > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin > Häuser > Sent: Wednesday, February 28, 2018 3:21 PM > To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Hey Laszlo, > > I cut your rant because it is not strictly related to this patch. However, thank > you for composing it nevertheless because it was an interesting read! > Comments are inline. > > Michael, Liming, > Do you have any comments regarding the discussion? Thanks in advance. > > Best regards, > Marvin. > > > -----Original Message----- > > From: Laszlo Ersek <lersek@redhat.com> > > Sent: Wednesday, February 28, 2018 2:57 PM > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > devel@lists.01.org > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > On 02/28/18 12:43, Marvin Häuser wrote: > [...] > > > as edk2 does not support vendor extensions such as __int128 anyway. > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table 5, > > "Common UEFI Data Types". I believe those typedefs may have been > added for RISC-V. > > Oh yikes, I have not noticed that before. Besides that I wonder how that will > be implemented by edk2 for non-RISC-V platforms, maybe that should be > considered? > As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > UINT64, later UINT128) should be introduced and any BIT or bitmask > definition being explicitly casted to that? > Are BIT definitions or masks occasionally used in preprocessor operations? > That might break after all. > Anyway, if that idea would be approved, there really would have to be a > note regarding this design in some of the EDK2 specifications, probably C > Code Style. > > [...] > > > > > -1) The 'truncating constant value' warning would probably need to > > > be disabled globally, however I don't understand how an explicit > > > cast is a problem anyway. > > > > > > Did I overlook anything contra regarding that? > > > > Hmmm... Do you think it could have a performance impact on 32-bit > > platforms? (I don't think so, at least not in optimized / RELEASE > > builds.) > > I don't think any proper optimizer would not optimize this. After all, it can not > only evaluate the value directly and notice that the value does not reach into > the 'long long range', but also consider the type of the other operand. > > [...] > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 18:45 ` Marvin Häuser @ 2018-02-28 21:07 ` Marvin Häuser 2018-03-01 10:39 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Marvin Häuser @ 2018-02-28 21:07 UTC (permalink / raw) To: edk2-devel@lists.01.org, lersek@redhat.com Cc: michael.d.kinney@intel.com, liming.gao@intel.com One comment is inline. Thank you in advance, Marvin. > -----Original Message----- > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin > Häuser > Sent: Wednesday, February 28, 2018 7:46 PM > To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > I have just locally updated all BIT defines to use the ULL prefix and added > casts to defines using them. > I did that to ensure that 1) inversions always produce the correct value and 2) > assignments never result in implicit casts to a smaller int, which would raise a > warning. > > After I was done doing it for MdePkg, a build showed that (N)ASM files > consumed these definitions. > > I only see a bunch of possible solutions to that: > * Prohibit the usage of such defines in assembly code (which I would strongly > dislike). > * Introduce a "DEFINE_BIT" macro which produces one definition for C code > and one for assembly. I only just realized that including C headers was not a NASM feature, but it is actually edk2 invoking the PP. Might the best solution just be to introduce a casting macro, which casts when it's invoked for a C compiler and doesn't when it's invoked for an assembler? Basically would require nothing else than adding a "-D__EDK2_ASSEMBLER__" or something alike to the PP flags when applicable. Any opinion on that? > * Rely on 'ULL' always producing the biggest possible value (including the 128- > bit range new to the spec) or documenting an exception for it, and insist on > the caller casting (which I would find quite ugly). > * Scrap the patch and continue to rely on compiler-/architecture-specific > behavior, which could cause issues seemingly randomly. > > Thanks, > Marvin. > > > -----Original Message----- > > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin > > Häuser > > Sent: Wednesday, February 28, 2018 3:21 PM > > To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > Hey Laszlo, > > > > I cut your rant because it is not strictly related to this patch. > > However, thank you for composing it nevertheless because it was an > interesting read! > > Comments are inline. > > > > Michael, Liming, > > Do you have any comments regarding the discussion? Thanks in advance. > > > > Best regards, > > Marvin. > > > > > -----Original Message----- > > > From: Laszlo Ersek <lersek@redhat.com> > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > > > devel@lists.01.org > > > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > > operations. > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > [...] > > > > as edk2 does not support vendor extensions such as __int128 anyway. > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table > > > 5, "Common UEFI Data Types". I believe those typedefs may have been > > added for RISC-V. > > > > Oh yikes, I have not noticed that before. Besides that I wonder how > > that will be implemented by edk2 for non-RISC-V platforms, maybe that > > should be considered? > > As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > > UINT64, later UINT128) should be introduced and any BIT or bitmask > > definition being explicitly casted to that? > > Are BIT definitions or masks occasionally used in preprocessor operations? > > That might break after all. > > Anyway, if that idea would be approved, there really would have to be > > a note regarding this design in some of the EDK2 specifications, > > probably C Code Style. > > > > [...] > > > > > > > -1) The 'truncating constant value' warning would probably need to > > > > be disabled globally, however I don't understand how an explicit > > > > cast is a problem anyway. > > > > > > > > Did I overlook anything contra regarding that? > > > > > > Hmmm... Do you think it could have a performance impact on 32-bit > > > platforms? (I don't think so, at least not in optimized / RELEASE > > > builds.) > > > > I don't think any proper optimizer would not optimize this. After all, > > it can not only evaluate the value directly and notice that the value > > does not reach into the 'long long range', but also consider the type of the > other operand. > > > > [...] > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-02-28 21:07 ` Marvin Häuser @ 2018-03-01 10:39 ` Laszlo Ersek 2018-03-01 11:25 ` Marvin Häuser 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2018-03-01 10:39 UTC (permalink / raw) To: Marvin Häuser, edk2-devel@lists.01.org Cc: michael.d.kinney@intel.com, liming.gao@intel.com On 02/28/18 22:07, Marvin Häuser wrote: > One comment is inline. > > Thank you in advance, > Marvin. > >> -----Original Message----- >> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin >> Häuser >> Sent: Wednesday, February 28, 2018 7:46 PM >> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> >> Cc: michael.d.kinney@intel.com; liming.gao@intel.com >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >> operations. >> >> I have just locally updated all BIT defines to use the ULL prefix and added >> casts to defines using them. >> I did that to ensure that 1) inversions always produce the correct value and 2) >> assignments never result in implicit casts to a smaller int, which would raise a >> warning. >> >> After I was done doing it for MdePkg, a build showed that (N)ASM files >> consumed these definitions. >> >> I only see a bunch of possible solutions to that: >> * Prohibit the usage of such defines in assembly code (which I would strongly >> dislike). >> * Introduce a "DEFINE_BIT" macro which produces one definition for C code >> and one for assembly. > > I only just realized that including C headers was not a NASM feature, but it is actually edk2 invoking the PP. > Might the best solution just be to introduce a casting macro, which casts when it's invoked for a C compiler and doesn't when it's invoked for an assembler? > Basically would require nothing else than adding a "-D__EDK2_ASSEMBLER__" or something alike to the PP flags when applicable. > > Any opinion on that? Sigh, I don't know what to answer. On one hand (if we can get it to work without regressions) I like the idea of making all BITx macros ULL. On the other hand, defining the same macro with different replacement text, dependent on whether the including source code is assembly or C, looks dirty. I can't really put my finger on it, but I feel such dual definitions could cause issues or confusion. If BaseTools people are OK with the dual definition, I guess I could live with it. Thanks, Laszlo > >> * Rely on 'ULL' always producing the biggest possible value (including the 128- >> bit range new to the spec) or documenting an exception for it, and insist on >> the caller casting (which I would find quite ugly). >> * Scrap the patch and continue to rely on compiler-/architecture-specific >> behavior, which could cause issues seemingly randomly. >> >> Thanks, >> Marvin. >> >>> -----Original Message----- >>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin >>> Häuser >>> Sent: Wednesday, February 28, 2018 3:21 PM >>> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> >>> Cc: michael.d.kinney@intel.com; liming.gao@intel.com >>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >>> operations. >>> >>> Hey Laszlo, >>> >>> I cut your rant because it is not strictly related to this patch. >>> However, thank you for composing it nevertheless because it was an >> interesting read! >>> Comments are inline. >>> >>> Michael, Liming, >>> Do you have any comments regarding the discussion? Thanks in advance. >>> >>> Best regards, >>> Marvin. >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek <lersek@redhat.com> >>>> Sent: Wednesday, February 28, 2018 2:57 PM >>>> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- >>>> devel@lists.01.org >>>> Cc: michael.d.kinney@intel.com; liming.gao@intel.com >>>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >>>> operations. >>>> >>>> On 02/28/18 12:43, Marvin Häuser wrote: >>> [...] >>>>> as edk2 does not support vendor extensions such as __int128 anyway. >>>> >>>> Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table >>>> 5, "Common UEFI Data Types". I believe those typedefs may have been >>> added for RISC-V. >>> >>> Oh yikes, I have not noticed that before. Besides that I wonder how >>> that will be implemented by edk2 for non-RISC-V platforms, maybe that >>> should be considered? >>> As ridiculous as it sounds, maybe some kind of UINT_MAX type (now >>> UINT64, later UINT128) should be introduced and any BIT or bitmask >>> definition being explicitly casted to that? >>> Are BIT definitions or masks occasionally used in preprocessor operations? >>> That might break after all. >>> Anyway, if that idea would be approved, there really would have to be >>> a note regarding this design in some of the EDK2 specifications, >>> probably C Code Style. >>> >>> [...] >>>> >>>>> -1) The 'truncating constant value' warning would probably need to >>>>> be disabled globally, however I don't understand how an explicit >>>>> cast is a problem anyway. >>>>> >>>>> Did I overlook anything contra regarding that? >>>> >>>> Hmmm... Do you think it could have a performance impact on 32-bit >>>> platforms? (I don't think so, at least not in optimized / RELEASE >>>> builds.) >>> >>> I don't think any proper optimizer would not optimize this. After all, >>> it can not only evaluate the value directly and notice that the value >>> does not reach into the 'long long range', but also consider the type of the >> other operand. >>> >>> [...] >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations. 2018-03-01 10:39 ` Laszlo Ersek @ 2018-03-01 11:25 ` Marvin Häuser 0 siblings, 0 replies; 18+ messages in thread From: Marvin Häuser @ 2018-03-01 11:25 UTC (permalink / raw) To: edk2-devel@lists.01.org, Laszlo Ersek Cc: michael.d.kinney@intel.com, liming.gao@intel.com > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Thursday, March 1, 2018 11:40 AM > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > devel@lists.01.org > Cc: michael.d.kinney@intel.com; liming.gao@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > On 02/28/18 22:07, Marvin Häuser wrote: > > One comment is inline. > > > > Thank you in advance, > > Marvin. > > > >> -----Original Message----- > >> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of > >> Marvin Häuser > >> Sent: Wednesday, February 28, 2018 7:46 PM > >> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> > >> Cc: michael.d.kinney@intel.com; liming.gao@intel.com > >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > >> operations. > >> > >> I have just locally updated all BIT defines to use the ULL prefix and > >> added casts to defines using them. > >> I did that to ensure that 1) inversions always produce the correct > >> value and 2) assignments never result in implicit casts to a smaller > >> int, which would raise a warning. > >> > >> After I was done doing it for MdePkg, a build showed that (N)ASM > >> files consumed these definitions. > >> > >> I only see a bunch of possible solutions to that: > >> * Prohibit the usage of such defines in assembly code (which I would > >> strongly dislike). > >> * Introduce a "DEFINE_BIT" macro which produces one definition for C > >> code and one for assembly. > > > > I only just realized that including C headers was not a NASM feature, but it > is actually edk2 invoking the PP. > > Might the best solution just be to introduce a casting macro, which casts > when it's invoked for a C compiler and doesn't when it's invoked for an > assembler? > > Basically would require nothing else than adding a "- > D__EDK2_ASSEMBLER__" or something alike to the PP flags when applicable. > > > > Any opinion on that? > > Sigh, I don't know what to answer. On one hand (if we can get it to work > without regressions) I like the idea of making all BITx macros ULL. On the > other hand, defining the same macro with different replacement text, > dependent on whether the including source code is assembly or C, looks > dirty. I can't really put my finger on it, but I feel such dual definitions could > cause issues or confusion. If BaseTools people are OK with the dual > definition, I guess I could live with it. Indeed it is dirty, however I don't think there is any choice but the smallest devil. Leaving them signed might become dangerous, relying on suffixes is not a proper solution considering the new 128-bit type and casting results in the sharing issue between C and NASM. Actually I would abandon the "two definitions" concept as of the idea of introducing __EDK2_ASSEMBLER__. The solution I think would be the best to ensure a safe and forward-compatible is: 1) Cast all generic defines that might be used as masks to the highest available integer type (macro), including BITx. 2) Introduce a casting macro which would roughly look like this and apply it to all "named bit" definitions: #ifdef __EDK2_ASSEMLER__ #define PP_CAST(Value, Type) (Value) #else #define PP_CAST(Value, Type) ((Type)(Value)) #endif This way: * Bit operations on all types of unsigned integers are safe and well-defined. * One can intuitively use inverses for both generic and "named" masks. * One can continue to intuitively assign "named bits" to variables of their type (except for when integer promotion happens as part of an OP, of course, but this is unrelated). * Code not casting correctly will raise compile-time errors. The only alternative worth arguing I see is scrapping it all and introducing Unit Tests. However, should a Unit Test ever fail a specific compiler, we would be back here again. Regards, Marvin. > > Thanks, > Laszlo > > > > >> * Rely on 'ULL' always producing the biggest possible value > >> (including the 128- bit range new to the spec) or documenting an > >> exception for it, and insist on the caller casting (which I would find quite > ugly). > >> * Scrap the patch and continue to rely on > >> compiler-/architecture-specific behavior, which could cause issues > seemingly randomly. > >> > >> Thanks, > >> Marvin. > >> > >>> -----Original Message----- > >>> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of > >>> Marvin Häuser > >>> Sent: Wednesday, February 28, 2018 3:21 PM > >>> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com> > >>> Cc: michael.d.kinney@intel.com; liming.gao@intel.com > >>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > >>> operations. > >>> > >>> Hey Laszlo, > >>> > >>> I cut your rant because it is not strictly related to this patch. > >>> However, thank you for composing it nevertheless because it was an > >> interesting read! > >>> Comments are inline. > >>> > >>> Michael, Liming, > >>> Do you have any comments regarding the discussion? Thanks in > advance. > >>> > >>> Best regards, > >>> Marvin. > >>> > >>>> -----Original Message----- > >>>> From: Laszlo Ersek <lersek@redhat.com> > >>>> Sent: Wednesday, February 28, 2018 2:57 PM > >>>> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2- > >>>> devel@lists.01.org > >>>> Cc: michael.d.kinney@intel.com; liming.gao@intel.com > >>>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > >>>> operations. > >>>> > >>>> On 02/28/18 12:43, Marvin Häuser wrote: > >>> [...] > >>>>> as edk2 does not support vendor extensions such as __int128 > anyway. > >>>> > >>>> Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table > >>>> 5, "Common UEFI Data Types". I believe those typedefs may have been > >>> added for RISC-V. > >>> > >>> Oh yikes, I have not noticed that before. Besides that I wonder how > >>> that will be implemented by edk2 for non-RISC-V platforms, maybe > >>> that should be considered? > >>> As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > >>> UINT64, later UINT128) should be introduced and any BIT or bitmask > >>> definition being explicitly casted to that? > >>> Are BIT definitions or masks occasionally used in preprocessor > operations? > >>> That might break after all. > >>> Anyway, if that idea would be approved, there really would have to > >>> be a note regarding this design in some of the EDK2 specifications, > >>> probably C Code Style. > >>> > >>> [...] > >>>> > >>>>> -1) The 'truncating constant value' warning would probably need to > >>>>> be disabled globally, however I don't understand how an explicit > >>>>> cast is a problem anyway. > >>>>> > >>>>> Did I overlook anything contra regarding that? > >>>> > >>>> Hmmm... Do you think it could have a performance impact on 32-bit > >>>> platforms? (I don't think so, at least not in optimized / RELEASE > >>>> builds.) > >>> > >>> I don't think any proper optimizer would not optimize this. After > >>> all, it can not only evaluate the value directly and notice that the > >>> value does not reach into the 'long long range', but also consider > >>> the type of the > >> other operand. > >>> > >>> [...] > >>> > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org > >>> https://lists.01.org/mailman/listinfo/edk2-devel > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-03-01 17:22 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-02-27 16:47 [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations Marvin Häuser 2018-02-27 19:54 ` Laszlo Ersek 2018-02-27 20:31 ` Marvin Häuser 2018-02-28 11:00 ` Laszlo Ersek 2018-02-28 11:43 ` Marvin Häuser 2018-02-28 13:57 ` Laszlo Ersek 2018-02-28 14:01 ` Laszlo Ersek 2018-02-28 14:21 ` Marvin Häuser 2018-02-28 18:37 ` Kinney, Michael D 2018-02-28 18:52 ` Marvin Häuser 2018-03-01 1:41 ` Kinney, Michael D 2018-03-01 11:10 ` Marvin Häuser 2018-03-01 17:18 ` Kinney, Michael D 2018-03-01 17:28 ` Marvin Häuser 2018-02-28 18:45 ` Marvin Häuser 2018-02-28 21:07 ` Marvin Häuser 2018-03-01 10:39 ` Laszlo Ersek 2018-03-01 11:25 ` Marvin Häuser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox