Re: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

[Linus Torvalds <torvalds@linuxfoundation.org>]

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

On Mon, 29 Jul 2024 at 15:25, Arnd Bergmann <arnd@kernel.org> wrote:
>
> - My macros use __builtin_choose_expr() instead of ?: to
>   ensure that the arguments are constant, this produces a
>   relatively clear compiler warning when they are not.
>   Without that, I would expect random drivers to start
>   using MIN()/MAX() in places where it's not safe.

Hmm. We have known non-constant uses, which Stephen Rothwell pointed
out elsewhere, with PowerPC having things like this:

  #define PAGE_SHIFT              CONFIG_PAGE_SHIFT
  extern unsigned int hpage_shift;
  #define HPAGE_SHIFT hpage_shift
  #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)

and honestly, considering the absolutely *horrid* mess that is the
current min/max expansion, I really think that using MIN/MAX for these
kinds of expressions is the right thing to do.

I don't worry about architecture code that has a boot-time
pseudo-constant for some inherent property.  I worry about random
drivers doing crazy things.

But it is *not* a constant, and any MIN/MAX that disallows it is imho
not a good MIN/MAX.

What we actually care about is not "constant", but "no side effects".

And obviously the typing, but that ends up being really hard.

We could possibly require that the typing be extra strict for MIN/MAX,
using the old __typecheck(x, y) macro we used to use. That literally
requires the *same* types, but that then ends up being pointlessly
painful for the "actual constant" cases, because dammit, a regular
non-negative integer constant should compare with any type.

It's a real pain that compiler writers can't just get the simple cases
right and give us a sane "__builtin_ordered_ok()" kind of thing.
Instead they push things like the absolute unusable garbage that is
-Wsign-compare.

Anyway, I also completely gave up on another absolute standard garbage
thing: _static_assert(). What a horrendous piece of sh*t that is.
Replacing our use of that with our own BUILD_BUG_ON_MSG() made a lot
of things much much simpler.

Attached is the patch I have in my tree right now - it complains about
a 'bcachefs' comparison between an 'u16' and a 's64', because I also
removed the 'implicit integer promotion is ok' logic, because I think
it's wrong.

I don't think a min(u16,s64) is a valid minimum, for exactly the same
reason a min(u32,s64) is not valid.

Despite the C integer expression promotion rules.

             Linus

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

 include/linux/minmax.h | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index e3e4353df983..7ad992d5e464 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -26,19 +26,23 @@
 #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)
+/*
+ * __sign_use for integer expressions:
+ *   bit 1 set if ok for signed comparisons,
+ *   bit 2 set if ok for unsigned comparisons
+ *
+ * In particular, non-negative integer constants
+ * are ok for both.
+ */
+#define __sign_use(x,ux) \
+	(is_signed_type(typeof(ux))?1+2*__is_nonneg(x,ux):2)
 
 /* 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 __is_nonneg(x,ux) \
+	(__builtin_constant_p(x) && !((long long)(x)>>63))
 
-#define __types_ok(x, y, ux, uy) 				\
-	(__is_signed(ux) == __is_signed(uy) ||			\
-	 __is_signed((ux) + 0) == __is_signed((uy) + 0) ||	\
-	 __is_noneg_int(x) || __is_noneg_int(y))
+#define __types_ok(x, y, ux, uy) \
+	(!!(__sign_use(x,ux) & __sign_use(y,uy)))
 
 #define __cmp_op_min <
 #define __cmp_op_max >
@@ -53,8 +57,8 @@
 
 #define __careful_cmp_once(op, x, y, ux, uy) ({		\
 	__auto_type ux = (x); __auto_type uy = (y);	\
-	static_assert(__types_ok(x, y, ux, uy),		\
-		#op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \
+	BUILD_BUG_ON_MSG(!__types_ok(x, y, ux, uy), 	\
+		#op"("#x", "#y") signedness error");	\
 	__cmp(op, ux, uy); })
 
 #define __careful_cmp(op, x, y) \
@@ -70,8 +74,10 @@
 	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(uval, lo, uval, ulo), "clamp() 'lo' signedness error");	\
-	static_assert(__types_ok(uval, hi, uval, uhi), "clamp() 'hi' signedness error");	\
+	BUILD_BUG_ON_MSG(!__types_ok(uval, lo, uval, ulo),			\
+		"clamp("#val", "#lo", ...) signedness error");			\
+	BUILD_BUG_ON_MSG(!__types_ok(uval, hi, uval, uhi),			\
+		"clamp("#val", ..., "#hi") signedness error");			\
 	__clamp(uval, ulo, uhi); })
 
 #define __careful_clamp(val, lo, hi) \