Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

[Andy Shevchenko]

On Fri, Jun 26, 2020 at 2:37 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
> Den fre 26 juni 2020 08:32Linus Torvalds <torvalds@linux-foundation.org> skrev:

...

> I'll just say no and point to this email next time someone complains instead.

"No" is not constructive here. People can be annoyed with warning
messages, but the real issue here are the various CI systems which
send a lot of spam because of that. As a maintainer I would need to
drop CI in order to see a good patch. If Linus considers that warning
useless, then probably you can change your patch to do what he
proposed.

-- 
With Best Regards,
Andy Shevchenko

Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 3:24 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 2:37 PM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> > Den fre 26 juni 2020 08:32Linus Torvalds <torvalds@linux-foundation.org> skrev:
>
> ...
>
> > I'll just say no and point to this email next time someone complains instead.
>
> "No" is not constructive here. People can be annoyed with warning
> messages, but the real issue here are the various CI systems which
> send a lot of spam because of that. As a maintainer I would need to
> drop CI in order to see a good patch. If Linus considers that warning
> useless, then probably you can change your patch to do what he
> proposed.

How about moving that warning from W=1 to W=2? Generally speaking
I'd expect W=1 warnings to be in a category of "it's generally better to
address this in the code, but we can't turn it on by default because the
output gets too noisy", as opposed to W=2 meaning "this sometimes
finds a real problem, but fixing the warning often makes code worse."

     Arnd

Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

[Andy Shevchenko]

On Fri, Jun 26, 2020 at 5:03 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 26, 2020 at 3:24 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jun 26, 2020 at 2:37 PM Rikard Falkeborn
> > <rikard.falkeborn@gmail.com> wrote:
> > > Den fre 26 juni 2020 08:32Linus Torvalds <torvalds@linux-foundation.org> skrev:
> >
> > ...
> >
> > > I'll just say no and point to this email next time someone complains instead.
> >
> > "No" is not constructive here. People can be annoyed with warning
> > messages, but the real issue here are the various CI systems which
> > send a lot of spam because of that. As a maintainer I would need to
> > drop CI in order to see a good patch. If Linus considers that warning
> > useless, then probably you can change your patch to do what he
> > proposed.
>
> How about moving that warning from W=1 to W=2? Generally speaking
> I'd expect W=1 warnings to be in a category of "it's generally better to
> address this in the code, but we can't turn it on by default because the
> output gets too noisy", as opposed to W=2 meaning "this sometimes
> finds a real problem, but fixing the warning often makes code worse."

It would work for me if it had been
a) documented (I didn't check if it had been already done, though);
b) understood by all CIs in the same way (see a) as well :-).

That said, I like any compromise that will reduce unneeded spam for
submitted patches from CIs and, as a bonus, get rid of warnings in my
local compilations (yes, I usually do W=1).

-- 
With Best Regards,
Andy Shevchenko

Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

[Arnd Bergmann]

On Fri, Jun 26, 2020 at 4:09 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jun 26, 2020 at 5:03 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Fri, Jun 26, 2020 at 3:24 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Jun 26, 2020 at 2:37 PM Rikard Falkeborn
> > > <rikard.falkeborn@gmail.com> wrote:
> > > > Den fre 26 juni 2020 08:32Linus Torvalds <torvalds@linux-foundation.org> skrev:
> > >
> > > ...
> > >
> > > > I'll just say no and point to this email next time someone complains instead.
> > >
> > > "No" is not constructive here. People can be annoyed with warning
> > > messages, but the real issue here are the various CI systems which
> > > send a lot of spam because of that. As a maintainer I would need to
> > > drop CI in order to see a good patch. If Linus considers that warning
> > > useless, then probably you can change your patch to do what he
> > > proposed.
> >
> > How about moving that warning from W=1 to W=2? Generally speaking
> > I'd expect W=1 warnings to be in a category of "it's generally better to
> > address this in the code, but we can't turn it on by default because the
> > output gets too noisy", as opposed to W=2 meaning "this sometimes
> > finds a real problem, but fixing the warning often makes code worse."
>
> It would work for me if it had been
> a) documented (I didn't check if it had been already done, though);
> b) understood by all CIs in the same way (see a) as well :-).

I checked the 'make help' output, which describes them as

make W=n   [targets] Enable extra build checks, n=1,2,3 where
   1: warnings which may be relevant and do not occur too often
   2: warnings which occur quite often but may still be relevant
   3: more obscure warnings, can most likely be ignored
   Multiple levels can be combined with W=12 or W=123

which is less specific than the interpretation I had in mind but
I think still fits a).

      Arnd

Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

[Kees Cook]

On Fri, Jun 26, 2020 at 04:43:36PM +0200, Arnd Bergmann wrote:
> On Fri, Jun 26, 2020 at 4:09 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Fri, Jun 26, 2020 at 5:03 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > It would work for me if it had been
> > a) documented (I didn't check if it had been already done, though);
> > b) understood by all CIs in the same way (see a) as well :-).
> 
> I checked the 'make help' output, which describes them as
> 
> make W=n   [targets] Enable extra build checks, n=1,2,3 where
>    1: warnings which may be relevant and do not occur too often
>    2: warnings which occur quite often but may still be relevant
>    3: more obscure warnings, can most likely be ignored
>    Multiple levels can be combined with W=12 or W=123
> 
> which is less specific than the interpretation I had in mind but
> I think still fits a).

How about tweaking this as:

make W=n   [targets] Enable extra build checks, n=1,2,3 where
   1: warnings which may be relevant and do not occur too often
      and can normally be fixed in code. (Reasonable for CIs to
      use without generating too much spam.)
   2: warnings which occur quite often but may still be relevant
      to developers looking for ways to improve compilers or
      looking for very special cases. (Not usually a good idea
      for automated systems.)
   3: more obscure warnings, can most likely be ignored but have
      value to some very narrow areas of code/compiler analysis.
      Very noisy!



-- 
Kees Cook

Re: [patch 10/32] linux/bits.h: fix unsigned less than zero warnings

[Linus Torvalds]

On Fri, Jun 26, 2020 at 7:03 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> How about moving that warning from W=1 to W=2?

That sounds like the right thing to do, perhaps with some extra
warnings about W=2 (and W=3) being things that you definitely
shouldn't be running in automated environments (unless you are then
very very careful to look very critically at the warnings).

            Linus

[PATCH] kbuild: Move -Wtype-limits to W=2

[Rikard Falkeborn]

-Wtype-limits is included in -Wextra which is added at W=1. It warns
(among other things) that 'comparison of an unsigned variable `< 0` is
always false. This causes noisy warnings, especially when used in
macros, hence it is more suitable for W=2.

Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
See also discussion at https://lore.kernel.org/lkml/CAHk-=wiKCXEWKJ9dWUimGbrVRo_N2RosESUw8E7m9AEtyZcu=w@mail.gmail.com/

 scripts/Makefile.extrawarn | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 4aea7cf71d11..62c275685b75 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -35,6 +35,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation)
 # The following turn off the warnings enabled by -Wextra
 KBUILD_CFLAGS += -Wno-missing-field-initializers
 KBUILD_CFLAGS += -Wno-sign-compare
+KBUILD_CFLAGS += -Wno-type-limits
 
 KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
 
@@ -66,6 +67,7 @@ KBUILD_CFLAGS += -Wshadow
 KBUILD_CFLAGS += $(call cc-option, -Wlogical-op)
 KBUILD_CFLAGS += -Wmissing-field-initializers
 KBUILD_CFLAGS += -Wsign-compare
+KBUILD_CFLAGS += -Wtype-limits
 KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized)
 KBUILD_CFLAGS += $(call cc-option, -Wunused-macros)
 
-- 
2.27.0

Re: [PATCH] kbuild: Move -Wtype-limits to W=2

[Andy Shevchenko]

On Wed, Jul 8, 2020 at 10:08 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> -Wtype-limits is included in -Wextra which is added at W=1. It warns
> (among other things) that 'comparison of an unsigned variable `< 0` is
> always false. This causes noisy warnings, especially when used in
> macros, hence it is more suitable for W=2.
>

Suggested-by: Arnd ?

LGTM!

> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> See also discussion at https://lore.kernel.org/lkml/CAHk-=wiKCXEWKJ9dWUimGbrVRo_N2RosESUw8E7m9AEtyZcu=w@mail.gmail.com/
>
>  scripts/Makefile.extrawarn | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 4aea7cf71d11..62c275685b75 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -35,6 +35,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation)
>  # The following turn off the warnings enabled by -Wextra
>  KBUILD_CFLAGS += -Wno-missing-field-initializers
>  KBUILD_CFLAGS += -Wno-sign-compare
> +KBUILD_CFLAGS += -Wno-type-limits
>
>  KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
>
> @@ -66,6 +67,7 @@ KBUILD_CFLAGS += -Wshadow
>  KBUILD_CFLAGS += $(call cc-option, -Wlogical-op)
>  KBUILD_CFLAGS += -Wmissing-field-initializers
>  KBUILD_CFLAGS += -Wsign-compare
> +KBUILD_CFLAGS += -Wtype-limits
>  KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized)
>  KBUILD_CFLAGS += $(call cc-option, -Wunused-macros)
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko