| Message ID | 8657dd5c2264456f8a005520a3b90e2b@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | minmax: Optimise to reduce .i line length | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
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/202402270937.9kmO5PFt-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240227/202402270937.9kmO5PFt-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402270937.9kmO5PFt-lkp@intel.com/
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
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- > lkp@intel.com/config) > compiler: alpha-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): (https://download.01.org/0day- > ci/archive/20240227/202402270937.9kmO5PFt-lkp@intel.com/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 <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202402270937.9kmO5PFt-lkp@intel.com/ > > 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)
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 >
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 <david.laight@aculab.com> --- 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.