Message-ID: <a74be3ec15294206a13cd5b3a4b35858@AcuMS.aculab.com>
Date:   Mon, 28 Nov 2022 09:10:16 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Linus Torvalds' <torvalds@...ux-foundation.org>
CC:     LKML <linux-kernel@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        "Joe Perches" <joe@...ches.com>
Subject: RE: [PATCH 1/1] minmax.h: Slightly relax the type checking done by
 min() and max().

From: Linus Torvalds
> Sent: 28 November 2022 02:43
..
> So your patch that makes "one side is signed, other side is unsigned,
> so normally we'd do it as an unsigned compare. But because the
> unsigned side was a constant, we made it signed after all, and do the
> comparison as signed with no warning".
> 
> That's just *horrible*. It's even crazier than the regular C type
> conversions that (some) people have at least had decades of getting
> used to.

Ah, you might be missing the point that the compiler
converts it back to an unsigned compare.

So you start with:
	unsigned int a;
	min(a, 5u)
This gets changed to:
	min(a, (int)5u)
The compiler sees:
	if (a < (int)5u)
and changes the RHS back to unsigned, so you actually get:
	if (a < (unsigned int)(int)5u)
which is exactly where you started.

> 
> Don't you see how nasty that kind of completely random thing is?
> 
> Now, I agree that sometimes we warn *too* much, but no, it's not
> acceptable to change that to "let's not warn as much, and in the
> process also change the sign of the comparison in strange ways".
> 
> If it was the *other* way around, where we warned too much, but at
> least didn't change the actual semantics, that would be one thing. So,
> for example, I think that if you have
> 
>     unsigned long a;
> 
> then:
> 
>  - min(a,5)
> 
>  - min(a,5u)
> 
> both currently warn "annoyingly", even though the result is obvious.
> 
> One because "5" is an "int" and thus signed, but hey, comparing a
> signed *positive* constant to a unsigned variable is pretty darn safe.
> 
> So getting rid of the warning in that case - and just doing it as an
> unsigned comparison which then also gives the smallest possible result
> range and as such cannot possibly confuse anything - would likely be a
> good thing.
> 
> And the fact that '5u' _also_ warns is just annoying. There's zero
> ambiguity about the result (which will always fit in 'unsigned int'),
> but the comparison should always be done in 'unsigned long'.

If the 5u is cast to (int) then, in this case, the comparison
is still done as 'unsigned long'.

There is also this one:
	unsigned char c;
	min(c,5u)
Again pretty unambiguous but the types don't match.
Integer promotions start playing havoc here.
	(c < 5u) => ((int)c < 5u) => (unsigned int)(int)c < 5u)
which is actually what you expect.
But repeat with 'signed char' and negatives get an unexpected
result.

> And for that '5u' case there is even _less_ of a chance that there
> could be any sign confusion.
> 
> But note that "signed 5" thing: it's really really important to
> understand that doing that constant comparison as an *unsigned*
> comparison is much safer for 'min()', because it minimizes the result
> range. Returning a negative number because you converted it to a
> signed comparison would be potentially dangerous, because people often
> forget to thin kabout the negative case. Returning the range 0..5 is
> _clearly_ safe.
> 
> And that danger is very much when the '5' is a 'sizeof(xyz)'. We're
> clearly talking about object sizes that cannot be negative, so
> negative numbers are almost certainly wrong. Making the 'min()' return
> a negative number there is horrendous.
> 
> Now, for 'max()', that 'minimize the range' argument doesn't work.
> 
> Right now we just have "both must be the same type". At least that is
> always unambiguous. It can be annoying, yes. But then the expectation
> is that when somebody changes it to a "min_t()", they activel;y
> *THINK* about it.

Not from looking at some of the types used.
Some code seems to written thinking that the type for min_t is that
of the result type needed (like a pointless cast for the assignment
rather than a cast that is applied to both inputs.

I'm testing some changes that allow:
	min(any_unsigned_expr, any_unsigned_expr)
	min(any_signed_expr, any_signed_expr)
and also allow signed v unsigned if either:
	the unsigned type is smaller than the signed one
	(the unsigned value is promoted to the larger signed type)
or
	the signed value is constant and non-negative
in all those cases the normal C rules are sensible.

The one you seem to object to is the sign v unsigned
when the unsigned value is a constant.

I has to be said that there are likely to be very few cases
of min (or max) where the domain of either values can be
negative.
This is assuming no one has committed return min(result, 0).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists