[PATCH -v5] kbuild: Add extra gcc checks

[PATCH -v5] 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

-v5: Document internal kbuild variable which is introduced by this.
Simplify assignment.

-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: linux-kbuild@vger.kernel.org
Acked-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Borislav Petkov <bp@alien8.de>
---
 Documentation/kbuild/kbuild.txt |    5 +++++
 Makefile                        |    5 +++++
 scripts/Makefile.build          |   35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/Documentation/kbuild/kbuild.txt b/Documentation/kbuild/kbuild.txt
index 4a99031..376538c 100644
--- a/Documentation/kbuild/kbuild.txt
+++ b/Documentation/kbuild/kbuild.txt
@@ -196,3 +196,8 @@ to be included in the databases, separated by blank space. E.g.:
 To get all available archs you can also specify all. E.g.:
 
     $ make ALLSOURCE_ARCHS=all tags
+
+KBUILD_ENABLE_EXTRA_GCC_CHECKS
+--------------------------------------------------
+If enabled over the make command line with "W=1", it turns on additional
+gcc -W... options for more extensive build-time checking.
diff --git a/Makefile b/Makefile
index c9c8c8f..8c4614f 100644
--- a/Makefile
+++ b/Makefile
@@ -102,6 +102,10 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)
 endif
 
+ifeq ("$(origin W)", "command line")
+  export KBUILD_ENABLE_EXTRA_GCC_CHECKS := 1
+endif
+
 # That's our default target when none is given on the command line
 PHONY := _all
 _all:
@@ -1262,6 +1266,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..d5f925a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -49,6 +49,40 @@ 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
+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
+
 include scripts/Makefile.lib
 
 ifdef host-progs
@@ -403,7 +437,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 -v5] kbuild: Add extra gcc checks

[Michal Marek]

On Tue, Mar 01, 2011 at 09:35:29AM +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
> 
> -v5: Document internal kbuild variable which is introduced by this.
> Simplify assignment.

Thanks, applied to kbuild-2.6.git#packaging. If someone comes up with a
good selection of warnings for W=1 and W=2, we can change it later.

Michal

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

[Borislav Petkov]

On Wed, Mar 09, 2011 at 09:45:04AM -0500, Michal Marek wrote:
> On Tue, Mar 01, 2011 at 09:35:29AM +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
> > 
> > -v5: Document internal kbuild variable which is introduced by this.
> > Simplify assignment.
> 
> Thanks, applied to kbuild-2.6.git#packaging. If someone comes up with a
> good selection of warnings for W=1 and W=2, we can change it later.

Yeah, I was going to do that but am terribly busy at the moment. I'll do
an initial proposal against your kbuild branch when I get a chance.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

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

[Ingo Molnar]

* Michal Marek <mmarek@suse.cz> wrote:

> On Tue, Mar 01, 2011 at 09:35:29AM +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
> > 
> > -v5: Document internal kbuild variable which is introduced by this.
> > Simplify assignment.
> 
> Thanks, applied to kbuild-2.6.git#packaging. If someone comes up with a
> good selection of warnings for W=1 and W=2, we can change it later.

Another, related, very nice kbuild feature would be to allow for arch maintainers to 
mark certain files as "should only build fine without warnings" - i.e. -Werror 
should be the default. There would be a Kconfig feature to opt out of this, 
CONFIG_CC_IGNORE_WARNINGS=y or so. This would allow for people to still build the 
kernel with old (or buggy) versions of GCC.

arch/x86/ would start using this by gradually marking more and more files as -Werror 
by default. Some architectures like arch/sparc/ and arch/powerpc/ already build with 
-Werror enabled - but for arch/x86/ we cannot force this (we cannot break the build 
for who knows how many people) so we want to do it gradually and with a way out for 
users with buggy compilers.

Thanks,

	Ingo

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

[Sam Ravnborg]

> Another, related, very nice kbuild feature would be to allow for arch maintainers to 
> mark certain files as "should only build fine without warnings" - i.e. -Werror 
> should be the default. There would be a Kconfig feature to opt out of this, 
> CONFIG_CC_IGNORE_WARNINGS=y or so. This would allow for people to still build the 
> kernel with old (or buggy) versions of GCC.

To add -Werror for all files conditionally you can do:

    ccflags-$(CONFIG_WERROR) += -Werror

For individual files we can then drop -Werror like this:

    CFLAGS_REMOVE_foobar.o := -Werror

So it should be doable with the existing infrastructure.

	Sam

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

[Ingo Molnar]

* Sam Ravnborg <sam@ravnborg.org> wrote:

> > Another, related, very nice kbuild feature would be to allow for arch maintainers to 
> > mark certain files as "should only build fine without warnings" - i.e. -Werror 
> > should be the default. There would be a Kconfig feature to opt out of this, 
> > CONFIG_CC_IGNORE_WARNINGS=y or so. This would allow for people to still build the 
> > kernel with old (or buggy) versions of GCC.
> 
> To add -Werror for all files conditionally you can do:
> 
>     ccflags-$(CONFIG_WERROR) += -Werror
> 
> For individual files we can then drop -Werror like this:
> 
>     CFLAGS_REMOVE_foobar.o := -Werror
> 
> So it should be doable with the existing infrastructure.

Stupid question: is there an existing kbuild rule that i could use to enable -Werror 
for a single .o file, such as kernel/sched.o - without affecting other files in 
kernel/ that i do not maintain?

Also, adding 3 lines per object file is pretty ugly - would it be possible to create 
a nicer, compact, single-line way to someone condense a CONFIG_ERROR opt-out 
mechanism, the -Werror default and the single-object-file into a single rule?

Something like:

  obj-werror-y += sched.o

Although i'm not sure if it is wise to mix build details into the object build tree 
hierarchy like that ...

Maybe we can live with some multi-line definition after all - until all of the 
kernel is covered by such a mechanism. But the per object file rule would still be 
important, for multi-maintainenace-boundaries directories like kernel/*.o.

Thanks,

	Ingo

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

[Michal Marek]

On 10.3.2011 10:04, Ingo Molnar wrote:
> 
> * Sam Ravnborg <sam@ravnborg.org> wrote:
> 
>>> Another, related, very nice kbuild feature would be to allow for arch maintainers to 
>>> mark certain files as "should only build fine without warnings" - i.e. -Werror 
>>> should be the default. There would be a Kconfig feature to opt out of this, 
>>> CONFIG_CC_IGNORE_WARNINGS=y or so. This would allow for people to still build the 
>>> kernel with old (or buggy) versions of GCC.
>>
>> To add -Werror for all files conditionally you can do:
>>
>>     ccflags-$(CONFIG_WERROR) += -Werror
>>
>> For individual files we can then drop -Werror like this:
>>
>>     CFLAGS_REMOVE_foobar.o := -Werror
>>
>> So it should be doable with the existing infrastructure.
> 
> Stupid question: is there an existing kbuild rule that i could use to enable -Werror 
> for a single .o file, such as kernel/sched.o - without affecting other files in 
> kernel/ that i do not maintain?

CFLAGS_sched.o := -Werror


> Also, adding 3 lines per object file is pretty ugly - would it be possible to create 
> a nicer, compact, single-line way to someone condense a CONFIG_ERROR opt-out 
> mechanism, the -Werror default and the single-object-file into a single rule?
> 
> Something like:
> 
>   obj-werror-y += sched.o
> 
> Although i'm not sure if it is wise to mix build details into the object build tree 
> hierarchy like that ...

One way without extending the current rules could be:

ifdef CONFIG_CC_WERROR
Werror := -Werror
endif

CFLAGS_sched.o := $(Werror)
CFLAGS_another.o := $(Werror)

What do you think?

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

[Borislav Petkov]

On Thu, Mar 10, 2011 at 10:04:44AM +0100, Ingo Molnar wrote:
> 
> * Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > Another, related, very nice kbuild feature would be to allow for arch maintainers to 
> > > mark certain files as "should only build fine without warnings" - i.e. -Werror 
> > > should be the default. There would be a Kconfig feature to opt out of this, 
> > > CONFIG_CC_IGNORE_WARNINGS=y or so. This would allow for people to still build the 
> > > kernel with old (or buggy) versions of GCC.
> > 
> > To add -Werror for all files conditionally you can do:
> > 
> >     ccflags-$(CONFIG_WERROR) += -Werror
> > 
> > For individual files we can then drop -Werror like this:
> > 
> >     CFLAGS_REMOVE_foobar.o := -Werror
> > 
> > So it should be doable with the existing infrastructure.
> 
> Stupid question: is there an existing kbuild rule that i could use to enable -Werror 
> for a single .o file, such as kernel/sched.o - without affecting other files in 
> kernel/ that i do not maintain?
> 
> Also, adding 3 lines per object file is pretty ugly - would it be possible to create 
> a nicer, compact, single-line way to someone condense a CONFIG_ERROR opt-out 
> mechanism, the -Werror default and the single-object-file into a single rule?
> 
> Something like:
> 
>   obj-werror-y += sched.o

I believe you can do

CFLAGS_sched.o = -Werror

or similar but you'll have to try it out. I remember doing a more
involved per-object flags fumbling with the popcnt stuff, i.e. you might
want to take a look at d61931d89be506372d01a90d1755f6d0a9fafe2d where
we pass it an arch-specific config option so you can have per-object,
per-arch build flags :).

HTH.

-- 
Regards/Gruss,
    Boris.

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

[Sam Ravnborg]

> One way without extending the current rules could be:
> 
> ifdef CONFIG_CC_WERROR
> Werror := -Werror
> endif
> 
> CFLAGS_sched.o := $(Werror)
> CFLAGS_another.o := $(Werror)
> 
> What do you think?

We could extend kbuild to support something like this:

    ccflags-<filename>-y := -Werror

Sample:

    ccflags-sched.o-$(CONFIG_WERROR) := -Werror

The above is a nice counterpart to the existing ccflags-y.
And we could then deprecate CLFAGS_<filename> as we have today.

To remove options we could use:

ccflags-<filename>-remove-y := -pg

This is a replacement of the undocumented CFLAGS_REMOVE_<filename> variant.

Today:
    ifdef CONFIG_FUNCTION_TRACER
    CFLAGS_REMOVE_lockdep.o = -pg
    endif

Could be replaced with:

   ccflags-lockdep.o-remove-$(CONFIG_FUNCTION_TRACER) := -pg

The ifdef often cover more than one file so we save less lines in
reality than the example says.


All looks rather trivial to implement.
But it is maybe a bit too cryptic?

	Sam

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

[Arnd Bergmann]

On Thursday 10 March 2011, Ingo Molnar wrote:
> Also, adding 3 lines per object file is pretty ugly - would it be possible to create 
> a nicer, compact, single-line way to someone condense a CONFIG_ERROR opt-out 
> mechanism, the -Werror default and the single-object-file into a single rule?
> 
> Something like:
> 
>   obj-werror-y += sched.o

I haven't tried it, but it could work (or be made to work) recursively,
so you can link multiple objects together and have them use the same flag e.g.

obj-y += obj_werror.o
ccflags-obj_werror.o += -Werror
obj-werror-y += sched.o module.o user.o

	Arnd