[PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Jesper Juhl]

Hi,

This patch removes a pointless comparison. "arg" is an unsigned long, thus 
it can never be <0, so testing that is pointless.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -up linux-2.6.10-rc2-bk6-orig/fs/fcntl.c linux-2.6.10-rc2-bk6/fs/fcntl.c
--- linux-2.6.10-rc2-bk6-orig/fs/fcntl.c	2004-11-17 01:20:14.000000000 +0100
+++ linux-2.6.10-rc2-bk6/fs/fcntl.c	2004-11-21 23:49:20.000000000 +0100
@@ -340,7 +340,7 @@ static long do_fcntl(int fd, unsigned in
 		break;
 	case F_SETSIG:
 		/* arg == 0 restores default behaviour. */
-		if (arg < 0 || arg > _NSIG) {
+		if (arg > _NSIG) {
 			break;
 		}
 		err = 0;

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Matthew Wilcox]

On Sun, Nov 21, 2004 at 11:55:23PM +0100, Jesper Juhl wrote:
> This patch removes a pointless comparison. "arg" is an unsigned long, thus 
> it can never be <0, so testing that is pointless.
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
>  	case F_SETSIG:
>  		/* arg == 0 restores default behaviour. */
> -		if (arg < 0 || arg > _NSIG) {
> +		if (arg > _NSIG) {
>  			break;

I've seen patches like this before.  I'm generally in favour of removing
the unnecessary test, but Linus rejected it on the grounds the compiler
shouldn't be warning about it and it's better to be more explicit about
the range test.  Maybe he's changed his mind between then and now ;-)

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Jesper Juhl]

Matthew Wilcox wrote:
> On Sun, Nov 21, 2004 at 11:55:23PM +0100, Jesper Juhl wrote:
> 
>>This patch removes a pointless comparison. "arg" is an unsigned long, thus 
>>it can never be <0, so testing that is pointless.
>>
>>Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
>>
>> 	case F_SETSIG:
>> 		/* arg == 0 restores default behaviour. */
>>-		if (arg < 0 || arg > _NSIG) {
>>+		if (arg > _NSIG) {
>> 			break;
> 
> 
> I've seen patches like this before.  I'm generally in favour of removing
> the unnecessary test, but Linus rejected it on the grounds the compiler
> shouldn't be warning about it and it's better to be more explicit about
> the range test.  Maybe he's changed his mind between then and now ;-)
> 
Let's find out.
Linus, would you accept patches like this?

I've been building recent kernels with -W and there are tons of places 
with similar comparisons like the one above, as well as places where 
signed and unsigned values are compared, places where values are 
potentially truncated in signed/unsigned assignments and similar.
At the very least a review of these code locations to make sure they are 
all safe would make sense, and I think it would also make sense to get 
rid of the comparisons that always evaluate true or false due to the 
signedness or range of datatypes.
I probably won't be able to properly evaluate/review *all* the instances 
of this in the kernel, but I don't mind spending some time reviewing 
what I can and submit patches, but I don't want to waste my time with it 
if you already know up-front that you'll just be dropping all such 
patches. Or is this up to the individual maintainers to accept/reject? 
If so, then I'll go ahead and submit patches, then the individual 
maintainers can ack/nack as they please.

Comments?


--
Jesper Juhl

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Jamie Lokier]

Jesper Juhl wrote:
> >>	case F_SETSIG:
> >>		/* arg == 0 restores default behaviour. */
> >>-		if (arg < 0 || arg > _NSIG) {
> >>+		if (arg > _NSIG) {
> >>			break;
> >
> Let's find out.

The unusual thing about this function is that "arg" is really
polymorphic, but given type "unsigned long" in the kernel.  It is
really a way to hold arbitrary values of any type.

Just look at the way it becomes "unsigned int" (dupfd) or "struct
flock" (lock) or "long" (leases) or "int" (setown).

F_SETOWN is interesting because you really can pass a negative int
argument and get a meaningful result, even though it's passed around
as unsigned long for a little while.

Signal numbers are usually "int".  The intended behaviour of fcntl(fd,
F_SETSIG, sig) from userspace is that a negative sig returns EINVAL.

I.e. writing fcntl(fd, F_SETSIG, -1) in userspace will compile without
any warnings.  The intended behaviour is that a negative sig returns
EINVAL.  The kernel code illustrates that intention.

It isn't obvious that arg is unsigned long in this function, when
reading the code.  I had to scroll to the top of the function to check
that this patch doesn't change its behaviour.  For that reason I think
the "< 0" test is useful, as it illustrates the intended behaviour and
causes no harm.

-- Jamie

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Linus Torvalds]

On Tue, 23 Nov 2004, Jesper Juhl wrote:
>
> Linus, would you accept patches like this?

No, please don't.

The warning is sometimes useful, but when it comes to a construct like

	if (a < 0 || a > X)

the fact that "a" is unsigned does not make the construct silly. First 
off, it's (a) very readable and (b) the type of "a" may not be immediately 
obvious if it's a user typedef, for example. 

In fact, the type of "a" might depend on the architecture, or even 
compiler flags. Think about "char" - which may or may not be signed 
depending on ABI and things like -funsigned-char.

In other places, it's not "unsigned" that is the problem, but the fact 
that the range of a type is smaller on one architecture than another. So 
you might have

	inf fn(pid_t a)
	{
		if (a > 0xffff)
			...
	}

which might warn on an architecture where "pid_t" is just sixteen bits 
wide. Does that make the code wrong? Hell no.

IOW, a lot of the gcc warnings are just not valid, and trying to shut gcc 
up about them can break (and _has_ broken) code that was correct before.

> I probably won't be able to properly evaluate/review *all* the instances 
> of this in the kernel,

It's not even that I will drop the patches, it's literally that "fixing" 
the code so that gcc doesn't complain can be a BUG. We've gone through 
that. 

		Linus

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Bryan Henderson]

>The unusual thing about this function is that "arg" is really
>polymorphic, but given type "unsigned long" in the kernel.  It is
>really a way to hold arbitrary values of any type.

As you've described it, what's wrong with this code is not that it tests 
arg < 0, but that it should cast arg to int before doing so:

  int signal_arg = (int) arg;
  if (signal_arg < 0) ...

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Linus Torvalds]

On Tue, 23 Nov 2004, Jesper Juhl wrote:
>
> Shutting up gcc is not the primary goal here, the goal is/was to  
> a) review the code and make sure that it is safe and correct, and fix it 
> when it is not.
> b) remove comparisons that are just a waste of CPU cycles when the result 
> is always true or false (in *all* cases on *all* archs).

Well, I'm convinced that (b) is unnecessary, as any compiler that notices 
the range thing enough to warn will also be smart enough to just remove 
the test internally.

But yes, as long as the thing is a "review and fix bugs" and not a quest 
to remove warnings which may well be compiler figments, that's obviously 
ok.

			Linus

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[Jesper Juhl]

On Tue, 23 Nov 2004, Linus Torvalds wrote:
> 
> 
> On Tue, 23 Nov 2004, Jesper Juhl wrote:
> >
> > Linus, would you accept patches like this?
> 
> No, please don't.
> 
> The warning is sometimes useful, but when it comes to a construct like
> 
> 	if (a < 0 || a > X)
> 
> the fact that "a" is unsigned does not make the construct silly. First 
> off, it's (a) very readable and (b) the type of "a" may not be immediately 
> obvious if it's a user typedef, for example. 
> 
> In fact, the type of "a" might depend on the architecture, or even 
> compiler flags. Think about "char" - which may or may not be signed 
> depending on ABI and things like -funsigned-char.
> 
> In other places, it's not "unsigned" that is the problem, but the fact 
> that the range of a type is smaller on one architecture than another. So 
> you might have
> 
> 	inf fn(pid_t a)
> 	{
> 		if (a > 0xffff)
> 			...
> 	}
> 
> which might warn on an architecture where "pid_t" is just sixteen bits 
> wide. Does that make the code wrong? Hell no.
> 
I'm aware that there are pitfalls, one of the very first things I looked 
at was the usage of FIRST_USER_PGD_NR in mm/mmap.c:1513 On my main 
platform (i386) FIRST_USER_PGD_NR is zero which causes gcc -W to warn 
about   if (start_index < FIRST_USER_PGD_NR)   but, after seeing that it 
is not 0 on all platforms I left that one alone.


> IOW, a lot of the gcc warnings are just not valid, and trying to shut gcc 
> up about them can break (and _has_ broken) code that was correct before.
> 
Shutting up gcc is not the primary goal here, the goal is/was to  
a) review the code and make sure that it is safe and correct, and fix it 
when it is not.
b) remove comparisons that are just a waste of CPU cycles when the result 
is always true or false (in *all* cases on *all* archs).


> > I probably won't be able to properly evaluate/review *all* the instances 
> > of this in the kernel,
> 
> It's not even that I will drop the patches, it's literally that "fixing" 
> the code so that gcc doesn't complain can be a BUG. We've gone through 
> that. 
> 
I'll keep that firmly in mind and only submit patches for these kind of 
things if I find usage that is actually (provably) buggy or where it's 
completely clear that a comparison will *always* be true or false on all 
architectures and removing it does not decrease readability.
I hope that's a more resonable aproach.

Whether or not the list of potential patches is now down to zero remains 
to be seen, but it just got a hell of a lot shorter. :)

Thank you for the feedback.


--
Jesper Juhl

Re: [PATCH] Remove pointless <0 comparison for unsigned variable in fs/fcntl.c

[linux-os]

On Tue, 23 Nov 2004, Linus Torvalds wrote:

>
>
> On Tue, 23 Nov 2004, Jesper Juhl wrote:
>>
>> Shutting up gcc is not the primary goal here, the goal is/was to
>> a) review the code and make sure that it is safe and correct, and fix it
>> when it is not.
>> b) remove comparisons that are just a waste of CPU cycles when the result
>> is always true or false (in *all* cases on *all* archs).
>
> Well, I'm convinced that (b) is unnecessary, as any compiler that notices
> the range thing enough to warn will also be smart enough to just remove
> the test internally.
>
> But yes, as long as the thing is a "review and fix bugs" and not a quest
> to remove warnings which may well be compiler figments, that's obviously
> ok.
>
> 			Linus
> -

There are some pretty scary macros like:
MIN(a, b) (a < b) ? a:b  You can throw any parentheses you want
around and it doesn't make it correct for selecting the lowest value
of the amount to read/write from a buffer. The a and b need to be cast
to unsigned before the comparison is made --and then some compilers
will (correctly, I'm told) compain about range.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by John Ashcroft.
                  98.36% of all statistics are fiction.