[RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Jesper Juhl]

(arrgh, for some reason this just won't hit LKML - trying a third
time, this time without a Cc: list, I hope the people on Cc: got the
mail originally)...


Hi,

I've been down this road before, with mixed reception, but I think it's
worth doing, so I'll try again.

What I want to do is make the kernel build cleanly with -Wshadow.
Shadowing a symbol from an enclosing scope is an easy way to create
bugs and from a "keep namespaces seperate" viewpoint it's messy.
So, I propose that we clean it up so that in the future we can have
-Wshadow enabled in the top-level Makefile and avoid it.

If I just add -Wshadow to the top-level Makefile (as [PATCH 1/9] does)
I get the following results for some different configs :

allnoconfig     :       1224 -Wshadow related warnings.
allmodconfig    :       39886 -Wshadow related warnings.
allyesconfig    :       32358 -Wshadow related warnings.
my own config   :       5271 -Wshadow related warnings.

After applying the patches in this series the warnings are reduced to :

allnoconfig     :       112             (~9%)
allmodconfig    :       9633    (~24%)
allyesconfig    :       9612    (~30%)
my own config   :       1296    (~25%)

In addition, the patches clean up a few shadow warnings generated when
running  make all[no|mod|yes]config && make menuconfig .

So with just a few patches the vast majority of the warnings are gone,
and most of the remaining ones look fairly trivial as well.
This is only for x86, but I suspect that once that arch is cleaned up,
what's left for the others should be minor.

I'm willing to do a complete cleanup, but before I do more than these
initial 9 patches I would like to be sure that such patches are wanted.
It's not exactely interresting work and it's going to take some time
so if it's not wanted I'd rather not waste my time on it.

By posting these initial patches I hope people will comment on the
usefulness of this, my choice of (new) variable names etc. so that I
can create the best possible cleanup patches going forward.

If these patches are acceptable as-is then getting them merged into -mm
would be great (possibly excluding [PATCH 1/9] for now) so that I can
continue working against -mm going forward. Then when all (or almost all)
warnings are gone we can add -Wshadow to the Makefile in -mm for a few
releases and then eventually merge with mainline.

The patches in this series are :

0/9 -Wshadow: Making the kernel build clean with -Wshadow
1/9 -Wshadow: Add -Wshadow to toplevel Makefile
2/9 -Wshadow: Fix warnings in mconf
3/9 -Wshadow: lxdialog warning fixes
4/9 -Wshadow: fix warnings caused by jiffies.h
5/9 -Wshadow: variables named 'up' clash with up()
6/9 -Wshadow: 'map_bh' and 'wbc' shadow fixes
7/9 -Wshadow: fixes for checksum.h
8/9 -Wshadow: vgacon fixes
9/9 -Wshadow: fixes for drivers/char/keyboard.c

So, what do people say?


/Jesper Juhl <jesper.juhl@gmail.com>

Re: [RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Arjan van de Ven]

> So, what do people say?


Hi,

I'm just about always in favor of having automated tools help us find
bugs. However... can you give an indication of how many real bugs you
have encountered? If it's "mostly noise" all the time.. then it's maybe
not worth the effort... while if you find real bugs then it's obviously
worthwhile to go through this.

Greetings,
   Arjan van de Ven

Re: [RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Jesper Juhl]

On 10/07/06, Arjan van de Ven <arjan@infradead.org> wrote:
>
> > So, what do people say?
>
>
> Hi,
>
> I'm just about always in favor of having automated tools help us find
> bugs. However... can you give an indication of how many real bugs you
> have encountered? If it's "mostly noise" all the time.. then it's maybe
> not worth the effort... while if you find real bugs then it's obviously
> worthwhile to go through this.
>
For the parts I've done so far, the only "real" issue is one case of a
superfluous variable. But I'm far from done going through all the
warnings.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Dmitry Torokhov]

On 7/10/06, Arjan van de Ven <arjan@infradead.org> wrote:
>
> > So, what do people say?
>
>
> Hi,
>
> I'm just about always in favor of having automated tools help us find
> bugs. However... can you give an indication of how many real bugs you
> have encountered? If it's "mostly noise" all the time.. then it's maybe
> not worth the effort... while if you find real bugs then it's obviously
> worthwhile to go through this.
>

While we may not have any issues with the present code it can help
avoiding problems in new code if we have -Wshadow by default.

Just my $.02

-- 
Dmitry

Re: [RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Arjan van de Ven]

On Mon, 2006-07-10 at 09:03 -0400, Dmitry Torokhov wrote:
> On 7/10/06, Arjan van de Ven <arjan@infradead.org> wrote:
> >
> > > So, what do people say?
> >
> >
> > Hi,
> >
> > I'm just about always in favor of having automated tools help us find
> > bugs. However... can you give an indication of how many real bugs you
> > have encountered? If it's "mostly noise" all the time.. then it's maybe
> > not worth the effort... while if you find real bugs then it's obviously
> > worthwhile to go through this.
> >
> 
> While we may not have any issues with the present code it can help
> avoiding problems in new code if we have -Wshadow by default.

I agree with that; however that still depends on the ratio of real bugs
vs "noise". While it's hard to estimate for future code, the existing
code base can be an indication...

Re: [RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Jesper Juhl]

On 10/07/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Mon, 2006-07-10 at 09:03 -0400, Dmitry Torokhov wrote:
> >
> > While we may not have any issues with the present code it can help
> > avoiding problems in new code if we have -Wshadow by default.
>
> I agree with that; however that still depends on the ratio of real bugs
> vs "noise". While it's hard to estimate for future code, the existing
> code base can be an indication...
>
One little twist here is that if I end up going through all the
current warnings to clean them up then once I know that ratio I'll
have already done all the cleanup work, so whatever the bugs/noise
ratio turns out to be, all the work will already be done and then
(from my point of view) we might as well do it ;-)

More seriously; I'll try and sift through the pile I have at the
moment and pick out those that look like real bugs, then we can look
at that and see if it's worth-while...
In any case, even if there are only very few real bugs (or even none)
I'm still full willing to do the work to help us protect against
future bugs.


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [RFC][PATCH 0/9] -Wshadow: Making the kernel build clean with -Wshadow

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Mon, 10 Jul 2006 14:53:19 +0200, Arjan van de Ven said:

> I'm just about always in favor of having automated tools help us find
> bugs. However... can you give an indication of how many real bugs you
> have encountered? If it's "mostly noise" all the time.. then it's maybe
> not worth the effort... while if you find real bugs then it's obviously
> worthwhile to go through this.

I started doing similar a while back, and I hadn't come across any
actual bugs either.  However, there's 2 aspects to this:

1) The cleanup cases that Jesper is doing (which are pretty similar
to what I was doing) are mostly "function prototypes in .h files use
a variable name that collides with another global variable" ('up' for
example). (My nominee for patch 10/9:

include/net/tcp.h:469: warning: declaration of '__x' shadows a previous local
include/net/tcp.h:469: warning: shadowed declaration is here

2) The actual *bugs* are most probably in the "variable in .c file
shadows a global".

As Jesper notes, it's hard to see the latter when there's 38,000+ noise
warnings....

To get a good estimate of the *actual* bug rate, grep for how many *.c files
trigger the warning when built with -Wshadow - there's 695 of *those* in
my current config.

When I did a bunch of cleanups a while ago to make -Wundef work, I think
I scared up exactly one actual bug - but it was a subtle one in the NFS
code that wouldn't have been easu to find otherwise...


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]