| Message ID | 20201001011232.4050282-2-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | driver/net/ethernet W=1 by default | expand |
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.
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
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
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
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
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
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
> 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
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
> 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
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
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
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
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
> 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
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
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
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(-)