Message-ID: <CAHk-=wgxzGTsqcNv7B5Cr_BshyRkhrvsPMratxhNb0LA1EnwdA@mail.gmail.com>
Date:   Sun, 27 Nov 2022 18:42:47 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     David Laight <David.Laight@...lab.com>
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().

On Sun, Nov 27, 2022 at 2:26 PM David Laight <David.Laight@...lab.com> wrote:
>
> But what actually happens is the compiler bleats about min()
> so rather then change a constant to be unsigned (etc) the code
> is rewritten with min_t() and both sides are cast to (usually)
> an unsigned type.

Sure, and at least then there is no question about "what is the type
that the comparison is done in".

> There are a non-zero number of cases where the cast masks high
> bits off the large value.

So? At least somebody looked at them.

I'd be ok with a warning for that case of 'min_t()' narrowing a cast,
perhaps. But the normal 'min()' has to be *completely* unambiguous.

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.

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'.

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.

                Linus

Powered by blists - more mailing lists