[PATCH -v4] kbuild: Add extra gcc checks

[PATCH -v4] kbuild: Add extra gcc checks

[Borislav Petkov]

Add a 'W=1' Makefile switch which adds additional checking per build
object.

The idea behind this option is targeted at developers who, in the
process of writing their code, want to do the occasional

make W=1 [target.o]

and let gcc do more extensive code checking for them. Then, they
could eyeball the output for valid gcc warnings about various
bugs/discrepancies which are not reported during the normal build
process.

For more background information and a use case, read through this
thread: http://marc.info/?l=kernel-janitors&m=129802065918147&w=2

-v4: Enable this for gcc only. Also, check for options which are not
supported by every gcc version we use to build the kernel. Adding those
checks for _every_ option slows down the build noticeably so be sensible
here and add it only when your gcc version chokes on a particular
option.

-v[2..3]: move to Makefile.build

Cc: Michal Marek <mmarek@suse.cz>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: linux-kbuild@vger.kernel.org
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Borislav Petkov <bp@alien8.de>
---
 Makefile               |    6 ++++++
 scripts/Makefile.build |   37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index c9c8c8f..c3bca9c 100644
--- a/Makefile
+++ b/Makefile
@@ -102,6 +102,11 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)
 endif
 
+ifeq ("$(origin W)", "command line")
+  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
+  export KBUILD_ENABLE_EXTRA_GCC_CHECKS
+endif
+
 # That's our default target when none is given on the command line
 PHONY := _all
 _all:
@@ -1262,6 +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  ''
 	@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 4eb99ab..fa4b40d 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -49,6 +49,42 @@ ifeq ($(KBUILD_NOPEDANTIC),)
                 $(error CFLAGS was changed in "$(kbuild-file)". Fix it to use EXTRA_CFLAGS)
         endif
 endif
+
+#
+# make W=1 settings
+#
+# $(call cc-option... ) handles gcc -W.. options which
+# are not supported by all versions of the compiler
+ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
+ifneq ($(call cc-version),)
+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)
+endif
+endif
+
 include scripts/Makefile.lib
 
 ifdef host-progs
@@ -403,7 +439,6 @@ ifneq ($(cmd_files),)
   include $(cmd_files)
 endif
 
-
 # Declare the contents of the .PHONY variable as phony.  We keep that
 # information in a variable se we can use it in if_changed and friends.
 
-- 
1.7.4.1.48.g5673d

-- 
Regards/Gruss,
    Boris.

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Borislav Petkov]

On Mon, Feb 21, 2011 at 12:03:22PM +0100, Borislav Petkov wrote:
> 
> Add a 'W=1' Makefile switch which adds additional checking per build
> object.
> 
> The idea behind this option is targeted at developers who, in the
> process of writing their code, want to do the occasional
> 
> make W=1 [target.o]
> 
> and let gcc do more extensive code checking for them. Then, they
> could eyeball the output for valid gcc warnings about various
> bugs/discrepancies which are not reported during the normal build
> process.
> 
> For more background information and a use case, read through this
> thread: http://marc.info/?l=kernel-janitors&m=129802065918147&w=2
> 
> -v4: Enable this for gcc only. Also, check for options which are not
> supported by every gcc version we use to build the kernel. Adding those
> checks for _every_ option slows down the build noticeably so be sensible
> here and add it only when your gcc version chokes on a particular
> option.
> 
> -v[2..3]: move to Makefile.build
> 
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: linux-kbuild@vger.kernel.org
> Acked-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Borislav Petkov <bp@alien8.de>

Ping?

Michal, Sam,

any news here? FWIW, I've been build-testing amd64_edac.c with it and
have caught a bunch of buglets with it. IMHO, it really helps to enable
more extensive gcc checking in debugging builds.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Sam Ravnborg]

On Mon, Feb 21, 2011 at 12:03:22PM +0100, Borislav Petkov wrote:
> 
> Add a 'W=1' Makefile switch which adds additional checking per build
> object.
> 
> The idea behind this option is targeted at developers who, in the
> process of writing their code, want to do the occasional
> 
> make W=1 [target.o]
> 
> and let gcc do more extensive code checking for them. Then, they
> could eyeball the output for valid gcc warnings about various
> bugs/discrepancies which are not reported during the normal build
> process.
> 
> For more background information and a use case, read through this
> thread: http://marc.info/?l=kernel-janitors&m=129802065918147&w=2
> 
> -v4: Enable this for gcc only. Also, check for options which are not
> supported by every gcc version we use to build the kernel. Adding those
> checks for _every_ option slows down the build noticeably so be sensible
> here and add it only when your gcc version chokes on a particular
> option.
> 
> -v[2..3]: move to Makefile.build
> 
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: linux-kbuild@vger.kernel.org
> Acked-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Borislav Petkov <bp@alien8.de>
> ---
>  Makefile               |    6 ++++++
>  scripts/Makefile.build |   37 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index c9c8c8f..c3bca9c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -102,6 +102,11 @@ ifeq ("$(origin O)", "command line")
>    KBUILD_OUTPUT := $(O)
>  endif
>  
> +ifeq ("$(origin W)", "command line")
> +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
> +  export KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +endif

It is simpler to just write:
export KBUILD_ENABLE_EXTRA_GCC_CHECKS := 1


> +#
> +# make W=1 settings
> +#
> +# $(call cc-option... ) handles gcc -W.. options which
> +# are not supported by all versions of the compiler
> +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS

> +ifneq ($(call cc-version),)
This check looks redundant.

You introduce a new environment variable "KBUILD_ENABLE_EXTRA_GCC_CHECKS".
We actually ty to document these in Documentation/kbuild/kbuild.txt

Fix the above and you can add my:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Arnd Bergmann]

On Monday 21 February 2011 12:03:22 Borislav Petkov wrote:
> Add a 'W=1' Makefile switch which adds additional checking per build
> object.
>
> The idea behind this option is targeted at developers who, in the
> process of writing their code, want to do the occasional
>
> make W=1 [target.o]

Great stuff, I really like the idea!

> +# $(call cc-option... ) handles gcc -W.. options which
> +# are not supported by all versions of the compiler
> +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +ifneq ($(call cc-version),)
> +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)
> +endif
> +endif

I would be a little more selective here. Maybe we can have two levels
W=1 and W=2, with the full set getting enabled by W=2, and the smaller
set getting enabled in W=1. The reason is that many of the warnings
are pointless or even hurting code quality, while others (e.g.
-Wmissing-declarations) are generally useful and the only reason for
not enabling them is that they cause too many warnings with existing
code.

	Arnd

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Borislav Petkov]

On Mon, Feb 28, 2011 at 10:07:33PM +0100, Arnd Bergmann wrote:
> On Monday 21 February 2011 12:03:22 Borislav Petkov wrote:
> > Add a 'W=1' Makefile switch which adds additional checking per build
> > object.
> >
> > The idea behind this option is targeted at developers who, in the
> > process of writing their code, want to do the occasional
> >
> > make W=1 [target.o]
> 
> Great stuff, I really like the idea!

Thanks. :)

> 
> > +# $(call cc-option... ) handles gcc -W.. options which
> > +# are not supported by all versions of the compiler
> > +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> > +ifneq ($(call cc-version),)
> > +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)
> > +endif
> > +endif
> 
> I would be a little more selective here. Maybe we can have two levels
> W=1 and W=2, with the full set getting enabled by W=2, and the smaller
> set getting enabled in W=1. The reason is that many of the warnings
> are pointless or even hurting code quality, while others (e.g.
> -Wmissing-declarations) are generally useful and the only reason for
> not enabling them is that they cause too many warnings with existing
> code.

My intention was not to have multiple levels of warnings because then
you have to go and enable the different levels and have to remember
which level you used last, etc, etc.

Instead I am thinking along with the following lines:

make W=1 [path/to/kernel/file.o] 2>w.log

and then take a look at w.log and start fixing warnings.

You can selectively ignore some of the warnings since, as you say
yourself above, some simply make you write ugly code like enforcing
casts just for the sake of shutting up the compiler. A great deal of the
warnings come from includes which are hard to fix or gcc is issuing the
warning wrong since we do sick sh*t with C in the kernel and that's OK
:).

But in all cases you have all the warnings in one single file and that's
it. If a certain -W option is useless, we should rather remove it since
it doesn't help anyway. The selection above is clearly not complete so
I'd rather drop some instead of including different W=x levels.

Hmm... ?

-- 
Regards/Gruss,
    Boris.

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Arnd Bergmann]

On Monday 28 February 2011, Borislav Petkov wrote:
> My intention was not to have multiple levels of warnings because then
> you have to go and enable the different levels and have to remember
> which level you used last, etc, etc.

I wasn't suggesting more than two, so the two would have very distinct
definitions:

W=1: Warnings that we would like to fix all over the tree, patches to
     remove these are always welcome and you can build the entire kernel
     with it. Once they are all fixed, we can make the warnings the default.

W=2: Warnings that we know we don't always want to fix, meant for what
     you describe here -- you build a single file and decide what to
     do based on common sense. 

> Instead I am thinking along with the following lines:
> 
> make W=1 [path/to/kernel/file.o] 2>w.log
> 
> and then take a look at w.log and start fixing warnings.
> 
> You can selectively ignore some of the warnings since, as you say
> yourself above, some simply make you write ugly code like enforcing
> casts just for the sake of shutting up the compiler. A great deal of the
> warnings come from includes which are hard to fix or gcc is issuing the
> warning wrong since we do sick sh*t with C in the kernel and that's OK
> :).

Yes, I understand that.

> But in all cases you have all the warnings in one single file and that's
> it. If a certain -W option is useless, we should rather remove it since
> it doesn't help anyway. The selection above is clearly not complete so
> I'd rather drop some instead of including different W=x levels.
> 
> Hmm... ?

I'd have to see the output myself. My feeling is that a lot of the
-Wextra warnings are not that useful, but I don't know which ones
are included in -Wextra these days.

	Arnd

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Américo Wang]

On Tue, Mar 01, 2011 at 12:35:00PM +0100, Arnd Bergmann wrote:
>On Monday 28 February 2011, Borislav Petkov wrote:
>> My intention was not to have multiple levels of warnings because then
>> you have to go and enable the different levels and have to remember
>> which level you used last, etc, etc.
>
>I wasn't suggesting more than two, so the two would have very distinct
>definitions:
>
>W=1: Warnings that we would like to fix all over the tree, patches to
>     remove these are always welcome and you can build the entire kernel
>     with it. Once they are all fixed, we can make the warnings the default.
>
>W=2: Warnings that we know we don't always want to fix, meant for what
>     you describe here -- you build a single file and decide what to
>     do based on common sense. 
>

Right, this makes sense. Borislav, could you implement this?

Thanks.

Re: [PATCH -v4] kbuild: Add extra gcc checks

[Borislav Petkov]

On Tue, Mar 01, 2011 at 09:20:50PM +0800, Américo Wang wrote:
> On Tue, Mar 01, 2011 at 12:35:00PM +0100, Arnd Bergmann wrote:
> >On Monday 28 February 2011, Borislav Petkov wrote:
> >> My intention was not to have multiple levels of warnings because then
> >> you have to go and enable the different levels and have to remember
> >> which level you used last, etc, etc.
> >
> >I wasn't suggesting more than two, so the two would have very distinct
> >definitions:
> >
> >W=1: Warnings that we would like to fix all over the tree, patches to
> >     remove these are always welcome and you can build the entire kernel
> >     with it. Once they are all fixed, we can make the warnings the default.
> >
> >W=2: Warnings that we know we don't always want to fix, meant for what
> >     you describe here -- you build a single file and decide what to
> >     do based on common sense. 
> >
> 
> Right, this makes sense. Borislav, could you implement this?

Yeah, I could try to come up with a sensible choice for mutual-exclusive
sets of -W.. options. Any preferences?

-- 
Regards/Gruss,
Boris.