Message-ID: <433b8b44fe6e43b2b576c311bb55cc8a@AcuMS.aculab.com>
Date:   Sun, 27 Nov 2022 21:42:06 +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: 27 November 2022 20:37
> 
> On Fri, Nov 25, 2022 at 7:00 AM David Laight <David.Laight@...lab.com> wrote:
> >
> > - Skip the type test if either argument is a positive 'int' constant.
> >   Instead cast the constant to 'int', the compiler may promote it
> >   back to 'unsigned int' when doing the test.
> 
> No. This looks very wrong to me.
> 
> Maybe I'm mis-reading something, but it looks like this makes a
> "sizeof()" essentially be compatible with an "int" variable.
> 
> That is horrendously wrong. It should warn.
> 
> If you are doing a "min(i,sizeof(X))", and "i" is a signed integer,
> then something is wrong. What does that code expect? It shouldn't
> silently say "this is ok", because it most definitely isn't.

Why should it be a problem?
min(-4, sizeof(X)) becomes min(-4, (int)sizeof(X)) and thus -4.
Without the cast the -4 is converted to a very large unsigned
value so the result is sizeof(X) - not at all expected.

But it is much more likely that there is an earlier test for
negative values, so the domain of 'i' is non-negative.
Then the type-check just forces the coder to use min_t()
and pick some type to 'shut the compiler up'.

So I'm just treating both min(i, 16) and min(i, 16u) as signed
operations.

> So maybe I'm  mis-reading this all and it doesn't actually do what I
> think it does, but this seems to relax things *much* too much.

I did manage to f*ck up the patch in some subtle ways.
Mostly due to the non-intuitive nature of the 'void *' trick.

> There's a reason we require types to be compatible, and you just
> removed some of the important signedness checks.

I'd assumed the main reason was to avoid negative integers being
converted to very large unsigned values.
That is definitely a good idea.

As well as the comparisons of int v small-unsigned constants
there are some others which are currently rejected.

Consider min(u8_var, 16u) no reason to reject that one.
But the types don't match, and the u8_var is first converted
to signed int and then to unsigned int before the comparison.

I also found many examples of code trying to bound u8 variables using
'u8_var = min_t(u8, [u8_var|constant_below_256], unsigned_expression)'.
Maybe the constant should be unsigned, but the 'u8' cast is just
plain wrong.
All the false-positives in the type check in min() just make these
more likely.

I'm looking at also allowing:
	'any unsigned type' v 'any unsigned type'
	'any signed type' v 'any signed type'
Neither of which ever does anything other than what is expected.
And also:
	'any signed type' v 'any smaller unsigned type'
which is also ok because the compiler converts the unsigned
type to the larger signed one and does an unsigned compare.
(Here the 'signed type' can be assumed to be at least 'int'
due to the integer promotions before any arithmetic.)

I need to find a compile-time check for a signed type that
doesn't barf on a pointer!

	David

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

Powered by blists - more mailing lists