Messages in this thread | | | From | David Laight <> | Subject | RE: [PATCH 1/1] minmax.h: Slightly relax the type checking done by min() and max(). | Date | Sun, 27 Nov 2022 21:42:06 +0000 |
| |
From: Linus Torvalds > Sent: 27 November 2022 20:37 > > On Fri, Nov 25, 2022 at 7:00 AM David Laight <David.Laight@aculab.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)
| |