Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

[Vincent MAILHOL]

On Tue. 8 Mar 2022 à 01:30, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
> <alexandr.lobakin@xxxxxxxxx> wrote:
> > From: Vincent MAILHOL <mailhol.vincent@xxxxxxxxxx>
> > Date: Mon, 7 Mar 2022 22:50:56 +0900
>
> ...
>
> > For example, people tend to make the following mistake:
> >
> >         unsigned int i;
> >
> >         for (i = 0; i ...) {
> >                 ret = setup_something(array[i]);
> >                 if (ret)
> >                         goto unroll;
> >         }
> >
> > unroll:
> >         while (--i)
> >                 unroll_something(array[i]);
> >
> > The loop will never end as `i` was declared as unsigned.
> > -Wtype-limits catches this.
>
> This looks like a wrapping value issue, not sure if the type limits
> makes logical sense. What I'm saying is that the waning is
> controversial. It may help or it may make noise.
>
> > Not speaking of checking unsigned variables on < 0:
> >
> >         unsigned int num;
> >
> >         /* calculate_something() returns the number of something
> >          * or -ERRNO in case of an error
> >          */
> >         num = calculate_something();
> >         if (num < 0)
> >                 ...
>
> Depends on the context. Here is a mistake, but there are plenty of
> cases when it's okay to do so.

I am curious to see which case you are thinking of.

Personally, I see two cases, both with macro:

  1/ Cases similar to GENMASK_INPUT_CHECK() in which the macro is
  made for generic purpose and in which there was no way to know
  in advance that one parameter would be unsigned and the other
  zero.

  2/ Comparaison again a macro which might or might not be
  zero. e.g.:

#ifdef FOO
#define BAR 42
#else
#define BAR 0
#endif

void baz(void)
{
        unsigned int i;

        if (i > BAR) /* yields -Wtype-limits if FOO is not defined. */
}

And because of those two false positives, moving it to W=2 was a
wise choice.

But I am not aware of any use cases outside of macro where doing
an:

unsigned int num;
/* ... */
if (num < 0)

would be okay. At best it is dead code, at worse, it is a bug as
pointed out by Alexander.

I am not sure what I am missing here.

> And in the above the variable name is
> misleading with its semantics, The proper code should be
>
>   unsigned int num;
>   int ret;
>
>   ret = ...
>   if (ret < 0)
>     ...
>   num = ret;
>
> Again, the warning is controversial in my opinion.
>
> --
> With Best Regards,
> Andy Shevchenko