Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220304124416.1181029-1-mailhol.vincent@wanadoo.fr>
 <CAHp75VeT3LbdbSaiwcC2YW40LnA2h8ADtGva-CKU_xh8Edi0nw@mail.gmail.com>
 <20220307105810.1747024-1-alexandr.lobakin@intel.com> <CAK8P3a07uXLTpNz3t1oMwz_imZOTs+1Fw5hhELjUrJ8Zs=8bpQ@mail.gmail.com>
 <CAMZ6RqKaq8GiPYMyL4D1ksUtgketAF905oBgwKuGLtk7BmobZw@mail.gmail.com>
 <20220307150750.1762040-1-alexandr.lobakin@intel.com> <CAHp75VfXeo0rrKB74t7gJOcv4CEnkByucE+Sa1hT2GTap=oQNA@mail.gmail.com>
In-Reply-To: <CAHp75VfXeo0rrKB74t7gJOcv4CEnkByucE+Sa1hT2GTap=oQNA@mail.gmail.com>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Tue, 8 Mar 2022 21:20:32 +0900
Message-ID: <CAMZ6RqKPicgiU15g3xkFD15-TVsSYTyxNqkCEYVn2EpzrmJcNw@mail.gmail.com>
Subject: Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Alexander Lobakin <alexandr.lobakin@intel.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Rikard Falkeborn <rikard.falkeborn@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue. 8 Mar 2022 à 01:30, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
> <alexandr.lobakin@intel.com> wrote:
> > From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> > 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