| Message ID | 20110421213903.GB9660@merkur.ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2011-04-21 at 23:39 +0200, Sam Ravnborg wrote: > Building a kernel with "make W=1" produce far too much noise > to be usefull. > Divide the warning options in three groups: > W=1 - usefull warning options > W=2 - noisy but semi usefull warnign options > W=3 - almost pure noise options > +warning-1 := -Wextra [] > +warning-2 += -Waggregate-return This is a different form for this first warning-2 declaration than the first warning-1 declaration. Maybe this should be := instead? Maybe there's a way to do something akin to warning_type(level, option) So these become warning_type(1, extra) warning_type(2, aggregate-return) etc. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/04/2011 12:39 ??, Sam Ravnborg wrote: > Building a kernel with "make W=1" produce far too much noise > to be usefull. > > Divide the warning options in three groups: > > W=1 - usefull warning options > W=2 - noisy but semi usefull warnign options > W=3 - almost pure noise options > > I do not feel strongly about the distinction between > group 2 and 3. But we should consider what we add in group 1. > > > Sample run on my box (x86, 32bit allyesconfig build) > CC kernel/bounds.s > kernel/bounds.c:14: warning: no previous prototype for ‘foo’ > GEN include/generated/bounds.h > CC arch/x86/kernel/asm-offsets.s > In file included from include/linux/sched.h:73, > from arch/x86/kernel/asm-offsets.c:9: > include/linux/signal.h: In function ‘sigorsets’: > include/linux/signal.h:121: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > include/linux/signal.h: In function ‘sigandsets’: > include/linux/signal.h:124: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > include/linux/signal.h: In function ‘signandsets’: > include/linux/signal.h:127: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > include/linux/signal.h: In function ‘signotset’: > include/linux/signal.h:151: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > arch/x86/kernel/asm-offsets.c: At top level: > arch/x86/kernel/asm-offsets.c:30: warning: no previous prototype for ‘common’ > > the patch is only an RFC - and is not made on top > of an upstream kernel with no additiona stuff applied. > > Sam > > diff --git a/Makefile b/Makefile > index b967b96..f0e138b 100644 > --- a/Makefile > +++ b/Makefile > @@ -103,7 +103,7 @@ ifeq ("$(origin O)", "command line") > endif > > ifeq ("$(origin W)", "command line") > - export KBUILD_ENABLE_EXTRA_GCC_CHECKS := 1 > + export KBUILD_ENABLE_EXTRA_GCC_CHECKS := $(W) > endif > > # That's our default target when none is given on the command line > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index d5f925a..b65f820 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -56,31 +56,43 @@ endif > # $(call cc-option... ) handles gcc -W.. options which > # are not supported by all versions of the compiler > ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS > -KBUILD_EXTRA_WARNINGS := -Wextra > -KBUILD_EXTRA_WARNINGS += -Wunused -Wno-unused-parameter > -KBUILD_EXTRA_WARNINGS += -Waggregate-return > -KBUILD_EXTRA_WARNINGS += -Wbad-function-cast > -KBUILD_EXTRA_WARNINGS += -Wcast-qual > -KBUILD_EXTRA_WARNINGS += -Wcast-align > -KBUILD_EXTRA_WARNINGS += -Wconversion > -KBUILD_EXTRA_WARNINGS += -Wdisabled-optimization > -KBUILD_EXTRA_WARNINGS += -Wlogical-op > -KBUILD_EXTRA_WARNINGS += -Wmissing-declarations > -KBUILD_EXTRA_WARNINGS += -Wmissing-format-attribute > -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wmissing-include-dirs,) > -KBUILD_EXTRA_WARNINGS += -Wmissing-prototypes > -KBUILD_EXTRA_WARNINGS += -Wnested-externs > -KBUILD_EXTRA_WARNINGS += -Wold-style-definition > -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Woverlength-strings,) > -KBUILD_EXTRA_WARNINGS += -Wpacked > -KBUILD_EXTRA_WARNINGS += -Wpacked-bitfield-compat > -KBUILD_EXTRA_WARNINGS += -Wpadded > -KBUILD_EXTRA_WARNINGS += -Wpointer-arith > -KBUILD_EXTRA_WARNINGS += -Wredundant-decls > -KBUILD_EXTRA_WARNINGS += -Wshadow > -KBUILD_EXTRA_WARNINGS += -Wswitch-default > -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wvla,) > -KBUILD_CFLAGS += $(KBUILD_EXTRA_WARNINGS) > +warning-1 := -Wextra > +warning-1 += -Wunused -Wno-unused-parameter > +warning-2 += -Waggregate-return > +warning-2 += -Wbad-function-cast > +warning-2 += -Wcast-qual > +warning-2 += -Wcast-align > +warning-3 += -Wconversion > +warning-2 += -Wdisabled-optimization > +warning-2 += -Wlogical-op > +warning-1 += -Wmissing-declarations > +warning-1 += -Wmissing-format-attribute > +warning-1 += $(call cc-option, -Wmissing-include-dirs,) > +warning-1 += -Wmissing-prototypes > +warning-1 += -Wnested-externs > +warning-2 += -Wold-style-definition > +warning-2 += $(call cc-option, -Woverlength-strings,) > +warning-3 += -Wpacked > +warning-3 += -Wpacked-bitfield-compat > +warning-3 += -Wpadded > +warning-3 += -Wpointer-arith > +warning-2 += -Wredundant-decls > +warning-2 += -Wshadow > +warning-3 += -Wswitch-default > +warning-3 += $(call cc-option, -Wvla,) > +ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),1) > + KBUILD_CFLAGS += $(warning-1) > +else > + ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),2) > + KBUILD_CFLAGS += $(warning-1) $(warning-2) > + else > + ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),3) > + KBUILD_CFLAGS += $(warning-1) $(warning-2) $(warning-3) > + else > + $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown) > + endif > + endif > +endif > endif > > include scripts/Makefile.lib > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ Just started looking at the patch. I tested it and -Wnested-externs is causing a *lot* of noise, because of the nested extern ‘_NSIG_WORDS_is_unsupported_size’ in include/linux/signal.h, which gets included a lot. So, unless that changes, I think that it shouldn't be enabled for W=1.
On 22/04/2011 12:58 ??, Joe Perches wrote: > On Thu, 2011-04-21 at 23:39 +0200, Sam Ravnborg wrote: >> Building a kernel with "make W=1" produce far too much noise >> to be usefull. >> Divide the warning options in three groups: >> W=1 - usefull warning options >> W=2 - noisy but semi usefull warnign options >> W=3 - almost pure noise options >> +warning-1 := -Wextra > [] >> +warning-2 += -Waggregate-return > This is a different form for this first warning-2 declaration > than the first warning-1 declaration. > > Maybe this should be := instead? yeap...for the first warning-3 assignment too... maybe something like this: warning-1:= warning-2:= warning-3:= right after 'ifeq' could help, so that you can alter/test the warnings' levels more easily(without having to change between := and += for the first warning of each warning level).
> Just started looking at the patch. > I tested it and -Wnested-externs is causing a *lot* of noise, because of > the nested extern ‘_NSIG_WORDS_is_unsupported_size’ in > include/linux/signal.h, which gets included a lot. > So, unless that changes, I think that it shouldn't be enabled for W=1. It is just the same issue that pops up for each file. It is easy to fix. The ones that does not belong in W=1 is those that are triggered by many different places in the kernel. If they are triggered in a few .h files they look noisy, but it is easy to fix. Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 21, 2011 at 11:39:03PM +0200, Sam Ravnborg wrote: > Building a kernel with "make W=1" produce far too much noise > to be usefull. > > Divide the warning options in three groups: > > W=1 - usefull warning options > W=2 - noisy but semi usefull warnign options > W=3 - almost pure noise options > > I do not feel strongly about the distinction between > group 2 and 3. But we should consider what we add in group 1. > > > Sample run on my box (x86, 32bit allyesconfig build) > CC kernel/bounds.s > kernel/bounds.c:14: warning: no previous prototype for ‘foo’ > GEN include/generated/bounds.h > CC arch/x86/kernel/asm-offsets.s > In file included from include/linux/sched.h:73, > from arch/x86/kernel/asm-offsets.c:9: > include/linux/signal.h: In function ‘sigorsets’: > include/linux/signal.h:121: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > include/linux/signal.h: In function ‘sigandsets’: > include/linux/signal.h:124: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > include/linux/signal.h: In function ‘signandsets’: > include/linux/signal.h:127: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > include/linux/signal.h: In function ‘signotset’: > include/linux/signal.h:151: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ > arch/x86/kernel/asm-offsets.c: At top level: > arch/x86/kernel/asm-offsets.c:30: warning: no previous prototype for ‘common’ > > the patch is only an RFC - and is not made on top > of an upstream kernel with no additiona stuff applied. Thanks for doing this, a couple of suggestions below. > > Sam > > diff --git a/Makefile b/Makefile > index b967b96..f0e138b 100644 > --- a/Makefile > +++ b/Makefile > @@ -103,7 +103,7 @@ ifeq ("$(origin O)", "command line") > endif > > ifeq ("$(origin W)", "command line") > - export KBUILD_ENABLE_EXTRA_GCC_CHECKS := 1 > + export KBUILD_ENABLE_EXTRA_GCC_CHECKS := $(W) > endif > > # That's our default target when none is given on the command line > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index d5f925a..b65f820 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -56,31 +56,43 @@ endif > # $(call cc-option... ) handles gcc -W.. options which > # are not supported by all versions of the compiler > ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS > -KBUILD_EXTRA_WARNINGS := -Wextra > -KBUILD_EXTRA_WARNINGS += -Wunused -Wno-unused-parameter > -KBUILD_EXTRA_WARNINGS += -Waggregate-return > -KBUILD_EXTRA_WARNINGS += -Wbad-function-cast > -KBUILD_EXTRA_WARNINGS += -Wcast-qual > -KBUILD_EXTRA_WARNINGS += -Wcast-align > -KBUILD_EXTRA_WARNINGS += -Wconversion > -KBUILD_EXTRA_WARNINGS += -Wdisabled-optimization > -KBUILD_EXTRA_WARNINGS += -Wlogical-op > -KBUILD_EXTRA_WARNINGS += -Wmissing-declarations > -KBUILD_EXTRA_WARNINGS += -Wmissing-format-attribute > -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wmissing-include-dirs,) > -KBUILD_EXTRA_WARNINGS += -Wmissing-prototypes > -KBUILD_EXTRA_WARNINGS += -Wnested-externs > -KBUILD_EXTRA_WARNINGS += -Wold-style-definition > -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Woverlength-strings,) > -KBUILD_EXTRA_WARNINGS += -Wpacked > -KBUILD_EXTRA_WARNINGS += -Wpacked-bitfield-compat > -KBUILD_EXTRA_WARNINGS += -Wpadded > -KBUILD_EXTRA_WARNINGS += -Wpointer-arith > -KBUILD_EXTRA_WARNINGS += -Wredundant-decls > -KBUILD_EXTRA_WARNINGS += -Wshadow > -KBUILD_EXTRA_WARNINGS += -Wswitch-default > -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wvla,) > -KBUILD_CFLAGS += $(KBUILD_EXTRA_WARNINGS) > +warning-1 := -Wextra > +warning-1 += -Wunused -Wno-unused-parameter Ok, I went and enabled each switch separately to look at output and recheck whether each one makes sense. The following is all IMHO: The first two W=1 warnings should be merged into one to denote that we're actually tweaking -Wextra behavior (otherwise there's too much output from unused parameters in headers): warning-1 := -Wextra -Wunused -Wno-unused-parameter > +warning-2 += -Waggregate-return warning-2 := -Waggregate-return since this is a first-time assignment to warning-2 otherwise I get $ make W=2 drivers/edac/amd64_edac.o CHK include/linux/version.h CHK include/generated/utsrelease.h UPD include/generated/utsrelease.h scripts/Makefile.build:87: *** Recursive variable `KBUILD_CFLAGS' references itself (eventually). Stop. make: *** [prepare0] Error 2 > +warning-2 += -Wbad-function-cast I'd move this one to W=3 since it is almost useless for kernel code. mm is one good example where we cast unsigned longs to the pagetable types back and forth. > +warning-2 += -Wcast-qual maybe also W=3 due to too noisy with *get_user* calls in headers. > +warning-2 += -Wcast-align this one doesn't trigger anything on x86, maybe on other arches? > +warning-3 += -Wconversion also first assignment so warning-3 := -Wconversion > +warning-2 += -Wdisabled-optimization this one doesn't trigger anything here either > +warning-2 += -Wlogical-op > +warning-1 += -Wmissing-declarations > +warning-1 += -Wmissing-format-attribute > +warning-1 += $(call cc-option, -Wmissing-include-dirs,) > +warning-1 += -Wmissing-prototypes this one is in the top-level Makefile in HOSTCFLAGS so no need for it here. > +warning-1 += -Wnested-externs I don't think this one makes too much sense since we use the trick it warns for to artificially cause a build error. Maybe downgrade it to W=2 or W=3. > +warning-2 += -Wold-style-definition this one actually finds bugs :), maybe W=1. > +warning-2 += $(call cc-option, -Woverlength-strings,) triggers in tracepoints declarations: include/trace/events/kmem.h:11: warning: string length ‘1643’ is greater than the length ‘509’ ISO C90 compilers are required to support I'm not sure we want it at all since the warning message is useless. > +warning-3 += -Wpacked > +warning-3 += -Wpacked-bitfield-compat gcc docs says this one is enabled by default: http://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/Warning-Options.html > +warning-3 += -Wpadded > +warning-3 += -Wpointer-arith > +warning-2 += -Wredundant-decls this might help clean up a bunch of headers > +warning-2 += -Wshadow > +warning-3 += -Wswitch-default > +warning-3 += $(call cc-option, -Wvla,) oh yeah, maybe sort the warning-{1,2,3} assignments alphanumerically :) > +ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),1) > + KBUILD_CFLAGS += $(warning-1) > +else > + ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),2) > + KBUILD_CFLAGS += $(warning-1) $(warning-2) > + else > + ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),3) > + KBUILD_CFLAGS += $(warning-1) $(warning-2) $(warning-3) > + else > + $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown) Also, I think it'll make more sense to enable a warning level exclusively instead of inclusively, so that W=1 gives you only the most important and sane warnings, W=2 the more noisy ones which are a disjunct set from the W=1 ones etc. Thanks.
Hi Borislav. > > the patch is only an RFC - and is not made on top > > of an upstream kernel with no additiona stuff applied. > > Thanks for doing this, a couple of suggestions below. ... You already spend more time analysing this than it took me to write the patch. Can I persuade you to implement the various W= levels based on your analysis and send to Michal for merging? You can use my v2 PATCH as inspiration. Please take authorship on the patch as the main work is to decide what warnings belongs to which level. [Snipped - lots of good comments] Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sam, On Fri, Apr 22, 2011 at 12:15:24PM +0200, Sam Ravnborg wrote: > Hi Borislav. > > > > the patch is only an RFC - and is not made on top > > > of an upstream kernel with no additiona stuff applied. > > > > Thanks for doing this, a couple of suggestions below. > ... > > You already spend more time analysing this than it took me > to write the patch. > Can I persuade you to implement the various W= levels based > on your analysis and send to Michal for merging? > You can use my v2 PATCH as inspiration. > > Please take authorship on the patch as the main work > is to decide what warnings belongs to which level. > > [Snipped - lots of good comments] thanks but according to Documentation/SubmittingPatches, the _initial_ author is the author of the patch. Besides, I just had the talk with Ingo the other day on how to acknowledge multuple authors so I'm not making the same mistake again :). But seriously, let me add my comments to your patch while they're fresh in my head and let's see what we come up with in our combined and perfectly orchestrated effort :). Thanks.
diff --git a/Makefile b/Makefile index b967b96..f0e138b 100644 --- a/Makefile +++ b/Makefile @@ -103,7 +103,7 @@ ifeq ("$(origin O)", "command line") endif ifeq ("$(origin W)", "command line") - export KBUILD_ENABLE_EXTRA_GCC_CHECKS := 1 + export KBUILD_ENABLE_EXTRA_GCC_CHECKS := $(W) endif # That's our default target when none is given on the command line diff --git a/scripts/Makefile.build b/scripts/Makefile.build index d5f925a..b65f820 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -56,31 +56,43 @@ endif # $(call cc-option... ) handles gcc -W.. options which # are not supported by all versions of the compiler ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS -KBUILD_EXTRA_WARNINGS := -Wextra -KBUILD_EXTRA_WARNINGS += -Wunused -Wno-unused-parameter -KBUILD_EXTRA_WARNINGS += -Waggregate-return -KBUILD_EXTRA_WARNINGS += -Wbad-function-cast -KBUILD_EXTRA_WARNINGS += -Wcast-qual -KBUILD_EXTRA_WARNINGS += -Wcast-align -KBUILD_EXTRA_WARNINGS += -Wconversion -KBUILD_EXTRA_WARNINGS += -Wdisabled-optimization -KBUILD_EXTRA_WARNINGS += -Wlogical-op -KBUILD_EXTRA_WARNINGS += -Wmissing-declarations -KBUILD_EXTRA_WARNINGS += -Wmissing-format-attribute -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wmissing-include-dirs,) -KBUILD_EXTRA_WARNINGS += -Wmissing-prototypes -KBUILD_EXTRA_WARNINGS += -Wnested-externs -KBUILD_EXTRA_WARNINGS += -Wold-style-definition -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Woverlength-strings,) -KBUILD_EXTRA_WARNINGS += -Wpacked -KBUILD_EXTRA_WARNINGS += -Wpacked-bitfield-compat -KBUILD_EXTRA_WARNINGS += -Wpadded -KBUILD_EXTRA_WARNINGS += -Wpointer-arith -KBUILD_EXTRA_WARNINGS += -Wredundant-decls -KBUILD_EXTRA_WARNINGS += -Wshadow -KBUILD_EXTRA_WARNINGS += -Wswitch-default -KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wvla,) -KBUILD_CFLAGS += $(KBUILD_EXTRA_WARNINGS) +warning-1 := -Wextra +warning-1 += -Wunused -Wno-unused-parameter +warning-2 += -Waggregate-return +warning-2 += -Wbad-function-cast +warning-2 += -Wcast-qual +warning-2 += -Wcast-align +warning-3 += -Wconversion +warning-2 += -Wdisabled-optimization +warning-2 += -Wlogical-op +warning-1 += -Wmissing-declarations +warning-1 += -Wmissing-format-attribute +warning-1 += $(call cc-option, -Wmissing-include-dirs,) +warning-1 += -Wmissing-prototypes +warning-1 += -Wnested-externs +warning-2 += -Wold-style-definition +warning-2 += $(call cc-option, -Woverlength-strings,) +warning-3 += -Wpacked +warning-3 += -Wpacked-bitfield-compat +warning-3 += -Wpadded +warning-3 += -Wpointer-arith +warning-2 += -Wredundant-decls +warning-2 += -Wshadow +warning-3 += -Wswitch-default +warning-3 += $(call cc-option, -Wvla,) +ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),1) + KBUILD_CFLAGS += $(warning-1) +else + ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),2) + KBUILD_CFLAGS += $(warning-1) $(warning-2) + else + ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),3) + KBUILD_CFLAGS += $(warning-1) $(warning-2) $(warning-3) + else + $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown) + endif + endif +endif endif include scripts/Makefile.lib
Building a kernel with "make W=1" produce far too much noise to be usefull. Divide the warning options in three groups: W=1 - usefull warning options W=2 - noisy but semi usefull warnign options W=3 - almost pure noise options I do not feel strongly about the distinction between group 2 and 3. But we should consider what we add in group 1. Sample run on my box (x86, 32bit allyesconfig build) CC kernel/bounds.s kernel/bounds.c:14: warning: no previous prototype for ‘foo’ GEN include/generated/bounds.h CC arch/x86/kernel/asm-offsets.s In file included from include/linux/sched.h:73, from arch/x86/kernel/asm-offsets.c:9: include/linux/signal.h: In function ‘sigorsets’: include/linux/signal.h:121: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ include/linux/signal.h: In function ‘sigandsets’: include/linux/signal.h:124: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ include/linux/signal.h: In function ‘signandsets’: include/linux/signal.h:127: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ include/linux/signal.h: In function ‘signotset’: include/linux/signal.h:151: warning: nested extern declaration of ‘_NSIG_WORDS_is_unsupported_size’ arch/x86/kernel/asm-offsets.c: At top level: arch/x86/kernel/asm-offsets.c:30: warning: no previous prototype for ‘common’ the patch is only an RFC - and is not made on top of an upstream kernel with no additiona stuff applied. Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html