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 <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: [email protected]
Signed-off-by: Borislav Petkov <[email protected]>
---
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
* Borislav Petkov <[email protected]> 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 <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: [email protected]
> Signed-off-by: Borislav Petkov <[email protected]>
Nice.
Acked-by: Ingo Molnar <[email protected]>
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
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 <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: [email protected]
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> 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
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 <[email protected]>
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 <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: [email protected]
Acked-by: Ingo Molnar <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
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.
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
>
> 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
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 <[email protected]> http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
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 <[email protected]>
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 <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: [email protected]
Acked-by: Ingo Molnar <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
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.
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.
Hi,
On Sun, Feb 20, 2011 at 9:23 PM, Borislav Petkov <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Cc: Sam Ravnborg <[email protected]>
> Cc: [email protected]
> Acked-by: Ingo Molnar <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> ?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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
Hi,
On Sun, Feb 20, 2011 at 9:27 PM, Borislav Petkov <[email protected]> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
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.
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 <[email protected]> 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.
Hi,
On Sun, Feb 20, 2011 at 11:37 PM, Borislav Petkov <[email protected]> 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
Hi,
On Sun, Feb 20, 2011 at 11:54 PM, Borislav Petkov <[email protected]> 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 <[email protected]> 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