The changes to minmax.h that changed the type check to a signedness
check significantly increased the length of the expansion.
In some cases it has also significantly increased compile type.
This is particularly noticable for nested expansions.
The fact that _Static_assert() only requires a compile time constant
not a constant expression allows a lot of simplification.
The other thing that compilicates the expansion is the necessity of
returning a constant expression from constant arguments (for VLA).
I can only find a handful of places this is done.
Penalising most of the code for these few cases seems 'suboptimal'.
Instead I've added min_const() and max_const() for VLA and static
initialisers, these check the arguments are constant to avoid misuse.
Patch [9] is dependant on the earlier patches.
Patch [10] isn't dependant on them.
Patch [11] depends on both 9 and 10.
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
David Laight (11):
[1] minmax: Put all the clamp() definitions together
[2] minmax: Use _Static_assert() instead of static_assert()
[3] minmax: Simplify signedness check
[4] minmax: Replace multiple __UNIQUE_ID() by directly using __COUNTER__
[5] minmax: Move the signedness check out of __cmp_once() and
__clamp_once()
[6] minmax: Remove 'constexpr' check from __careful_clamp()
[7] minmax: minmax: Add __types_ok3() and optimise defines with 3
arguments
[8] minmax: Add min_const() and max_const()
[9] tree-wide: minmax: Replace all the uses of max() for array sizes with
max_const().
[10] block: Use a boolean expression instead of max() on booleans
[11] minmax: min() and max() don't need to return constant expressions
block/blk-settings.c | 2 +-
drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 2 +-
drivers/gpu/drm/drm_color_mgmt.c | 4 +-
drivers/input/touchscreen/cyttsp4_core.c | 2 +-
.../net/can/usb/etas_es58x/es58x_devlink.c | 2 +-
fs/btrfs/tree-checker.c | 2 +-
include/linux/minmax.h | 211 ++++++++++--------
lib/vsprintf.c | 4 +-
net/ipv4/proc.c | 2 +-
net/ipv6/proc.c | 2 +-
10 files changed, 127 insertions(+), 106 deletions(-)
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
The defines for clamp() have got separated, move togther for readability.
Update description of signedness check.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 120 +++++++++++++++++++----------------------
1 file changed, 56 insertions(+), 64 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 2ec559284a9f..63c45865b48a 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -57,26 +57,6 @@
__cmp(op, x, y), \
__cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
-#define __clamp(val, lo, hi) \
- ((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
-
-#define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({ \
- typeof(val) unique_val = (val); \
- typeof(lo) unique_lo = (lo); \
- typeof(hi) unique_hi = (hi); \
- static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
- (lo) <= (hi), true), \
- "clamp() low limit " #lo " greater than high limit " #hi); \
- static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
- static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
- __clamp(unique_val, unique_lo, unique_hi); })
-
-#define __careful_clamp(val, lo, hi) ({ \
- __builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)), \
- __clamp(val, lo, hi), \
- __clamp_once(val, lo, hi, __UNIQUE_ID(__val), \
- __UNIQUE_ID(__lo), __UNIQUE_ID(__hi))); })
-
/**
* min - return minimum of two values of the same or compatible types
* @x: first value
@@ -124,6 +104,22 @@
*/
#define max3(x, y, z) max((typeof(x))max(x, y), z)
+/**
+ * min_t - return minimum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define min_t(type, x, y) __careful_cmp(min, (type)(x), (type)(y))
+
+/**
+ * max_t - return maximum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define max_t(type, x, y) __careful_cmp(max, (type)(x), (type)(y))
+
/**
* min_not_zero - return the minimum that is _not_ zero, unless both are zero
* @x: value1
@@ -134,39 +130,60 @@
typeof(y) __y = (y); \
__x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
+#define __clamp(val, lo, hi) \
+ ((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
+
+#define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({ \
+ typeof(val) unique_val = (val); \
+ typeof(lo) unique_lo = (lo); \
+ typeof(hi) unique_hi = (hi); \
+ static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
+ (lo) <= (hi), true), \
+ "clamp() low limit " #lo " greater than high limit " #hi); \
+ static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
+ static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
+ __clamp(unique_val, unique_lo, unique_hi); })
+
+#define __careful_clamp(val, lo, hi) ({ \
+ __builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)), \
+ __clamp(val, lo, hi), \
+ __clamp_once(val, lo, hi, __UNIQUE_ID(__val), \
+ __UNIQUE_ID(__lo), __UNIQUE_ID(__hi))); })
+
/**
* clamp - return a value clamped to a given range with strict typechecking
* @val: current value
* @lo: lowest allowable value
* @hi: highest allowable value
*
- * This macro does strict typechecking of @lo/@hi to make sure they are of the
- * same type as @val. See the unnecessary pointer comparisons.
+ * This macro checks that @val, @lo and @hi have the same signedness.
*/
#define clamp(val, lo, hi) __careful_clamp(val, lo, hi)
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max/clamp at all, of course.
- */
-
/**
- * min_t - return minimum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
+ * clamp_t - return a value clamped to a given range using a given type
+ * @type: the type of variable to use
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of type
+ * @type to make all the comparisons.
*/
-#define min_t(type, x, y) __careful_cmp(min, (type)(x), (type)(y))
+#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
/**
- * max_t - return maximum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
+ * clamp_val - return a value clamped to a given range using val's type
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of whatever
+ * type the input argument @val is. This is useful when @val is an unsigned
+ * type and @lo and @hi are literals that will otherwise be assigned a signed
+ * integer type.
*/
-#define max_t(type, x, y) __careful_cmp(max, (type)(x), (type)(y))
+#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
/*
* Do not check the array parameter using __must_be_array().
@@ -211,31 +228,6 @@
*/
#define max_array(array, len) __minmax_array(max, array, len)
-/**
- * clamp_t - return a value clamped to a given range using a given type
- * @type: the type of variable to use
- * @val: current value
- * @lo: minimum allowable value
- * @hi: maximum allowable value
- *
- * This macro does no typechecking and uses temporary variables of type
- * @type to make all the comparisons.
- */
-#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
-
-/**
- * clamp_val - return a value clamped to a given range using val's type
- * @val: current value
- * @lo: minimum allowable value
- * @hi: maximum allowable value
- *
- * This macro does no typechecking and uses temporary variables of whatever
- * type the input argument @val is. This is useful when @val is an unsigned
- * type and @lo and @hi are literals that will otherwise be assigned a signed
- * integer type.
- */
-#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
-
static inline bool in_range64(u64 val, u64 start, u64 len)
{
return (val - start) < len;
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
The wrapper just adds two more lines of error output when the test fails.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 63c45865b48a..900eec7a28e5 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -48,7 +48,7 @@
#define __cmp_once(op, x, y, unique_x, unique_y) ({ \
typeof(x) unique_x = (x); \
typeof(y) unique_y = (y); \
- static_assert(__types_ok(x, y), \
+ _Static_assert(__types_ok(x, y), \
#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
__cmp(op, unique_x, unique_y); })
@@ -137,11 +137,11 @@
typeof(val) unique_val = (val); \
typeof(lo) unique_lo = (lo); \
typeof(hi) unique_hi = (hi); \
- static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
+ _Static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
(lo) <= (hi), true), \
"clamp() low limit " #lo " greater than high limit " #hi); \
- static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
- static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
+ _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
+ _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
__clamp(unique_val, unique_lo, unique_hi); })
#define __careful_clamp(val, lo, hi) ({ \
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
It is enough to check that both 'x' and 'y' are valid for either
a signed compare or an unsigned compare.
For unsigned they must be an unsigned type or a positive constant.
For signed they must be signed after unsigned char/short are promoted.
The predicate for _Static_assert() only needs to be a compile-time
constant not a constant integeger expression.
In particular the short-circuit evaluation of || && ?: can be used
to avoid the non-constantness of (pointer_type)1 in is_signed_type().
The '+ 0' in '(x) + 0 > = 0' is there to convert 'bool' to 'int'
and avoid a compiler warning because max() gets used for 'bool'
in one place (a very expensive 'or').
(The code is optimised away by two earlier checks - but the compiler
still bleats.)
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 900eec7a28e5..c32b4b40ce01 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -8,7 +8,7 @@
#include <linux/types.h>
/*
- * min()/max()/clamp() macros must accomplish three things:
+ * min()/max()/clamp() macros must accomplish several things:
*
* - Avoid multiple evaluations of the arguments (so side-effects like
* "x++" happen only once) when non-constant.
@@ -26,19 +26,17 @@
#define __typecheck(x, y) \
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
-/* is_signed_type() isn't a constexpr for pointer types */
-#define __is_signed(x) \
- __builtin_choose_expr(__is_constexpr(is_signed_type(typeof(x))), \
- is_signed_type(typeof(x)), 0)
+/* Allow unsigned compares against non-negative signed constants. */
+#define __is_ok_unsigned(x) \
+ (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) + 0 >= 0 : 0))
-/* True for a non-negative signed int constant */
-#define __is_noneg_int(x) \
- (__builtin_choose_expr(__is_constexpr(x) && __is_signed(x), x, -1) >= 0)
+/* Check for signed after promoting unsigned char/short to int */
+#define __is_ok_signed(x) is_signed_type(typeof((x) + 0))
-#define __types_ok(x, y) \
- (__is_signed(x) == __is_signed(y) || \
- __is_signed((x) + 0) == __is_signed((y) + 0) || \
- __is_noneg_int(x) || __is_noneg_int(y))
+/* Allow if both x and y are valid for either signed or unsigned compares. */
+#define __types_ok(x, y) \
+ ((__is_ok_signed(x) && __is_ok_signed(y)) || \
+ (__is_ok_unsigned(x) && __is_ok_unsigned(y)))
#define __cmp_op_min <
#define __cmp_op_max >
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Provided __COUNTER__ is passed through an extra #define it can be pasted
onto multiple local variables to give unique names.
This saves having 3 __UNIQUE_ID() for #defines with three locals and
look less messy in general.
Stop the umin()/umax() lines being overlong by factoring out the
zero-extension logic.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 48 +++++++++++++++++++++---------------------
1 file changed, 24 insertions(+), 24 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index c32b4b40ce01..8ee003d8abaf 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -8,7 +8,7 @@
#include <linux/types.h>
/*
- * min()/max()/clamp() macros must accomplish several things:
+ * min()/max()/clamp() macros must accomplish three things:
*
* - Avoid multiple evaluations of the arguments (so side-effects like
* "x++" happen only once) when non-constant.
@@ -43,31 +43,31 @@
#define __cmp(op, x, y) ((x) __cmp_op_##op (y) ? (x) : (y))
-#define __cmp_once(op, x, y, unique_x, unique_y) ({ \
- typeof(x) unique_x = (x); \
- typeof(y) unique_y = (y); \
- _Static_assert(__types_ok(x, y), \
+#define __cmp_once(op, x, y, uniq) ({ \
+ typeof(x) __x_##uniq = (x); \
+ typeof(y) __y_##uniq = (y); \
+ _Static_assert(__types_ok(x, y), \
#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
- __cmp(op, unique_x, unique_y); })
+ __cmp(op, __x_##uniq, __y_##uniq); })
-#define __careful_cmp(op, x, y) \
+#define __careful_cmp(op, x, y, uniq) \
__builtin_choose_expr(__is_constexpr((x) - (y)), \
__cmp(op, x, y), \
- __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
+ __cmp_once(op, x, y, uniq))
/**
* min - return minimum of two values of the same or compatible types
* @x: first value
* @y: second value
*/
-#define min(x, y) __careful_cmp(min, x, y)
+#define min(x, y) __careful_cmp(min, x, y, __COUNTER__)
/**
* max - return maximum of two values of the same or compatible types
* @x: first value
* @y: second value
*/
-#define max(x, y) __careful_cmp(max, x, y)
+#define max(x, y) __careful_cmp(max, x, y, __COUNTER__)
/**
* umin - return minimum of two non-negative values
@@ -75,8 +75,9 @@
* @x: first value
* @y: second value
*/
+#define __zero_extend(x) ((x) + 0u + 0ul + 0ull)
#define umin(x, y) \
- __careful_cmp(min, (x) + 0u + 0ul + 0ull, (y) + 0u + 0ul + 0ull)
+ __careful_cmp(min, __zero_extend(x), _zero_extend(y), __COUNTER__)
/**
* umax - return maximum of two non-negative values
@@ -84,7 +85,7 @@
* @y: second value
*/
#define umax(x, y) \
- __careful_cmp(max, (x) + 0u + 0ul + 0ull, (y) + 0u + 0ul + 0ull)
+ __careful_cmp(max, __zero_extend(x), _zero_extend(y), __COUNTER__)
/**
* min3 - return minimum of three values
@@ -108,7 +109,7 @@
* @x: first value
* @y: second value
*/
-#define min_t(type, x, y) __careful_cmp(min, (type)(x), (type)(y))
+#define min_t(type, x, y) __careful_cmp(min, (type)(x), (type)(y), __COUNTER__)
/**
* max_t - return maximum of two values, using the specified type
@@ -116,7 +117,7 @@
* @x: first value
* @y: second value
*/
-#define max_t(type, x, y) __careful_cmp(max, (type)(x), (type)(y))
+#define max_t(type, x, y) __careful_cmp(max, (type)(x), (type)(y), __COUNTER__)
/**
* min_not_zero - return the minimum that is _not_ zero, unless both are zero
@@ -131,22 +132,21 @@
#define __clamp(val, lo, hi) \
((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
-#define __clamp_once(val, lo, hi, unique_val, unique_lo, unique_hi) ({ \
- typeof(val) unique_val = (val); \
- typeof(lo) unique_lo = (lo); \
- typeof(hi) unique_hi = (hi); \
+#define __clamp_once(val, lo, hi, uniq) ({ \
+ typeof(val) __val_##uniq = (val); \
+ typeof(lo) __lo_##uniq = (lo); \
+ typeof(hi) __hi_##uniq = (hi); \
_Static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
(lo) <= (hi), true), \
"clamp() low limit " #lo " greater than high limit " #hi); \
_Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
_Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
- __clamp(unique_val, unique_lo, unique_hi); })
+ __clamp(__val_##uniq, __lo_##uniq, __hi_##uniq); })
-#define __careful_clamp(val, lo, hi) ({ \
+#define __careful_clamp(val, lo, hi, uniq) ({ \
__builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)), \
__clamp(val, lo, hi), \
- __clamp_once(val, lo, hi, __UNIQUE_ID(__val), \
- __UNIQUE_ID(__lo), __UNIQUE_ID(__hi))); })
+ __clamp_once(val, lo, hi, uniq)); })
/**
* clamp - return a value clamped to a given range with strict typechecking
@@ -156,7 +156,7 @@
*
* This macro checks that @val, @lo and @hi have the same signedness.
*/
-#define clamp(val, lo, hi) __careful_clamp(val, lo, hi)
+#define clamp(val, lo, hi) __careful_clamp(val, lo, hi, __COUNTER__)
/**
* clamp_t - return a value clamped to a given range using a given type
@@ -168,7 +168,7 @@
* This macro does no typechecking and uses temporary variables of type
* @type to make all the comparisons.
*/
-#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
+#define clamp_t(type, val, lo, hi) clamp((type)(val), (type)(lo), (type)(hi))
/**
* clamp_val - return a value clamped to a given range using val's type
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
There is no need to do the signedness/type check when the arguments
are being cast to a fixed type.
So move the check out of __xxx_once() into __careful_xxx().
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 8ee003d8abaf..111c52a14fe5 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -46,14 +46,14 @@
#define __cmp_once(op, x, y, uniq) ({ \
typeof(x) __x_##uniq = (x); \
typeof(y) __y_##uniq = (y); \
- _Static_assert(__types_ok(x, y), \
- #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
__cmp(op, __x_##uniq, __y_##uniq); })
#define __careful_cmp(op, x, y, uniq) \
__builtin_choose_expr(__is_constexpr((x) - (y)), \
__cmp(op, x, y), \
- __cmp_once(op, x, y, uniq))
+ ({ _Static_assert(__types_ok(x, y), \
+ #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
+ __cmp_once(op, x, y, uniq); }))
/**
* min - return minimum of two values of the same or compatible types
@@ -139,14 +139,14 @@
_Static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
(lo) <= (hi), true), \
"clamp() low limit " #lo " greater than high limit " #hi); \
- _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
- _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
__clamp(__val_##uniq, __lo_##uniq, __hi_##uniq); })
-#define __careful_clamp(val, lo, hi, uniq) ({ \
+#define __careful_clamp(val, lo, hi, uniq) \
__builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)), \
__clamp(val, lo, hi), \
- __clamp_once(val, lo, hi, uniq)); })
+ ({ _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
+ _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
+ __clamp_once(val, lo, hi, uniq); }))
/**
* clamp - return a value clamped to a given range with strict typechecking
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Nothing requires that clamp() return a constant expression.
The logic to do so significantly increases the .i file.
Remove the check and directly expand __clamp_once() from clamp_t()
since the type check can't fail.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 111c52a14fe5..5c7fce76abe5 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -141,12 +141,10 @@
"clamp() low limit " #lo " greater than high limit " #hi); \
__clamp(__val_##uniq, __lo_##uniq, __hi_##uniq); })
-#define __careful_clamp(val, lo, hi, uniq) \
- __builtin_choose_expr(__is_constexpr((val) - (lo) + (hi)), \
- __clamp(val, lo, hi), \
- ({ _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
- _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
- __clamp_once(val, lo, hi, uniq); }))
+#define __careful_clamp(val, lo, hi, uniq) ({ \
+ _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
+ _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
+ __clamp_once(val, lo, hi, uniq); })
/**
* clamp - return a value clamped to a given range with strict typechecking
@@ -168,7 +166,9 @@
* This macro does no typechecking and uses temporary variables of type
* @type to make all the comparisons.
*/
-#define clamp_t(type, val, lo, hi) clamp((type)(val), (type)(lo), (type)(hi))
+#define __clamp_t(type, val, lo, hi, uniq) \
+ __clamp_once((type)(val), (type)(lo), (type)(hi), uniq)
+#define clamp_t(type, val, lo, hi) __clamp_t(type, val, lo, hi, __COUNTER__)
/**
* clamp_val - return a value clamped to a given range using val's type
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
min3() and max3() were added to optimise nested min(x, min(y, z))
sequences, but only moved where the expansion was requiested.
Add a separate implementation for 3 argument calls.
These are never required to generate constant expressions to
remove that logic.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 5c7fce76abe5..278a390b8a4c 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -38,6 +38,11 @@
((__is_ok_signed(x) && __is_ok_signed(y)) || \
(__is_ok_unsigned(x) && __is_ok_unsigned(y)))
+/* Check three values for min3(), max3() and clamp() */
+#define __types_ok3(x, y, z) \
+ ((__is_ok_signed(x) && __is_ok_signed(y) && __is_ok_signed(z)) || \
+ (__is_ok_unsigned(x) && __is_ok_unsigned(y) && __is_ok_unsigned(z)))
+
#define __cmp_op_min <
#define __cmp_op_max >
@@ -87,13 +92,24 @@
#define umax(x, y) \
__careful_cmp(max, __zero_extend(x), _zero_extend(y), __COUNTER__)
+#define __cmp_once3(op, x, y, z, uniq) ({ \
+ typeof(x) __x_##uniq = (x); \
+ typeof(x) __y_##uniq = (y); \
+ typeof(x) __z_##uniq = (z); \
+ __cmp(op, __cmp(op, __x_##uniq, __y_##uniq), __z_##uniq); })
+
+#define __careful_cmp3(op, x, y, z, uniq) ({ \
+ static_assert(__types_ok3(x, y, z), \
+ #op "3(" #x ", " #y ", " #z ") signedness error"); \
+ __cmp_once3(op, x, y, z, uniq); })
+
/**
* min3 - return minimum of three values
* @x: first value
* @y: second value
* @z: third value
*/
-#define min3(x, y, z) min((typeof(x))min(x, y), z)
+#define min3(x, y, z) __careful_cmp3(min, x, y, z, __COUNTER__)
/**
* max3 - return maximum of three values
@@ -101,7 +117,7 @@
* @y: second value
* @z: third value
*/
-#define max3(x, y, z) max((typeof(x))max(x, y), z)
+#define max3(x, y, z) __careful_cmp3(max, x, y, z, __COUNTER__)
/**
* min_t - return minimum of two values, using the specified type
@@ -142,8 +158,7 @@
__clamp(__val_##uniq, __lo_##uniq, __hi_##uniq); })
#define __careful_clamp(val, lo, hi, uniq) ({ \
- _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
- _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
+ _Static_assert(__types_ok3(val, lo, hi), "clamp() signedness error"); \
__clamp_once(val, lo, hi, uniq); })
/**
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
The expansions of min() and max() contain statement expressions so are
not valid for static intialisers.
min_const() and max_const() are expressions so can be used for static
initialisers.
The arguments are checked for being constant and for negative signed
values being converted to large unsigned values.
Using these to size on-stack arrays lets min/max be simplified.
Zero is added before the compare to convert enum values to integers
avoinding the need for casts when enums have been used for constants.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 278a390b8a4c..c08916588425 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -60,19 +60,34 @@
#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
__cmp_once(op, x, y, uniq); }))
+#define __careful_cmp_const(op, x, y) \
+ (BUILD_BUG_ON_ZERO(!__is_constexpr((x) - (y))) + \
+ BUILD_BUG_ON_ZERO(!__types_ok(x, y)) + \
+ __cmp(op, (x) + 0, (y) + 0))
+
/**
* min - return minimum of two values of the same or compatible types
* @x: first value
* @y: second value
+ *
+ * If @x and @y are constants the return value is constant, but not 'constant
+ * enough' for things like static initialisers.
+ * min_const(@x, @y) is a constant expression for constant inputs.
*/
#define min(x, y) __careful_cmp(min, x, y, __COUNTER__)
+#define min_const(x, y) __careful_cmp_const(min, x, y)
/**
* max - return maximum of two values of the same or compatible types
* @x: first value
* @y: second value
+ *
+ * If @x and @y are constants the return value is constant, but not 'constant
+ * enough' for things like static initialisers.
+ * max_const(@x, @y) is a constant expression for constant inputs.
*/
#define max(x, y) __careful_cmp(max, x, y, __COUNTER__)
+#define max_const(x, y) __careful_cmp_const(max, x, y)
/**
* umin - return minimum of two non-negative values
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
These are the only uses of max() that require a constant value
from constant parameters.
There don't seem to be any similar uses of min().
Replacing the max() by max_const() lets min()/max() be simplified
speeding up compilation.
max_const() will convert enums to int (or unsigned int) so that the
casts added by max_t() are no longer needed.
Signed-off-by: David Laight <[email protected]>
---
drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c | 2 +-
drivers/gpu/drm/drm_color_mgmt.c | 4 ++--
drivers/input/touchscreen/cyttsp4_core.c | 2 +-
drivers/net/can/usb/etas_es58x/es58x_devlink.c | 2 +-
fs/btrfs/tree-checker.c | 2 +-
lib/vsprintf.c | 4 ++--
net/ipv4/proc.c | 2 +-
net/ipv6/proc.c | 2 +-
8 files changed, 10 insertions(+), 10 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
index 00cd615bbcdc..935fb4014f7c 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu_cmn.c
@@ -708,7 +708,7 @@ static const char *smu_get_feature_name(struct smu_context *smu,
size_t smu_cmn_get_pp_feature_mask(struct smu_context *smu,
char *buf)
{
- int8_t sort_feature[max(SMU_FEATURE_COUNT, SMU_FEATURE_MAX)];
+ int8_t sort_feature[max_const(SMU_FEATURE_COUNT, SMU_FEATURE_MAX)];
uint64_t feature_mask;
int i, feature_index;
uint32_t count = 0;
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index d021497841b8..43a6bd0ca960 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -532,8 +532,8 @@ int drm_plane_create_color_properties(struct drm_plane *plane,
{
struct drm_device *dev = plane->dev;
struct drm_property *prop;
- struct drm_prop_enum_list enum_list[max_t(int, DRM_COLOR_ENCODING_MAX,
- DRM_COLOR_RANGE_MAX)];
+ struct drm_prop_enum_list enum_list[max_const(DRM_COLOR_ENCODING_MAX,
+ DRM_COLOR_RANGE_MAX)];
int i, len;
if (WARN_ON(supported_encodings == 0 ||
diff --git a/drivers/input/touchscreen/cyttsp4_core.c b/drivers/input/touchscreen/cyttsp4_core.c
index 7cb26929dc73..c6884c3c3fca 100644
--- a/drivers/input/touchscreen/cyttsp4_core.c
+++ b/drivers/input/touchscreen/cyttsp4_core.c
@@ -871,7 +871,7 @@ static void cyttsp4_get_mt_touches(struct cyttsp4_mt_data *md, int num_cur_tch)
struct cyttsp4_touch tch;
int sig;
int i, j, t = 0;
- int ids[max(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
+ int ids[max_const(CY_TMA1036_MAX_TCH, CY_TMA4XX_MAX_TCH)];
memset(ids, 0, si->si_ofs.tch_abs[CY_TCH_T].max * sizeof(int));
for (i = 0; i < num_cur_tch; i++) {
diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
index 635edeb8f68c..28fa87668bf8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_devlink.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c
@@ -215,7 +215,7 @@ static int es58x_devlink_info_get(struct devlink *devlink,
struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
- char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
+ char buf[max_const(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))];
int ret = 0;
if (es58x_sw_version_is_valid(fw_ver)) {
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 6eccf8496486..aec4729a9a82 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -615,7 +615,7 @@ static int check_dir_item(struct extent_buffer *leaf,
*/
if (key->type == BTRFS_DIR_ITEM_KEY ||
key->type == BTRFS_XATTR_ITEM_KEY) {
- char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
+ char namebuf[max_const(BTRFS_NAME_LEN, XATTR_NAME_MAX)];
read_extent_buffer(leaf, namebuf,
(unsigned long)(di + 1), name_len);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 552738f14275..6c3c319afd86 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1080,8 +1080,8 @@ char *resource_string(char *buf, char *end, struct resource *res,
#define FLAG_BUF_SIZE (2 * sizeof(res->flags))
#define DECODED_BUF_SIZE sizeof("[mem - 64bit pref window disabled]")
#define RAW_BUF_SIZE sizeof("[mem - flags 0x]")
- char sym[max(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
- 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
+ char sym[max_const(2*RSRC_BUF_SIZE + DECODED_BUF_SIZE,
+ 2*RSRC_BUF_SIZE + FLAG_BUF_SIZE + RAW_BUF_SIZE)];
char *p = sym, *pend = sym + sizeof(sym);
int decode = (fmt[0] == 'R') ? 1 : 0;
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 5f4654ebff48..a4aff27f949b 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -43,7 +43,7 @@
#include <net/sock.h>
#include <net/raw.h>
-#define TCPUDP_MIB_MAX max_t(u32, UDP_MIB_MAX, TCP_MIB_MAX)
+#define TCPUDP_MIB_MAX max_const(UDP_MIB_MAX, TCP_MIB_MAX)
/*
* Report socket allocation statistics [[email protected]]
diff --git a/net/ipv6/proc.c b/net/ipv6/proc.c
index 6d1d9221649d..7fedb60aaeac 100644
--- a/net/ipv6/proc.c
+++ b/net/ipv6/proc.c
@@ -27,7 +27,7 @@
#include <net/ipv6.h>
#define MAX4(a, b, c, d) \
- max_t(u32, max_t(u32, a, b), max_t(u32, c, d))
+ max_const(max_const(a, b), max_const(c, d))
#define SNMP_MIB_MAX MAX4(UDP_MIB_MAX, TCP_MIB_MAX, \
IPSTATS_MIB_MAX, ICMP_MIB_MAX)
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
blk_stack_limits() contains:
t->zoned = max(t->zoned, b->zoned);
These are bool, so can be replaced by bitwise or.
However it generates:
error: comparison of constant '0' with boolean expression is always true [-Werror=bool-compare]
inside the signedness check that max() does unless a '+ 0' is added.
It is a shame the compiler generates this warning for code that will
be optimised away.
Change so that the extra '+ 0' can be removed.
Signed-off-by: David Laight <[email protected]>
---
block/blk-settings.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 06ea91e51b8b..9ca21fea039d 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -688,7 +688,7 @@ int blk_stack_limits(struct queue_limits *t, struct queue_limits *b,
b->max_secure_erase_sectors);
t->zone_write_granularity = max(t->zone_write_granularity,
b->zone_write_granularity);
- t->zoned = max(t->zoned, b->zoned);
+ t->zoned = t->zoned | b->zoned;
return ret;
}
EXPORT_SYMBOL(blk_stack_limits);
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
After changing the handful of places max() was used to size an on-stack
array to use max_const() it is no longer necessary for min() and max()
to return constant expressions from constant inputs.
Remove the associated logic to reduce the expanded text.
Remove the 'hack' that allowed max(bool, bool).
Fixup the initial block comment to match current reality.
Signed-off-by: David Laight <[email protected]>
---
include/linux/minmax.h | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
Changes for v2:
- Typographical and spelling corrections to the commit messages.
Patches unchanged.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index c08916588425..5e65c98ff256 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -8,13 +8,10 @@
#include <linux/types.h>
/*
- * min()/max()/clamp() macros must accomplish three things:
+ * min()/max()/clamp() macros must accomplish several things:
*
* - Avoid multiple evaluations of the arguments (so side-effects like
* "x++" happen only once) when non-constant.
- * - Retain result as a constant expressions when called with only
- * constant expressions (to avoid tripping VLA warnings in stack
- * allocation usage).
* - Perform signed v unsigned type-checking (to generate compile
* errors instead of nasty runtime surprises).
* - Unsigned char/short are always promoted to signed int and can be
@@ -22,13 +19,19 @@
* - Unsigned arguments can be compared against non-negative signed constants.
* - Comparison of a signed argument against an unsigned constant fails
* even if the constant is below __INT_MAX__ and could be cast to int.
+ *
+ * The return value of min()/max() is not a constant expression for
+ * constant parameters - so will trigger a VLA warging if used to size
+ * an on-stack array.
+ * Instead use min_const() or max_const() which do generate constant
+ * expressions and are also valid for static initialisers.
*/
#define __typecheck(x, y) \
(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
/* Allow unsigned compares against non-negative signed constants. */
#define __is_ok_unsigned(x) \
- (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) + 0 >= 0 : 0))
+ (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) >= 0 : 0))
/* Check for signed after promoting unsigned char/short to int */
#define __is_ok_signed(x) is_signed_type(typeof((x) + 0))
@@ -53,12 +56,10 @@
typeof(y) __y_##uniq = (y); \
__cmp(op, __x_##uniq, __y_##uniq); })
-#define __careful_cmp(op, x, y, uniq) \
- __builtin_choose_expr(__is_constexpr((x) - (y)), \
- __cmp(op, x, y), \
- ({ _Static_assert(__types_ok(x, y), \
- #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
- __cmp_once(op, x, y, uniq); }))
+#define __careful_cmp(op, x, y, uniq) ({ \
+ _Static_assert(__types_ok(x, y), \
+ #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
+ __cmp_once(op, x, y, uniq); })
#define __careful_cmp_const(op, x, y) \
(BUILD_BUG_ON_ZERO(!__is_constexpr((x) - (y))) + \
--
2.17.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Sun, 25 Feb 2024 at 08:53, David Laight <[email protected]> wrote:
>
> The expansions of min() and max() contain statement expressions so are
> not valid for static intialisers.
> min_const() and max_const() are expressions so can be used for static
> initialisers.
I hate the name.
Naming shouldn't be about an implementation detail, particularly not
an esoteric one like the "C constant expression" rule. That can be
useful for some internal helper functions or macros, but not for
something that random people are supposed to USE.
Telling some random developer that inside an array size declaration or
a static initializer you need to use "max_const()" because it needs to
syntactically be a constant expression, and our regular "max()"
function isn't that, is just *horrid*.
No, please just use the traditional C model of just using ALL CAPS for
macro names that don't act like a function.
Yes, yes, that may end up requiring getting rid of some current users of
#define MIN(a,b) ((a)<(b) ? (a):(b))
but dammit, we don't actually have _that_ many of them, and why should
we have random drivers doing that anyway?
Linus
From: Linus Torvalds
> Sent: 25 February 2024 17:14
>
> On Sun, 25 Feb 2024 at 08:53, David Laight <[email protected]> wrote:
> >
> > The expansions of min() and max() contain statement expressions so are
> > not valid for static intialisers.
> > min_const() and max_const() are expressions so can be used for static
> > initialisers.
>
> I hate the name.
Picking name is always hard...
> Naming shouldn't be about an implementation detail, particularly not
> an esoteric one like the "C constant expression" rule. That can be
> useful for some internal helper functions or macros, but not for
> something that random people are supposed to USE.
>
> Telling some random developer that inside an array size declaration or
> a static initializer you need to use "max_const()" because it needs to
> syntactically be a constant expression, and our regular "max()"
> function isn't that, is just *horrid*.
>
> No, please just use the traditional C model of just using ALL CAPS for
> macro names that don't act like a function.
>
> Yes, yes, that may end up requiring getting rid of some current users of
>
> #define MIN(a,b) ((a)<(b) ? (a):(b))
>
> but dammit, we don't actually have _that_ many of them, and why should
> we have random drivers doing that anyway?
I'll have a look at what is there.
It might take a three part patch though.
Unless you apply it as a tree-wide patch?
One option is to add as max_const(), then change any existing MAX()
to be max() or max_const() and then finally rename to MAX()?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
...
> Yes, yes, that may end up requiring getting rid of some current users of
>
> #define MIN(a,b) ((a)<(b) ? (a):(b))
>
> but dammit, we don't actually have _that_ many of them, and why should
> we have random drivers doing that anyway?
They look like they could be changed to min().
It is even likely the header gets pulled in somewhere.
I'm not sure about the ones in drivers/gpu/drm/amd/display/*..*/*.c, but it
wouldn't surprise me if that code doesn't use any standard kernel headers.
Isn't that also the code that manages to pass 42 integer parameters
to functions?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, 26 Feb 2024 at 07:26, David Laight <[email protected]> wrote:
>
> ...
> > Yes, yes, that may end up requiring getting rid of some current users of
> >
> > #define MIN(a,b) ((a)<(b) ? (a):(b))
> >
> > but dammit, we don't actually have _that_ many of them, and why should
> > we have random drivers doing that anyway?
>
> They look like they could be changed to min().
> It is even likely the header gets pulled in somewhere.
>
> I'm not sure about the ones in drivers/gpu/drm/amd/display/*..*/*.c, but it
> wouldn't surprise me if that code doesn't use any standard kernel headers.
> Isn't that also the code that manages to pass 42 integer parameters
> to functions?
They are all separate in C files, I think we could get rid of them
pretty easily in favour of the standard ones,
Adding Harry to cc.
Dave.
On Sun, 25 Feb 2024, David Laight <[email protected]> wrote:
> The wrapper just adds two more lines of error output when the test fails.
There are only a handful of places in kernel code that use
_Static_assert() directly. Nearly 900 instances of static_assert().
Are we now saying it's fine to use _Static_assert() directly all over
the place? People will copy-paste and cargo cult.
BR,
Jani.
>
> Signed-off-by: David Laight <[email protected]>
> ---
> include/linux/minmax.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Changes for v2:
> - Typographical and spelling corrections to the commit messages.
> Patches unchanged.
>
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 63c45865b48a..900eec7a28e5 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -48,7 +48,7 @@
> #define __cmp_once(op, x, y, unique_x, unique_y) ({ \
> typeof(x) unique_x = (x); \
> typeof(y) unique_y = (y); \
> - static_assert(__types_ok(x, y), \
> + _Static_assert(__types_ok(x, y), \
> #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
> __cmp(op, unique_x, unique_y); })
>
> @@ -137,11 +137,11 @@
> typeof(val) unique_val = (val); \
> typeof(lo) unique_lo = (lo); \
> typeof(hi) unique_hi = (hi); \
> - static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
> + _Static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \
> (lo) <= (hi), true), \
> "clamp() low limit " #lo " greater than high limit " #hi); \
> - static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
> - static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
> + _Static_assert(__types_ok(val, lo), "clamp() 'lo' signedness error"); \
> + _Static_assert(__types_ok(val, hi), "clamp() 'hi' signedness error"); \
> __clamp(unique_val, unique_lo, unique_hi); })
>
> #define __careful_clamp(val, lo, hi) ({ \
--
Jani Nikula, Intel
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linux/master mkl-can-next/testing kdave/for-next akpm-mm/mm-nonmm-unstable linus/master v6.8-rc6]
[cannot apply to next-20240223 dtor-input/next dtor-input/for-linus axboe-block/for-next horms-ipvs/master next-20240223]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-Put-all-the-clamp-definitions-together/20240226-005902
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/a18dcae310f74dcb9c6fc01d5bdc0568%40AcuMS.aculab.com
patch subject: [PATCH next v2 11/11] minmax: min() and max() don't need to return constant expressions
config: i386-buildonly-randconfig-001-20240226 (https://download.01.org/0day-ci/archive/20240226/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240226/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> arch/x86/mm/pgtable.c:437:14: warning: variable length array used [-Wvla]
437 | pmd_t *pmds[MAX_PREALLOCATED_PMDS];
| ^~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/pgtable.c:180:31: note: expanded from macro 'MAX_PREALLOCATED_PMDS'
180 | #define MAX_PREALLOCATED_PMDS MAX_UNSHARED_PTRS_PER_PGD
| ^~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/pgtable.c:113:2: note: expanded from macro 'MAX_UNSHARED_PTRS_PER_PGD'
113 | max_t(size_t, KERNEL_PGD_BOUNDARY, PTRS_PER_PGD)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:152:27: note: expanded from macro 'max_t'
152 | #define max_t(type, x, y) __careful_cmp(max, (type)(x), (type)(y), __COUNTER__)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:62:2: note: expanded from macro '__careful_cmp'
59 | #define __careful_cmp(op, x, y, uniq) ({ \
| ~~~~~~~~~~~
60 | _Static_assert(__types_ok(x, y), \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
61 | #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
62 | __cmp_once(op, x, y, uniq); })
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/minmax.h:57:2: note: expanded from macro '__cmp_once'
57 | __cmp(op, __x_##uniq, __y_##uniq); })
| ^
include/linux/minmax.h:52:26: note: expanded from macro '__cmp'
52 | #define __cmp(op, x, y) ((x) __cmp_op_##op (y) ? (x) : (y))
| ^
1 warning generated.
vim +437 arch/x86/mm/pgtable.c
1db491f77b6ed0 Fenghua Yu 2015-01-15 432
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 433 pgd_t *pgd_alloc(struct mm_struct *mm)
1ec1fe73dfb711 Ingo Molnar 2008-03-19 434 {
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 435 pgd_t *pgd;
184d47f0fd3651 Kees Cook 2018-10-08 436 pmd_t *u_pmds[MAX_PREALLOCATED_USER_PMDS];
184d47f0fd3651 Kees Cook 2018-10-08 @437 pmd_t *pmds[MAX_PREALLOCATED_PMDS];
1ec1fe73dfb711 Ingo Molnar 2008-03-19 438
1db491f77b6ed0 Fenghua Yu 2015-01-15 439 pgd = _pgd_alloc();
1ec1fe73dfb711 Ingo Molnar 2008-03-19 440
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 441 if (pgd == NULL)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 442 goto out;
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 443
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 444 mm->pgd = pgd;
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 445
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 446 if (sizeof(pmds) != 0 &&
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 447 preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 448 goto out_free_pgd;
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 449
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 450 if (sizeof(u_pmds) != 0 &&
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 451 preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 452 goto out_free_pmds;
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 453
f59dbe9ca6707e Joerg Roedel 2018-07-18 454 if (paravirt_pgd_alloc(mm) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 455 goto out_free_user_pmds;
f59dbe9ca6707e Joerg Roedel 2018-07-18 456
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 457 /*
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 458 * Make sure that pre-populating the pmds is atomic with
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 459 * respect to anything walking the pgd_list, so that they
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 460 * never see a partially populated pgd.
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 461 */
a79e53d85683c6 Andrea Arcangeli 2011-02-16 462 spin_lock(&pgd_lock);
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 463
617d34d9e5d832 Jeremy Fitzhardinge 2010-09-21 464 pgd_ctor(mm, pgd);
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 465 if (sizeof(pmds) != 0)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 466 pgd_prepopulate_pmd(mm, pgd, pmds);
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 467
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 468 if (sizeof(u_pmds) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 469 pgd_prepopulate_user_pmd(mm, pgd, u_pmds);
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 470
a79e53d85683c6 Andrea Arcangeli 2011-02-16 471 spin_unlock(&pgd_lock);
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 472
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 473 return pgd;
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 474
f59dbe9ca6707e Joerg Roedel 2018-07-18 475 out_free_user_pmds:
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 476 if (sizeof(u_pmds) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 477 free_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS);
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 478 out_free_pmds:
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 479 if (sizeof(pmds) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 480 free_pmds(mm, pmds, PREALLOCATED_PMDS);
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 481 out_free_pgd:
1db491f77b6ed0 Fenghua Yu 2015-01-15 482 _pgd_free(pgd);
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 483 out:
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 484 return NULL;
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 485 }
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 486
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
From: Jani Nikula
> Sent: 26 February 2024 09:28
>
> On Sun, 25 Feb 2024, David Laight <[email protected]> wrote:
> > The wrapper just adds two more lines of error output when the test fails.
>
> There are only a handful of places in kernel code that use
> _Static_assert() directly. Nearly 900 instances of static_assert().
How many of those supply an error message?
> Are we now saying it's fine to use _Static_assert() directly all over
> the place? People will copy-paste and cargo cult.
Is that actually a problem?
The wrapper allows the error message to be omitted and substitutes
the text of the conditional.
But it isn't 'free'.
As well as slightly slowing down the compilation, the error messages
from the compiler get more difficult to interpret.
Most of the static_assert() will probably never generate an error.
But the ones in min()/max() will so it is best to make them as
readable as possible.
(Don't even look as the mess clang makes....)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: kernel test robot <[email protected]>
> Sent: 26 February 2024 09:42
...
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> >> arch/x86/mm/pgtable.c:437:14: warning: variable length array used [-Wvla]
> 437 | pmd_t *pmds[MAX_PREALLOCATED_PMDS];
Not surprisingly I missed X86_CONFIG_PAE builds :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Mon, 26 Feb 2024, David Laight <[email protected]> wrote:
> From: Jani Nikula
>> Sent: 26 February 2024 09:28
>>
>> On Sun, 25 Feb 2024, David Laight <[email protected]> wrote:
>> > The wrapper just adds two more lines of error output when the test fails.
>>
>> There are only a handful of places in kernel code that use
>> _Static_assert() directly. Nearly 900 instances of static_assert().
>
> How many of those supply an error message?
At a glance, not many.
>> Are we now saying it's fine to use _Static_assert() directly all over
>> the place? People will copy-paste and cargo cult.
>
> Is that actually a problem?
I don't know. I'm asking.
Usually when we have compiler wrappers, they're meant to be used instead
of the thing being wrapped.
This series deviates from that, so it would seem to fair to mention it
slightly more verbosely than just stating what's being done.
> The wrapper allows the error message to be omitted and substitutes
> the text of the conditional.
> But it isn't 'free'.
> As well as slightly slowing down the compilation, the error messages
> from the compiler get more difficult to interpret.
>
> Most of the static_assert() will probably never generate an error.
> But the ones in min()/max() will so it is best to make them as
> readable as possible.
> (Don't even look as the mess clang makes....)
I'm not arguing any of this. :)
BR,
Jani.
--
Jani Nikula, Intel
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linux/master mkl-can-next/testing kdave/for-next akpm-mm/mm-nonmm-unstable linus/master v6.8-rc6]
[cannot apply to next-20240223 dtor-input/next dtor-input/for-linus axboe-block/for-next horms-ipvs/master next-20240223]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-Put-all-the-clamp-definitions-together/20240226-005902
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/a18dcae310f74dcb9c6fc01d5bdc0568%40AcuMS.aculab.com
patch subject: [PATCH next v2 11/11] minmax: min() and max() don't need to return constant expressions
config: i386-randconfig-011-20240226 (https://download.01.org/0day-ci/archive/20240226/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240226/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
arch/x86/mm/pgtable.c: In function 'pgd_alloc':
>> arch/x86/mm/pgtable.c:437:9: warning: ISO C90 forbids variable length array 'pmds' [-Wvla]
437 | pmd_t *pmds[MAX_PREALLOCATED_PMDS];
| ^~~~~
vim +/pmds +437 arch/x86/mm/pgtable.c
1db491f77b6ed0 Fenghua Yu 2015-01-15 432
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 433 pgd_t *pgd_alloc(struct mm_struct *mm)
1ec1fe73dfb711 Ingo Molnar 2008-03-19 434 {
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 435 pgd_t *pgd;
184d47f0fd3651 Kees Cook 2018-10-08 436 pmd_t *u_pmds[MAX_PREALLOCATED_USER_PMDS];
184d47f0fd3651 Kees Cook 2018-10-08 @437 pmd_t *pmds[MAX_PREALLOCATED_PMDS];
1ec1fe73dfb711 Ingo Molnar 2008-03-19 438
1db491f77b6ed0 Fenghua Yu 2015-01-15 439 pgd = _pgd_alloc();
1ec1fe73dfb711 Ingo Molnar 2008-03-19 440
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 441 if (pgd == NULL)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 442 goto out;
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 443
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 444 mm->pgd = pgd;
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 445
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 446 if (sizeof(pmds) != 0 &&
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 447 preallocate_pmds(mm, pmds, PREALLOCATED_PMDS) != 0)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 448 goto out_free_pgd;
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 449
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 450 if (sizeof(u_pmds) != 0 &&
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 451 preallocate_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS) != 0)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 452 goto out_free_pmds;
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 453
f59dbe9ca6707e Joerg Roedel 2018-07-18 454 if (paravirt_pgd_alloc(mm) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 455 goto out_free_user_pmds;
f59dbe9ca6707e Joerg Roedel 2018-07-18 456
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 457 /*
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 458 * Make sure that pre-populating the pmds is atomic with
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 459 * respect to anything walking the pgd_list, so that they
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 460 * never see a partially populated pgd.
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 461 */
a79e53d85683c6 Andrea Arcangeli 2011-02-16 462 spin_lock(&pgd_lock);
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 463
617d34d9e5d832 Jeremy Fitzhardinge 2010-09-21 464 pgd_ctor(mm, pgd);
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 465 if (sizeof(pmds) != 0)
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 466 pgd_prepopulate_pmd(mm, pgd, pmds);
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 467
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 468 if (sizeof(u_pmds) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 469 pgd_prepopulate_user_pmd(mm, pgd, u_pmds);
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 470
a79e53d85683c6 Andrea Arcangeli 2011-02-16 471 spin_unlock(&pgd_lock);
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 472
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 473 return pgd;
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 474
f59dbe9ca6707e Joerg Roedel 2018-07-18 475 out_free_user_pmds:
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 476 if (sizeof(u_pmds) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 477 free_pmds(mm, u_pmds, PREALLOCATED_USER_PMDS);
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 478 out_free_pmds:
25226df4b9be7f Gustavo A. R. Silva 2022-09-21 479 if (sizeof(pmds) != 0)
f59dbe9ca6707e Joerg Roedel 2018-07-18 480 free_pmds(mm, pmds, PREALLOCATED_PMDS);
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 481 out_free_pgd:
1db491f77b6ed0 Fenghua Yu 2015-01-15 482 _pgd_free(pgd);
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 483 out:
d8d5900ef8afc5 Jeremy Fitzhardinge 2008-06-25 484 return NULL;
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 485 }
4f76cd382213b2 Jeremy Fitzhardinge 2008-03-17 486
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linux/master mkl-can-next/testing kdave/for-next akpm-mm/mm-nonmm-unstable axboe-block/for-next linus/master v6.8-rc6 next-20240226]
[cannot apply to next-20240223 dtor-input/next dtor-input/for-linus horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-Put-all-the-clamp-definitions-together/20240226-005902
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/8657dd5c2264456f8a005520a3b90e2b%40AcuMS.aculab.com
patch subject: [PATCH next v2 03/11] minmax: Simplify signedness check
config: alpha-defconfig (https://download.01.org/0day-ci/archive/20240227/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240227/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from include/linux/kernel.h:28,
from include/linux/cpumask.h:10,
from include/linux/smp.h:13,
from include/linux/lockdep.h:14,
from include/linux/spinlock.h:63,
from include/linux/swait.h:7,
from include/linux/completion.h:12,
from include/linux/crypto.h:15,
from include/crypto/aead.h:13,
from include/crypto/internal/aead.h:11,
from crypto/skcipher.c:12:
crypto/skcipher.c: In function 'skcipher_get_spot':
>> include/linux/minmax.h:31:70: warning: ordered comparison of pointer with integer zero [-Wextra]
31 | (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) + 0 >= 0 : 0))
| ^~
include/linux/minmax.h:39:11: note: in expansion of macro '__is_ok_unsigned'
39 | (__is_ok_unsigned(x) && __is_ok_unsigned(y)))
| ^~~~~~~~~~~~~~~~
include/linux/minmax.h:49:24: note: in expansion of macro '__types_ok'
49 | _Static_assert(__types_ok(x, y), \
| ^~~~~~~~~~
include/linux/minmax.h:56:17: note: in expansion of macro '__cmp_once'
56 | __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
| ^~~~~~~~~~
include/linux/minmax.h:70:25: note: in expansion of macro '__careful_cmp'
70 | #define max(x, y) __careful_cmp(max, x, y)
| ^~~~~~~~~~~~~
crypto/skcipher.c:83:16: note: in expansion of macro 'max'
83 | return max(start, end_page);
| ^~~
>> include/linux/minmax.h:31:70: warning: ordered comparison of pointer with integer zero [-Wextra]
31 | (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) + 0 >= 0 : 0))
| ^~
include/linux/minmax.h:39:34: note: in expansion of macro '__is_ok_unsigned'
39 | (__is_ok_unsigned(x) && __is_ok_unsigned(y)))
| ^~~~~~~~~~~~~~~~
include/linux/minmax.h:49:24: note: in expansion of macro '__types_ok'
49 | _Static_assert(__types_ok(x, y), \
| ^~~~~~~~~~
include/linux/minmax.h:56:17: note: in expansion of macro '__cmp_once'
56 | __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y)))
| ^~~~~~~~~~
include/linux/minmax.h:70:25: note: in expansion of macro '__careful_cmp'
70 | #define max(x, y) __careful_cmp(max, x, y)
| ^~~~~~~~~~~~~
crypto/skcipher.c:83:16: note: in expansion of macro 'max'
83 | return max(start, end_page);
| ^~~
vim +31 include/linux/minmax.h
9
10 /*
11 * min()/max()/clamp() macros must accomplish several things:
12 *
13 * - Avoid multiple evaluations of the arguments (so side-effects like
14 * "x++" happen only once) when non-constant.
15 * - Retain result as a constant expressions when called with only
16 * constant expressions (to avoid tripping VLA warnings in stack
17 * allocation usage).
18 * - Perform signed v unsigned type-checking (to generate compile
19 * errors instead of nasty runtime surprises).
20 * - Unsigned char/short are always promoted to signed int and can be
21 * compared against signed or unsigned arguments.
22 * - Unsigned arguments can be compared against non-negative signed constants.
23 * - Comparison of a signed argument against an unsigned constant fails
24 * even if the constant is below __INT_MAX__ and could be cast to int.
25 */
26 #define __typecheck(x, y) \
27 (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
28
29 /* Allow unsigned compares against non-negative signed constants. */
30 #define __is_ok_unsigned(x) \
> 31 (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) + 0 >= 0 : 0))
32
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
From: kernel test robot
> Sent: 27 February 2024 01:34
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on drm-misc/drm-misc-next]
> [also build test WARNING on linux/master mkl-can-next/testing kdave/for-next akpm-mm/mm-nonmm-unstable
> axboe-block/for-next linus/master v6.8-rc6 next-20240226]
> [cannot apply to next-20240223 dtor-input/next dtor-input/for-linus horms-ipvs/master]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-Put-all-the-clamp-
> definitions-together/20240226-005902
> base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> patch link: https://lore.kernel.org/r/8657dd5c2264456f8a005520a3b90e2b%40AcuMS.aculab.com
> patch subject: [PATCH next v2 03/11] minmax: Simplify signedness check
> config: alpha-defconfig (https://download.01.org/0day-ci/archive/20240227/202402270937.9kmO5PFt-
> [email protected]/config)
> compiler: alpha-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-
> ci/archive/20240227/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/kernel.h:28,
> from include/linux/cpumask.h:10,
> from include/linux/smp.h:13,
> from include/linux/lockdep.h:14,
> from include/linux/spinlock.h:63,
> from include/linux/swait.h:7,
> from include/linux/completion.h:12,
> from include/linux/crypto.h:15,
> from include/crypto/aead.h:13,
> from include/crypto/internal/aead.h:11,
> from crypto/skcipher.c:12:
> crypto/skcipher.c: In function 'skcipher_get_spot':
> >> include/linux/minmax.h:31:70: warning: ordered comparison of pointer with integer zero [-Wextra]
> 31 | (is_unsigned_type(typeof(x)) || (__is_constexpr(x) ? (x) + 0 >= 0 : 0))
Hmmm -Wextra isn't normally set.
But I do wish the compiler would do dead code elimination before
these warnings.
Apart from stopping code using min()/max() for pointer types
(all the type checking is pointless) I think that __is_constextr()
can be implemented using _Generic (instead of sizeof(type)) and then the
true/false return values can be specified and need not be the same types.
That test can then be:
(__if_constexpr(x, x, -1) >= 0)
(The '+ 0' is there to convert bool to int and won't be needed
for non-constant bool.)
I may drop the last few patches until MIN/MAX have been removed
from everywhere else to free up the names.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)