login
Search
A recent string of patches posted to the lkml attempted to clean up compiler warnings, and lead to numerous discussions about the appropriate way to fix warnings. One thread discussed a warning caused by what the compiler inappropriately believed was an unitialized variable, to which Alan Cox replied, "hiding warnings like this can be a hazard as it will hide real warnings later on." He went on to suggest the warning was a good thing as it encourages developers to continue to review the code, "while the warning is present people will check it now and again."
Al Viro was a little more direct saying, "Don't. Fix. Correct. Code. Ever. Because sooner or later you will paper over [a] real bug. It's far better to reject patches that just make $TOOL to STFU than risk [a] blind 'fix' hiding a real bug." He went on to note, "unless you show a real codepath that leads to use without initialization (and do that in [the] commit message, so it could be verified as [a] real issue), these patches are worthless in the best case and dangerous in the worst one."
Andrew Morton [interview] pointed out that the kernel does generate far too many warnings, and that real bugs do get lost in the noise, "I occasionally receive patches which generate new warnings, and those warnings flag real bugs. The developer simply missing the warning amongst all the other crap." He went on to propose a method for muffling certain types of warnings on certain version of GCC. Al Viro suggested that instead GCC needs to be fixed, "gcc could get saner. Seriously, some very common patterns trigger that shit - e.g. [a] function that returns [an] error or 0 and stores [the] value to [a] *pointer_argument in case of success. It's a clear regression in 4.x and IMO should be treated as [a] gcc bug."
From: Daniel Walker [email blocked]
To: akpm
Subject: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Tue, 9 May 2006 19:56:08 -0700
Fixes the following warnings,
ipc/sem.c: In function 'sys_semctl':
ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function
ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function
ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function
Signed-Off-By: Daniel Walker [email blocked]
Index: linux-2.6.16/ipc/sem.c
===================================================================
--- linux-2.6.16.orig/ipc/sem.c
+++ linux-2.6.16/ipc/sem.c
@@ -807,7 +807,7 @@ static int semctl_down(int semid, int se
{
struct sem_array *sma;
int err;
- struct sem_setbuf setbuf;
+ struct sem_setbuf setbuf = {0, 0, 0};
struct kern_ipc_perm *ipcp;
if(cmd == IPC_SET) {
From: Alan Cox [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 11:34:26 +0100
On Maw, 2006-05-09 at 19:56 -0700, Daniel Walker wrote:
> Fixes the following warnings,
>
> ipc/sem.c: In function 'sys_semctl':
> ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function
>
> Signed-Off-By: Daniel Walker [email blocked]
>
> Index: linux-2.6.16/ipc/sem.c
> ===================================================================
> --- linux-2.6.16.orig/ipc/sem.c
> +++ linux-2.6.16/ipc/sem.c
> @@ -807,7 +807,7 @@ static int semctl_down(int semid, int se
> {
> struct sem_array *sma;
> int err;
> - struct sem_setbuf setbuf;
> + struct sem_setbuf setbuf = {0, 0, 0};
This causes very poor code as its initializing an object on the stack.
It also appears from inspection to be entirely un-neccessary. Instead
the compiler needs some help.
Hiding warnings like this can be a hazard as it will hide real warnings
later on.
From: Daniel Walker [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 07:31:29 -0700
On Wed, 2006-05-10 at 11:34 +0100, Alan Cox wrote:
>
> This causes very poor code as its initializing an object on the stack.
> It also appears from inspection to be entirely un-neccessary. Instead
> the compiler needs some help.
It's just a warning .. The compiler flagged a spot which look suspect .
We have -Wall turn on now , so we could possibly disable these
warnings.
> Hiding warnings like this can be a hazard as it will hide real warnings
> later on.
How could it hide real warnings? If anything these patch allow other
(real warnings) to be seen .
From: Alan Cox [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 16:09:47 +0100
On Mer, 2006-05-10 at 07:31 -0700, Daniel Walker wrote:
> > Hiding warnings like this can be a hazard as it will hide real warnings
> > later on.
>
> How could it hide real warnings? If anything these patch allow other
> (real warnings) to be seen .
Because while the warning is present people will check it now and again.
If you set the variable to zero then you generate extra code and you
ensure nobody will look in future.
From: Daniel Walker [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 08:06:37 -0700
On Wed, 2006-05-10 at 16:09 +0100, Alan Cox wrote:
> On Mer, 2006-05-10 at 07:31 -0700, Daniel Walker wrote:
> > > Hiding warnings like this can be a hazard as it will hide real warnings
> > > later on.
> >
> > How could it hide real warnings? If anything these patch allow other
> > (real warnings) to be seen .
>
> Because while the warning is present people will check it now and again.
But it's pointless to review it, in this case and for this warning .
> If you set the variable to zero then you generate extra code and you
> ensure nobody will look in future.
The extra code is a problem , I'll admit that . But the warning should
disappear , it's just a waste .
Daniel
From: Alan Cox [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 16:39:31 +0100
On Mer, 2006-05-10 at 08:06 -0700, Daniel Walker wrote:
> > Because while the warning is present people will check it now and again.
>
> But it's pointless to review it, in this case and for this warning .
Then why did you review it ?
Alan
From: Daniel Walker [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 08:38:41 -0700
On Wed, 2006-05-10 at 16:39 +0100, Alan Cox wrote:
> On Mer, 2006-05-10 at 08:06 -0700, Daniel Walker wrote:
> > > Because while the warning is present people will check it now and again.
> >
> > But it's pointless to review it, in this case and for this warning .
>
> Then why did you review it ?
So I wouldn't have to see that warning again ..
> It reminds people that it does need checking, and ensures now and then
> people do check that there isn't actually a bug there.
I don't see a reason why it needs checking .. People are just going to
spin their wheel reviewing code that's been reviewed .. Maybe someone
like me will write a patch to fix this warning , and we'll see this
process happening all over again ..
Daniel
From: Al Viro [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 17:21:06 +0100
On Wed, May 10, 2006 at 08:38:41AM -0700, Daniel Walker wrote:
> On Wed, 2006-05-10 at 16:39 +0100, Alan Cox wrote:
> > On Mer, 2006-05-10 at 08:06 -0700, Daniel Walker wrote:
> > > > Because while the warning is present people will check it now and again.
> > >
> > > But it's pointless to review it, in this case and for this warning .
> >
> > Then why did you review it ?
>
> So I wouldn't have to see that warning again ..
>
> > It reminds people that it does need checking, and ensures now and then
> > people do check that there isn't actually a bug there.
>
> I don't see a reason why it needs checking .. People are just going to
> spin their wheel reviewing code that's been reviewed .. Maybe someone
> like me will write a patch to fix this warning , and we'll see this
> process happening all over again ..
Don't. Fix. Correct. Code.
Ever. Because sooner or later you will paper over real bug. It's far better
to reject patches that just make $TOOL to STFU than risk blind "fix" hiding
a real bug.
Unless you show a real codepath that leads to use without initialization
(and do that in commit message, so it could be verified as real issue),
these patches are worthless in the best case and dangerous in the worst
one.
From: Andrew Morton [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 15:03:21 -0700
Al Viro [email blocked] wrote:
>
> Don't. Fix. Correct. Code.
>
> Ever. Because sooner or later you will paper over real bug.
I occasionally receive patches which generate new warnings, and those
warnings flag real bugs. The developer simply missing the warning amongst
all the other crap.
It seems to especialy affect ia64 developers, whose build is especially
noisy.
From: Al Viro [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 23:10:24 +0100
On Wed, May 10, 2006 at 03:03:21PM -0700, Andrew Morton wrote:
> Al Viro [email blocked] wrote:
> >
> > Don't. Fix. Correct. Code.
> >
> > Ever. Because sooner or later you will paper over real bug.
>
> I occasionally receive patches which generate new warnings, and those
> warnings flag real bugs. The developer simply missing the warning amongst
> all the other crap.
>
> It seems to especialy affect ia64 developers, whose build is especially
> noisy.
I know. But that's the argument in favour of using diff, not shutting the
bogus warnings up...
From: David S. Miller [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 15:31:29 -0700 (PDT)
From: Al Viro [email blocked]
Date: Wed, 10 May 2006 23:10:24 +0100
> But that's the argument in favour of using diff, not shutting the
> bogus warnings up...
IMHO, the tree should build with -Werror without exception.
Once you have that basis, new ones will not show up easily
and the hard part of the battle has been won.
Yes, people will post a lot of bogus versions of warning fixes, but
we're already good at flaming those off already :-)
From: Al Viro [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 23:45:49 +0100
On Wed, May 10, 2006 at 03:31:29PM -0700, David S. Miller wrote:
> From: Al Viro [email blocked]
> Date: Wed, 10 May 2006 23:10:24 +0100
>
> > But that's the argument in favour of using diff, not shutting the
> > bogus warnings up...
>
> IMHO, the tree should build with -Werror without exception.
> Once you have that basis, new ones will not show up easily
> and the hard part of the battle has been won.
>
> Yes, people will post a lot of bogus versions of warning fixes, but
> we're already good at flaming those off already :-)
Alternatively, gcc could get saner. Seriously, some very common patterns
trigger that shit - e.g. function that returns error or 0 and stores
value to *pointer_argument in case of success. It's a clear regression
in 4.x and IMO should be treated as gcc bug.
From: Andrew Morton [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 16:05:48 -0700
Al Viro [email blocked] wrote:
>
> On Wed, May 10, 2006 at 03:31:29PM -0700, David S. Miller wrote:
> > From: Al Viro [email blocked]
> > Date: Wed, 10 May 2006 23:10:24 +0100
> >
> > > But that's the argument in favour of using diff, not shutting the
> > > bogus warnings up...
> >
> > IMHO, the tree should build with -Werror without exception.
> > Once you have that basis, new ones will not show up easily
> > and the hard part of the battle has been won.
> >
> > Yes, people will post a lot of bogus versions of warning fixes, but
> > we're already good at flaming those off already :-)
>
> Alternatively, gcc could get saner. Seriously, some very common patterns
> trigger that shit - e.g. function that returns error or 0 and stores
> value to *pointer_argument in case of success. It's a clear regression
> in 4.x and IMO should be treated as gcc bug.
Sure - it's sad and we need some workaround.
The init_self() thingy seemed reasonable to me - it shuts up the warning
and has no runtime cost. What we could perhaps do is to make
#define init_self(x) (x = x)
only if the problematic gcc versions are detected. Later, if/when gcc gets
fixed up, we use
#define init_self(x) x
Or something. Probably a more specific name than "init_self" is needed in
this case - something that indicates that it's a specific workaround for
specific gcc versions.
From: Al Viro [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Thu, 11 May 2006 00:20:42 +0100
On Wed, May 10, 2006 at 04:05:48PM -0700, Andrew Morton wrote:
> Sure - it's sad and we need some workaround.
>
> The init_self() thingy seemed reasonable to me - it shuts up the warning
> and has no runtime cost. What we could perhaps do is to make
>
> #define init_self(x) (x = x)
>
> only if the problematic gcc versions are detected. Later, if/when gcc gets
> fixed up, we use
Sorry, no - it shuts up too much. Look, there are two kinds of warnings
here. "May be used" and "is used". This stuff shuts both. And unlike
"may be used", "is used" has fairly high S/N ratio.
Moreover, once you do that, you lose all future "is used" warnings on
that variable. So your ability to catch future bugs is decreased, not
increased.
From: Andrew Morton [email blocked]
Subject: Re: [PATCH -mm] sys_semctl gcc 4.1 warning fix
Date: Wed, 10 May 2006 16:45:54 -0700
Al Viro [email blocked] wrote:
>
> On Wed, May 10, 2006 at 04:05:48PM -0700, Andrew Morton wrote:
> > Sure - it's sad and we need some workaround.
> >
> > The init_self() thingy seemed reasonable to me - it shuts up the warning
> > and has no runtime cost. What we could perhaps do is to make
> >
> > #define init_self(x) (x = x)
> >
> > only if the problematic gcc versions are detected. Later, if/when gcc gets
> > fixed up, we use
>
> Sorry, no - it shuts up too much. Look, there are two kinds of warnings
> here. "May be used" and "is used". This stuff shuts both. And unlike
> "may be used", "is used" has fairly high S/N ratio.
>
> Moreover, once you do that, you lose all future "is used" warnings on
> that variable. So your ability to catch future bugs is decreased, not
> increased.
Only for certain gcc versions. Other people use other versions, so it'll
be noticed. If/when gcc gets fixed, more and more people will see the real
warnings.
Look, of course it has problems. But the current build has problems too.
It's a question of which problem is worse..
skimming through the warnings
I watch for new warnings with a script that runs after a build, the guts of it look like this (I build 10 different configs, each leaves a "make.out" file in the build directory):
cat */make.out | egrep '(WARNING|warning:)' | sort -u | comm -23 - ~/Expected-warnings
Warning should be handled
With all the respect to these people's opinions, I think that leaving in warnings that should be ignored will mostly lead to people just ignoring stuff, just like Morton said.
I suppose the Al is right here - fixing GCC's warnings to be smarter is probably the best way to go.
I agree. While hacks to remov
I agree. While hacks to remove warnings because 'I know better than the compiler' are bad, spewing out thousands of bogus warnings, thus drowning out all the _real_ problems, is much worse. Sure you can diff the output of makes to find new warnings, but that is just another way of hiding the warnings, with exactly the same risks of hiding warnings about real problems.
It shouldn't be enabled by default, but developers should try to occasionally build their software using -Werror.
yep.... when I'm writing C or
yep.... when I'm writing C or C++ I always have -Werror -Wall and friends kicking around. OK, some of the "warnings" might be pretty spurious, but then I can easily see the important ones because they are the only ones.
Similarly, perl is always -w and use strict; for me.
and PHP is always ini_set("error_reporting",E_ALL).
As far as I'm concerned, if it's not clean code, it's not easily maintainable code. (Of course, you often end up with more lines of code to make sure vars are used, initialised etc, but the penalty there is small compared to not being able find important warnings.)
OK.. so the kernel guys are now in the situation of having bazillions of warnings from their code and I can understand why they are edgy about including patches that don't do anything useful and might introduce bugs... "unnecessary" patches (even just supposed whitespace patches) make me edgy too... but I think Andrew Morton's comment is very telling here -- they are missing important warnings themselves because they are drowned out in the noise.
Ugh... This looks like a tricky issue, too.
I looked at the code in question in the kernel to understand why the warning was happening to begin with. It ain't purty.
At a high level, the code is roughly doing something like this:
void foo(int cmd, ...) { struct foo temp; if (cmd == XYZ) { blah(&temp, ...); /* this initializes temp */ } /* ... */ /* common code */ /* ... */ switch (cmd) { case BLAH: ...; break; case XYZ: var1 = temp.foo; /* This uses temp, triggering the warning. */ var2 = temp.bar; break; /* ... */ } }So, it's obvious upon inspection that temp gets initialized only when "cmd == XYZ", and it also only gets used when "cmd == XYZ". The problem is that there's a bit of common code and other control flow there to confuse things.
I'd imagine GCC just knows "temp is only conditionally initialized", and "temp is conditionally used" without realizing the two conditions are equivalent. So, what did other older GCCs do with this type of code? Did it not generate a warning because it was smarter, or because it knew its own limitations and ignored it?
Is there a better way to structure this code? I can't think of a better way off the top of my head, though I think the current structure is "icky."
easy fix to that code
Thanks for that overview -- nice to see some concrete code without having to delve through lkml and source code. The code you show is a nice illustration of the problem, but it's also trivially fixed:
void foo(int cmd, ...) { struct foo temp; /* ... */ /* common code */ /* ... */ switch (cmd) { case BLAH: ...; break; case XYZ: blah(&temp, ...); /* this initializes temp */ var1 = temp.foo; /* This uses temp, triggering the warning. */ var2 = temp.bar; break; /* ... */ } }The only exceptions to this are:
Now I conceed that #3 is quite possible... such a design makes me uncomfortable, but it doesn't seem to make the kernel hackers uncomfortable as they happily use side-effects and gotos etc all over the place :)
But a simple refactoring like that which gets rid of the warning also (to me at least) makes the code more readable and more maintainable.
hmmm......
In this case it's not so simple.
It seems "obvious" that one might try to sink the code into the switch/case statement, but that doesn't work in this situation.
Some more detail may make it clear why you can't refactor as you suggest. Here's the "if (cmd ...)" portion:
if(cmd == IPC_SET) { if(copy_semid_from_user (&setbuf, arg.buf, version)) return -EFAULT; if ((err = audit_ipc_perms(0, setbuf.uid, setbuf.gid, setbuf.mode))) return err; }
Here's the main landmine in the common code:
sma = sem_lock(semid);
I guess the main reason you don't want to sink the code below the lock is that copy_semid_from_user() calls copy_from_user(), which could sleep. You don't want to sleep with a lock held.
Oh, and here's a better link to that function. Somehow I goofed when I pasted in the link before and omitted the "#L796" which takes you right to the function.
ah... figured it couldn't be that simple!
Thanks for the extra info. Landmine indeed. Ouch.
It's been a while since I've worked with non-OO code and I have to say that it's pretty damned insane looking code. I know they have their reasons for writing the code in that way and they find it quite easy to read and maintainable.... but so many exit points from the function and jumping through the code with gotos just blows my mind. I'm just glad they're comfortable with it all!
I have sympathy for gcc not understanding what's initialised and what is not...!
Gotos, etc.
I understand the need for the gotos... they need to make all these tests with the lock held, and release the lock on the failing path.
This is where C could benefit from nested functions: You could have written this with "return (error value);" everywhere you see "err = (error value); goto out_unlock;" and have the outer function do the locking/unlocking as simply as:
sma = sem_lock(semid); if (!sma) return -EINVAL; err = inner_function(); if (cmd != IPC_RMID) sem_unlock(semid); return err;
Unfortunately, full nested function support requires trampolines, etc. GCC's extension puts that as executable code on the stack... No thank you. I really just want an "exit from block" that works on any compound statement, not just loops and switch-case. (A multilevel break would work nicely.)
If I were to make any changes to semctl_down, I'd put "return 0" in place of "break;" in the case IPC_RMID: code, and eliminate the sem_unlock() calls in the other cases and the return err; after the switch statement. EVERY path in that function after taking the lock ends up doing "sem_unlock(); return err;" except one, so they may as well use the same code. I'm not even sure why they used a switch-case there--they could have done as well with if/else, and maybe avoided the warning.
The compiler simply can't dec
The compiler simply can't decide if a variable is initialized in all possible cases, and a false warning should be better then it keeping silent. Altough it might make your code less succint to help the compiler allong.
Expecting gcc to get a DWYM algorithm in its warnings engine isn't a realistic way of fixing this.
Being nice for the compiler isn't a bad thing, and improving S/N ration of the compiler output is a good thing for any project.
Only GCC 4 gives these warnings
> Expecting gcc to get a DWYM algorithm in its warnings engine isn't a realistic way of fixing this.
Hmm, gcc 3.x and 2.x didn't give such warnings. Only 4.x (4.0.3 and 4.1.0 tested) gives them (I know because I got them previously).
About S/N ratio, on i386 there are almost no warnings with gcc 3.x and only these bogus ones with 4.x; on x86_64 there are a bit more, which are still bogus however (mainly because long, long long and int are the same thing but are considered as different - GCC would be right if we used them, but they occur as conditional expansion of types like sector_t and so on).
Hmm... Blame the Tree-SSA code?
Isn't GCC 4.x the version that cut over to the Tree-SSA based infrastructure? It wouldn't surprise me too much if some of the analysis passes weren't as strong as in previous GCCs yet, and would agree w/ the kernel devs that this is a regression.
GCC problem
Yep. Hit it many times. GCC can be pretty silly about warnings. Unused variable is probably top offender. And from report it looks like GCC doesn't do yet cross check of variables. I used to workaround that warning by splitting functions in two - one doing checks and another doing the job. But most of the times the workaroud is very superfluous, usable for small parts of the crucial code.
Most annoying part is that some warnings are optimization dependent. IOW, if you make debug builds with -O0 no warnings would be logged. Then for release you do -O2 which spills bunch of new stuff you might have missed during development.
I think GCC people can try improving -Wall by running all optimizations regardless of how -O is set - but the results of optimization would be emmited regarding -O option.
That'd be kinda silly
Two very common reasons for running with less optimization during debug are: Less reordering of code by the compiler, and faster compile time. A lot of people want that faster compile time. If you ran full optimizations, throwing away the result, you'd lose that.
Honestly, you should be doing periodic development builds at full optimization. Subtly broken code may continue to function with optimization disabled and then break when you turn optimization back on.
I personally do all my development at full optimization, and only lower it if I can't follow what the debugger's telling me.
Episode 2: The return of Al Viro
An idea
What about lint-style comments (e.g. /*NOTREACHED*/ and the link) that can give a nod to the compiler, saying "Yeah, I know about this one; shaddup already?" Such things tend to be smaller in scope and much more obvious to casual observers than "tweaks" to the code to get the compiler to shut up.
FWIW, these sorts of problems go beyond GCC. Our compiler at work is based around the EDG front end which offers three levels of diagnostic: Remarks, Warnings and Errors. By default, it'll bring warnings out on all sorts of wacky stuff that is otherwise innocuous.
It's strict enough that it also offers flags for specifically suppressing certain remarks and warnings by number. We offer that since some shops hold as a development rule: Thou shalt not ship code which compiles with warnings. As an out, they can suppress meaningless warnings and still have something they can ship.