Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
On 29/11/06, Linus Torvalds <[EMAIL PROTECTED]> 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 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 <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
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 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
On 29/11/06, Linus Torvalds <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl()
On 28/11/06, Linus Torvalds <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Don't compare unsigned variable for <0 in sys_prctl()
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 <[EMAIL PROTECTED]>
---
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Don't compare unsigned variable for 0 in sys_prctl()
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 [EMAIL PROTECTED]
---
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for 0 in sys_prctl()
On 28/11/06, Linus Torvalds [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for 0 in sys_prctl()
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for 0 in sys_prctl()
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for 0 in sys_prctl()
On 29/11/06, Linus Torvalds [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for 0 in sys_prctl()
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Don't compare unsigned variable for 0 in sys_prctl()
On 29/11/06, Linus Torvalds [EMAIL PROTECTED] 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 [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
