[RFC PATCH net-next v2 0/2] driver/net/ethernet W=1 by default

[RFC PATCH net-next v2 0/2] driver/net/ethernet W=1 by default

[Andrew Lunn]

There is a movement to make the code base compile clean with W=1. Some
subsystems are already clean. In order to keep them clean, we need
developers to build new code with W=1 by default in these
subsystems. Otherwise new warnings will be added. To prove the point:

commit e666a4c668528ae1f5b8b3a2e7cb6a5be488dfbb
Merge: d0186842ec5f d0ea5cbdc286
Author: David S. Miller <davem@davemloft.net>
Date:   Fri Sep 25 16:29:00 2020 -0700

    Merge branch 'drivers-net-warning-clean'

    Jesse Brandeburg says:

    ====================
    make drivers/net/ethernet W=1 clean

Then 4 days later a new W=1 warning has added:

drivers/net/ethernet//chelsio/inline_crypto/ch_ktls/chcr_ktls.c: In function ‘chcr_ktls_cpl_set_tcb_rpl’:
drivers/net/ethernet//chelsio/inline_crypto/ch_ktls/chcr_ktls.c:684:22: warning: implicit conversion from ‘enum <anonymous>’ to ‘enum ch_ktls_open_state’ [-Wenum-conversion]
  684 |  tx_info->open_state = false;

This patchset refactors the core Makefile warning code to allow the
additional warnings W=1 adds available to any Makefile. The Ethernet
driver subsystem Makefiles then make use of this to make W=1 the
default for this subsystem.

v2:

Address the comment that we need to be able to add new W=1 compiler
flags without actually causing new warnings for builds which don't have W=1

Andrew Lunn (2):
  Makefile.extrawarn: Add symbol for W=1 warnings for today
  driver/net/ethernet: Sign up for W=1 as defined on 20200930

 drivers/net/ethernet/Makefile |  3 +++
 scripts/Makefile.extrawarn    | 34 ++++++++++++++++++----------------
 2 files changed, 21 insertions(+), 16 deletions(-)

-- 
2.28.0

[PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

There is a movement to try to make more and more of /drivers W=1
clean. But it will only stay clean if new warnings are quickly
detected and fixed, ideally by the developer adding the new code.

To allow subdirectories to sign up to being W=1 clean for a given
definition of W=1, export the current set of additional compile flags
using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
then use:

subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)

To indicate they want to W=1 warnings as defined on 20200930.

Additional warnings can be added to the W=1 definition. This will not
affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
start appearing unless W=1 is actually added to the command
line. Developers can then take their time to fix any new W=1 warnings,
and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 scripts/Makefile.extrawarn | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 95e4cdb94fe9..957dca35ae3e 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -20,24 +20,26 @@ export KBUILD_EXTRA_WARN
 #
 # W=1 - warnings which may be relevant and do not occur too often
 #
-ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
-
-KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter
-KBUILD_CFLAGS += -Wmissing-declarations
-KBUILD_CFLAGS += -Wmissing-format-attribute
-KBUILD_CFLAGS += -Wmissing-prototypes
-KBUILD_CFLAGS += -Wold-style-definition
-KBUILD_CFLAGS += -Wmissing-include-dirs
-KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable)
-KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable)
-KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned)
-KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation)
+KBUILD_CFLAGS_W1_20200930 += -Wextra -Wunused -Wno-unused-parameter
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-declarations
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-format-attribute
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-prototypes
+KBUILD_CFLAGS_W1_20200930 += -Wold-style-definition
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-include-dirs
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-but-set-variable)
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-const-variable)
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wpacked-not-aligned)
+KBUILD_CFLAGS_W1_20200930 += $(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_CFLAGS_W1_20200930 += -Wno-missing-field-initializers
+KBUILD_CFLAGS_W1_20200930 += -Wno-sign-compare
+KBUILD_CFLAGS_W1_20200930 += -Wno-type-limits
+
+export KBUILD_CFLAGS_W1_20200930
+
+ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
 
-KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
+KBUILD_CPPFLAGS += $(KBUILD_CFLAGS_W1_20200930) -DKBUILD_EXTRA_WARN1
 
 else
 
-- 
2.28.0

[PATCH net-next v2 2/2] driver/net/ethernet: Sign up for W=1 as defined on 20200930

[Andrew Lunn]

Make all Ethernet drivers be compiled with the equivalent of W=1
as defined today.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index f8f38dcb5f8a..8162b2f6ec81 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -3,6 +3,9 @@
 # Makefile for the Linux network Ethernet device drivers.
 #
 
+# Enable W=1, as defined on the given date
+subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
+
 obj-$(CONFIG_NET_VENDOR_3COM) += 3com/
 obj-$(CONFIG_NET_VENDOR_8390) += 8390/
 obj-$(CONFIG_NET_VENDOR_ADAPTEC) += adaptec/
-- 
2.28.0

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Nick Desaulniers]

On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> There is a movement to try to make more and more of /drivers W=1
> clean. But it will only stay clean if new warnings are quickly
> detected and fixed, ideally by the developer adding the new code.
>
> To allow subdirectories to sign up to being W=1 clean for a given
> definition of W=1, export the current set of additional compile flags
> using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> then use:
>
> subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
>
> To indicate they want to W=1 warnings as defined on 20200930.
>
> Additional warnings can be added to the W=1 definition. This will not
> affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> start appearing unless W=1 is actually added to the command
> line. Developers can then take their time to fix any new W=1 warnings,
> and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.

I'm not a fan of this approach.  Are DATESTAMP configs just going to
keep being added? Is it going to complicate isolating the issue from a
randconfig build?  If we want more things to build warning-free at
W=1, then why don't we start moving warnings from W=1 into the
default, until this is no W=1 left?  That way we're cutting down on
the kernel's configuration combinatorial explosion, rather than adding
to it?

>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  scripts/Makefile.extrawarn | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 95e4cdb94fe9..957dca35ae3e 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -20,24 +20,26 @@ export KBUILD_EXTRA_WARN
>  #
>  # W=1 - warnings which may be relevant and do not occur too often
>  #
> -ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
> -
> -KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter
> -KBUILD_CFLAGS += -Wmissing-declarations
> -KBUILD_CFLAGS += -Wmissing-format-attribute
> -KBUILD_CFLAGS += -Wmissing-prototypes
> -KBUILD_CFLAGS += -Wold-style-definition
> -KBUILD_CFLAGS += -Wmissing-include-dirs
> -KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable)
> -KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable)
> -KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned)
> -KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation)
> +KBUILD_CFLAGS_W1_20200930 += -Wextra -Wunused -Wno-unused-parameter
> +KBUILD_CFLAGS_W1_20200930 += -Wmissing-declarations
> +KBUILD_CFLAGS_W1_20200930 += -Wmissing-format-attribute
> +KBUILD_CFLAGS_W1_20200930 += -Wmissing-prototypes
> +KBUILD_CFLAGS_W1_20200930 += -Wold-style-definition
> +KBUILD_CFLAGS_W1_20200930 += -Wmissing-include-dirs
> +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-but-set-variable)
> +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-const-variable)
> +KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wpacked-not-aligned)
> +KBUILD_CFLAGS_W1_20200930 += $(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_CFLAGS_W1_20200930 += -Wno-missing-field-initializers
> +KBUILD_CFLAGS_W1_20200930 += -Wno-sign-compare
> +KBUILD_CFLAGS_W1_20200930 += -Wno-type-limits
> +
> +export KBUILD_CFLAGS_W1_20200930
> +
> +ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
>
> -KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
> +KBUILD_CPPFLAGS += $(KBUILD_CFLAGS_W1_20200930) -DKBUILD_EXTRA_WARN1
>
>  else
>
> --
> 2.28.0
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201001011232.4050282-2-andrew%40lunn.ch.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > There is a movement to try to make more and more of /drivers W=1
> > clean. But it will only stay clean if new warnings are quickly
> > detected and fixed, ideally by the developer adding the new code.
> >
> > To allow subdirectories to sign up to being W=1 clean for a given
> > definition of W=1, export the current set of additional compile flags
> > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> > then use:
> >
> > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
> >
> > To indicate they want to W=1 warnings as defined on 20200930.
> >
> > Additional warnings can be added to the W=1 definition. This will not
> > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> > start appearing unless W=1 is actually added to the command
> > line. Developers can then take their time to fix any new W=1 warnings,
> > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.
> 
> I'm not a fan of this approach.  Are DATESTAMP configs just going to
> keep being added? Is it going to complicate isolating the issue from a
> randconfig build?  If we want more things to build warning-free at
> W=1, then why don't we start moving warnings from W=1 into the
> default, until this is no W=1 left?  That way we're cutting down on
> the kernel's configuration combinatorial explosion, rather than adding
> to it?

Hi Nick

I don't see randconfig being an issue. driver/net/ethernet would
always be build W=1, by some stable definition of W=1. randconfig
would not enable or disable additional warnings. It to make it clear,
KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It
is a Makefile constant, a list of warnings which define what W=1 means
on that specific day. See patch 1/2.

I see a few issues with moving individual warnings from W=1 to the
default:

One of the comments for v1 of this patchset is that we cannot
introduce new warnings in the build. The complete tree needs to clean
of a particularly warning, before it can be added to the default list.
But that is not how people are cleaning up code, nor how the
infrastructure is designed. Those doing the cleanup are not take the
first from the list, -Wextra and cleanup up the whole tree for that
one warnings. They are rather enabling W=1 on a subdirectory, and
cleanup up all warnings on that subdirectory. So using this approach,
in order to move a warning from W=1 to the default, we are going to
have to get the entire tree W=1 clean, and move them all the warnings
are once.

People generally don't care about the tree as a whole. They care about
their own corner. The idea of fixing one warning thought the whole
tree is 'slicing and dicing' the kernel the wrong way. As we have seen
with the recent work with W=1, the more natural way to slice/dice the
kernel is by subdirectories.

I do however understand the fear that we end up with lots of
KBUILD_CFLAGS_W1_<DATESTAMP> constants. So i looked at the git history
of script/Makefile.extrawarn. These are historically the symbols we
would have, if we started this idea 1/1/2018:

KBUILD_CFLAGS_W1_20200326    # CLANG only change
KBUILD_CFLAGS_W1_20190907
KBUILD_CFLAGS_W1_20190617    # CLANG only change
KBUILD_CFLAGS_W1_20190614    # CLANG only change
KBUILD_CFLAGS_W1_20190509
KBUILD_CFLAGS_W1_20180919
KBUILD_CFLAGS_W1_20180111

So not too many.

   Andrew

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

Hi Andrew,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e13dbc4f41db7f7b86f17a2efd7fbe19fc5b6d7a
config: parisc-randconfig-r016-20200930 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b50d78df08d105cf0f0f2a1f4d2225656fd531cc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
        git checkout b50d78df08d105cf0f0f2a1f4d2225656fd531cc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/parisc/boot/compressed/firmware.c: In function 'set_firmware_width_unlocked':
>> arch/parisc/boot/compressed/firmware.c:159:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
     159 |  int ret;
         |      ^~~
   arch/parisc/boot/compressed/firmware.c: In function 'set_firmware_width':
   arch/parisc/boot/compressed/firmware.c:176:16: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
     176 |  unsigned long flags;
         |                ^~~~~
   arch/parisc/boot/compressed/firmware.c: In function 'pdc_iodc_print':
   arch/parisc/boot/compressed/firmware.c:1234:16: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
    1234 |  unsigned long flags;
         |                ^~~~~
   At top level:
   arch/parisc/boot/compressed/firmware.c:121:22: warning: 'f_extend' defined but not used [-Wunused-function]
     121 | static unsigned long f_extend(unsigned long address)
         |                      ^~~~~~~~

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27348 bytes --]

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Arnd Bergmann]

On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > There is a movement to try to make more and more of /drivers W=1
> > > clean. But it will only stay clean if new warnings are quickly
> > > detected and fixed, ideally by the developer adding the new code.

Nice, I think everyone agrees that this is a good goal.

> > > To allow subdirectories to sign up to being W=1 clean for a given
> > > definition of W=1, export the current set of additional compile flags
> > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> > > then use:
> > >
> > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
> > >
> > > To indicate they want to W=1 warnings as defined on 20200930.
> > >
> > > Additional warnings can be added to the W=1 definition. This will not
> > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> > > start appearing unless W=1 is actually added to the command
> > > line. Developers can then take their time to fix any new W=1 warnings,
> > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.
> >
> > I'm not a fan of this approach.  Are DATESTAMP configs just going to
> > keep being added? Is it going to complicate isolating the issue from a
> > randconfig build?  If we want more things to build warning-free at
> > W=1, then why don't we start moving warnings from W=1 into the
> > default, until this is no W=1 left?  That way we're cutting down on
> > the kernel's configuration combinatorial explosion, rather than adding
> > to it?

I'm also a little sceptical about the datestamp.

> Hi Nick
>
> I don't see randconfig being an issue. driver/net/ethernet would
> always be build W=1, by some stable definition of W=1. randconfig
> would not enable or disable additional warnings. It to make it clear,
> KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It
> is a Makefile constant, a list of warnings which define what W=1 means
> on that specific day. See patch 1/2.

It won't change with the configuration, but it will change with the
specific compiler version. When you enable a warning in a
particular driver or directory, this may have different effects
on one compiler compared to another: clang and gcc sometimes
differ in their interpretation of which specific forms of an expression
to warn about or not, and any multiplexing options like -Wextra
or -Wformat may turn on additional warnings in later releases.

> I see a few issues with moving individual warnings from W=1 to the
> default:
>
> One of the comments for v1 of this patchset is that we cannot
> introduce new warnings in the build. The complete tree needs to clean
> of a particularly warning, before it can be added to the default list.
> But that is not how people are cleaning up code, nor how the
> infrastructure is designed. Those doing the cleanup are not take the
> first from the list, -Wextra and cleanup up the whole tree for that
> one warnings. They are rather enabling W=1 on a subdirectory, and
> cleanup up all warnings on that subdirectory. So using this approach,
> in order to move a warning from W=1 to the default, we are going to
> have to get the entire tree W=1 clean, and move them all the warnings
> are once.

I think the two approaches are orthogonal, and I would like to
see both happening as much as possible:

- any warning flag in the W=1 set (including many things implied
  by -Wextra that have or should have their own flags) that
  only causes a few lines of output should just be enabled by
  default after we address the warnings

- Code with maintainers that care should have a way to enable
  the entire W=1 set per directory or per file after addressing all
  the warnings they do see, with new flags only getting added to
  W=1 when they don't cause regressions.

There are more things that we might want to do on top of this:

- identify additional warning flags that we be good to add to W=1

- identify warning flags that are better off being turned into errors,
  like we do with -Werror=strict-prototypes

- Fix the warnings in W=2 that show up in common header files,
  to actually make it realistic to build specific drivers with W=2
  and not have interesting issues drowned out in the noise.

- ensure that any warning flag we turn *off* in W=1 or by default
  is turned on again in one of the higher levels

> People generally don't care about the tree as a whole. They care about
> their own corner. The idea of fixing one warning thought the whole
> tree is 'slicing and dicing' the kernel the wrong way. As we have seen
> with the recent work with W=1, the more natural way to slice/dice the
> kernel is by subdirectories.

I do care about the tree as a whole, and I'm particularly interested in
having -Wmissing-declarations/-Wmissing-prototypes enabled globally
at some point in the future.

        Arnd

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

On Fri, Oct 02, 2020 at 02:20:50PM +0200, Arnd Bergmann wrote:
> On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > There is a movement to try to make more and more of /drivers W=1
> > > > clean. But it will only stay clean if new warnings are quickly
> > > > detected and fixed, ideally by the developer adding the new code.
> 
> Nice, I think everyone agrees that this is a good goal.
> 
> > > > To allow subdirectories to sign up to being W=1 clean for a given
> > > > definition of W=1, export the current set of additional compile flags
> > > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> > > > then use:
> > > >
> > > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
> > > >
> > > > To indicate they want to W=1 warnings as defined on 20200930.
> > > >
> > > > Additional warnings can be added to the W=1 definition. This will not
> > > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> > > > start appearing unless W=1 is actually added to the command
> > > > line. Developers can then take their time to fix any new W=1 warnings,
> > > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.
> > >
> > > I'm not a fan of this approach.  Are DATESTAMP configs just going to
> > > keep being added? Is it going to complicate isolating the issue from a
> > > randconfig build?  If we want more things to build warning-free at
> > > W=1, then why don't we start moving warnings from W=1 into the
> > > default, until this is no W=1 left?  That way we're cutting down on
> > > the kernel's configuration combinatorial explosion, rather than adding
> > > to it?
> 
> I'm also a little sceptical about the datestamp.

Hi Arnd

Any suggestions for an alternative?

> It won't change with the configuration, but it will change with the
> specific compiler version. When you enable a warning in a
> particular driver or directory, this may have different effects
> on one compiler compared to another: clang and gcc sometimes
> differ in their interpretation of which specific forms of an expression
> to warn about or not, and any multiplexing options like -Wextra
> or -Wformat may turn on additional warnings in later releases.

How do we deal with this at the moment? Or will -Wextra and -Wformat
never get moved into the default?

> I think the two approaches are orthogonal, and I would like to
> see both happening as much as possible:
> 
> - any warning flag in the W=1 set (including many things implied
>   by -Wextra that have or should have their own flags) that
>   only causes a few lines of output should just be enabled by
>   default after we address the warnings

Is there currently any simple way to get statistics about how many
actual warnings there are per warnings flag in W=1? W=1 on the tree as
a whole does not look good, but maybe there is some low hanging fruit
and we can quickly promote some from W=1 to default?

> - Code with maintainers that care should have a way to enable
>   the entire W=1 set per directory or per file after addressing all
>   the warnings they do see, with new flags only getting added to
>   W=1 when they don't cause regressions.

Yes, this is what i'm trying to push forward here. I don't
particularly care how it happen, so if somebody comes up with a
generally acceptable idea, i will go away and implement it.

> > People generally don't care about the tree as a whole. They care about
> > their own corner. The idea of fixing one warning thought the whole
> > tree is 'slicing and dicing' the kernel the wrong way. As we have seen
> > with the recent work with W=1, the more natural way to slice/dice the
> > kernel is by subdirectories.
> 
> I do care about the tree as a whole, and I'm particularly interested in
> having -Wmissing-declarations/-Wmissing-prototypes enabled globally
> at some point in the future.

I know you care. But you are vastly out numbered by developers who
care about their own little corner. Which is why i said 'generally'.

We definitely should come at the problem from both directions, but i
guess we can make more progress with tools for the large number of
developers each in their own corner, than tools for the few who work
tree wide.

     Andrew

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Arnd Bergmann]

On Fri, Oct 2, 2020 at 2:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
> On Fri, Oct 02, 2020 at 02:20:50PM +0200, Arnd Bergmann wrote:
> > On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > > >
> > > > I'm not a fan of this approach.  Are DATESTAMP configs just going to
> > > > keep being added? Is it going to complicate isolating the issue from a
> > > > randconfig build?  If we want more things to build warning-free at
> > > > W=1, then why don't we start moving warnings from W=1 into the
> > > > default, until this is no W=1 left?  That way we're cutting down on
> > > > the kernel's configuration combinatorial explosion, rather than adding
> > > > to it?
> >
> > I'm also a little sceptical about the datestamp.
>
> Hi Arnd
>
> Any suggestions for an alternative?

I think we should deal with it the same way we deal with new
compiler versions: before a new compiler starts getting widely
used, someone has to address the new warnings that show up,
or at the minimum they have to get turned off by default until
they are addressed.

Today, moving a warning flag from W=1 to default requires that
there won't be any regressions in the output. The same should
apply to adding W=1 warnings if there is a way for drivers to
default-enable them.

> > It won't change with the configuration, but it will change with the
> > specific compiler version. When you enable a warning in a
> > particular driver or directory, this may have different effects
> > on one compiler compared to another: clang and gcc sometimes
> > differ in their interpretation of which specific forms of an expression
> > to warn about or not, and any multiplexing options like -Wextra
> > or -Wformat may turn on additional warnings in later releases.
>
> How do we deal with this at the moment? Or will -Wextra and -Wformat
> never get moved into the default?

I think for Wextra, that would likely stay with W=1, though individual
warnings in that set should be enabled by default whenever they
make sense. For -Wformat, we probably want the opposite and
enable the global option by default but make individual sub-options
W=1 or W=2 if there is too much undesired output.

> > I think the two approaches are orthogonal, and I would like to
> > see both happening as much as possible:
> >
> > - any warning flag in the W=1 set (including many things implied
> >   by -Wextra that have or should have their own flags) that
> >   only causes a few lines of output should just be enabled by
> >   default after we address the warnings
>
> Is there currently any simple way to get statistics about how many
> actual warnings there are per warnings flag in W=1? W=1 on the tree as
> a whole does not look good, but maybe there is some low hanging fruit
> and we can quickly promote some from W=1 to default?

I have done this a few times in the past, essentially building a
few hundred randconfig kernels across multiple architectures
and then processing the output in a script. I usually treat a
file:line:warning tuple as a single instance and then count
how many there are.

> > - Code with maintainers that care should have a way to enable
> >   the entire W=1 set per directory or per file after addressing all
> >   the warnings they do see, with new flags only getting added to
> >   W=1 when they don't cause regressions.
>
> Yes, this is what i'm trying to push forward here. I don't
> particularly care how it happen, so if somebody comes up with a
> generally acceptable idea, i will go away and implement it.

I actually have a set of patches that I started a while ago to
move the logic from scripts/Makefile.extrawarn into
include/linux/warnings.h, using '_Pragma("GCC diagnostic ...")'
with some infrastructure around it, to also allow drivers to
set the level as well as individual warnings when needed.

I never managed to get that patch series into a state for submission
so far, with a few things that need to be addressed first:

- any Makefile that changes warning options needs to be
  converted to use macro syntax

- I need to check that the patches don't accidentally disable
  warnings that are currently enabled (this is harder than
  checking the reverse)

- some specific warnings have problems with this new method
  for options that control multiple other options.

        Arnd

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Nick Desaulniers]

On Thu, Oct 1, 2020 at 6:44 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > There is a movement to try to make more and more of /drivers W=1
> > > clean. But it will only stay clean if new warnings are quickly
> > > detected and fixed, ideally by the developer adding the new code.
> > >
> > > To allow subdirectories to sign up to being W=1 clean for a given
> > > definition of W=1, export the current set of additional compile flags
> > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> > > then use:
> > >
> > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
> > >
> > > To indicate they want to W=1 warnings as defined on 20200930.
> > >
> > > Additional warnings can be added to the W=1 definition. This will not
> > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> > > start appearing unless W=1 is actually added to the command
> > > line. Developers can then take their time to fix any new W=1 warnings,
> > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.
> >
> > I'm not a fan of this approach.  Are DATESTAMP configs just going to
> > keep being added? Is it going to complicate isolating the issue from a
> > randconfig build?  If we want more things to build warning-free at
> > W=1, then why don't we start moving warnings from W=1 into the
> > default, until this is no W=1 left?  That way we're cutting down on
> > the kernel's configuration combinatorial explosion, rather than adding
> > to it?
>
> Hi Nick
>
> I don't see randconfig being an issue. driver/net/ethernet would
> always be build W=1, by some stable definition of W=1. randconfig
> would not enable or disable additional warnings. It to make it clear,
> KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It
> is a Makefile constant, a list of warnings which define what W=1 means
> on that specific day. See patch 1/2.
>
> I see a few issues with moving individual warnings from W=1 to the
> default:
>
> One of the comments for v1 of this patchset is that we cannot
> introduce new warnings in the build. The complete tree needs to clean
> of a particularly warning, before it can be added to the default list.
> But that is not how people are cleaning up code, nor how the
> infrastructure is designed. Those doing the cleanup are not take the
> first from the list, -Wextra and cleanup up the whole tree for that
> one warnings. They are rather enabling W=1 on a subdirectory, and
> cleanup up all warnings on that subdirectory. So using this approach,
> in order to move a warning from W=1 to the default, we are going to
> have to get the entire tree W=1 clean, and move them all the warnings
> are once.

Sorry, to be more specific about my concern; I like the idea of
exporting the W=* flags, then selectively applying them via
subdir-ccflags-y.  I don't like the idea of supporting W=1 as defined
at a precise point in time via multiple date specific symbols.  If
someone adds something to W=1, then they should need to ensure subdirs
build warning-free, so I don't think you need to "snapshot" W=1 based
on what it looked like on 20200930.

>
> People generally don't care about the tree as a whole. They care about
> their own corner. The idea of fixing one warning thought the whole
> tree is 'slicing and dicing' the kernel the wrong way. As we have seen
> with the recent work with W=1, the more natural way to slice/dice the
> kernel is by subdirectories.

I'm not sure I agree with this paragraph. ^  If a warning is not
enabled by default implicitly, then someone would need to clean the
tree to turn it on.  It's very messy to apply it on a child directory,
then try to work up.  We've done multiple tree wide warning cleanups
and it's not too bad.

>
> I do however understand the fear that we end up with lots of
> KBUILD_CFLAGS_W1_<DATESTAMP> constants. So i looked at the git history
> of script/Makefile.extrawarn. These are historically the symbols we
> would have, if we started this idea 1/1/2018:
>
> KBUILD_CFLAGS_W1_20200326    # CLANG only change
> KBUILD_CFLAGS_W1_20190907
> KBUILD_CFLAGS_W1_20190617    # CLANG only change
> KBUILD_CFLAGS_W1_20190614    # CLANG only change
> KBUILD_CFLAGS_W1_20190509
> KBUILD_CFLAGS_W1_20180919
> KBUILD_CFLAGS_W1_20180111
>
> So not too many.

It's a useful visualization.  I still would prefer W=1 to get enabled
by default if all of the CI systems have flushed out the existing
warnings.  That way we have one less combination of things to test;
not more.
--
Thanks,
~Nick Desaulniers

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

> Sorry, to be more specific about my concern; I like the idea of
> exporting the W=* flags, then selectively applying them via
> subdir-ccflags-y.  I don't like the idea of supporting W=1 as defined
> at a precise point in time via multiple date specific symbols.  If
> someone adds something to W=1, then they should need to ensure subdirs
> build warning-free, so I don't think you need to "snapshot" W=1 based
> on what it looked like on 20200930.

Hi Nick

That then contradicts what Masahiro Yamada said to the first version i
posted:

https://www.spinics.net/lists/netdev/msg685284.html
> With this patch series applied, where should we add -Wfoo-bar?
> Adding it to W=1 would emit warnings under drivers/net/ since W=1 is
> now the default for the net subsystem.

The idea with the date stamps was to allow new warnings to be added to
W=1 without them immediately causing warnings on normal builds. You
are saying that whoever adds a new warning to W=1 needs to cleanup the
tree which is already W=1 clean? That might have the side effect that
no more warnings are added to W=1 :-(

   Andrew

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Arnd Bergmann]

On Mon, Oct 5, 2020 at 9:49 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Sorry, to be more specific about my concern; I like the idea of
> > exporting the W=* flags, then selectively applying them via
> > subdir-ccflags-y.  I don't like the idea of supporting W=1 as defined
> > at a precise point in time via multiple date specific symbols.  If
> > someone adds something to W=1, then they should need to ensure subdirs
> > build warning-free, so I don't think you need to "snapshot" W=1 based
> > on what it looked like on 20200930.
>
> That then contradicts what Masahiro Yamada said to the first version i
> posted:
>
> https://www.spinics.net/lists/netdev/msg685284.html
> > With this patch series applied, where should we add -Wfoo-bar?
> > Adding it to W=1 would emit warnings under drivers/net/ since W=1 is
> > now the default for the net subsystem.
>
> The idea with the date stamps was to allow new warnings to be added to
> W=1 without them immediately causing warnings on normal builds. You
> are saying that whoever adds a new warning to W=1 needs to cleanup the
> tree which is already W=1 clean? That might have the side effect that
> no more warnings are added to W=1 :-(

It depends a lot on what portion of the kernel gets enabled for W=1.

As long as it's only drivers that are actively maintained, and they
make up a fairly small portion of all code, it should not be a problem
to find someone to fix useful warnings.

The only reason to add a flag to W=1 would be that the bugs it reports
are important enough to look at the false positives and address
those as well. Whoever decided to enable W=1 by default for their
subsystem should then also be interested in adding the new warnings.

If I wanted to add a new flag to W=1 and this introduces output
for allmodconfig, I would start by mailing that output to the
respective maintainers.

        Arnd

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

> It depends a lot on what portion of the kernel gets enabled for W=1.
> 
> As long as it's only drivers that are actively maintained, and they
> make up a fairly small portion of all code, it should not be a problem
> to find someone to fix useful warnings.

Well, drivers/net/ethernet is around 1.5M LOC. The tree as a whole is
just short of 23M LOC. So i guess that is a small portion of all the
code.

	Andrew

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Masahiro Yamada]

On Tue, Oct 6, 2020 at 6:08 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > It depends a lot on what portion of the kernel gets enabled for W=1.
> >
> > As long as it's only drivers that are actively maintained, and they
> > make up a fairly small portion of all code, it should not be a problem
> > to find someone to fix useful warnings.
>
> Well, drivers/net/ethernet is around 1.5M LOC. The tree as a whole is
> just short of 23M LOC. So i guess that is a small portion of all the
> code.
>
>         Andrew


I am not a big fan of KBUILD_CFLAGS_W1_<timestamp>
since it is ugly.

I'd like to start with adding individual flags
like drivers/gpu/drm/i915/Makefile, and see
how difficult it would be to maintain it.

One drawback of your approach is that
you cannot set KBUILD_CFLAGS_W1_20200930
until you eliminate all the warnings in the
sub-directory in interest.
(i.e. all or nothing approach)

At best, you can only work out from 'old -> new' order
because KBUILD_CFLAGS_W1_20200326 is a suer-set of
KBUILD_CFLAGS_W1_20190907, which is a suer-set of
KBUILD_CFLAGS_W1_20190617 ...



If you add flags individually, you can start with
low-hanging fruits, or ones with higher priority
as Arnd mentions about -Wmissing-{declaration,prototypes}.


For example, you might be able to set
'subdir-ccflags-y += -Wmissing-declarations'
to drivers/net/Makefile, while
'subdir-ccflags-y += -Wunused-but-set-variable'
stays in drivers/net/ethernet/Makefile.



--
Best Regards
Masahiro Yamada

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Masahiro Yamada]

On Fri, Oct 2, 2020 at 9:21 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> I do care about the tree as a whole, and I'm particularly interested in
> having -Wmissing-declarations/-Wmissing-prototypes enabled globally
> at some point in the future.
>
>         Arnd


BTW, if possible, please educate me about the difference
between -Wmissing-declarations and -Wmissing-prototypes.


The GCC manual says as follows:


-Wmissing-prototypes (C and Objective-C only)

Warn if a global function is defined without a previous prototype
declaration. This warning is issued even if the definition itself
provides a prototype. Use this option to detect global functions that
do not have a matching prototype declaration in a header file. This
option is not valid for C++ because all function declarations provide
prototypes and a non-matching declaration declares an overload rather
than conflict with an earlier declaration. Use -Wmissing-declarations
to detect missing declarations in C++.

-Wmissing-declarations

Warn if a global function is defined without a previous declaration.
Do so even if the definition itself provides a prototype. Use this
option to detect global functions that are not declared in header
files. In C, no warnings are issued for functions with previous
non-prototype declarations; use -Wmissing-prototypes to detect missing
prototypes. In C++, no warnings are issued for function templates, or
for inline functions, or for functions in anonymous namespaces.



The difference is still unclear to me...



For example, if I add -Wmissing-declarations, I get the following:


kernel/sched/core.c:2380:6: warning: no previous declaration for
‘sched_set_stop_task’ [-Wmissing-declarations]
 2380 | void sched_set_stop_task(int cpu, struct task_struct *stop)
      |      ^~~~~~~~~~~~~~~~~~~



But, if I add both -Wmissing-declarations and -Wmissing-prototypes,
-Wmissing-declarations is superseded by -Wmissing-prototypes.



kernel/sched/core.c:2380:6: warning: no previous prototype for
‘sched_set_stop_task’ [-Wmissing-prototypes]
 2380 | void sched_set_stop_task(int cpu, struct task_struct *stop)
      |      ^~~~~~~~~~~~~~~~~~~




Do we need to specify both in W=1 ?
If yes, what is the difference between them?


--
Best Regards
Masahiro Yamada

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Arnd Bergmann]

On Mon, Oct 12, 2020 at 3:00 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> BTW, if possible, please educate me about the difference
> between -Wmissing-declarations and -Wmissing-prototypes.
>
...
> Do we need to specify both in W=1 ?
> If yes, what is the difference between them?

I think they do the same thing in the kernel, as we already set
 '-Wstrict-prototypes', which requires that there are no declarations
without having a prototype first. Adding either one should be sufficient.

      Arnd

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Arnd Bergmann]

On Sun, Oct 11, 2020 at 3:04 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Tue, Oct 6, 2020 at 6:08 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
>
> I am not a big fan of KBUILD_CFLAGS_W1_<timestamp>
> since it is ugly.
>
> I'd like to start with adding individual flags
> like drivers/gpu/drm/i915/Makefile, and see
> how difficult it would be to maintain it.

I don't really like that either, to be honest, mostly because that is
somewhat incompatible with my plan to move all the warning flags
out the command line and into _Pragma() lines in header files.

> One drawback of your approach is that
> you cannot set KBUILD_CFLAGS_W1_20200930
> until you eliminate all the warnings in the
> sub-directory in interest.
> (i.e. all or nothing approach)
>
> At best, you can only work out from 'old -> new' order
> because KBUILD_CFLAGS_W1_20200326 is a suer-set of
> KBUILD_CFLAGS_W1_20190907, which is a suer-set of
> KBUILD_CFLAGS_W1_20190617 ...
>
>
>
> If you add flags individually, you can start with
> low-hanging fruits, or ones with higher priority
> as Arnd mentions about -Wmissing-{declaration,prototypes}.
>
>
> For example, you might be able to set
> 'subdir-ccflags-y += -Wmissing-declarations'
> to drivers/net/Makefile, while
> 'subdir-ccflags-y += -Wunused-but-set-variable'
> stays in drivers/net/ethernet/Makefile.

I agree with the goal though. In particular it may be helpful to pick
a set of warning flags to always be enabled without also setting
-Wextra, which has different meanings depending on compiler
version, or between gcc and clang.

I wonder how different they really are, and we can probably find
out from https://github.com/Barro/compiler-warnings, but I have
not checked myself.

      Arnd

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

> One drawback of your approach is that
> you cannot set KBUILD_CFLAGS_W1_20200930
> until you eliminate all the warnings in the
> sub-directory in interest.
> (i.e. all or nothing approach)

Hi Mashiro

That actual works well for my use case. drivers/net/ethernet is W=1
clean. So is drivers/net/phy, drivers/net/mdio. Developers generally
clean up a subsystem by adding W=1 to the command line because that is
the simple tool they have.

And my aim here is to keep those subsystem W=1 clean. I don't care
about individual warnings within W=1, because those subsystems are
passed that stage already.

    Andrew

Re: [PATCH net-next v2 1/2] Makefile.extrawarn: Add symbol for W=1 warnings for today

[Andrew Lunn]

On Sat, Oct 17, 2020 at 02:48:56PM +0200, Arnd Bergmann wrote:
> On Fri, Oct 16, 2020 at 4:12 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > One drawback of your approach is that
> > > you cannot set KBUILD_CFLAGS_W1_20200930
> > > until you eliminate all the warnings in the
> > > sub-directory in interest.
> > > (i.e. all or nothing approach)
> >
> > Hi Mashiro
> >
> > That actual works well for my use case. drivers/net/ethernet is W=1
> > clean. So is drivers/net/phy, drivers/net/mdio. Developers generally
> > clean up a subsystem by adding W=1 to the command line because that is
> > the simple tool they have.
> >
> > And my aim here is to keep those subsystem W=1 clean. I don't care
> > about individual warnings within W=1, because those subsystems are
> > passed that stage already.
> 
> I tried to get a better grasp of what kind of warnings we are actually talking
> about and looked at the x86 allmodconfig W=1 output on today's linux-next.

Hi Arnd

The work done to cleanup drivers/net/ethernet was mostly done by an
Intel team. When built for ARM there are few warnings left, mostly due
to missing COMPILE_TEST. I have fixes for that.

But this raises the question, can we be a bit more tolerant of
warnings for not x86 to start with? 0-day should help us weed out the
remaining warnings on other architectures.

As for the plan, it looks O.K. to me. I can definitely help with
driver/net and net.

	   Andrew