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 <[email protected]>
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;
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 <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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
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
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
>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) ...
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
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
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
[email protected]
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.
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
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
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
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
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
[email protected]
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
[email protected]
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