[PATCH] kbuild: Add extra gcc checks

[PATCH] 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/?i=20110218091716.GA4384@bicker

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Michal Marek <mmarek@suse.cz>
Cc: linux-kbuild@vger.kernel.org
Signed-off-by: Borislav Petkov <bp@alien8.de>
---
 Makefile |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index c9c8c8f..a783a69 100644
--- a/Makefile
+++ b/Makefile
@@ -102,6 +102,10 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)
 endif
 
+ifeq ("$(origin W)", "command line")
+  KBUILD_ENABLE_EXTRA_WARNINGS = 1
+endif
+
 # That's our default target when none is given on the command line
 PHONY := _all
 _all:
@@ -363,6 +367,32 @@ KBUILD_AFLAGS_MODULE  := -DMODULE
 KBUILD_CFLAGS_MODULE  := -DMODULE
 KBUILD_LDFLAGS_MODULE := -T $(srctree)/scripts/module-common.lds
 
+ifdef KBUILD_ENABLE_EXTRA_WARNINGS
+KBUILD_CFLAGS += -Wextra -Wno-unused
+KBUILD_CFLAGS += -Waggregate-return
+KBUILD_CFLAGS += -Wbad-function-cast
+KBUILD_CFLAGS += -Wcast-qual
+KBUILD_CFLAGS += -Wcast-align
+KBUILD_CFLAGS += -Wconversion
+KBUILD_CFLAGS += -Wdisabled-optimization
+KBUILD_CFLAGS += -Wlogical-op
+KBUILD_CFLAGS += -Wmissing-declarations
+KBUILD_CFLAGS += -Wmissing-format-attribute
+KBUILD_CFLAGS += -Wmissing-include-dirs
+KBUILD_CFLAGS += -Wmissing-prototypes
+KBUILD_CFLAGS += -Wnested-externs
+KBUILD_CFLAGS += -Wold-style-definition
+KBUILD_CFLAGS += -Woverlength-strings
+KBUILD_CFLAGS += -Wpacked
+KBUILD_CFLAGS += -Wpacked-bitfield-compat
+KBUILD_CFLAGS += -Wpadded
+KBUILD_CFLAGS += -Wpointer-arith
+KBUILD_CFLAGS += -Wredundant-decls
+KBUILD_CFLAGS += -Wshadow
+KBUILD_CFLAGS += -Wswitch-default
+KBUILD_CFLAGS += -Wvla
+endif
+
 # Read KERNELRELEASE from include/config/kernel.release (if it exists)
 KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
 KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
@@ -1262,6 +1292,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'
-- 
1.7.4.1.48.g5673d

[PATCH] 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/?i=20110218091716.GA4384@bicker

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Michal Marek <mmarek@suse.cz>
Cc: linux-kbuild@vger.kernel.org
Signed-off-by: Borislav Petkov <bp@alien8.de>
---
 Makefile |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index c9c8c8f..a783a69 100644
--- a/Makefile
+++ b/Makefile
@@ -102,6 +102,10 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)
 endif
 
+ifeq ("$(origin W)", "command line")
+  KBUILD_ENABLE_EXTRA_WARNINGS = 1
+endif
+
 # That's our default target when none is given on the command line
 PHONY := _all
 _all:
@@ -363,6 +367,32 @@ KBUILD_AFLAGS_MODULE  := -DMODULE
 KBUILD_CFLAGS_MODULE  := -DMODULE
 KBUILD_LDFLAGS_MODULE := -T $(srctree)/scripts/module-common.lds
 
+ifdef KBUILD_ENABLE_EXTRA_WARNINGS
+KBUILD_CFLAGS += -Wextra -Wno-unused
+KBUILD_CFLAGS += -Waggregate-return
+KBUILD_CFLAGS += -Wbad-function-cast
+KBUILD_CFLAGS += -Wcast-qual
+KBUILD_CFLAGS += -Wcast-align
+KBUILD_CFLAGS += -Wconversion
+KBUILD_CFLAGS += -Wdisabled-optimization
+KBUILD_CFLAGS += -Wlogical-op
+KBUILD_CFLAGS += -Wmissing-declarations
+KBUILD_CFLAGS += -Wmissing-format-attribute
+KBUILD_CFLAGS += -Wmissing-include-dirs
+KBUILD_CFLAGS += -Wmissing-prototypes
+KBUILD_CFLAGS += -Wnested-externs
+KBUILD_CFLAGS += -Wold-style-definition
+KBUILD_CFLAGS += -Woverlength-strings
+KBUILD_CFLAGS += -Wpacked
+KBUILD_CFLAGS += -Wpacked-bitfield-compat
+KBUILD_CFLAGS += -Wpadded
+KBUILD_CFLAGS += -Wpointer-arith
+KBUILD_CFLAGS += -Wredundant-decls
+KBUILD_CFLAGS += -Wshadow
+KBUILD_CFLAGS += -Wswitch-default
+KBUILD_CFLAGS += -Wvla
+endif
+
 # Read KERNELRELEASE from include/config/kernel.release (if it exists)
 KERNELRELEASE = $(shell cat include/config/kernel.release 2> /dev/null)
 KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)
@@ -1262,6 +1292,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'
-- 
1.7.4.1.48.g5673d

Re: [PATCH] kbuild: Add extra gcc checks

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> 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/?i=20110218091716.GA4384@bicker
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: linux-kbuild@vger.kernel.org
> Signed-off-by: Borislav Petkov <bp@alien8.de>

Nice.

Acked-by: Ingo Molnar <mingo@elte.hu>

We enable a lot of GCC warnings in tools/perf/ as well, and while there are false 
positives occasionally, the general effect on code quality is positive. We combine 
it with -Werror to make sure warnings do not accumulate.

Thanks,

	Ingo

Re: [PATCH] kbuild: Add extra gcc checks

[Sam Ravnborg]

On Sun, Feb 20, 2011 at 05:35:10PM +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/?i=20110218091716.GA4384@bicker
> 
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: linux-kbuild@vger.kernel.org
> Signed-off-by: Borislav Petkov <bp@alien8.de>
> ---
>  Makefile |   31 +++++++++++++++++++++++++++++++
>  1 files changed, 31 insertions(+), 0 deletions(-)

I like the idea.
But can we avoid polluting the top-level Makefile?
That file is full of unreadable stuff, and adding more
is not good.

Makefile.lib or Makefile.build seems like a better place for this.

	Sam

Re: [PATCH] kbuild: Add extra gcc checks

[Borislav Petkov]

On Sun, Feb 20, 2011 at 06:57:09PM +0100, Sam Ravnborg wrote:
> I like the idea.
> But can we avoid polluting the top-level Makefile?
> That file is full of unreadable stuff, and adding more
> is not good.
> 
> Makefile.lib or Makefile.build seems like a better place for this.

You're right, ./Makefile shouldn't get any fatter than it is right now :).

Here's a second version; Ingo, I've left your ack in even though it was
referencing the initial version since the idea is kept the same - only
the code placement is different.

Thanks.

--
From: Borislav Petkov <bp@alien8.de>
Date: Sun, 20 Feb 2011 17:18:40 +0100
Subject: [PATCH] kbuild: Add extra gcc checks

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

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               |   10 ++++++++++
 scripts/Makefile.build |   28 +++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index c9c8c8f..03989a1 100644
--- a/Makefile
+++ b/Makefile
@@ -102,6 +102,15 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)
 endif
 
+ifeq ("$(origin W)", "command line")
+  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
+endif
+ifndef KBUILD_ENABLE_EXTRA_GCC_CHECKS
+  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 0
+endif
+
+export KBUILD_ENABLE_EXTRA_GCC_CHECKS
+
 # That's our default target when none is given on the command line
 PHONY := _all
 _all:
@@ -1262,6 +1271,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..5bf9f40 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -30,6 +30,33 @@ ldflags-y  :=
 subdir-asflags-y :=
 subdir-ccflags-y :=
 
+# make W=1 settings
+ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
+EXTRA_CFLAGS += -Wextra -Wno-unused
+EXTRA_CFLAGS += -Waggregate-return
+EXTRA_CFLAGS += -Wbad-function-cast
+EXTRA_CFLAGS += -Wcast-qual
+EXTRA_CFLAGS += -Wcast-align
+EXTRA_CFLAGS += -Wconversion
+EXTRA_CFLAGS += -Wdisabled-optimization
+EXTRA_CFLAGS += -Wlogical-op
+EXTRA_CFLAGS += -Wmissing-declarations
+EXTRA_CFLAGS += -Wmissing-format-attribute
+EXTRA_CFLAGS += -Wmissing-include-dirs
+EXTRA_CFLAGS += -Wmissing-prototypes
+EXTRA_CFLAGS += -Wnested-externs
+EXTRA_CFLAGS += -Wold-style-definition
+EXTRA_CFLAGS += -Woverlength-strings
+EXTRA_CFLAGS += -Wpacked
+EXTRA_CFLAGS += -Wpacked-bitfield-compat
+EXTRA_CFLAGS += -Wpadded
+EXTRA_CFLAGS += -Wpointer-arith
+EXTRA_CFLAGS += -Wredundant-decls
+EXTRA_CFLAGS += -Wshadow
+EXTRA_CFLAGS += -Wswitch-default
+EXTRA_CFLAGS += -Wvla
+endif
+
 # Read auto.conf if it exists, otherwise ignore
 -include include/config/auto.conf
 
@@ -403,7 +430,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] kbuild: Add extra gcc checks

[Sam Ravnborg]

> 
> diff --git a/Makefile b/Makefile
> index c9c8c8f..03989a1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -102,6 +102,15 @@ ifeq ("$(origin O)", "command line")
>    KBUILD_OUTPUT := $(O)
>  endif
>  
> +ifeq ("$(origin W)", "command line")
> +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
> +endif
> +ifndef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 0
> +endif

I do not see the purpose of setting KBUILD_ENABLE_EXTRA_GCC_CHECKS
equal to 0.

> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 4eb99ab..5bf9f40 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -30,6 +30,33 @@ ldflags-y  :=
>  subdir-asflags-y :=
>  subdir-ccflags-y :=
>  
> +# make W=1 settings
> +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS

The symbol KBUILD_ENABLE_EXTRA_GCC_CHECKS is always defined,
as you set it to "0" in the top-level Makefile.
So you will always have the extra checks enabled.

You also need to move the assignments down.
As it is now any kbuild file that assign
EXTRA_CFLAGS with

    EXTRA_CFLAGS := -D DEBUG

Will drop all the extra warnings.

From Makefile.build:
# If the save-* variables changed error out
ifeq ($(KBUILD_NOPEDANTIC),)
        ifneq ("$(save-cflags)","$(CFLAGS)")
                $(error CFLAGS was changed in "$(kbuild-file)". Fix it to use EXTRA_CFLAGS)
        endif
endif

<<<<< Here would be the better place to add this.

include scripts/Makefile.lib

> +EXTRA_CFLAGS += -Wextra -Wno-unused

Use of EXTRA_CFLAGS is deprecated - so that is not the right choice.
I suggest to use KBUILD_CFLAGS that is an KBUILD internal variable.

	Sam

Re: [PATCH] kbuild: Add extra gcc checks

[Sam Ravnborg]

> 
> diff --git a/Makefile b/Makefile
> index c9c8c8f..03989a1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -102,6 +102,15 @@ ifeq ("$(origin O)", "command line")
>    KBUILD_OUTPUT := $(O)
>  endif
>  
> +ifeq ("$(origin W)", "command line")
> +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
> +endif
> +ifndef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 0
> +endif

I do not see the purpose of setting KBUILD_ENABLE_EXTRA_GCC_CHECKS
equal to 0.

> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 4eb99ab..5bf9f40 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -30,6 +30,33 @@ ldflags-y  :=
>  subdir-asflags-y :=
>  subdir-ccflags-y :=
>  
> +# make W=1 settings
> +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS

The symbol KBUILD_ENABLE_EXTRA_GCC_CHECKS is always defined,
as you set it to "0" in the top-level Makefile.
So you will always have the extra checks enabled.

You also need to move the assignments down.
As it is now any kbuild file that assign
EXTRA_CFLAGS with

    EXTRA_CFLAGS := -D DEBUG

Will drop all the extra warnings.

>From Makefile.build:
# If the save-* variables changed error out
ifeq ($(KBUILD_NOPEDANTIC),)
        ifneq ("$(save-cflags)","$(CFLAGS)")
                $(error CFLAGS was changed in "$(kbuild-file)". Fix it to use EXTRA_CFLAGS)
        endif
endif

<<<<< Here would be the better place to add this.

include scripts/Makefile.lib

> +EXTRA_CFLAGS += -Wextra -Wno-unused

Use of EXTRA_CFLAGS is deprecated - so that is not the right choice.
I suggest to use KBUILD_CFLAGS that is an KBUILD internal variable.

	Sam

[PATCH -v3] kbuild: Add extra gcc checks

[Borislav Petkov]

On Sun, Feb 20, 2011 at 08:52:22PM +0100, Sam Ravnborg wrote:

..

> Use of EXTRA_CFLAGS is deprecated - so that is not the right choice.
> I suggest to use KBUILD_CFLAGS that is an KBUILD internal variable.

Hey Sam,

thanks for the suggestions/review, here's hopefully a better version:

--
From: Borislav Petkov <bp@alien8.de>
Date: Sun, 20 Feb 2011 17:18:40 +0100
Subject: [PATCH -v3] kbuild: Add extra gcc checks

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

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 |   29 ++++++++++++++++++++++++++++-
 2 files changed, 34 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..b4d7661 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -49,6 +49,34 @@ ifeq ($(KBUILD_NOPEDANTIC),)
                 $(error CFLAGS was changed in "$(kbuild-file)". Fix it to use EXTRA_CFLAGS)
         endif
 endif
+
+# make W=1 settings
+ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
+KBUILD_CFLAGS += -Wextra -Wno-unused
+KBUILD_CFLAGS += -Waggregate-return
+KBUILD_CFLAGS += -Wbad-function-cast
+KBUILD_CFLAGS += -Wcast-qual
+KBUILD_CFLAGS += -Wcast-align
+KBUILD_CFLAGS += -Wconversion
+KBUILD_CFLAGS += -Wdisabled-optimization
+KBUILD_CFLAGS += -Wlogical-op
+KBUILD_CFLAGS += -Wmissing-declarations
+KBUILD_CFLAGS += -Wmissing-format-attribute
+KBUILD_CFLAGS += -Wmissing-include-dirs
+KBUILD_CFLAGS += -Wmissing-prototypes
+KBUILD_CFLAGS += -Wnested-externs
+KBUILD_CFLAGS += -Wold-style-definition
+KBUILD_CFLAGS += -Woverlength-strings
+KBUILD_CFLAGS += -Wpacked
+KBUILD_CFLAGS += -Wpacked-bitfield-compat
+KBUILD_CFLAGS += -Wpadded
+KBUILD_CFLAGS += -Wpointer-arith
+KBUILD_CFLAGS += -Wredundant-decls
+KBUILD_CFLAGS += -Wshadow
+KBUILD_CFLAGS += -Wswitch-default
+KBUILD_CFLAGS += -Wvla
+endif
+
 include scripts/Makefile.lib
 
 ifdef host-progs
@@ -403,7 +431,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 -v3] kbuild: Add extra gcc checks

[Arnaud Lacombe]

Hi,

On Sun, Feb 20, 2011 at 9:23 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Feb 20, 2011 at 08:52:22PM +0100, Sam Ravnborg wrote:
>
> ..
>
>> Use of EXTRA_CFLAGS is deprecated - so that is not the right choice.
>> I suggest to use KBUILD_CFLAGS that is an KBUILD internal variable.
>
> Hey Sam,
>
> thanks for the suggestions/review, here's hopefully a better version:
>
> --
> From: Borislav Petkov <bp@alien8.de>
> Date: Sun, 20 Feb 2011 17:18:40 +0100
> Subject: [PATCH -v3] kbuild: Add extra gcc checks
>
> 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
>
> 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 |   29 ++++++++++++++++++++++++++++-
>  2 files changed, 34 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
> +
You never check CC is gcc. How can you assert this ? Moreover, can you
certify that all the compiler supported to build Linux do support the
set of new warnings ? What about icc ? old gcc ? LLVM/clang ?

>  # 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'
same as above.

>        @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..b4d7661 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -49,6 +49,34 @@ ifeq ($(KBUILD_NOPEDANTIC),)
>                 $(error CFLAGS was changed in "$(kbuild-file)". Fix it to use EXTRA_CFLAGS)
>         endif
>  endif
> +
> +# make W=1 settings
> +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> +KBUILD_CFLAGS += -Wextra -Wno-unused
> +KBUILD_CFLAGS += -Waggregate-return
> +KBUILD_CFLAGS += -Wbad-function-cast
> +KBUILD_CFLAGS += -Wcast-qual
> +KBUILD_CFLAGS += -Wcast-align
> +KBUILD_CFLAGS += -Wconversion
> +KBUILD_CFLAGS += -Wdisabled-optimization
> +KBUILD_CFLAGS += -Wlogical-op
> +KBUILD_CFLAGS += -Wmissing-declarations
> +KBUILD_CFLAGS += -Wmissing-format-attribute
> +KBUILD_CFLAGS += -Wmissing-include-dirs
> +KBUILD_CFLAGS += -Wmissing-prototypes
> +KBUILD_CFLAGS += -Wnested-externs
> +KBUILD_CFLAGS += -Wold-style-definition
> +KBUILD_CFLAGS += -Woverlength-strings
> +KBUILD_CFLAGS += -Wpacked
> +KBUILD_CFLAGS += -Wpacked-bitfield-compat
> +KBUILD_CFLAGS += -Wpadded
> +KBUILD_CFLAGS += -Wpointer-arith
> +KBUILD_CFLAGS += -Wredundant-decls
> +KBUILD_CFLAGS += -Wshadow
> +KBUILD_CFLAGS += -Wswitch-default
> +KBUILD_CFLAGS += -Wvla
> +endif
> +
Why not making it simple at the beginning by:
 1) s/GCC/CC/
 2) using `cc-option' provided by Kbuild for each new option. As an
example, for `-Wvla':

KBUILD_CFLAGS += $(call cc-option, -Wvla)

so so on.

 - Arnaud

>  include scripts/Makefile.lib
>
>  ifdef host-progs
> @@ -403,7 +431,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.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Borislav Petkov]

On Sun, Feb 20, 2011 at 10:26:13PM -0500, Arnaud Lacombe wrote:

> > 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.
> >

[..]

> > +ifeq ("$(origin W)", "command line")
> > +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
> > +  export KBUILD_ENABLE_EXTRA_GCC_CHECKS
> > +endif
> > +
> You never check CC is gcc. How can you assert this ?

Hmm, I somehow knew that the other compilers are going to come into the
discussion :).

> Moreover, can you certify that all the compiler supported to build
> Linux do support the set of new warnings ?

Well, as you've probably already read in the commit message, this
option is for devs only, in case they want to do a build-check whether
the couple of lines they just added to a .c file trigger new compiler
warnings. So, no, I cannot certify this and I don't have to - this is
not meant for production use anyway.

> What about icc ?

I don't know, is anyone using it to build the kernel? Even if so, icc
might have a completely different set of -W options (totally guessing
here) so you shouldn't use 'make W=1' with it.

> old gcc?

Yes, cc-option should be used in that case, I'll redo the patch.

> LLVM/clang ?

Can you even build the kernel with it?

But to make sure we're on the realistic side of things. First of all,
the purpose of this is to quickly get gcc scream out while building your
changes. Secondly, let's face it, the majority of developers, if not
all, use gcc, the kernel code is full of gcc-isms so accomodating all
the compilers to such a quick-and-dirty test option is just plain too
much. Look at it this way: it is cheaper to upgrade your dev environment
than add unnecessary and ugly code to kbuild. Even the argument with
older gcc versions cannot weigh in enough in favor of the cc-option -
simply upgrade your gcc.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

[Arnaud Lacombe]

Hi,

On Sun, Feb 20, 2011 at 11:37 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Feb 20, 2011 at 10:26:13PM -0500, Arnaud Lacombe wrote:
>
>> > 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.
>> >
>
> [..]
>
>> > +ifeq ("$(origin W)", "command line")
>> > +  KBUILD_ENABLE_EXTRA_GCC_CHECKS = 1
>> > +  export KBUILD_ENABLE_EXTRA_GCC_CHECKS
>> > +endif
>> > +
>> You never check CC is gcc. How can you assert this ?
>
> Hmm, I somehow knew that the other compilers are going to come into the
> discussion :).
>
>> Moreover, can you certify that all the compiler supported to build
>> Linux do support the set of new warnings ?
>
> Well, as you've probably already read in the commit message, this
> option is for devs only, in case they want to do a build-check whether
> the couple of lines they just added to a .c file trigger new compiler
> warnings. So, no, I cannot certify this and I don't have to - this is
> not meant for production use anyway.
>
People _will_ end up using it in production.

>> What about icc ?
>
> I don't know, is anyone using it to build the kernel?
then what would be the point of, say `include/linux/compiler-intel.h' ?

> Even if so, icc
> might have a completely different set of -W options (totally guessing
> here) so you shouldn't use 'make W=1' with it.
>

>> old gcc?
>
> Yes, cc-option should be used in that case, I'll redo the patch.
>
>> LLVM/clang ?
>
> Can you even build the kernel with it?
>
At first sight, partly[0].

> But to make sure we're on the realistic side of things.
Whose realistic side ? yours ? If not, are you speaking for all the
Linux community ? I do not think so.

> First of all,
> the purpose of this is to quickly get gcc scream out while building your
> changes. Secondly, let's face it, the majority of developers, if not
> all, use gcc the kernel code is full of gcc-isms so accomodating all
> the compilers to such a quick-and-dirty test option is just plain too
> much.
That's a statement I would not make. Lots of compiler dependent stuff
is buried in <linux/compiler.h> for a good reason.

> Look at it this way: it is cheaper to upgrade your dev environment
> than add unnecessary and ugly code to kbuild. Even the argument with
> older gcc versions cannot weigh in enough in favor of the cc-option -
> simply upgrade your gcc.
>
Do you speak for users, in the embedded world, of BSP whose supporting
company either went defunct or is no longer maintaining the toolchain
for a dark platform, or say an architecture, in kernel, which do not
have support in mainstream binutils and support's patches are tied
with a given version of binutils/gcc ?

 - Arnaud

[0]: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-October/011711.html

Re: [PATCH] kbuild: Add extra gcc checks

[Joe Perches]

On Sun, 2011-02-20 at 20:39 +0100, Borislav Petkov wrote:
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> +# make W=1 settings
> +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS

ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),1)

> +EXTRA_CFLAGS += -Wextra -Wno-unused

Why add -Wno-unused ?

If it's because of verbosity, maybe

ifeq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),2)
EXTRA_CFLAGS += -Wunused
endif

Re: [PATCH] kbuild: Add extra gcc checks

[Borislav Petkov]

On Sun, Feb 20, 2011 at 12:00:47PM -0800, Joe Perches wrote:
> > +EXTRA_CFLAGS += -Wextra -Wno-unused
> 
> Why add -Wno-unused ?
> 
> If it's because of verbosity, maybe

Nah, it's because it is too noisy and spits too many false positives.
For example, it reports the arguments of all those stubs from the
headers which are provided for the else-branch of a CONFIG_* option,
etc.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] kbuild: Add extra gcc checks

[Arnaud Lacombe]

Hi,

On Sun, Feb 20, 2011 at 9:27 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Feb 20, 2011 at 12:00:47PM -0800, Joe Perches wrote:
>> > +EXTRA_CFLAGS += -Wextra -Wno-unused
>>
>> Why add -Wno-unused ?
>>
>> If it's because of verbosity, maybe
>
> Nah, it's because it is too noisy and spits too many false positives.
>
"too noisy" is a subjective point of view.

> For example, it reports the arguments of all those stubs from the
> headers which are provided for the else-branch of a CONFIG_* option,
> etc.
>
and by the same way, you silence function marked with
`warn_unused_result', unless I misread the manpage. If you want to
silence something specific, why not just the `no' variant of the thing
you do not want ?

Btw, could you not take the same approach as the one taken by the BSD,
which is 3 or 4 different level of new warnings. That way, you keep
the noisy stuff for the highest warning level.

 - Arnaud

> --
> Regards/Gruss,
>    Boris.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] kbuild: Add extra gcc checks

[Borislav Petkov]

On Sun, Feb 20, 2011 at 10:34:57PM -0500, Arnaud Lacombe wrote:
> Hi,
> 
> On Sun, Feb 20, 2011 at 9:27 PM, Borislav Petkov <bp@alien8.de> wrote:
> > On Sun, Feb 20, 2011 at 12:00:47PM -0800, Joe Perches wrote:
> >> > +EXTRA_CFLAGS += -Wextra -Wno-unused
> >>
> >> Why add -Wno-unused ?
> >>
> >> If it's because of verbosity, maybe
> >
> > Nah, it's because it is too noisy and spits too many false positives.
> >
> "too noisy" is a subjective point of view.

Ok, does "too many false positives" objectify it a bit more to your
taste?

> > For example, it reports the arguments of all those stubs from the
> > headers which are provided for the else-branch of a CONFIG_* option,
> > etc.
> >
> and by the same way, you silence function marked with
> `warn_unused_result', unless I misread the manpage.

Can you point me to that passage, I cannot find it in my gcc manpage.

> If you want to silence something specific, why not just the `no'
> variant of the thing you do not want ?

Yes, '-Wunused -Wno-unused-parameter' looks better.

> Btw, could you not take the same approach as the one taken by the BSD,
> which is 3 or 4 different level of new warnings. That way, you keep
> the noisy stuff for the highest warning level.

Nope, because there's no reason for it. I want to have one switch that
craps out all the possible warnings gcc can spit, I catch the build
output, fix the bugs and that's it.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH] kbuild: Add extra gcc checks

[Arnaud Lacombe]

Hi,

On Sun, Feb 20, 2011 at 11:54 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Feb 20, 2011 at 10:34:57PM -0500, Arnaud Lacombe wrote:
>> Hi,
>>
>> On Sun, Feb 20, 2011 at 9:27 PM, Borislav Petkov <bp@alien8.de> wrote:
>> > On Sun, Feb 20, 2011 at 12:00:47PM -0800, Joe Perches wrote:
>> >> > +EXTRA_CFLAGS += -Wextra -Wno-unused
>> >>
>> >> Why add -Wno-unused ?
>> >>
>> >> If it's because of verbosity, maybe
>> >
>> > Nah, it's because it is too noisy and spits too many false positives.
>> >
>> "too noisy" is a subjective point of view.
>
> Ok, does "too many false positives" objectify it a bit more to your
> taste?
>
The degree of acceptable verbosity should be left to the end user.

>> > For example, it reports the arguments of all those stubs from the
>> > headers which are provided for the else-branch of a CONFIG_* option,
>> > etc.
>> >
>> and by the same way, you silence function marked with
>> `warn_unused_result', unless I misread the manpage.
>
> Can you point me to that passage, I cannot find it in my gcc manpage.
>
my mistake, I just checked, `-Wno-unused' does not affect the
`warn_unused_result' warning. `-Wno-unused-result' is required to
silent that one.

>> If you want to silence something specific, why not just the `no'
>> variant of the thing you do not want ?
>
> Yes, '-Wunused -Wno-unused-parameter' looks better.
>
>> Btw, could you not take the same approach as the one taken by the BSD,
>> which is 3 or 4 different level of new warnings. That way, you keep
>> the noisy stuff for the highest warning level.
>
> Nope, because there's no reason for it. I want to have one switch that
> craps out all the possible warnings gcc can spit, I catch the build
> output, fix the bugs and that's it.
>
Looks like I'll submit the patch implementing multiple warnings level then ;-)

 - Arnaud

Re: [PATCH] kbuild: Add extra gcc checks

[Jesper Juhl]

On Sun, 20 Feb 2011, 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.
> 

I like this. I often modify the Makefile to add something similar to this 
for my local builds. This will save me having to do that in the future.

-- 
Jesper Juhl <jj@chaosbits.net>            http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html