LinuxLists.cc - [RFC PATCH] kbuild: implement several W= levels

[permalink] [raw]

Subject: Re: [RFC PATCH] kbuild: implement several W= levels

> 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

2011-04-22 07:38:07

[permalink] [raw]

Subject: [RFC PATCH v2] kbuild: implement several W= levels

>From 7f1edfc13eadc1cf8c80d72ff850a121bf472e00 Mon Sep 17 00:00:00 2001
From: Sam Ravnborg <[email protected]>
Date: Fri, 22 Apr 2011 08:22:25 +0200
Subject: [PATCH] kbuild: implement several W= levels

Building a kernel with "make W=1" produce far too much noise
to be usefull.

Divide the warning options in three groups:

W=1 - warnings that may be relevant and does not occur too often
W=2 - warnings that occur quite often but may still be relevant
W=3 - the more obscure warnings, can most likely be ignored

When building init/ on my box the levels produces:

W=1 - 46 warnings
W=2 - 863 warnings
W=3 - 6496 warnings

Many warnings occur from .h files so fixing one file may have a nice
effect on the total number of warnings.

With these changes I am actually tempted to try W=1 now and then.
Previously there were just too much noise.

Signed-off-by: Sam Ravnborg <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Jones <[email protected]>
---
This is based on top of -linus to make it easy to test.
But I included fixes for older gcc as posted in a separate patch.

Changes since v1:
- updated help text
- updated grouping, less warnings in W=1
- reimplment selection of warnings for the individual levels
- added a comment describing the individual levels

Sam

Makefile | 4 +-
scripts/Makefile.build | 69 +++++++++++++++++++++++++++++------------------
2 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index b967b96..37a6062 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
@@ -1267,7 +1267,7 @@ help:
@echo ' make O=dir [targets] Locate all output files in "dir", including .config'
@echo ' make C=1 [targets] Check all c source with $$CHECK (sparse by default)'
@echo ' make C=2 [targets] Force check of all c source with $$CHECK'
- @echo ' make W=1 [targets] Enable extra gcc checks'
+ @echo ' make W=n [targets] Enable extra gcc checks, n=1,2,3'
@echo ''
@echo 'Execute "make" or "make all" to build all targets marked with [*] '
@echo 'For further info see the ./README file'
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index d5f925a..6874c3b 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -51,36 +51,51 @@ ifeq ($(KBUILD_NOPEDANTIC),)
endif

#
-# make W=1 settings
+# make W=... settings
#
-# $(call cc-option... ) handles gcc -W.. options which
+# W=1 - warnings that may be relevant and does not occur too often
+# W=2 - warnings that occur quite often but may still be relevant
+# W=3 - the more obscure warnings, can most likely be ignored
+#
+# $(call cc-option, -W...) 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-1 += -Wmissing-declarations
+warning-1 += -Wmissing-format-attribute
+warning-1 += -Wmissing-prototypes
+warning-1 += -Wnested-externs
+warning-1 += $(call cc-option, -Wmissing-include-dirs)
+
+warning-2 := $(warning-1)
+warning-2 += -Waggregate-return
+warning-2 += -Wbad-function-cast
+warning-2 += -Wcast-qual
+warning-2 += -Wcast-align
+warning-2 += -Wdisabled-optimization
+warning-2 += -Wold-style-definition
+warning-2 += -Wshadow
+warning-2 += $(call cc-option, -Wlogical-op)
+warning-2 += $(call cc-option, -Woverlength-strings)
+
+warning-3 := $(warning-2)
+warning-3 += -Wconversion
+warning-3 += -Wpacked
+warning-3 += -Wpadded
+warning-3 += -Wpointer-arith
+warning-3 += -Wredundant-decls
+warning-3 += -Wswitch-default
+warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
+warning-3 += $(call cc-option, -Wvla)
+
+warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
+
+ifeq ("$(warning)","")
+ $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
+endif
+
+KBUILD_CFLAGS += $(warning)
endif

include scripts/Makefile.lib
--
1.6.0.6

2011-04-22 08:19:39

[permalink] [raw]

Subject: Re: [RFC PATCH] kbuild: implement several W= levels

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.

--
Regards/Gruss,
Boris.

2011-04-22 10:15:27

[permalink] [raw]

Subject: Re: [RFC PATCH] kbuild: implement several W= levels

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

2011-04-22 10:20:33

[permalink] [raw]

Subject: Re: [RFC PATCH] kbuild: implement several W= levels

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.

--
Regards/Gruss,
Boris.

2011-04-22 10:46:53

[permalink] [raw]

Subject: [PATCH v3] kbuild: implement several W= levels

On Fri, Apr 22, 2011 at 12:20:28PM +0200, Borislav Petkov wrote:
> 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 :).

Ok, this is starting to look like another kernelnewbies.org task:

make W=x 2>>w.log

and stare at compiler output in the other xterm:

tail -f w.log

and the bugs are just waiting there to be fixed! :)

Below is v3, please take a look.

--
From: Sam Ravnborg <[email protected]>
Date: Fri, 22 Apr 2011 08:22:25 +0200
Subject: [PATCH] kbuild: implement several W= levels

Building a kernel with "make W=1" produce far too much noise
to be usefull.

Divide the warning options in three groups:

W=1 - warnings that may be relevant and does not occur too often
W=2 - warnings that occur quite often but may still be relevant
W=3 - the more obscure warnings, can most likely be ignored

When building init/ on my box the levels produces:

W=1 - 46 warnings
W=2 - 863 warnings
W=3 - 6496 warnings

Many warnings occur from .h files so fixing one file may have a nice
effect on the total number of warnings.

With these changes I am actually tempted to try W=1 now and then.
Previously there were just too much noise.

Borislav:

- make the W= levels exclusive
- drop -Wmissing-prototypes since it is being added to the toplevel Makefile
- move very noisy and making little sense for the kernel warnings to W=3
- drop -Woverlength-strings due to useless warning message
- copy explanatory text for the different warning levels to 'make help'

Signed-off-by: Sam Ravnborg <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Dave Jones <[email protected]>
---
Makefile | 8 ++++-
scripts/Makefile.build | 64 +++++++++++++++++++++++++++--------------------
2 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index b967b96..17ce5d5 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
@@ -1267,7 +1267,11 @@ help:
@echo ' make O=dir [targets] Locate all output files in "dir", including .config'
@echo ' make C=1 [targets] Check all c source with $$CHECK (sparse by default)'
@echo ' make C=2 [targets] Force check of all c source with $$CHECK'
- @echo ' make W=1 [targets] Enable extra gcc checks'
+ @echo ' make W=n [targets] Enable extra gcc checks, n=1,2,3 where'
+ @echo ' 1: warnings which may be relevant and do not occur too often'
+ @echo ' 2: warnings which occur quite often but may still be relevant'
+ @echo ' 3: more obscure warnings, can most likely be ignored'
+
@echo ''
@echo 'Execute "make" or "make all" to build all targets marked with [*] '
@echo 'For further info see the ./README file'
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index d5f925a..3c6e7ed 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -51,36 +51,46 @@ ifeq ($(KBUILD_NOPEDANTIC),)
endif

#
-# make W=1 settings
+# make W=... settings
#
-# $(call cc-option... ) handles gcc -W.. options which
+# W=1 - warnings that may be relevant and does not occur too often
+# W=2 - warnings that occur quite often but may still be relevant
+# W=3 - the more obscure warnings, can most likely be ignored
+#
+# $(call cc-option, -W...) 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 -Wunused -Wno-unused-parameter
+warning-1 += -Wmissing-declarations
+warning-1 += -Wmissing-format-attribute
+warning-1 += -Wold-style-definition
+warning-1 += $(call cc-option, -Wmissing-include-dirs)
+
+warning-2 := -Waggregate-return
+warning-2 += -Wcast-align
+warning-2 += -Wdisabled-optimization
+warning-2 += -Wnested-externs
+warning-2 += -Wshadow
+warning-2 += $(call cc-option, -Wlogical-op)
+
+warning-3 := -Wbad-function-cast
+warning-3 += -Wcast-qual
+warning-3 += -Wconversion
+warning-3 += -Wpacked
+warning-3 += -Wpadded
+warning-3 += -Wpointer-arith
+warning-3 += -Wredundant-decls
+warning-3 += -Wswitch-default
+warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
+warning-3 += $(call cc-option, -Wvla)
+
+warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
+
+ifeq ("$(warning)","")
+ $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
+endif
+
+KBUILD_CFLAGS += $(warning)
endif

include scripts/Makefile.lib
--
1.7.5.rc1.16.g9db1

--
Regards/Gruss,
Boris.

2011-04-22 11:09:52

[permalink] [raw]

Subject: Re: [PATCH v3] kbuild: implement several W= levels

> Borislav:
>
> - drop -Wmissing-prototypes since it is being added to the toplevel Makefile

This is actually bogus..
We only add this to HOSTCFLAGS - which is used to build programs
that run on your build host. These options do not propagate to gcc
used to build the kernel modules.

The rest looks good.

Sam

2011-04-22 17:50:50

[permalink] [raw]

Subject: [PATCH v3.1] kbuild: implement several W= levels

From: Sam Ravnborg <[email protected]>

Building a kernel with "make W=1" produce far too much noise
to be usefull.

Divide the warning options in three groups:

W=1 - warnings that may be relevant and does not occur too often
W=2 - warnings that occur quite often but may still be relevant
W=3 - the more obscure warnings, can most likely be ignored

When building init/ on my box the levels produces:

W=1 - 46 warnings
W=2 - 863 warnings
W=3 - 6496 warnings

Many warnings occur from .h files so fixing one file may have a nice
effect on the total number of warnings.

With these changes I am actually tempted to try W=1 now and then.
Previously there were just too much noise.

Borislav:

- make the W= levels exclusive
- move very noisy and making little sense for the kernel warnings to W=3
- drop -Woverlength-strings due to useless warning message
- copy explanatory text for the different warning levels to 'make help'

Signed-off-by: Sam Ravnborg <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Dave Jones <[email protected]>
---
Makefile | 8 ++++-
scripts/Makefile.build | 65 ++++++++++++++++++++++++++++--------------------
2 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index b967b96..17ce5d5 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
@@ -1267,7 +1267,11 @@ help:
@echo ' make O=dir [targets] Locate all output files in "dir", including .config'
@echo ' make C=1 [targets] Check all c source with $$CHECK (sparse by default)'
@echo ' make C=2 [targets] Force check of all c source with $$CHECK'
- @echo ' make W=1 [targets] Enable extra gcc checks'
+ @echo ' make W=n [targets] Enable extra gcc checks, n=1,2,3 where'
+ @echo ' 1: warnings which may be relevant and do not occur too often'
+ @echo ' 2: warnings which occur quite often but may still be relevant'
+ @echo ' 3: more obscure warnings, can most likely be ignored'
+
@echo ''
@echo 'Execute "make" or "make all" to build all targets marked with [*] '
@echo 'For further info see the ./README file'
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index d5f925a..ffb383c 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -51,36 +51,47 @@ ifeq ($(KBUILD_NOPEDANTIC),)
endif

#
-# make W=1 settings
+# make W=... settings
#
-# $(call cc-option... ) handles gcc -W.. options which
+# W=1 - warnings that may be relevant and does not occur too often
+# W=2 - warnings that occur quite often but may still be relevant
+# W=3 - the more obscure warnings, can most likely be ignored
+#
+# $(call cc-option, -W...) 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 -Wunused -Wno-unused-parameter
+warning-1 += -Wmissing-declarations
+warning-1 += -Wmissing-format-attribute
+warning-1 += -Wmissing-prototypes
+warning-1 += -Wold-style-definition
+warning-1 += $(call cc-option, -Wmissing-include-dirs)
+
+warning-2 := -Waggregate-return
+warning-2 += -Wcast-align
+warning-2 += -Wdisabled-optimization
+warning-2 += -Wnested-externs
+warning-2 += -Wshadow
+warning-2 += $(call cc-option, -Wlogical-op)
+
+warning-3 := -Wbad-function-cast
+warning-3 += -Wcast-qual
+warning-3 += -Wconversion
+warning-3 += -Wpacked
+warning-3 += -Wpadded
+warning-3 += -Wpointer-arith
+warning-3 += -Wredundant-decls
+warning-3 += -Wswitch-default
+warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
+warning-3 += $(call cc-option, -Wvla)
+
+warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
+
+ifeq ("$(warning)","")
+ $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
+endif
+
+KBUILD_CFLAGS += $(warning)
endif

include scripts/Makefile.lib
--
1.7.5.rc1.16.g9db1

2011-04-26 19:52:43

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On 22.4.2011 19:50, Borislav Petkov wrote:
> From: Sam Ravnborg <[email protected]>
>
> Building a kernel with "make W=1" produce far too much noise
> to be usefull.
>
> Divide the warning options in three groups:
>
> W=1 - warnings that may be relevant and does not occur too often
> W=2 - warnings that occur quite often but may still be relevant
> W=3 - the more obscure warnings, can most likely be ignored
>
> When building init/ on my box the levels produces:
>
> W=1 - 46 warnings
> W=2 - 863 warnings
> W=3 - 6496 warnings

I guess these numbers are not valid after your changes? Not that the
exact numbers are important, but maybe the distribution change?
Otherwise the patch looks good, thanks a lot to both of you!

Michal
>
> Many warnings occur from .h files so fixing one file may have a nice
> effect on the total number of warnings.
>
> With these changes I am actually tempted to try W=1 now and then.
> Previously there were just too much noise.
>
> Borislav:
>
> - make the W= levels exclusive
> - move very noisy and making little sense for the kernel warnings to W=3
> - drop -Woverlength-strings due to useless warning message
> - copy explanatory text for the different warning levels to 'make help'
>
> Signed-off-by: Sam Ravnborg <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Cc: Dave Jones <[email protected]>
> ---
> Makefile | 8 ++++-
> scripts/Makefile.build | 65 ++++++++++++++++++++++++++++--------------------
> 2 files changed, 44 insertions(+), 29 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index b967b96..17ce5d5 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
> @@ -1267,7 +1267,11 @@ help:
> @echo ' make O=dir [targets] Locate all output files in "dir", including .config'
> @echo ' make C=1 [targets] Check all c source with $$CHECK (sparse by default)'
> @echo ' make C=2 [targets] Force check of all c source with $$CHECK'
> - @echo ' make W=1 [targets] Enable extra gcc checks'
> + @echo ' make W=n [targets] Enable extra gcc checks, n=1,2,3 where'
> + @echo ' 1: warnings which may be relevant and do not occur too often'
> + @echo ' 2: warnings which occur quite often but may still be relevant'
> + @echo ' 3: more obscure warnings, can most likely be ignored'
> +
> @echo ''
> @echo 'Execute "make" or "make all" to build all targets marked with [*] '
> @echo 'For further info see the ./README file'
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index d5f925a..ffb383c 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -51,36 +51,47 @@ ifeq ($(KBUILD_NOPEDANTIC),)
> endif
>
> #
> -# make W=1 settings
> +# make W=... settings
> #
> -# $(call cc-option... ) handles gcc -W.. options which
> +# W=1 - warnings that may be relevant and does not occur too often
> +# W=2 - warnings that occur quite often but may still be relevant
> +# W=3 - the more obscure warnings, can most likely be ignored
> +#
> +# $(call cc-option, -W...) 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 -Wunused -Wno-unused-parameter
> +warning-1 += -Wmissing-declarations
> +warning-1 += -Wmissing-format-attribute
> +warning-1 += -Wmissing-prototypes
> +warning-1 += -Wold-style-definition
> +warning-1 += $(call cc-option, -Wmissing-include-dirs)
> +
> +warning-2 := -Waggregate-return
> +warning-2 += -Wcast-align
> +warning-2 += -Wdisabled-optimization
> +warning-2 += -Wnested-externs
> +warning-2 += -Wshadow
> +warning-2 += $(call cc-option, -Wlogical-op)
> +
> +warning-3 := -Wbad-function-cast
> +warning-3 += -Wcast-qual
> +warning-3 += -Wconversion
> +warning-3 += -Wpacked
> +warning-3 += -Wpadded
> +warning-3 += -Wpointer-arith
> +warning-3 += -Wredundant-decls
> +warning-3 += -Wswitch-default
> +warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
> +warning-3 += $(call cc-option, -Wvla)
> +
> +warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
> +
> +ifeq ("$(warning)","")
> + $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
> +endif
> +
> +KBUILD_CFLAGS += $(warning)
> endif
>
> include scripts/Makefile.lib

2011-04-26 20:43:15

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On Tue, Apr 26, 2011 at 09:52:35PM +0200, Michal Marek wrote:
> On 22.4.2011 19:50, Borislav Petkov wrote:
> > From: Sam Ravnborg <[email protected]>
> >
> > Building a kernel with "make W=1" produce far too much noise
> > to be usefull.
> >
> > Divide the warning options in three groups:
> >
> > W=1 - warnings that may be relevant and does not occur too often
> > W=2 - warnings that occur quite often but may still be relevant
> > W=3 - the more obscure warnings, can most likely be ignored
> >
> > When building init/ on my box the levels produces:
> >
> > W=1 - 46 warnings
> > W=2 - 863 warnings
> > W=3 - 6496 warnings
>
> I guess these numbers are not valid after your changes? Not that the
> exact numbers are important, but maybe the distribution change?

I think so too that those numbers don't mean a lot. Instead, this
feature makes more sense IMHO if you use it on a single file:

make W=1 <file.c> 2>before.log

<make your changes>

make W=1 <file.c> 2>after.log

diff -uprN before.log after.log

and you let the compiler tell you which warnings you've introduced. Then
you do the same game with W=2 and W=3.

Nice, huh. :)

--
Regards/Gruss,
Boris.

2011-04-27 08:22:31

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On Tue, Apr 26, 2011 at 10:43:07PM +0200, Borislav Petkov wrote:
> On Tue, Apr 26, 2011 at 09:52:35PM +0200, Michal Marek wrote:
> > On 22.4.2011 19:50, Borislav Petkov wrote:
> > > From: Sam Ravnborg <[email protected]>
> > >
> > > Building a kernel with "make W=1" produce far too much noise
> > > to be usefull.
> > >
> > > Divide the warning options in three groups:
> > >
> > > W=1 - warnings that may be relevant and does not occur too often
> > > W=2 - warnings that occur quite often but may still be relevant
> > > W=3 - the more obscure warnings, can most likely be ignored
> > >
> > > When building init/ on my box the levels produces:
> > >
> > > W=1 - 46 warnings
> > > W=2 - 863 warnings
> > > W=3 - 6496 warnings
> >
> > I guess these numbers are not valid after your changes? Not that the
> > exact numbers are important, but maybe the distribution change?
>
> I think so too that those numbers don't mean a lot.

The numbers _only_ tell you that the amoun of warnings on W=1
is down to a manageable number and they increase be the higher
level.

Will you cook up a more correct changelog and resubmit?

Sam

2011-04-27 08:25:58

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On Fri, Apr 22, 2011 at 07:50:42PM +0200, Borislav Petkov wrote:
> From: Sam Ravnborg <[email protected]>
>
> Building a kernel with "make W=1" produce far too much noise
> to be usefull.
>
> Divide the warning options in three groups:
>
> W=1 - warnings that may be relevant and does not occur too often
> W=2 - warnings that occur quite often but may still be relevant
> W=3 - the more obscure warnings, can most likely be ignored
>
> When building init/ on my box the levels produces:
>
> W=1 - 46 warnings
> W=2 - 863 warnings
> W=3 - 6496 warnings
>
> Many warnings occur from .h files so fixing one file may have a nice
> effect on the total number of warnings.
>
> With these changes I am actually tempted to try W=1 now and then.
> Previously there were just too much noise.
>
> Borislav:
>
> - make the W= levels exclusive
I do not see the point in this really. This is not what most people would
expect.
When you ask for more you get more - not something else.

We see it with verbose levels where -vv give more output than -v etc.

Anyway - the important thing is to keep the relevant warnings at W=1 level.
Which is independendt of this change.
So consider the input and decide - I do not want to make a fuzz about it.

Sam

2011-04-27 08:28:05

by Geert Uytterhoeven

[permalink] [raw]

Subject: Re: [PATCH v3] kbuild: implement several W= levels

On Fri, Apr 22, 2011 at 12:46, Borislav Petkov <[email protected]> wrote:
> On Fri, Apr 22, 2011 at 12:20:28PM +0200, Borislav Petkov wrote:
>> 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 :).
>
> Ok, this is starting to look like another kernelnewbies.org task:
>
> make W=x 2>>w.log
>
> and stare at compiler output in the other xterm:
>
> tail -f w.log
>
> and the bugs are just waiting there to be fixed! :)

You can use http://www.kernel.org/pub/linux/kernel/people/geert/linux-log/linux-log-summary.pl
to get a summary, sorted by number of occurrences. So you see which
ones to fix first ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2011-04-27 11:35:08

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On Wed, Apr 27, 2011 at 10:25:55AM +0200, Sam Ravnborg wrote:
> > - make the W= levels exclusive
> I do not see the point in this really. This is not what most people would
> expect.
> When you ask for more you get more - not something else.
>
> We see it with verbose levels where -vv give more output than -v etc.
>
> Anyway - the important thing is to keep the relevant warnings at W=1 level.
> Which is independendt of this change.
> So consider the input and decide - I do not want to make a fuzz about it.

I know, -vv.. increases verbosity is probably part of old unix tradition
or common sense but it doesn't make much sense in this case, IMHO. When
I use this, I want to see what the most relevant warnings are, maybe
have a crack at them to fix them, and _then_ look at the less important
ones (for an arbitrary definition of important warnings).

If we do this inclusive, then W=2 dumps the, let's call it, level 1
_plus_ the new level 2 warnings, polluting the output with something
I've already seen, but only partially. And then I start to think, did
I see this one already, didn't I, which was it? By the time you enable
W=3, the output becomes pretty useless. For example, W=3 generates 190+
MB logfile here only with level 3 warnings. Now imagine all 3 levels
combined.

Dividing the output by level of importance doesn't have this problem and
is much more workable, IMHO.

But this is just my use case, it could be that I'm completely alone on
this one. I'd love to hear what other people think.

FWIW, we might even make this behavior configurable by having

make W=1o

meaning level 1 warnings only or whatever sick idea we come up with
eventually.

;-)

Thanks.

--
Regards/Gruss,
Boris.

2011-04-27 20:15:35

[permalink] [raw]

Subject: [PATCH v3.2] kbuild: implement several W= levels

From: Sam Ravnborg <[email protected]>

Building a kernel with "make W=1" produces far too much noise to be
useful.

Divide the warning options in three groups:

W=1 - warnings that may be relevant and does not occur too often
W=2 - warnings that occur quite often but may still be relevant
W=3 - the more obscure warnings, can most likely be ignored

When building the whole kernel, those levels produce:

W=1 - 4859 warnings
W=2 - 1394 warnings
W=3 - 86666 warnings

respectively. Warnings have been counted with Geert's script at

http://www.kernel.org/pub/linux/kernel/people/geert/linux-log/linux-log-summary.pl

Many warnings occur from .h files so fixing one file may have a nice
effect on the total number of warnings.

With these changes I am actually tempted to try W=1 now and then.
Previously there was just too much noise.

Borislav:

- make the W= levels exclusive
- move very noisy and making little sense for the kernel warnings to W=3
- drop -Woverlength-strings due to useless warning message
- copy explanatory text for the different warning levels to 'make help'
- recount warnings per level

Signed-off-by: Sam Ravnborg <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Cc: Dave Jones <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
---
Makefile | 8 ++++-
scripts/Makefile.build | 65 ++++++++++++++++++++++++++++--------------------
2 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index 5a7a2e4..b2a6808 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
@@ -1267,7 +1267,11 @@ help:
@echo ' make O=dir [targets] Locate all output files in "dir", including .config'
@echo ' make C=1 [targets] Check all c source with $$CHECK (sparse by default)'
@echo ' make C=2 [targets] Force check of all c source with $$CHECK'
- @echo ' make W=1 [targets] Enable extra gcc checks'
+ @echo ' make W=n [targets] Enable extra gcc checks, n=1,2,3 where'
+ @echo ' 1: warnings which may be relevant and do not occur too often'
+ @echo ' 2: warnings which occur quite often but may still be relevant'
+ @echo ' 3: more obscure warnings, can most likely be ignored'
+
@echo ''
@echo 'Execute "make" or "make all" to build all targets marked with [*] '
@echo 'For further info see the ./README file'
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index d5f925a..ffb383c 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -51,36 +51,47 @@ ifeq ($(KBUILD_NOPEDANTIC),)
endif

#
-# make W=1 settings
+# make W=... settings
#
-# $(call cc-option... ) handles gcc -W.. options which
+# W=1 - warnings that may be relevant and does not occur too often
+# W=2 - warnings that occur quite often but may still be relevant
+# W=3 - the more obscure warnings, can most likely be ignored
+#
+# $(call cc-option, -W...) 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 -Wunused -Wno-unused-parameter
+warning-1 += -Wmissing-declarations
+warning-1 += -Wmissing-format-attribute
+warning-1 += -Wmissing-prototypes
+warning-1 += -Wold-style-definition
+warning-1 += $(call cc-option, -Wmissing-include-dirs)
+
+warning-2 := -Waggregate-return
+warning-2 += -Wcast-align
+warning-2 += -Wdisabled-optimization
+warning-2 += -Wnested-externs
+warning-2 += -Wshadow
+warning-2 += $(call cc-option, -Wlogical-op)
+
+warning-3 := -Wbad-function-cast
+warning-3 += -Wcast-qual
+warning-3 += -Wconversion
+warning-3 += -Wpacked
+warning-3 += -Wpadded
+warning-3 += -Wpointer-arith
+warning-3 += -Wredundant-decls
+warning-3 += -Wswitch-default
+warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
+warning-3 += $(call cc-option, -Wvla)
+
+warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
+
+ifeq ("$(warning)","")
+ $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
+endif
+
+KBUILD_CFLAGS += $(warning)
endif

include scripts/Makefile.lib
--
1.7.5.rc1.16.g9db1

2011-04-27 20:21:05

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v3.2] kbuild: implement several W= levels

On Wed, 2011-04-27 at 22:15 +0200, Borislav Petkov wrote:
> From: Sam Ravnborg <[email protected]>
[]
> +warning-1 += -Wold-style-definition
> +warning-1 += $(call cc-option, -Wmissing-include-dirs)

Only thing I would suggest is a comment describing why
some entries use $(call cc-option, -Wfoo) and others don't.

Something akin to:

# Use call cc-option when the minimum supported gcc version does not
# support a specific option but a later gcc version does.

2011-04-27 20:46:12

[permalink] [raw]

Subject: Re: [PATCH v3.2] kbuild: implement several W= levels

On Wed, Apr 27, 2011 at 01:21:00PM -0700, Joe Perches wrote:
> On Wed, 2011-04-27 at 22:15 +0200, Borislav Petkov wrote:
> > From: Sam Ravnborg <[email protected]>
> []
> > +warning-1 += -Wold-style-definition
> > +warning-1 += $(call cc-option, -Wmissing-include-dirs)
>
> Only thing I would suggest is a comment describing why
> some entries use $(call cc-option, -Wfoo) and others don't.
>
> Something akin to:
>
> # Use call cc-option when the minimum supported gcc version does not
> # support a specific option but a later gcc version does.

>From the patch:
+# $(call cc-option, -W...) handles gcc -W.. options which
# are not supported by all versions of the compiler

So it is already included.

Sam

2011-04-27 20:46:45

[permalink] [raw]

Subject: Re: [PATCH v3.2] kbuild: implement several W= levels

On Wed, Apr 27, 2011 at 10:15:27PM +0200, Borislav Petkov wrote:
> From: Sam Ravnborg <[email protected]>
>
> Building a kernel with "make W=1" produces far too much noise to be
> useful.
>
> Divide the warning options in three groups:
>
> W=1 - warnings that may be relevant and does not occur too often
> W=2 - warnings that occur quite often but may still be relevant
> W=3 - the more obscure warnings, can most likely be ignored
>
> When building the whole kernel, those levels produce:
>
> W=1 - 4859 warnings
> W=2 - 1394 warnings
> W=3 - 86666 warnings
>
> respectively. Warnings have been counted with Geert's script at
>
> http://www.kernel.org/pub/linux/kernel/people/geert/linux-log/linux-log-summary.pl
>
> Many warnings occur from .h files so fixing one file may have a nice
> effect on the total number of warnings.
>
> With these changes I am actually tempted to try W=1 now and then.
> Previously there was just too much noise.
>
> Borislav:
>
> - make the W= levels exclusive
> - move very noisy and making little sense for the kernel warnings to W=3
> - drop -Woverlength-strings due to useless warning message
> - copy explanatory text for the different warning levels to 'make help'
> - recount warnings per level
>
> Signed-off-by: Sam Ravnborg <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Cc: Dave Jones <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> ---

Looks good. Michal - please apply.

Sam

2011-04-28 00:26:11

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On Wed, 27 Apr 2011 13:35:13 +0200, Borislav Petkov said:

> If we do this inclusive, then W=2 dumps the, let's call it, level 1
> _plus_ the new level 2 warnings, polluting the output with something
> I've already seen, but only partially. And then I start to think, did
> I see this one already, didn't I, which was it? By the time you enable
> W=3, the output becomes pretty useless. For example, W=3 generates 190+
> MB logfile here only with level 3 warnings. Now imagine all 3 levels
> combined.

If each level is averaging 10x the previous level, then all 3 levels will only be 11%
bigger, or 211MB.

You *really* want to get *all* the warnings - quite often, you'll be looking
at a set of 15 or 20 level-3 warnings. And if you had the Level-2's in there as
well, you'd immediately realize that the single level-2 was the real root-cause
of all the cascating warnings.

Also, having it as a "volume control" means I don't have to go reading the
Makefile to figure out which number I need to specify for which message - I can
just say W=3 and *know* the flavor I want will be in there somewhere. Let's
face it, if there's over 10 or 15 warnings involved, grep or your editor's
"locate next" will do a better job of finding it than you ever can...

Attachments:

(No filename) (227.00 B)

2011-04-28 16:18:15

[permalink] [raw]

Subject: Re: [PATCH v3.2] kbuild: implement several W= levels

2011-04-28 16:24:53

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On 28.4.2011 02:25, [email protected] wrote:
> On Wed, 27 Apr 2011 13:35:13 +0200, Borislav Petkov said:
>
>> If we do this inclusive, then W=2 dumps the, let's call it, level 1
>> _plus_ the new level 2 warnings, polluting the output with something
>> I've already seen, but only partially. And then I start to think, did
>> I see this one already, didn't I, which was it? By the time you enable
>> W=3, the output becomes pretty useless. For example, W=3 generates 190+
>> MB logfile here only with level 3 warnings. Now imagine all 3 levels
>> combined.
>
> If each level is averaging 10x the previous level, then all 3 levels will only be 11%
> bigger, or 211MB.
>
> You *really* want to get *all* the warnings - quite often, you'll be looking
> at a set of 15 or 20 level-3 warnings. And if you had the Level-2's in there as
> well, you'd immediately realize that the single level-2 was the real root-cause
> of all the cascating warnings.

How about W=12 for level 1 and 2 warnings and W=123 for all levels?

Michal

2011-04-28 18:00:04

[permalink] [raw]

Subject: Re: [PATCH v3.1] kbuild: implement several W= levels

On Thu, 28 Apr 2011 18:24:46 +0200, Michal Marek said:
> On 28.4.2011 02:25, [email protected] wrote:
> > On Wed, 27 Apr 2011 13:35:13 +0200, Borislav Petkov said:
> >
> >> If we do this inclusive, then W=2 dumps the, let's call it, level 1
> >> _plus_ the new level 2 warnings, polluting the output with something
> >> I've already seen, but only partially. And then I start to think, did
> >> I see this one already, didn't I, which was it? By the time you enable
> >> W=3, the output becomes pretty useless. For example, W=3 generates 190+
> >> MB logfile here only with level 3 warnings. Now imagine all 3 levels
> >> combined.
> >
> > If each level is averaging 10x the previous level, then all 3 levels will only be 11%
> > bigger, or 211MB.
> >
> > You *really* want to get *all* the warnings - quite often, you'll be looking
> > at a set of 15 or 20 level-3 warnings. And if you had the Level-2's in there as
> > well, you'd immediately realize that the single level-2 was the real root-cause
> > of all the cascating warnings.
>
> How about W=12 for level 1 and 2 warnings and W=123 for all levels?

I'd be OK with that, or a 'W=all', or whatever, as long as it's doable.

Attachments:

(No filename) (227.00 B)

2011-04-29 13:31:54

[permalink] [raw]

Subject: [PATCH] kbuild: Allow to combine multiple W= levels

Add support for make W=12, make W=123 and so on, to enable warnings from
multiple W= levels. Normally, make W=<level> does not include warnings
from the previous level.

Signed-off-by: Michal Marek <[email protected]>
---
scripts/Makefile.build | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 9c0c481..28cef2a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -60,6 +60,8 @@ endif
# $(call cc-option, -W...) handles gcc -W.. options which
# are not supported by all versions of the compiler
ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
+warning- := $(empty)
+
warning-1 := -Wextra -Wunused -Wno-unused-parameter
warning-1 += -Wmissing-declarations
warning-1 += -Wmissing-format-attribute
@@ -85,9 +87,11 @@ warning-3 += -Wswitch-default
warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
warning-3 += $(call cc-option, -Wvla)

-warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
+warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
+warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
+warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))

-ifeq ("$(warning)","")
+ifeq ("$(strip $(warning))","")
$(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
endif

--
1.7.4.1

2011-04-29 17:44:03

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On Fri, Apr 29, 2011 at 03:31:33PM +0200, Michal Marek wrote:
> Add support for make W=12, make W=123 and so on, to enable warnings from
> multiple W= levels. Normally, make W=<level> does not include warnings
> from the previous level.

Acked-by: Sam Ravnborg <[email protected]>

I was considering W=all - but that gives less flexibility, which is
what has been requested.

Sam

2011-04-29 18:13:13

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

Hi,

On Fri, Apr 29, 2011 at 9:31 AM, Michal Marek <[email protected]> wrote:
> Add support for make W=12, make W=123 and so on, to enable warnings from
> multiple W= levels. Normally, make W=<level> does not include warnings
> from the previous level.
>
> Signed-off-by: Michal Marek <[email protected]>
> ---
> ?scripts/Makefile.build | ? ?8 ++++++--
> ?1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 9c0c481..28cef2a 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -60,6 +60,8 @@ endif
> ?# $(call cc-option, -W...) handles gcc -W.. options which
> ?# are not supported by all versions of the compiler
> ?ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +warning- ?:= $(empty)
> +
> ?warning-1 := -Wextra -Wunused -Wno-unused-parameter
> ?warning-1 += -Wmissing-declarations
> ?warning-1 += -Wmissing-format-attribute
> @@ -85,9 +87,11 @@ warning-3 += -Wswitch-default
> ?warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
> ?warning-3 += $(call cc-option, -Wvla)
>
> -warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
> +warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
> +warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
> +warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>
I do not really like that, it would mean that W=123, W=321, W=231 and
W=132 would lead to the same result. What about a comma separated
string, ala:

LEVELS=1,2,3,4

comma:= ,
empty:=
space:= $(empty) $(empty)

warning-1:= a
warning-2:= b
warning-3:= c
warning-4:= d

all:
echo $(foreach level, $(subst ${comma},${space},${LEVELS},
${warning}), ${warning-${level}})

It has the advantage to scale up without adding new code.

- Arnaud

> -ifeq ("$(warning)","")
> +ifeq ("$(strip $(warning))","")
> ? ? ? ? $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
> ?endif
>
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

2011-04-29 18:28:47

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On Fri, 29 Apr 2011 14:13:11 EDT, Arnaud Lacombe said:

> I do not really like that, it would mean that W=123, W=321, W=231 and
> W=132 would lead to the same result. What about a comma separated
> string, ala:

OK, I'll bite - if W=321 should behaved differently than 123, exactly *how*
should it be different?

Attachments:

(No filename) (227.00 B)

2011-04-29 18:30:07

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On Fri, 29 Apr 2011 15:31:33 +0200, Michal Marek said:
> Add support for make W=12, make W=123 and so on, to enable warnings from
> multiple W= levels. Normally, make W=<level> does not include warnings
> from the previous level.
>
> Signed-off-by: Michal Marek <[email protected]>
> ---

OK, that addresses my concerns, feel free to stick a
Reviewed-By: Valdis Kletnieks <[email protected]>
on it...

Attachments:

(No filename) (227.00 B)

2011-05-02 15:38:18

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On Fri, Apr 29, 2011 at 03:31:33PM +0200, Michal Marek wrote:
> Add support for make W=12, make W=123 and so on, to enable warnings from
> multiple W= levels. Normally, make W=<level> does not include warnings
> from the previous level.
>
> Signed-off-by: Michal Marek <[email protected]>
> ---
> scripts/Makefile.build | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 9c0c481..28cef2a 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -60,6 +60,8 @@ endif
> # $(call cc-option, -W...) handles gcc -W.. options which
> # are not supported by all versions of the compiler
> ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +warning- := $(empty)
> +
> warning-1 := -Wextra -Wunused -Wno-unused-parameter
> warning-1 += -Wmissing-declarations
> warning-1 += -Wmissing-format-attribute
> @@ -85,9 +87,11 @@ warning-3 += -Wswitch-default
> warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
> warning-3 += $(call cc-option, -Wvla)
>
> -warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
> +warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
> +warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
> +warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>
> -ifeq ("$(warning)","")
> +ifeq ("$(strip $(warning))","")
> $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
> endif

Pushed to kbuild-2.6.git#kbuild with the following make help update:

diff --git a/Makefile b/Makefile
index 4527dc2..d342502 100644
--- a/Makefile
+++ b/Makefile
@@ -1290,7 +1290,7 @@ help:
@echo ' 1: warnings which may be relevant and do not occur too often'
@echo ' 2: warnings which occur quite often but may still be relevant'
@echo ' 3: more obscure warnings, can most likely be ignored'
-
+ @echo ' Multiple levels can be combined with W=12 or W=123'
@echo ''
@echo 'Execute "make" or "make all" to build all targets marked with [*] '
@echo 'For further info see the ./README file'

Michal

2011-05-02 15:53:59

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

Hi Michal,

On Mon, May 2, 2011 at 11:38 AM, Michal Marek <[email protected]> wrote:
> On Fri, Apr 29, 2011 at 03:31:33PM +0200, Michal Marek wrote:
>> Add support for make W=12, make W=123 and so on, to enable warnings from
>> multiple W= levels. Normally, make W=<level> does not include warnings
>> from the previous level.
>>
>> Signed-off-by: Michal Marek <[email protected]>
>> ---
>> ?scripts/Makefile.build | ? ?8 ++++++--
>> ?1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 9c0c481..28cef2a 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -60,6 +60,8 @@ endif
>> ?# $(call cc-option, -W...) handles gcc -W.. options which
>> ?# are not supported by all versions of the compiler
>> ?ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
>> +warning- ?:= $(empty)
>> +
>> ?warning-1 := -Wextra -Wunused -Wno-unused-parameter
>> ?warning-1 += -Wmissing-declarations
>> ?warning-1 += -Wmissing-format-attribute
>> @@ -85,9 +87,11 @@ warning-3 += -Wswitch-default
>> ?warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
>> ?warning-3 += $(call cc-option, -Wvla)
>>
>> -warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
>> +warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>> +warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>> +warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>
>> -ifeq ("$(warning)","")
>> +ifeq ("$(strip $(warning))","")
>> ? ? ? ? ?$(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
>> ?endif
>
> Pushed to kbuild-2.6.git#kbuild with the following make help update:
>
you did not comment on the point I raised.

Thanks,
- Arnaud

> diff --git a/Makefile b/Makefile
> index 4527dc2..d342502 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1290,7 +1290,7 @@ help:
> ? ? ? ?@echo ?' ? ? ? ? ? ? ? ?1: warnings which may be relevant and do not occur too often'
> ? ? ? ?@echo ?' ? ? ? ? ? ? ? ?2: warnings which occur quite often but may still be relevant'
> ? ? ? ?@echo ?' ? ? ? ? ? ? ? ?3: more obscure warnings, can most likely be ignored'
> -
> + ? ? ? @echo ?' ? ? ? ? ? ? ? ?Multiple levels can be combined with W=12 or W=123'
> ? ? ? ?@echo ?''
> ? ? ? ?@echo ?'Execute "make" or "make all" to build all targets marked with [*] '
> ? ? ? ?@echo ?'For further info see the ./README file'
>
> Michal
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

2011-05-02 16:05:24

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On 2.5.2011 17:53, Arnaud Lacombe wrote:
> Hi Michal,
>
> On Mon, May 2, 2011 at 11:38 AM, Michal Marek<[email protected]> wrote:
>> On Fri, Apr 29, 2011 at 03:31:33PM +0200, Michal Marek wrote:
>>> Add support for make W=12, make W=123 and so on, to enable warnings from
>>> multiple W= levels. Normally, make W=<level> does not include warnings
>>> from the previous level.
>>>
>>> Signed-off-by: Michal Marek<[email protected]>
>>> ---
>>> scripts/Makefile.build | 8 ++++++--
>>> 1 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>>> index 9c0c481..28cef2a 100644
>>> --- a/scripts/Makefile.build
>>> +++ b/scripts/Makefile.build
>>> @@ -60,6 +60,8 @@ endif
>>> # $(call cc-option, -W...) handles gcc -W.. options which
>>> # are not supported by all versions of the compiler
>>> ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
>>> +warning- := $(empty)
>>> +
>>> warning-1 := -Wextra -Wunused -Wno-unused-parameter
>>> warning-1 += -Wmissing-declarations
>>> warning-1 += -Wmissing-format-attribute
>>> @@ -85,9 +87,11 @@ warning-3 += -Wswitch-default
>>> warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
>>> warning-3 += $(call cc-option, -Wvla)
>>>
>>> -warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
>>> +warning := $(warning-$(findstring 1, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>> +warning += $(warning-$(findstring 2, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>> +warning += $(warning-$(findstring 3, $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>>
>>> -ifeq ("$(warning)","")
>>> +ifeq ("$(strip $(warning))","")
>>> $(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
>>> endif
>>
>> Pushed to kbuild-2.6.git#kbuild with the following make help update:
>>
> you did not comment on the point I raised.

Valdis did. Why is it a problem that W=123 can be interchanged with W=321?

Michal

2011-05-02 16:16:29

by Cong Wang

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On Fri, Apr 29, 2011 at 9:31 PM, Michal Marek <[email protected]> wrote:
> Add support for make W=12, make W=123 and so on, to enable warnings from
> multiple W= levels. Normally, make W=<level> does not include warnings
> from the previous level.
>

This interface is not friendly, at least not as normal as we often see.
W=x+1 is supposed to include warnings of W=x.

Please refine the interface.

Thanks.

2011-05-02 16:17:56

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

Hi,

On Mon, May 2, 2011 at 12:05 PM, Michal Marek <[email protected]> wrote:
> On 2.5.2011 17:53, Arnaud Lacombe wrote:
>>
>> Hi Michal,
>>
>> On Mon, May 2, 2011 at 11:38 AM, Michal Marek<[email protected]> ?wrote:
>>>
>>> On Fri, Apr 29, 2011 at 03:31:33PM +0200, Michal Marek wrote:
>>>>
>>>> Add support for make W=12, make W=123 and so on, to enable warnings from
>>>> multiple W= levels. Normally, make W=<level> ?does not include warnings
>>>> from the previous level.
>>>>
>>>> Signed-off-by: Michal Marek<[email protected]>
>>>> ---
>>>> ?scripts/Makefile.build | ? ?8 ++++++--
>>>> ?1 files changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>>>> index 9c0c481..28cef2a 100644
>>>> --- a/scripts/Makefile.build
>>>> +++ b/scripts/Makefile.build
>>>> @@ -60,6 +60,8 @@ endif
>>>> ?# $(call cc-option, -W...) handles gcc -W.. options which
>>>> ?# are not supported by all versions of the compiler
>>>> ?ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
>>>> +warning- ?:= $(empty)
>>>> +
>>>> ?warning-1 := -Wextra -Wunused -Wno-unused-parameter
>>>> ?warning-1 += -Wmissing-declarations
>>>> ?warning-1 += -Wmissing-format-attribute
>>>> @@ -85,9 +87,11 @@ warning-3 += -Wswitch-default
>>>> ?warning-3 += $(call cc-option, -Wpacked-bitfield-compat)
>>>> ?warning-3 += $(call cc-option, -Wvla)
>>>>
>>>> -warning := $(warning-$(KBUILD_ENABLE_EXTRA_GCC_CHECKS))
>>>> +warning := $(warning-$(findstring 1,
>>>> $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>>> +warning += $(warning-$(findstring 2,
>>>> $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>>> +warning += $(warning-$(findstring 3,
>>>> $(KBUILD_ENABLE_EXTRA_GCC_CHECKS)))
>>>>
>>>> -ifeq ("$(warning)","")
>>>> +ifeq ("$(strip $(warning))","")
>>>> ? ? ? ? ?$(error W=$(KBUILD_ENABLE_EXTRA_GCC_CHECKS) is unknown)
>>>> ?endif
>>>
>>> Pushed to kbuild-2.6.git#kbuild with the following make help update:
>>>
>> you did not comment on the point I raised.
>
> Valdis did. Why is it a problem that W=123 can be interchanged with W=321?
>
Because 321 != 123 and 321 > 123. As I am a dumb human being, I would
expect 321 to have a different meaning than 123, but also 321 to be
more verbose than 123. Said otherwise, humans tends to naturally
interpret 321 and 123 as number rather than concatenation of
character.

- Arnaud

> Michal
>

2011-05-02 17:07:40

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

On Tue, May 03, 2011 at 12:16:15AM +0800, Am?rico Wang wrote:
> On Fri, Apr 29, 2011 at 9:31 PM, Michal Marek <[email protected]> wrote:
> > Add support for make W=12, make W=123 and so on, to enable warnings from
> > multiple W= levels. Normally, make W=<level> does not include warnings
> > from the previous level.
> >
>
> This interface is not friendly, at least not as normal as we often see.
> W=x+1 is supposed to include warnings of W=x.
>
> Please refine the interface.

Until we see that several people have had benefit if W=...
we should leave it as is now.

Then when people really start to use if we can refine it.

IMO it is much more important to find the right sub-set
of warnings to keep on W=1 level than how we see additional warnings.

There may well be warnings where we say that the benefit of it is
zero - or it is plain wrong in the kernel.
Lets try to focus on this.

Sam

2011-05-02 17:35:00

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

Hi,

On Mon, May 2, 2011 at 1:07 PM, Sam Ravnborg <[email protected]> wrote:
> On Tue, May 03, 2011 at 12:16:15AM +0800, Am?rico Wang wrote:
>> On Fri, Apr 29, 2011 at 9:31 PM, Michal Marek <[email protected]> wrote:
>> > Add support for make W=12, make W=123 and so on, to enable warnings from
>> > multiple W= levels. Normally, make W=<level> does not include warnings
>> > from the previous level.
>> >
>>
>> This interface is not friendly, at least not as normal as we often see.
>> W=x+1 is supposed to include warnings of W=x.
>>
>> Please refine the interface.
>
> Until we see that several people have had benefit if W=...
> we should leave it as is now.
>
> Then when people really start to use if we can refine it.
>
> IMO it is much more important to find the right sub-set
> of warnings to keep on W=1 level than how we see additional warnings.
>
> There may well be warnings where we say that the benefit of it is
> zero - or it is plain wrong in the kernel.
> Lets try to focus on this.
>
Because you are not even sure of what you submitted is good for the kernel ?

I think this has been done completely wrong, first all the extra
warnings got in (without a detailed impact implied by each one), then
it was decided to be split in several level (which I proposed on Feb
20th, but was originally rejected by Borislav Petkov), then, well,
some may need to be removed because they are not good for Linux. I
would rather have provided the framework first, clean, natural
interface, then added new warnings gradually when we were sure had a
positive impact on the kernel, with the associated patch fixing them.
Right now, +80k warnings will be just impossible to fix.

Now, the drawback of your comment is that when it will be time to
change the interface, some will object because it will have been
started to be used by third party scripts, and as authors of third
party script, it is a PITA to have to check the kernel version to know
if I should W=1,2,3, or W=123 or if W=3 includes W=1 ...

my 0.2c,

- Arnaud

2011-05-02 18:03:13

by boris

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

I'm really getting tired of this!

> Because you are not even sure of what you submitted is good for the kernel
> ?

No, because we need to get a feeling of what makes sense for all these
options first. I'd rather do the learning-by-doing thing here and
implement what makes sense than braindead frameworks.

> I think this has been done completely wrong, first all the extra
> warnings got in (without a detailed impact implied by each one),

WTF? http://marc.info/?l=linux-kbuild&m=130346037604547&w=2

No one else did check those and said, ACK/NACK so we went in with this
initial splitting first.

> then it was decided to be split in several level (which I proposed on
> Feb 20th, but was originally rejected by Borislav Petkov),

Of course, because it made no sense at the time. Sam added it later with
his proposal patch.

> then, well, some may need to be removed because they are not good for
> Linux.

have you even tried all those options and come up with concrete
suggestions on which options should go in and which shouldn't, and for
what reasons? I don't think so. So until you do, I don't care what you
have to say.

[snip a bunch of bullshit]

> Now, the drawback of your comment is that when it will be time to
> change the interface, some will object because it will have been
> started to be used by third party scripts, and as authors of third
> party script, it is a PITA to have to check the kernel version to know
> if I should W=1,2,3, or W=123 or if W=3 includes W=1 ...

This is exactly why we're trying to get a feeling of this by _running_
it and _then_ _bitching_ about it. And this is for DEVELOPERS only - if
they've introduced it in their scripts, then they should be able to fix
any changes very easily.

2011-05-02 18:45:03

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

Hi,

On Mon, May 2, 2011 at 2:03 PM, <[email protected]> wrote:
> This is exactly why we're trying to get a feeling of this by _running_
> it and _then_ _bitching_ about it.
>
This is just what I did, might you consider it bullshit or not. I
really do _dislike_ Michal's interface. I proposed a solution, Americo
did too. I guess that unless a patch is submitted, this will go
nowhere.

> And this is for DEVELOPERS only - if
> they've introduced it in their scripts, then they should be able to fix
> any changes very easily.
This argument is worthless. As soon as it gets in mainline, everybody
will be free to use it, newbs, QA, packager, ...

- Arnaud

2011-05-02 18:52:00

by boris

[permalink] [raw]

Subject: Re: [PATCH] kbuild: Allow to combine multiple W= levels

> I proposed a solution, Americo did too. I guess that unless a patch is
> submitted, this will go nowhere.

Exactly, if you feel something is wrong with this interface and you
think you have a better idea, make a patch, test it and send it out.

This is how solutions are proposed properly on lkml. :)

2011-05-02 20:35:58