Re: [PATCH next v4 0/5] minmax: Relax type checks in min() and max().

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On Mon, 8 Jan 2024 at 10:19, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> That said, I'm sure this thing exists to a smaller degree elsewhere. I
> wonder if we could simplify our min/max type tests..

Hmm. Gcc seems to have fixed the old (horrid) behavior of warning
about comparing an unsigned variable with a (signed) positive constant
integer, which caused lots of completely unacceptable warnings.

Which means that maybe we could some day enable -Wsign-compare, if we
just fix all the cases we didn't care about because the warning was
fundamentally broken and useless anyway.

So we *could* plan on that, remove the checks from min/max, and use
something like the attached patch.

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4682 bytes --]

 arch/x86/Makefile          |  2 --
 include/linux/irqchip.h    |  3 +++
 include/linux/minmax.h     | 31 +++----------------------------
 init/Kconfig               |  4 ++++
 scripts/Makefile.extrawarn |  1 +
 5 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/arch/x86/Makefile b/arch/x86/Makefile
index 1a068de12a56..b4994eb934bc 100644
--- a/arch/x86/Makefile
+++ b/arch/x86/Makefile
@@ -186,8 +186,6 @@ ifeq ($(ACCUMULATE_OUTGOING_ARGS), 1)
 	KBUILD_CFLAGS += $(call cc-option,-maccumulate-outgoing-args,)
 endif
 
-# Workaround for a gcc prelease that unfortunately was shipped in a suse release
-KBUILD_CFLAGS += -Wno-sign-compare
 #
 KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
 
diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index d5e6024cb2a8..6488f3a3ca5c 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -20,6 +20,9 @@
 /* Undefined on purpose */
 extern of_irq_init_cb_t typecheck_irq_init_cb;
 
+#define __typecheck(x, y) \
+	(!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
+
 #define typecheck_irq_init_cb(fn)					\
 	(__typecheck(typecheck_irq_init_cb, &fn) ? fn : fn)
 
diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 2ec559284a9f..b2e42c741859 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -8,37 +8,16 @@
 #include <linux/types.h>
 
 /*
- * min()/max()/clamp() macros must accomplish three things:
+ * min()/max()/clamp() macros must accomplish two 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
- *   compared against signed or unsigned arguments.
- * - 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.
+ *
+ * Hopefully, sign comparison warnings can be done by the compilers.
  */
-#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)
-
-/* 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)
-
-#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))
 
 #define __cmp_op_min <
 #define __cmp_op_max >
@@ -48,8 +27,6 @@
 #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),			\
-		#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
 	__cmp(op, unique_x, unique_y); })
 
 #define __careful_cmp(op, x, y)					\
@@ -67,8 +44,6 @@
 	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) ({					\
diff --git a/init/Kconfig b/init/Kconfig
index 9ffb103fc927..0245253203c0 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -876,6 +876,10 @@ config CC_NO_ARRAY_BOUNDS
 	bool
 	default y if CC_IS_GCC && GCC_VERSION >= 110000 && GCC11_NO_ARRAY_BOUNDS
 
+# -Wsign-compare has traditionally been horrific
+config CC_NO_SIGN_COMPARE
+	bool
+	default y
 #
 # For architectures that know their GCC __int128 support is sound
 #
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 2fe6f2828d37..edef0cbcf7d4 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -25,6 +25,7 @@ endif
 KBUILD_CPPFLAGS-$(CONFIG_WERROR) += -Werror
 KBUILD_CPPFLAGS += $(KBUILD_CPPFLAGS-y)
 KBUILD_CFLAGS-$(CONFIG_CC_NO_ARRAY_BOUNDS) += -Wno-array-bounds
+KBUILD_CFLAGS-$(CONFIG_CC_NO_SIGN_COMPARE) += -Wno-sign-compare
 
 ifdef CONFIG_CC_IS_CLANG
 # The kernel builds with '-std=gnu11' so use of GNU extensions is acceptable.