Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32027
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M payloads/libpayload/include/stdlib.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h 5 files changed, 113 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/1
diff --git a/payloads/libpayload/include/compiler.h b/payloads/libpayload/include/compiler.h index a830239..4ca9bad 100644 --- a/payloads/libpayload/include/compiler.h +++ b/payloads/libpayload/include/compiler.h @@ -26,4 +26,18 @@ #define __always_unused __attribute__((unused)) #define __must_check __attribute__((warn_unused_result))
+/* This evaluates to the first expression, unless that is constant in which case + it evalutates to the second. This is useful to write typeof() macros in a way + that they don't get hung up on the type of literal expressions (e.g. compare + an integer literal which is signed by default with a size_t variable). It is + generally a safe thing to do because if the constant doesn't fit into the + chosen type, you'll get a -Werror=overflow at compile time. */ +#define __SELECT_NON_CONST(expr, fallback_expr) __builtin_choose_expr( \ + __builtin_constant_p(expr), fallback_expr, expr) + +/* This creates a unique local variable name for use in macros. */ +#define __TMPNAME_3(i) __tmpname_##i +#define __TMPNAME_2(i) __TMPNAME_3(i) +#define __TMPNAME __TMPNAME_2(__COUNTER__) + #endif diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 0b9ab0d..c889605 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -66,10 +66,32 @@ #include <pci.h> #include <archive.h>
-#define MIN(a,b) ((a) < (b) ? (a) : (b)) -#define MAX(a,b) ((a) > (b) ? (a) : (b)) +#define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) + +#define __CMP_SAFE(a, b, op, var_a, var_b) ({ \ + __typeof__(__SELECT_NON_CONST(a, b)) var_a = a; \ + __typeof__(__SELECT_NON_CONST(b, a)) var_b = b; \ + var_a op var_b ? var_a : var_b; \ +}) + +#define __CMP(a, b, op) (__builtin_constant_p(a) && __builtin_constant_p(b) ? \ + __CMP_UNSAFE(a, b, op) : __CMP_SAFE(a, b, op, __TMPNAME, __TMPNAME)) + +#define MIN(a, b) __CMP(a, b, <) +#define MAX(a, b) __CMP(a, b, >) + +/* __typeof__ doesn't work on bitfield members, so provide these alternatives */ +#define MIN_UNSAFE(a, b) __CMP_UNSAFE(a, b, <) +#define MAX_UNSAFE(a, b) __CMP_UNSAFE(a, b, >) + #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
+#define DIV_ROUND_UP(x, y) ({ \ + __typeof__(x) _div_local_x = x; \ + __typeof__(y) _div_local_y = y; \ + ((_div_local_x) + (_div_local_y) - 1) / (_div_local_y); \ +}) + static inline u32 div_round_up(u32 n, u32 d) { return (n + d - 1) / d; }
#define LITTLE_ENDIAN 1234 diff --git a/payloads/libpayload/include/stdlib.h b/payloads/libpayload/include/stdlib.h index aaacc87..0b42ce2 100644 --- a/payloads/libpayload/include/stdlib.h +++ b/payloads/libpayload/include/stdlib.h @@ -35,8 +35,12 @@ #include <stddef.h> #include <string.h>
+#define __ALIGN_MASK(x, mask) ({ \ + typeof(mask) _align_mask_local_mask = mask; \ + ((x) + _align_mask_local_mask) & ~_align_mask_local_mask; \ +}) + #define ALIGN(x,a) __ALIGN_MASK(x,(typeof(x))(a)-1UL) -#define __ALIGN_MASK(x,mask) (((x)+(mask))&~(mask)) #define ALIGN_UP(x,a) ALIGN((x),(a)) #define ALIGN_DOWN(x,a) ((x) & ~((typeof(x))(a)-1UL)) #define IS_ALIGNED(x,a) (((x) & ((typeof(x))(a)-1UL)) == 0) diff --git a/src/commonlib/include/commonlib/compiler.h b/src/commonlib/include/commonlib/compiler.h index 2e0c454..e3d0f95 100644 --- a/src/commonlib/include/commonlib/compiler.h +++ b/src/commonlib/include/commonlib/compiler.h @@ -48,4 +48,18 @@ #define __always_inline inline __attribute__((always_inline)) #endif
+/* This evaluates to the first expression, unless that is constant in which case + it evalutates to the second. This is useful to write typeof() macros in a way + that they don't get hung up on the type of literal expressions (e.g. compare + an integer literal which is signed by default with a size_t variable). It is + generally a safe thing to do because if the constant doesn't fit into the + chosen type, you'll get a -Werror=overflow at compile time. */ +#define __SELECT_NON_CONST(expr, fallback_expr) __builtin_choose_expr( \ + __builtin_constant_p(expr), fallback_expr, expr) + +/* This creates a unique local variable name for use in macros. */ +#define __TMPNAME_3(i) __tmpname_##i +#define __TMPNAME_2(i) __TMPNAME_3(i) +#define __TMPNAME __TMPNAME_2(__COUNTER__) + #endif diff --git a/src/commonlib/include/commonlib/helpers.h b/src/commonlib/include/commonlib/helpers.h index adc43ca..65c122e 100644 --- a/src/commonlib/include/commonlib/helpers.h +++ b/src/commonlib/include/commonlib/helpers.h @@ -23,41 +23,76 @@ #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) #endif
+#define __ALIGN_MASK(x, mask) ({ \ + __typeof__(mask) _align_mask_local_mask = mask; \ + ((x) + _align_mask_local_mask) & ~_align_mask_local_mask; \ +}) + #define ALIGN(x, a) __ALIGN_MASK(x, (__typeof__(x))(a)-1UL) -#define __ALIGN_MASK(x, mask) (((x)+(mask))&~(mask)) #define ALIGN_UP(x, a) ALIGN((x), (a)) #define ALIGN_DOWN(x, a) ((x) & ~((__typeof__(x))(a)-1UL)) #define IS_ALIGNED(x, a) (((x) & ((__typeof__(x))(a)-1UL)) == 0)
+#define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) + +#define __CMP_SAFE(a, b, op, var_a, var_b) ({ \ + __typeof__(__SELECT_NON_CONST(a, b)) var_a = a; \ + __typeof__(__SELECT_NON_CONST(b, a)) var_b = b; \ + var_a op var_b ? var_a : var_b; \ +}) + +#define __CMP(a, b, op) __builtin_choose_expr( \ + __builtin_constant_p(a) && __builtin_constant_p(b), \ + __CMP_UNSAFE(a, b, op), __CMP_SAFE(a, b, op, __TMPNAME, __TMPNAME)) + #ifndef MIN -#define MIN(a, b) ((a) < (b) ? (a) : (b)) +#define MIN(a, b) __CMP(a, b, <) #endif #ifndef MAX -#define MAX(a, b) ((a) > (b) ? (a) : (b)) +#define MAX(a, b) __CMP(a, b, >) #endif -#define ABS(a) (((a) < 0) ? (-(a)) : (a)) -#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0) -#define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y)) -#define SWAP(a, b) do { \ - typeof(a) tmp = a; \ - a = (typeof(a)) b; \ - b = (typeof(b)) tmp; \ - } while (0) + +/* __typeof__ doesn't work on bitfield members, so provide these alternatives */ +#define MIN_UNSAFE(a, b) __CMP_UNSAFE(a, b, <) +#define MAX_UNSAFE(a, b) __CMP_UNSAFE(a, b, >) + +#define ABS(a) ({ \ + __typeof__(a) _abs_local_a = a; \ + ((_abs_local_a) < 0) ? (-(_abs_local_a)) : (_abs_local_a); \ +}) + +#define IS_POWER_OF_2(x) ({ \ + __typeof__(x) _power_local_x = x; \ + ((_power_local_x) & ((_power_local_x) - 1)) == 0; \ +}) + +#define DIV_ROUND_UP(x, y) ({ \ + __typeof__(x) _div_local_x = x; \ + __typeof__(y) _div_local_y = y; \ + ((_div_local_x) + (_div_local_y) - 1) / (_div_local_y); \ +}) + +#define SWAP(a, b) do { \ + __typeof__(&a) _swap_local_a = &a; \ + __typeof__(&b) _swap_local_b = &b; \ + __typeof__(a) _swap_local_tmp = *_swap_local_a; \ + *_swap_local_a = *_swap_local_b; \ + *_swap_local_b = _swap_local_tmp; \ +} while (0) + /* * Divide positive or negative dividend by positive divisor and round * to closest integer. Result is undefined for negative divisors and * for negative dividends if the divisor variable type is unsigned. */ -#define DIV_ROUND_CLOSEST(x, divisor)( \ -{ \ - typeof(x) __x = x; \ - typeof(divisor) __d = divisor; \ - (((typeof(x))-1) > 0 || \ - ((typeof(divisor))-1) > 0 || (__x) > 0) ? \ - (((__x) + ((__d) / 2)) / (__d)) : \ - (((__x) - ((__d) / 2)) / (__d)); \ -} \ -) +#define DIV_ROUND_CLOSEST(x, divisor)({ \ + __typeof__(x) _div_local_x = x; \ + __typeof__(divisor) _div_local_d = divisor; \ + (((__typeof__(x))-1) > 0 || \ + ((__typeof__(divisor))-1) > 0 || (_div_local_x) > 0) ? \ + (((_div_local_x) + ((_div_local_d) / 2)) / (_div_local_d)) : \ + (((_div_local_x) - ((_div_local_d) / 2)) / (_div_local_d)); \ +})
/* Standard units. */ #define KiB (1<<10)
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/32027/1/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/1/payloads/libpayload/include/libpaylo... PS1, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/1/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/1/src/commonlib/include/commonlib/help... PS1, Line 36: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#2).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M payloads/libpayload/include/stdlib.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h 5 files changed, 113 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#3).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M payloads/libpayload/include/stdlib.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h 5 files changed, 116 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32027/3/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/3/payloads/libpayload/include/libpaylo... PS3, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/3/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/3/src/commonlib/include/commonlib/help... PS3, Line 37: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#4).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M payloads/libpayload/include/stdlib.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h 5 files changed, 116 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/32027/4/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/4/payloads/libpayload/include/libpaylo... PS4, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/4/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/4/src/commonlib/include/commonlib/help... PS4, Line 37: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#5).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M payloads/libpayload/include/stdlib.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c 7 files changed, 147 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32027/5/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/5/payloads/libpayload/include/libpaylo... PS5, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/5/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/5/src/commonlib/include/commonlib/help... PS5, Line 44: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/5/src/commonlib/include/commonlib/help... PS5, Line 103: __typeof__(x) _div_local_x = (x); \ line over 80 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#6).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c M src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h 7 files changed, 130 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32027/6/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/6/payloads/libpayload/include/libpaylo... PS6, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/6/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/6/src/commonlib/include/commonlib/help... PS6, Line 33: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#7).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c M src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h 9 files changed, 134 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/32027/7/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/7/payloads/libpayload/include/libpaylo... PS7, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/7/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/7/src/commonlib/include/commonlib/help... PS7, Line 33: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#8).
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
WIP: Make common macros double-evaluation safe
Just shaking the Jenkins tree to see what falls out for now...
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c M src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h 10 files changed, 140 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: WIP: Make common macros double-evaluation safe ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/#/c/32027/8/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/8/payloads/libpayload/include/libpaylo... PS8, Line 69: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/8/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/8/src/commonlib/include/commonlib/help... PS8, Line 33: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello Patrick Rudolph, Aaron Durbin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#9).
Change subject: Make common macros double-evaluation safe ......................................................................
Make common macros double-evaluation safe
I just got hit by a double-evaluation bug again, it's time to attempt to fix this once more. Unfortunately there are several issues that don't make this easy:
- bitfield variables don't support typeof() - local macro variables that shadow others trigger -Werror=shadow - sign warnings with integer literal and unsigned var in typeof-MIN() - ({ statement expressions }) can not be used outside functions - romcc doesn't support any of the fancy GCC/clang extensions
This patch tries to address all of them as far as possible with macro magic. We don't have the technology to solve the bitfield and non-function context issues yet (__builtin_choose_expr() still throws a "no statement expression outside a function" error if it's only in the branch that's not chosen, unfortunately), so we'll have to provide alternative macros for use in those cases (and we'll avoid making __ALIGN_MASK() double-evaluation safe for now, since it would be annoying to do that there and having an alignment mask with side effects seems very unlikely). romcc can continue using unsafe versions since we're hopefully not writing a lot of new code for it. Sign warnings can be avoided in literal/variable comparisons by always using the type of the variable there. Shadowing is avoided by picking very explicit local variable names and using a special __COUNTER__ solution for MIN() and MAX() (the only ones of these you're likely to nest).
Also add DIV_ROUND_UP() to libpayload since it's a generally quite useful thing to have.
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c M src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h 10 files changed, 131 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/#/c/32027/9/payloads/libpayload/include/libpaylo... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/9/payloads/libpayload/include/libpaylo... PS9, Line 70: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/9/src/commonlib/include/commonlib/help... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/9/src/commonlib/include/commonlib/help... PS9, Line 34: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32027
to look at the new patch set (#10).
Change subject: Make common macros double-evaluation safe ......................................................................
Make common macros double-evaluation safe
I just got hit by a double-evaluation bug again, it's time to attempt to fix this once more. Unfortunately there are several issues that don't make this easy:
- bitfield variables don't support typeof() - local macro variables that shadow others trigger -Werror=shadow - sign warnings with integer literal and unsigned var in typeof-MIN() - ({ statement expressions }) can not be used outside functions - romcc doesn't support any of the fancy GCC/clang extensions
This patch tries to address all of them as far as possible with macro magic. We don't have the technology to solve the bitfield and non-function context issues yet (__builtin_choose_expr() still throws a "no statement expression outside a function" error if it's only in the branch that's not chosen, unfortunately), so we'll have to provide alternative macros for use in those cases (and we'll avoid making __ALIGN_MASK() double-evaluation safe for now, since it would be annoying to do that there and having an alignment mask with side effects seems very unlikely). romcc can continue using unsafe versions since we're hopefully not writing a lot of new code for it. Sign warnings can be avoided in literal/variable comparisons by always using the type of the variable there. Shadowing is avoided by picking very explicit local variable names and using a special __COUNTER__ solution for MIN() and MAX() (the only ones of these you're likely to nest).
Also add DIV_ROUND_UP() to libpayload since it's a generally quite useful thing to have.
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c M src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h 10 files changed, 141 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/32027/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/#/c/32027/10/payloads/libpayload/include/libpayl... File payloads/libpayload/include/libpayload.h:
https://review.coreboot.org/#/c/32027/10/payloads/libpayload/include/libpayl... PS10, Line 70: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
https://review.coreboot.org/#/c/32027/10/src/commonlib/include/commonlib/hel... File src/commonlib/include/commonlib/helpers.h:
https://review.coreboot.org/#/c/32027/10/src/commonlib/include/commonlib/hel... PS10, Line 34: #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) space prohibited between function name and open parenthesis '('
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 10: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 10:
*ping*
Anyone got some time to look at my crazy macros? (It's perfectly safe, I assure you...)
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 10: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 10:
I won't expect you to resolve the ABS(INT_MIN) == INT_MIN despite INT_MIN < 0 issue ;-)
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Make common macros double-evaluation safe
I just got hit by a double-evaluation bug again, it's time to attempt to fix this once more. Unfortunately there are several issues that don't make this easy:
- bitfield variables don't support typeof() - local macro variables that shadow others trigger -Werror=shadow - sign warnings with integer literal and unsigned var in typeof-MIN() - ({ statement expressions }) can not be used outside functions - romcc doesn't support any of the fancy GCC/clang extensions
This patch tries to address all of them as far as possible with macro magic. We don't have the technology to solve the bitfield and non-function context issues yet (__builtin_choose_expr() still throws a "no statement expression outside a function" error if it's only in the branch that's not chosen, unfortunately), so we'll have to provide alternative macros for use in those cases (and we'll avoid making __ALIGN_MASK() double-evaluation safe for now, since it would be annoying to do that there and having an alignment mask with side effects seems very unlikely). romcc can continue using unsafe versions since we're hopefully not writing a lot of new code for it. Sign warnings can be avoided in literal/variable comparisons by always using the type of the variable there. Shadowing is avoided by picking very explicit local variable names and using a special __COUNTER__ solution for MIN() and MAX() (the only ones of these you're likely to nest).
Also add DIV_ROUND_UP() to libpayload since it's a generally quite useful thing to have.
Change-Id: Iea35156c9aa9f6f2c7b8f00991418b746f44315d Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32027 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Patrick Georgi pgeorgi@google.com --- M payloads/libpayload/include/compiler.h M payloads/libpayload/include/libpayload.h M src/commonlib/include/commonlib/compiler.h M src/commonlib/include/commonlib/helpers.h M src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c M src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c M src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h M src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h 10 files changed, 141 insertions(+), 46 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/payloads/libpayload/include/compiler.h b/payloads/libpayload/include/compiler.h index a830239..fa9c78b 100644 --- a/payloads/libpayload/include/compiler.h +++ b/payloads/libpayload/include/compiler.h @@ -26,4 +26,19 @@ #define __always_unused __attribute__((unused)) #define __must_check __attribute__((warn_unused_result))
+/* This evaluates to the type of the first expression, unless that is constant + in which case it evalutates to the type of the second. This is useful when + assigning macro parameters to temporary variables, because that would + normally circumvent the special loosened type promotion rules for integer + literals. By using this macro, the promotion can happen at the time the + literal is assigned to the temporary variable. If the literal doesn't fit in + the chosen type, -Werror=overflow will catch it, so this should be safe. */ +#define __TYPEOF_UNLESS_CONST(expr, fallback_expr) typeof( \ + __builtin_choose_expr(__builtin_constant_p(expr), fallback_expr, expr)) + +/* This creates a unique local variable name for use in macros. */ +#define __TMPNAME_3(i) __tmpname_##i +#define __TMPNAME_2(i) __TMPNAME_3(i) +#define __TMPNAME __TMPNAME_2(__COUNTER__) + #endif diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 0b9ab0d..3a84b3b 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -66,10 +66,32 @@ #include <pci.h> #include <archive.h>
-#define MIN(a,b) ((a) < (b) ? (a) : (b)) -#define MAX(a,b) ((a) > (b) ? (a) : (b)) +/* Double-evaluation unsafe min/max, for bitfields and outside of functions */ +#define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) +#define MIN_UNSAFE(a, b) __CMP_UNSAFE(a, b, <) +#define MAX_UNSAFE(a, b) __CMP_UNSAFE(a, b, >) + +#define __CMP_SAFE(a, b, op, var_a, var_b) ({ \ + __TYPEOF_UNLESS_CONST(a, b) var_a = (a); \ + __TYPEOF_UNLESS_CONST(b, a) var_b = (b); \ + var_a op var_b ? var_a : var_b; \ +}) + +#define __CMP(a, b, op) __builtin_choose_expr( \ + __builtin_constant_p(a) && __builtin_constant_p(b), \ + __CMP_UNSAFE(a, b, op), __CMP_SAFE(a, b, op, __TMPNAME, __TMPNAME)) + +#define MIN(a, b) __CMP(a, b, <) +#define MAX(a, b) __CMP(a, b, >) + #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
+#define DIV_ROUND_UP(x, y) ({ \ + typeof(x) _div_local_x = (x); \ + typeof(y) _div_local_y = (y); \ + (_div_local_x + _div_local_y - 1) / _div_local_y; \ +}) + static inline u32 div_round_up(u32 n, u32 d) { return (n + d - 1) / d; }
#define LITTLE_ENDIAN 1234 diff --git a/src/commonlib/include/commonlib/compiler.h b/src/commonlib/include/commonlib/compiler.h index 2e0c454..5078759 100644 --- a/src/commonlib/include/commonlib/compiler.h +++ b/src/commonlib/include/commonlib/compiler.h @@ -48,4 +48,19 @@ #define __always_inline inline __attribute__((always_inline)) #endif
+/* This evaluates to the type of the first expression, unless that is constant + in which case it evalutates to the type of the second. This is useful when + assigning macro parameters to temporary variables, because that would + normally circumvent the special loosened type promotion rules for integer + literals. By using this macro, the promotion can happen at the time the + literal is assigned to the temporary variable. If the literal doesn't fit in + the chosen type, -Werror=overflow will catch it, so this should be safe. */ +#define __TYPEOF_UNLESS_CONST(expr, fallback_expr) __typeof__( \ + __builtin_choose_expr(__builtin_constant_p(expr), fallback_expr, expr)) + +/* This creates a unique local variable name for use in macros. */ +#define __TMPNAME_3(i) __tmpname_##i +#define __TMPNAME_2(i) __TMPNAME_3(i) +#define __TMPNAME __TMPNAME_2(__COUNTER__) + #endif diff --git a/src/commonlib/include/commonlib/helpers.h b/src/commonlib/include/commonlib/helpers.h index adc43ca..4429ea4 100644 --- a/src/commonlib/include/commonlib/helpers.h +++ b/src/commonlib/include/commonlib/helpers.h @@ -16,6 +16,7 @@ /* This file is for helpers for both coreboot firmware and its utilities. */
#ifndef __ASSEMBLER__ +#include <commonlib/compiler.h> #include <stddef.h> #endif
@@ -29,35 +30,71 @@ #define ALIGN_DOWN(x, a) ((x) & ~((__typeof__(x))(a)-1UL)) #define IS_ALIGNED(x, a) (((x) & ((__typeof__(x))(a)-1UL)) == 0)
+/* Double-evaluation unsafe min/max, for bitfields and outside of functions */ +#define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) +#define MIN_UNSAFE(a, b) __CMP_UNSAFE(a, b, <) +#define MAX_UNSAFE(a, b) __CMP_UNSAFE(a, b, >) + +#define __CMP_SAFE(a, b, op, var_a, var_b) ({ \ + __TYPEOF_UNLESS_CONST(a, b) var_a = (a); \ + __TYPEOF_UNLESS_CONST(b, a) var_b = (b); \ + var_a op var_b ? var_a : var_b; \ +}) + +#ifdef __ROMCC__ /* romcc doesn't support __builtin_choose_expr() */ +#define __CMP(a, b, op) __CMP_UNSAFE(a, b, op) +#else +#define __CMP(a, b, op) __builtin_choose_expr( \ + __builtin_constant_p(a) && __builtin_constant_p(b), \ + __CMP_UNSAFE(a, b, op), __CMP_SAFE(a, b, op, __TMPNAME, __TMPNAME)) +#endif + #ifndef MIN -#define MIN(a, b) ((a) < (b) ? (a) : (b)) +#define MIN(a, b) __CMP(a, b, <) #endif #ifndef MAX -#define MAX(a, b) ((a) > (b) ? (a) : (b)) +#define MAX(a, b) __CMP(a, b, >) #endif -#define ABS(a) (((a) < 0) ? (-(a)) : (a)) -#define IS_POWER_OF_2(x) (((x) & ((x) - 1)) == 0) -#define DIV_ROUND_UP(x, y) (((x) + (y) - 1) / (y)) -#define SWAP(a, b) do { \ - typeof(a) tmp = a; \ - a = (typeof(a)) b; \ - b = (typeof(b)) tmp; \ - } while (0) + +#ifndef ABS +#define ABS(a) ({ \ + __typeof__(a) _abs_local_a = (a); \ + (_abs_local_a < 0) ? (-_abs_local_a) : _abs_local_a; \ +}) +#endif + +#define IS_POWER_OF_2(x) ({ \ + __typeof__(x) _power_local_x = (x); \ + (_power_local_x & (_power_local_x - 1)) == 0; \ +}) + +#define DIV_ROUND_UP(x, y) ({ \ + __typeof__(x) _div_local_x = (x); \ + __typeof__(y) _div_local_y = (y); \ + (_div_local_x + _div_local_y - 1) / _div_local_y; \ +}) + +#define SWAP(a, b) do { \ + __typeof__(&(a)) _swap_local_a = &(a); \ + __typeof__(&(b)) _swap_local_b = &(b); \ + __typeof__(a) _swap_local_tmp = *_swap_local_a; \ + *_swap_local_a = *_swap_local_b; \ + *_swap_local_b = _swap_local_tmp; \ +} while (0) + /* * Divide positive or negative dividend by positive divisor and round * to closest integer. Result is undefined for negative divisors and * for negative dividends if the divisor variable type is unsigned. */ -#define DIV_ROUND_CLOSEST(x, divisor)( \ -{ \ - typeof(x) __x = x; \ - typeof(divisor) __d = divisor; \ - (((typeof(x))-1) > 0 || \ - ((typeof(divisor))-1) > 0 || (__x) > 0) ? \ - (((__x) + ((__d) / 2)) / (__d)) : \ - (((__x) - ((__d) / 2)) / (__d)); \ -} \ -) +#define DIV_ROUND_CLOSEST(x, divisor)({ \ + __typeof__(x) _div_local_x = (x); \ + __typeof__(divisor) _div_local_d = (divisor); \ + (((__typeof__(x))-1) > 0 || \ + ((__typeof__(divisor))-1) > 0 || (_div_local_x) > 0) ? \ + ((_div_local_x + (_div_local_d / 2)) / _div_local_d) : \ + ((_div_local_x - (_div_local_d / 2)) / _div_local_d); \ +})
/* Standard units. */ #define KiB (1<<10) diff --git a/src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c b/src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c index 5c5ab45..69a60b1 100644 --- a/src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c +++ b/src/vendorcode/amd/agesa/f12/Proc/GNB/Gfx/GfxGmcInit.c @@ -355,12 +355,12 @@
if (ActiveChannel == 2) { // Both controllers enabled - GMMx277C.Field.ActRd = MIN (DctChannel[0].D18F2x0F4_x40.Field.Trcd, DctChannel[1].D18F2x0F4_x40.Field.Trcd) + 5; - GMMx277C.Field.RasMActRd = MIN ((DctChannel[0].D18F2x0F4_x40.Field.Trc + 11 - (DctChannel[0].D18F2x0F4_x40.Field.Trcd + 5)), + GMMx277C.Field.ActRd = MIN_UNSAFE (DctChannel[0].D18F2x0F4_x40.Field.Trcd, DctChannel[1].D18F2x0F4_x40.Field.Trcd) + 5; + GMMx277C.Field.RasMActRd = MIN_UNSAFE ((DctChannel[0].D18F2x0F4_x40.Field.Trc + 11 - (DctChannel[0].D18F2x0F4_x40.Field.Trcd + 5)), (DctChannel[1].D18F2x0F4_x40.Field.Trc + 11 - (DctChannel[1].D18F2x0F4_x40.Field.Trcd + 5))); - GMMx2780.Field.Ras2Ras = MIN (DctChannel[0].D18F2x0F4_x40.Field.Trc, DctChannel[1].D18F2x0F4_x40.Field.Trc) + 11 - 1; - GMMx2780.Field.Rp = MIN (DctChannel[0].D18F2x0F4_x40.Field.Trp, DctChannel[1].D18F2x0F4_x40.Field.Trp) + 5 - 1; - GMMx2780.Field.WrPlusRp = MIN ( + GMMx2780.Field.Ras2Ras = MIN_UNSAFE (DctChannel[0].D18F2x0F4_x40.Field.Trc, DctChannel[1].D18F2x0F4_x40.Field.Trc) + 11 - 1; + GMMx2780.Field.Rp = MIN_UNSAFE (DctChannel[0].D18F2x0F4_x40.Field.Trp, DctChannel[1].D18F2x0F4_x40.Field.Trp) + 5 - 1; + GMMx2780.Field.WrPlusRp = MIN_UNSAFE ( ((DctChannel[0].D18F2x084.Field.Twr == 0) ? 16 : ((DctChannel[0].D18F2x084.Field.Twr < 4) ? (DctChannel[0].D18F2x084.Field.Twr + 4) : (DctChannel[0].D18F2x084.Field.Twr * 2)) + DctChannel[0].D18F2x0F4_x40.Field.Trp + 5), @@ -368,7 +368,7 @@ ((DctChannel[1].D18F2x084.Field.Twr < 4) ? (DctChannel[1].D18F2x084.Field.Twr + 4) : (DctChannel[1].D18F2x084.Field.Twr * 2)) + DctChannel[1].D18F2x0F4_x40.Field.Trp + 5) ) - 1; - GMMx2780.Field.BusTurn = (MIN ( + GMMx2780.Field.BusTurn = (MIN_UNSAFE ( DctChannel[0].D18F2x084.Field.Tcwl + 5 + DctChannel[0].D18F2x0F4_x41.Field.Twtr + 4 + DctChannel[0].D18F2x08C.Field.TrwtTO + 2 , diff --git a/src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c b/src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c index 7929d02..b4fcfee 100644 --- a/src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c +++ b/src/vendorcode/amd/agesa/f14/Proc/GNB/Gfx/GfxGmcInit.c @@ -361,12 +361,12 @@
if (ActiveChannel == 2) { // Both controllers enabled - GMMx277C.Field.ActRd = MIN (DctChannel[0].D18F2x0F4_x40.Field.Trcd, DctChannel[1].D18F2x0F4_x40.Field.Trcd) + 5; - GMMx277C.Field.RasMActRd = MIN ((DctChannel[0].D18F2x0F4_x40.Field.Trc + 11 - (DctChannel[0].D18F2x0F4_x40.Field.Trcd + 5)), + GMMx277C.Field.ActRd = MIN_UNSAFE (DctChannel[0].D18F2x0F4_x40.Field.Trcd, DctChannel[1].D18F2x0F4_x40.Field.Trcd) + 5; + GMMx277C.Field.RasMActRd = MIN_UNSAFE ((DctChannel[0].D18F2x0F4_x40.Field.Trc + 11 - (DctChannel[0].D18F2x0F4_x40.Field.Trcd + 5)), (DctChannel[1].D18F2x0F4_x40.Field.Trc + 11 - (DctChannel[1].D18F2x0F4_x40.Field.Trcd + 5))); - GMMx2780.Field.Ras2Ras = MIN (DctChannel[0].D18F2x0F4_x40.Field.Trc, DctChannel[1].D18F2x0F4_x40.Field.Trc) + 11 - 1; - GMMx2780.Field.Rp = MIN (DctChannel[0].D18F2x0F4_x40.Field.Trp, DctChannel[1].D18F2x0F4_x40.Field.Trp) + 5 - 1; - GMMx2780.Field.WrPlusRp = MIN ( + GMMx2780.Field.Ras2Ras = MIN_UNSAFE (DctChannel[0].D18F2x0F4_x40.Field.Trc, DctChannel[1].D18F2x0F4_x40.Field.Trc) + 11 - 1; + GMMx2780.Field.Rp = MIN_UNSAFE (DctChannel[0].D18F2x0F4_x40.Field.Trp, DctChannel[1].D18F2x0F4_x40.Field.Trp) + 5 - 1; + GMMx2780.Field.WrPlusRp = MIN_UNSAFE ( ((DctChannel[0].D18F2x084.Field.Twr == 0) ? 16 : ((DctChannel[0].D18F2x084.Field.Twr < 4) ? (DctChannel[0].D18F2x084.Field.Twr + 4) : (DctChannel[0].D18F2x084.Field.Twr * 2)) + DctChannel[0].D18F2x0F4_x40.Field.Trp + 5), @@ -374,7 +374,7 @@ ((DctChannel[1].D18F2x084.Field.Twr < 4) ? (DctChannel[1].D18F2x084.Field.Twr + 4) : (DctChannel[1].D18F2x084.Field.Twr * 2)) + DctChannel[1].D18F2x0F4_x40.Field.Trp + 5) ) - 1; - GMMx2780.Field.BusTurn = (MIN ( + GMMx2780.Field.BusTurn = (MIN_UNSAFE ( DctChannel[0].D18F2x084.Field.Tcwl + 5 + DctChannel[0].D18F2x0F4_x41.Field.Twtr + 4 + DctChannel[0].D18F2x08C.Field.TrwtTO + 2 , diff --git a/src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c b/src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c index 3e151c1..75ec2fb 100644 --- a/src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c +++ b/src/vendorcode/amd/agesa/f15tn/Proc/GNB/Modules/GnbInitTN/GfxGmcInitTN.c @@ -340,30 +340,30 @@ GnbRegisterReadTN (TYPE_GMM , 0x27fc , &ex1061.Value, 0, GnbLibGetHeader (Gfx));
if (((DramChannelPresent & GNB_GFX_DRAM_CH_0_PRESENT) != 0) && ((DramChannelPresent & GNB_GFX_DRAM_CH_1_PRESENT) != 0)) { - ex1047.Field.ex1047_0 = (MIN (DctChannel.D18F2x200_dct0_mp0.Field.Trcd, DctChannel.D18F2x200_dct1_mp0.Field.Trcd) * scale_mp0) / 100; + ex1047.Field.ex1047_0 = (MIN_UNSAFE (DctChannel.D18F2x200_dct0_mp0.Field.Trcd, DctChannel.D18F2x200_dct1_mp0.Field.Trcd) * scale_mp0) / 100; ex1047.Field.ex1047_1 = ex1047.Field.ex1047_0; - ex1047.Field.ex1047_2 = (MIN ((DctChannel.D18F2x204_dct0_mp0.Field.Trc - DctChannel.D18F2x200_dct0_mp0.Field.Trcd), + ex1047.Field.ex1047_2 = (MIN_UNSAFE ((DctChannel.D18F2x204_dct0_mp0.Field.Trc - DctChannel.D18F2x200_dct0_mp0.Field.Trcd), (DctChannel.D18F2x204_dct1_mp0.Field.Trc - DctChannel.D18F2x200_dct1_mp0.Field.Trcd)) * scale_mp0) / 100; ex1047.Field.ex1047_3 = ex1047.Field.ex1047_2;
- ex1048.Field.ex1048_0 = (MIN (DctChannel.D18F2x204_dct0_mp0.Field.Trc, DctChannel.D18F2x204_dct1_mp0.Field.Trc) * scale_mp0) / 100; - ex1048.Field.ex1048_1 = (MIN (DctChannel.D18F2x200_dct0_mp0.Field.Trp, DctChannel.D18F2x200_dct1_mp0.Field.Trp) * scale_mp0) / 100; - ex1048.Field.ex1048_2 = (MIN ((DctChannel.D18F2x22C_dct0_mp0.Field.Twr + DctChannel.D18F2x200_dct0_mp0.Field.Trp), + ex1048.Field.ex1048_0 = (MIN_UNSAFE (DctChannel.D18F2x204_dct0_mp0.Field.Trc, DctChannel.D18F2x204_dct1_mp0.Field.Trc) * scale_mp0) / 100; + ex1048.Field.ex1048_1 = (MIN_UNSAFE (DctChannel.D18F2x200_dct0_mp0.Field.Trp, DctChannel.D18F2x200_dct1_mp0.Field.Trp) * scale_mp0) / 100; + ex1048.Field.ex1048_2 = (MIN_UNSAFE ((DctChannel.D18F2x22C_dct0_mp0.Field.Twr + DctChannel.D18F2x200_dct0_mp0.Field.Trp), (DctChannel.D18F2x22C_dct1_mp0.Field.Twr + DctChannel.D18F2x200_dct1_mp0.Field.Trp)) * scale_mp0) / 100; - ex1048.Field.ex1048_3 = ((MIN ((DctChannel.D18F2x20C_dct0_mp0.Field.Tcwl + 4 + DctChannel.D18F2x20C_dct0_mp0.Field.Twtr + DctChannel.D18F2x21C_dct0_mp0.Field.TrwtTO), + ex1048.Field.ex1048_3 = ((MIN_UNSAFE ((DctChannel.D18F2x20C_dct0_mp0.Field.Tcwl + 4 + DctChannel.D18F2x20C_dct0_mp0.Field.Twtr + DctChannel.D18F2x21C_dct0_mp0.Field.TrwtTO), (DctChannel.D18F2x20C_dct1_mp0.Field.Tcwl + 4 + DctChannel.D18F2x20C_dct1_mp0.Field.Twtr + DctChannel.D18F2x21C_dct1_mp0.Field.TrwtTO)) / 2) * scale_mp0) / 100;
- ex1060.Field.ex1060_0 = (MIN (DctChannel.D18F2x200_dct0_mp1.Field.Trcd, DctChannel.D18F2x200_dct1_mp1.Field.Trcd) * scale_mp1) / 100; + ex1060.Field.ex1060_0 = (MIN_UNSAFE (DctChannel.D18F2x200_dct0_mp1.Field.Trcd, DctChannel.D18F2x200_dct1_mp1.Field.Trcd) * scale_mp1) / 100; ex1060.Field.ex1060_1 = ex1060.Field.ex1060_0; - ex1060.Field.ex1060_2 = (MIN ((DctChannel.D18F2x204_dct0_mp1.Field.Trc - DctChannel.D18F2x200_dct0_mp1.Field.Trcd), + ex1060.Field.ex1060_2 = (MIN_UNSAFE ((DctChannel.D18F2x204_dct0_mp1.Field.Trc - DctChannel.D18F2x200_dct0_mp1.Field.Trcd), (DctChannel.D18F2x204_dct1_mp1.Field.Trc - DctChannel.D18F2x200_dct1_mp1.Field.Trcd)) * scale_mp1) / 100; ex1060.Field.ex1060_3 = ex1060.Field.ex1060_2;
- ex1061.Field.ex1061_0 = (MIN (DctChannel.D18F2x204_dct0_mp1.Field.Trc, DctChannel.D18F2x204_dct1_mp1.Field.Trc) * scale_mp1) / 100; - ex1061.Field.ex1061_1 = (MIN (DctChannel.D18F2x200_dct0_mp1.Field.Trp, DctChannel.D18F2x200_dct1_mp1.Field.Trp) * scale_mp1) / 100; - ex1061.Field.ex1061_2 = (MIN ((DctChannel.D18F2x22C_dct0_mp1.Field.Twr + DctChannel.D18F2x200_dct0_mp1.Field.Trp), + ex1061.Field.ex1061_0 = (MIN_UNSAFE (DctChannel.D18F2x204_dct0_mp1.Field.Trc, DctChannel.D18F2x204_dct1_mp1.Field.Trc) * scale_mp1) / 100; + ex1061.Field.ex1061_1 = (MIN_UNSAFE (DctChannel.D18F2x200_dct0_mp1.Field.Trp, DctChannel.D18F2x200_dct1_mp1.Field.Trp) * scale_mp1) / 100; + ex1061.Field.ex1061_2 = (MIN_UNSAFE ((DctChannel.D18F2x22C_dct0_mp1.Field.Twr + DctChannel.D18F2x200_dct0_mp1.Field.Trp), (DctChannel.D18F2x22C_dct1_mp1.Field.Twr + DctChannel.D18F2x200_dct1_mp1.Field.Trp)) * scale_mp1) / 100; - ex1061.Field.ex1061_3 = ((MIN ((DctChannel.D18F2x20C_dct0_mp1.Field.Tcwl + 4 + DctChannel.D18F2x20C_dct0_mp1.Field.Twtr + DctChannel.D18F2x21C_dct0_mp1.Field.TrwtTO), + ex1061.Field.ex1061_3 = ((MIN_UNSAFE ((DctChannel.D18F2x20C_dct0_mp1.Field.Tcwl + 4 + DctChannel.D18F2x20C_dct0_mp1.Field.Twtr + DctChannel.D18F2x21C_dct0_mp1.Field.TrwtTO), (DctChannel.D18F2x20C_dct1_mp1.Field.Tcwl + 4 + DctChannel.D18F2x20C_dct1_mp1.Field.Twtr + DctChannel.D18F2x21C_dct1_mp1.Field.TrwtTO)) / 2) * scale_mp1) / 100;
} else if ((DramChannelPresent & GNB_GFX_DRAM_CH_0_PRESENT) != 0) { diff --git a/src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h b/src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h index 70e4197..d0dd4c9 100644 --- a/src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h +++ b/src/vendorcode/intel/edk2/UDK2015/MdePkg/Include/Base.h @@ -735,8 +735,10 @@ @return The absolute value of the signed operand.
**/ +#ifndef ABS #define ABS(a) \ (((a) < 0) ? (-(a)) : (a)) +#endif
// // Status codes common to all execution phases diff --git a/src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h b/src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h index 694a94d..3a0cdb2 100644 --- a/src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h +++ b/src/vendorcode/intel/edk2/UDK2017/MdePkg/Include/Base.h @@ -941,8 +941,10 @@ @return The absolute value of the signed operand.
**/ +#ifndef ABS #define ABS(a) \ (((a) < 0) ? (-(a)) : (a)) +#endif
// // Status codes common to all execution phases diff --git a/src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h b/src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h index b7dc94d..c0ea9c1 100644 --- a/src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h +++ b/src/vendorcode/intel/edk2/uefi_2.4/MdePkg/Include/Base.h @@ -718,8 +718,10 @@ @return The absolute value of the signed operand.
**/ +#ifndef ABS #define ABS(a) \ (((a) < 0) ? (-(a)) : (a)) +#endif
// // Status codes common to all execution phases
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
I won't expect you to resolve the ABS(INT_MIN) == INT_MIN despite INT_MIN < 0 issue ;-)
I mean... it should be easy enough to check for if you want to?
if (_abs_local_a == (~(~(__typeof__(a))0) >> 1)) die(...);
The problem is that you can't do 'unsigned __typeof__(a)' so I don't think you could ever write this in a way that returns a valid result for that case (unless we just always make abs() return uint64_t). Also it's probably not enough of a problem to warrant the extra code size.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
I just got a report that this breaks the build on ubuntu 14.04. We might want to test it some more to make sure it doesn't break any other build environments.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
I just got a report that this breaks the build on ubuntu 14.04. We might want to test it some more to make sure it doesn't break any other build environments.
Do you have the error message? I assume it's a name collision with one of the new macros (maybe __CMP). It's impossible to predict all the possible conflicts with all possible environments this may have, but if we know which ones to check for, it should be easy to fix.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
Do you have the error message? I assume it's a name collision with one of the new macros(maybe __CMP). It's impossible to predict all the possible conflicts with all possible environments this may have, but if we know which ones to check for, it should be easy to fix.
Hey Julius,
It's clobbering my build of cbfstool. I don't yet have a strong sense of why:
coreboot/util/cbfstool/cbfstool.c: In function 'cbfs_add_component': coreboot/src/commonlib/include/commonlib/compiler.h:59:2: error: first argument to '__builtin_choose_expr' not a constant __builtin_choose_expr(__builtin_constant_p(expr), fallback_expr, expr)) ^ coreboot/src/commonlib/include/commonlib/helpers.h:39:2: note: in expansion of macro '__TYPEOF_UNLESS_CONST' __TYPEOF_UNLESS_CONST(a, b) var_a = (a); \ ^ coreboot/src/commonlib/include/commonlib/helpers.h:49:26: note: in expansion of macro '__CMP_SAFE' __CMP_UNSAFE(a, b, op), __CMP_SAFE(a, b, op, __TMPNAME, __TMPNAME)) ^ coreboot/src/commonlib/include/commonlib/helpers.h:56:19: note: in expansion of macro '__CMP' #define MAX(a, b) __CMP(a, b, >) ^ coreboot/util/cbfstool/cbfstool.c:546:19: note: in expansion of macro 'MAX' uint32_t size = MAX(hs, param.padding); ^ and the same problem for line 957: MAX(bootblock.size, sizeof(int32_t));
Is __builtin_constant_p() guaranteed to return a constant value? I get the impression it's not: https://www.lkml.org/lkml/2018/3/18/268
My crosstools are up to date (e.g. gcc at 8.2.0), but these were built on Unbuntu 14.04 system with gcc 4.8.4. I have a 17.04 VM sitting around, with older xgcc/bin files, and it builds fine. I'm building a plain google/grunt with no configuration changes.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
My crosstools are up to date (e.g. gcc at 8.2.0), but these were built on Unbuntu 14.04 system with gcc 4.8.4. I have a 17.04 VM sitting around, with older xgcc/bin files, and it builds fine. I'm building a plain google/grunt with no configuration changes.
Okay, this reads a lot like a compiler problem. I assume the result of __builtin_constant_p() used to not count as a constant due to some issue back in GCC 4.8.4 that was fixed in a later version. Can you check what version is on the 17.04 where it works?
I guess the question becomes what sort of compiler we need to support for build tools. I'd prefer if we can use the clean builtins that are intended for that purpose and not go through the same insanity that the kernel had to to create a constness-check that works on old compilers. We intentionally added the whole crossgcc thing to get rid of those issues.
Do we need to support Ubuntu 14.04? It's 5 years old, and it looks like even official Ubuntu LTS support for it is running out this month. Can we consider this WontFix? (The other option is to #ifdef this out for host utilities, but I'm not really sure we should be held back by ancient compilers. Or maybe we can find a way to have utils built by crossgcc as well, at least as long as we have one for that architecture?)
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
My crosstools are up to date (e.g. gcc at 8.2.0)...
Okay, this reads a lot like a compiler problem. I assume... ...We intentionally added the whole crossgcc thing to get rid of those issues.
Oh, I'm sorry for being slow. cbfstool.c gets compiled using $(HOSTCC), not any croosstools, so mentioning that was irrelevant.
Can you check what version is on the 17.04 where it works?
6.3.0 on 17.04. I found some more interesting discussion on compiler versions regarding solving the min/max problem in the kernel. I haven't found whether it was solved a different way vs. deciding on whether to raise the version requirement. But 4.6 might be the right minimum version. Middle of the discussion: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1633051.html
Do we need to support Ubuntu 14.04? It's 5 years old, and it looks like even official Ubuntu LTS support for it is running out this month.
That's what I'd said to Martin originally -- that the patch landed on the exact month/year the LTS ends, which is why I wasn't making a big deal of it. It's an inconvenience for me, but not the end of the world.
Can we consider this WontFix?
Probably.
(The other option is to #ifdef this out for host utilities, but I'm not really sure we should be held back by ancient compilers. Or maybe we can find a way to have utils built by crossgcc as well, at least as long as we have one for that architecture?)
I envision both those options being ugly. It seems like a better approach might be to decide that we have a minimum gcc version requirement, and be deliberate about what revision that is.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32027 )
Change subject: Make common macros double-evaluation safe ......................................................................
Patch Set 11:
I agree - I'm not super concerned about 14.04, but there are a lot of other environments that people compile in that we don't typically test. Windows with cygwin or mingw. The BSDs.
I guess I need to do some more work on getting builders set up for those to make sure things still work.
-
build bot (Jenkins) (Code Review) -
Julius Werner (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)