[PATCH] Don't compare unsigned variable for <0 in sys_prctl()
Jesper Juhl
In kernel/sys.c::sys_prctl() the argument named 'arg2' is very clearly
of type 'unsigned long', and when compiling with "gcc -W" gcc also warns :
kernel/sys.c:2089: warning: comparison of unsigned expression < 0 is always false
So this patch removes the test of "arg2 < 0".
For those of us who compile their kernels with "-W" this gets rid of an
annoying warning. For the rest of you it saves a few bytes of source code ;-)
Signed-off-by: Jesper Juhl <jespe...@gmail.com>
---
diff --git a/kernel/sys.c b/kernel/sys.c
index 98489d8..086ea37 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2086,7 +2086,7 @@ asmlinkage long sys_prctl(int option, un
error = current->mm->dumpable;
break;
case PR_SET_DUMPABLE:
- if (arg2 < 0 || arg2 > 1) {
+ if (arg2 > 1) {
error = -EINVAL;
break;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Linus Torvalds
On Tue, 28 Nov 2006, Jesper Juhl wrote:
>
> In kernel/sys.c::sys_prctl() the argument named 'arg2' is very clearly
> of type 'unsigned long', and when compiling with "gcc -W" gcc also warns :
> kernel/sys.c:2089: warning: comparison of unsigned expression < 0 is always false
>
> So this patch removes the test of "arg2 < 0".
No, we don't do this.
This is why we don't compile with "-W". Gcc is crap.
The fact is, if it's unsigned, it's not something that the programmer
should have to care about. We should write our code to be readable and
obviously safe, and that means that
if (x < 0 || x > MAX)
return -ERROR;
is the _right_ way to do things, without having to carry stupid context
around in our heads.
If the compiler (whose _job_ it is to carry all that context and use it to
generate good code) notices that the fact that "x" is unsignes means that
one of the tests is unnecessary, that does not make it wrong.
Gcc warns for a lot of wrong things. This is one of them.
Friends don't let friends use "-W".
Linus
Jesper Juhl
>
>
> On Tue, 28 Nov 2006, Jesper Juhl wrote:
> >
> > In kernel/sys.c::sys_prctl() the argument named 'arg2' is very clearly
> > of type 'unsigned long', and when compiling with "gcc -W" gcc also warns :
> > kernel/sys.c:2089: warning: comparison of unsigned expression < 0 is always false
> >
> > So this patch removes the test of "arg2 < 0".
>
> No, we don't do this.
>
> This is why we don't compile with "-W". Gcc is crap.
>
> The fact is, if it's unsigned, it's not something that the programmer
> should have to care about. We should write our code to be readable and
> obviously safe, and that means that
>
> if (x < 0 || x > MAX)
> return -ERROR;
>
> is the _right_ way to do things, without having to carry stupid context
> around in our heads.
>
> If the compiler (whose _job_ it is to carry all that context and use it to
> generate good code) notices that the fact that "x" is unsignes means that
> one of the tests is unnecessary, that does not make it wrong.
>
> Gcc warns for a lot of wrong things. This is one of them.
>
> Friends don't let friends use "-W".
>
Nice little hobby out the window there ;)
--
Jesper Juhl <jespe...@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
Valdis.K...@vt.edu
> In kernel/sys.c::sys_prctl() the argument named 'arg2' is very clearly
> of type 'unsigned long', and when compiling with "gcc -W" gcc also warns :
> kernel/sys.c:2089: warning: comparison of unsigned expression < 0 is always false
>
> So this patch removes the test of "arg2 < 0".
For those playing along at home - often the bug (not here though) is it should
have been a *signed* long, because arg2 could be passed from some other
function that returns negative numbers on error? Remember that papering over
a bug is a bad idea...
On Tue, 28 Nov 2006 14:27:51 PST, Linus Torvalds said:
> The fact is, if it's unsigned, it's not something that the programmer
> should have to care about. We should write our code to be readable and
> obviously safe, and that means that
Unfortunately, it's *easy* for GCC to determine that Something Odd has
happened. Either the variable was made unsigned in error, or the test is in
error. However, there's no real way for the compiler to know which was
intended. And let's face it - we've had enough of our share of bugs *both*
ways, which is why GCC emits a warning.
Linus Torvalds
On Tue, 28 Nov 2006, Jesper Juhl wrote:
>
> > Friends don't let friends use "-W".
>
> Hehe, ok, I'll stop cleaning this stuff up then.
> Nice little hobby out the window there ;)
You might want to look at some of the other warnings gcc spits out, but
this class isn't one of them.
Other warnings we have added over the years (and that really _are_ good
warnings) have included the "-Wstrict-prototypes", and some other ones.
If you can pinpoint _which_ gcc warning flag it is that causes gcc to emit
the bogus ones, you _could_ try "-W -Wno-xyz-warning", which should cause
gcc to enable all the "other" warnings, but then not the "xyz-warning"
that causes problems.
Of course, there is often a reason why a warning is in "-W" but not in
"-Wall". Most of the time it's sign that the warning is bogus. Not always,
though - we do tend to want to be fairly strict, and Wstrict-prototypes is
an example of a _good_ warning that is not in -Wall.
Linus
Jesper Juhl
>
>
> On Tue, 28 Nov 2006, Jesper Juhl wrote:
> >
> > > Friends don't let friends use "-W".
> >
> > Hehe, ok, I'll stop cleaning this stuff up then.
> > Nice little hobby out the window there ;)
>
> You might want to look at some of the other warnings gcc spits out, but
> this class isn't one of them.
>
> Other warnings we have added over the years (and that really _are_ good
> warnings) have included the "-Wstrict-prototypes", and some other ones.
>
> If you can pinpoint _which_ gcc warning flag it is that causes gcc to emit
> the bogus ones, you _could_ try "-W -Wno-xyz-warning", which should cause
> gcc to enable all the "other" warnings, but then not the "xyz-warning"
> that causes problems.
>
> Of course, there is often a reason why a warning is in "-W" but not in
> "-Wall". Most of the time it's sign that the warning is bogus. Not always,
> though - we do tend to want to be fairly strict, and Wstrict-prototypes is
> an example of a _good_ warning that is not in -Wall.
>
I would venture that "-Wshadow" is another one of those. I've, in the
past, submitted quite a few patches to clean up shadow warnings (some
accepted, some not) and I'll probably try going down that path again
in the near future. It's a class of warnings that have the potential
to uncover real bugs (even if we don't currently have any) and it
would be a nice one to be able to enable by default in the Makefile.
I agree with you though that the "expression always false|true due to
unsigned" type of warnings are usually bogus - although there have
actually been real bugs hiding behind some of those warnings in the
past. But, I'll make sure to only submit patches for that type of
warnings in the future if I can prove that the warning actually
uncovered a real bug.
--
Jesper Juhl <jespe...@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
Linus Torvalds
On Wed, 29 Nov 2006, Jesper Juhl wrote:
>
> I would venture that "-Wshadow" is another one of those.
I'd agree, except for the fact that gcc does a horribly _bad_ job of
-Wshadow, making it (again) totally unusable.
For example, it's often entirely interesting to hear about local variables
that shadow each other. No question about it.
HOWEVER. It's _not_ really interesting to hear about a local variable that
happens to have a common name that is also shared by a extern function.
There just isn't any room for confusion, and it's actually not even that
unusual - I tried using -Wshadow on real programs, and it was just
horribly irritating.
In the kernel, we had obvious things like local use of "jiffies" that just
make _total_ sense in a small inline function, and the fact that there
happens to be an extern declaration for "jiffies" just isn't very
interesting.
Similarly, with nested macro expansion, even the "local variable shadows
another local variable" case - that looks like it should have an obvious
warning on the face of it - really isn't always necessarily that
interesting after all. Maybe it is a bug, maybe it isn't, but it's no
longer _obviously_ bogus any more.
So I'm not convinced about the usefulness of "-Wshadow". ESPECIALLY the
way that gcc implements it, it's almost totally useless in real life.
For example, I tried it on "git" one time, and this is a perfect example
of why "-Wshadow" is totally broken:
diff-delta.c: In function 'create_delta_index':
diff-delta.c:142: warning: declaration of 'index' shadows a global declaration
(and there's a _lot_ of those). If I'm not allowed to use "index" as a
local variable and include <string.h> at the same time, something is
simply SERIOUSLY WRONG with the warning.
So the fact is, the C language has scoping rules for a reason. Can you
screw yourself by usign them badly? Sure. But that does NOT mean that the
same name in different scopes is a bad thing that should be warned about.
If I wanted a language that didn't allow me to do anything wrong, I'd be
using Pascal. As it is, it turns out that things that "look" wrong on a
local level are often not wrong after all.
Linus
Jesper Juhl
I can't really say anything else at this point but, point conceded...
--
Jesper Juhl <jespe...@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