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

Cc Ingo Molnar <[email protected]>

Cc Michal Marek <[email protected]>

Cc [email protected]

Signed-off-by Borislav Petkov <[email protected]>

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.

+EXTRA_CFLAGS += -Wextra -Wno-unused

Quoted 1 line

+EXTRA_CFLAGS += -Wextra -Wno-unused

Why add -Wno-unused ?

If it's because of verbosity, maybe

Quoted 5 lines · Joe Perches

Quoted 1 line

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

Quoted 3 lines · Borislav Petkov

Quoted 2 lines · Joe Perches

Quoted 1 line

+EXTRA_CFLAGS += -Wextra -Wno-unused

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 8 lines · Arnaud Lacombe

Hi,

Quoted 5 lines · Borislav Petkov

Quoted 4 lines · Joe Perches

Quoted 1 line

+EXTRA_CFLAGS += -Wextra -Wno-unused

Why add -Wno-unused ?

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

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?

Quoted 6 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Makefile +0 -0

index c9c8c8f..03989a1 100644

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.

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

..

Quoted 2 lines

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(-)

Makefile +0 -0

index c9c8c8f..c3bca9c 100644
@@ -102,6 +102,11 @@ ifeq ("$(origin O)", "command line")
   KBUILD_OUTPUT := $(O)

 endif

 # That's our default target when none is given on the command line PHONY := _all
 _all:

Makefile +0 -0

@@ -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'

scripts/Makefile.build +0 -0

index 4eb99ab..b4d7661 100644
@@ -49,6 +49,34 @@ ifeq ($(KBUILD_NOPEDANTIC),)
                 $(error CFLAGS was changed in "$(kbuild-file)". Fix it to use EXTRA_CFLAGS)

        endif
 endif

scripts/Makefile.build +18 -0

+
+# 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

 include scripts/Makefile.lib

 ifdef host-progs

scripts/Makefile.build +0 -0

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

Quoted 10 lines

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.

Makefile ifeq ("$(origin O)", "command line") · line 109

Quoted 5 lines

Makefile +5 -0

+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 ?

Quoted 10 lines · Arnaud Lacombe

Quoted 10 lines

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.

[..]

Quoted 6 lines

Makefile ifeq ("$(origin O)", "command line") · line 109

Quoted 5 lines

Makefile +5 -0

+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 :).

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 1 line

old gcc?

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

Quoted 1 line

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.

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.