lkml.org 
[lkml]   [2022]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()
    Date
    From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
    Date: Mon, 7 Mar 2022 22:50:56 +0900

    > Hi Arnd and Alexander,
    >
    > Thanks for the support!
    >
    > On Mon. 7 Mar 2022 at 21:15, Arnd Bergmann <arnd@arndb.de> wrote:
    > > On Mon, Mar 7, 2022 at 11:58 AM Alexander Lobakin
    > > <alexandr.lobakin@intel.com> wrote:
    > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
    > > > > Have you fixed W=1 warnings?
    > > > > Without fixing W=1 (which makes much more sense, when used with
    > > > > WERROR=y && COMPILE_TEST=y) this has no value.
    > > >
    > > > How is this connected?
    > > > When I do `make W=2 path/to/my/code`, I want to see the actual code
    > > > problems, not something that comes from the include files.
    > > > When I do `make W=2 path/to/new/code/from/lkml`, I want to see the
    > > > actual new warnings, not something coming from the includes.
    > > > It's much easier to overlook or miss some real warnings when the
    > > > stderr is being flooded by the warnings from the include files.
    > > > I'm aware there are some scripts to compare before/after, but I
    > > > don't want to use them just because "this has to value".
    > > > I don't want to do `make W=2 KCFLAGS='-Wno-shadow -Wno-type-limits'`
    > > > because then I'm not able to spot the actual shadow or type limit
    > > > problems in my/new code.
    > > > I fixed several `-Wshadow` warnings previously in the include files
    > > > related to networking, and *nobody* said "this has no value" or
    > > > NAKed it. And `-Wshadow` has always been in W=2.
    > >
    > > I agree: if we decide that W=2 warnings are completely useless, we should
    > > either remove the option to build a W=2 kernel or remove some of the warning
    > > flags that go into it. My feeling is that both W=2 in general, and the
    > > Wtype-limits have some value, and that reducing the number of W=2 by
    > > 30% as this patch does is a useful goal by itself.
    > >
    > > A different question is whether this particular patch is the best
    > > workaround for the warnings, or if a nicer alternative can be found,
    > > such as moving -Wtype-limits to W=3,
    >
    > I disagree with moving it to W=3 for two reasons:
    >
    > 1/ This would just move the issue elsewhere. If I had to
    > compile with W=3 (which I admittedly *almost* never do), the
    > -Wtype-limits spam would still be there.
    >
    > 2/ After this patch, the number of remaining -Wtype-limits
    > drops to only 431 for an allyesconfig (and I guess that there
    > are a fair amount of true positives here). This warning is not
    > *as broken* as people think. W=2 is a good place I think.

    Agree, W=2 is the best place for -Wtype-limits to me.
    I've never seen a single useful warning on W=3, but there were
    lots of them on W=2 which revealed some real bugs. That said, it
    makes no sense at all to even try to check all new code from LKML
    for warnings with W=3, but it's actually feasible to make it build
    cleanly with W=2 most of times.

    Re -Wtype-limits in particular, it's not useless at all.
    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.

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

    Catches as well.

    >
    > That said, moving it to W=3 would still solve the core issue: W=2
    > being spammed. Definitely not my favorite solution, but still an
    > acceptable consensus for me.
    >
    > > or using an open-coded variant
    > > of __is_constexpr() that includes the comparison in a way that avoids the
    > > warning.
    >
    > This is easier said than done. This is the __is_constexpr()
    > macro:
    >
    > | #define __is_constexpr(x) \
    > | (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
    >
    > Good luck doing an open-coded variant of it!
    >
    > What I mean here is that there definitely might be a smarter
    > way than my solution to tackle the issue, but I could not see
    > it. If you have any concrete ideas, please do not hesitate to
    > share :)
    >
    >
    > Yours sincerely,
    > Vincent Mailhol

    Thanks,
    Al

    \
     
     \ /
      Last update: 2022-03-07 16:11    [W:5.688 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site