[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

[Timur Tabi]

Linus Torvalds wrote:

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

If 'a' could be signed on some architectures and unsigned on others, 
then I agree that "a < 0" is not silly.  But if it's always going to be 
unsigned, then I can't see how it's not silly.

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

Wouldn't something like "sizeof(pid_t) > 2" be a better test?  It 
certainly would be a lot easier to understand than comparing with 0xffff.

-- 
Timur Tabi
Staff Software Engineer
timur.tabi@ammasso.com

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.

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

[Timur Tabi]

Jesper Juhl wrote:

> if pid_t is 16 bit, then the value can never be greater than 0xffff but, 
> if pid_t is greater than 16 bit, say 32 bit, then the argument "a" could 
> very well contain a value greater than 0xffff and then the comparison 
> makes perfect sense.

If pid_t is 32-bit, then what's wrong with the value being greater than 
0xFFFF?  After all, if pid_t a 32-bit number, that implies that 32-bit 
values are acceptable.

-- 
Timur Tabi
Staff Software Engineer
timur.tabi@ammasso.com

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

[Timur Tabi]

Jesper Juhl wrote:

> My understanding of it is that it was just an example of how code that 
> generated warnings about limited range of datatype could actually be 
> perfectly fine.

But if the example doesn't make any sense, then how does it prove the point?

-- 
Timur Tabi
Staff Software Engineer
timur.tabi@ammasso.com

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

[Jesper Juhl]

On Tue, 23 Nov 2004, Timur Tabi wrote:

> Linus Torvalds wrote:
> 
> > which might warn on an architecture where "pid_t" is just sixteen bits wide.
> > Does that make the code wrong? Hell no.
> 
> Wouldn't something like "sizeof(pid_t) > 2" be a better test?  It certainly
> would be a lot easier to understand than comparing with 0xffff.
> 
That was not the point of the example Linus gave.
The example Linus gave was a function taking a pid_t argument and then 
comparing the value of the argument passed against 0xffff - the /value/ of 
the pid_t argument passed, not the size of the datatype.

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

if pid_t is 16 bit, then the value can never be greater than 0xffff but, 
if pid_t is greater than 16 bit, say 32 bit, then the argument "a" could 
very well contain a value greater than 0xffff and then the comparison 
makes perfect sense. So, while you'd get a warning on architectures where 
pid_t is 16bit or less you won't get a warning when pid_t is greater than 
16 bit. "fixing" that warning would clearly be wrong, no argument about 
that.


--
Jesper Juhl

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:
> >
> > 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.
> 
That makes sense. I'll try and ignore (b).

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

That was the main thing, yes. Of course I may make mistakes and end up 
posting patches that are less than perfect, but review and fix bugs is 
my intent, building with -W is merely a way for me to find relevant bits 
to review.

But enough of this, I understand your views on the issue and thank you for 
your examples, now I'll focus on the code and see if it results in a few 
patches :)


--
Jesper Juhl

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

[Jesper Juhl]

On Tue, 23 Nov 2004, Timur Tabi wrote:

> Jesper Juhl wrote:
> 
> > if pid_t is 16 bit, then the value can never be greater than 0xffff but, if
> > pid_t is greater than 16 bit, say 32 bit, then the argument "a" could very
> > well contain a value greater than 0xffff and then the comparison makes
> > perfect sense.
> 
> If pid_t is 32-bit, then what's wrong with the value being greater than
> 0xFFFF?  After all, if pid_t a 32-bit number, that implies that 32-bit values
> are acceptable.
> 
My understanding of it is that it was just an example of how code that 
generated warnings about limited range of datatype could actually be 
perfectly fine.


--
Jesper Juhl

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

[Jesper Juhl]

On Tue, 23 Nov 2004, Timur Tabi wrote:

> Jesper Juhl wrote:
> 
> > My understanding of it is that it was just an example of how code that
> > generated warnings about limited range of datatype could actually be
> > perfectly fine.
> 
> But if the example doesn't make any sense, then how does it prove the point?
> 
How about this then; let's say that pid_t can be either 16 or 32 bits, and 
for some reason you want to handle something special if the value is 
greater than 0xffffff (or some other arbitrary value between 0xffff and 
0xffffffff), obviously that can only happen in the cases where pid_t is 32 
bit and never when it is only 16 bit, so code like

int foo(pid_t a) {
	if (a > 0xffffff) {
		...
	}
}

will generate warnings when pid_t is only 16 bit, but not when it is 32 
bit, but the code will still do the correct thing in every case and is 
perfectly OK.

--
Jesper Juhl

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

[Valdis.Kletnieks]

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

On Tue, 23 Nov 2004 17:03:10 CST, Timur Tabi said:
> Jesper Juhl wrote:
> 
> > if pid_t is 16 bit, then the value can never be greater than 0xffff but, 
> > if pid_t is greater than 16 bit, say 32 bit, then the argument "a" could 
> > very well contain a value greater than 0xffff and then the comparison 
> > makes perfect sense.
> 
> If pid_t is 32-bit, then what's wrong with the value being greater than 
> 0xFFFF?  After all, if pid_t a 32-bit number, that implies that 32-bit 
> values are acceptable.

Try setting max_pid to 256K or so on an i386 - although the result fits
nicely in a pid_t with plenty of bits to spare, Very Bad Things happen.

Long thread starting at http://marc.theaimsgroup.com/?l=linux-kernel&m=109497978724424&w=2

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