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

[Borislav Petkov <bp@alien8.de>]

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

Thanks. :)

> 
> > +# $(call cc-option... ) handles gcc -W.. options which
> > +# are not supported by all versions of the compiler
> > +ifdef KBUILD_ENABLE_EXTRA_GCC_CHECKS
> > +ifneq ($(call cc-version),)
> > +KBUILD_EXTRA_WARNINGS := -Wextra
> > +KBUILD_EXTRA_WARNINGS += -Wunused -Wno-unused-parameter
> > +KBUILD_EXTRA_WARNINGS += -Waggregate-return
> > +KBUILD_EXTRA_WARNINGS += -Wbad-function-cast
> > +KBUILD_EXTRA_WARNINGS += -Wcast-qual
> > +KBUILD_EXTRA_WARNINGS += -Wcast-align
> > +KBUILD_EXTRA_WARNINGS += -Wconversion
> > +KBUILD_EXTRA_WARNINGS += -Wdisabled-optimization
> > +KBUILD_EXTRA_WARNINGS += -Wlogical-op
> > +KBUILD_EXTRA_WARNINGS += -Wmissing-declarations
> > +KBUILD_EXTRA_WARNINGS += -Wmissing-format-attribute
> > +KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wmissing-include-dirs,)
> > +KBUILD_EXTRA_WARNINGS += -Wmissing-prototypes
> > +KBUILD_EXTRA_WARNINGS += -Wnested-externs
> > +KBUILD_EXTRA_WARNINGS += -Wold-style-definition
> > +KBUILD_EXTRA_WARNINGS += $(call cc-option, -Woverlength-strings,)
> > +KBUILD_EXTRA_WARNINGS += -Wpacked
> > +KBUILD_EXTRA_WARNINGS += -Wpacked-bitfield-compat
> > +KBUILD_EXTRA_WARNINGS += -Wpadded
> > +KBUILD_EXTRA_WARNINGS += -Wpointer-arith
> > +KBUILD_EXTRA_WARNINGS += -Wredundant-decls
> > +KBUILD_EXTRA_WARNINGS += -Wshadow
> > +KBUILD_EXTRA_WARNINGS += -Wswitch-default
> > +KBUILD_EXTRA_WARNINGS += $(call cc-option, -Wvla,)
> > +KBUILD_CFLAGS += $(KBUILD_EXTRA_WARNINGS)
> > +endif
> > +endif
> 
> I would be a little more selective here. Maybe we can have two levels
> W=1 and W=2, with the full set getting enabled by W=2, and the smaller
> set getting enabled in W=1. The reason is that many of the warnings
> are pointless or even hurting code quality, while others (e.g.
> -Wmissing-declarations) are generally useful and the only reason for
> not enabling them is that they cause too many warnings with existing
> code.

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

Instead I am thinking along with the following lines:

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

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

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

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

Hmm... ?

-- 
Regards/Gruss,
    Boris.