[PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[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 <jesper.juhl@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;
 			}

Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[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

Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[Jesper Juhl]

On 28/11/06, Linus Torvalds <torvalds@osdl.org> wrote:
>
>
> 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".
>
Hehe, ok, I'll stop cleaning this stuff up then.
Nice little hobby out the window there ;)

-- 
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: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[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

Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[Jesper Juhl]

On 29/11/06, Linus Torvalds <torvalds@osdl.org> wrote:
>
>
> 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 <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: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[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

Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()

[Jesper Juhl]

On 29/11/06, Linus Torvalds <torvalds@osdl.org> wrote:
>
>
> 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.
>

I can't really say anything else at this point but, point conceded...

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

Friends dont let friends use GCC -W (was Re: [PATCH] Don't compare unsigned

[Valdis.Kletnieks]

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

On Tue, 28 Nov 2006 23:17:13 +0100, Jesper Juhl said:
> 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.



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