i386, nmi: signed vs unsigned mixup
PATCH 1/1 i386, nmi: signed vs unsigned mixup
In arch/i386/kernel/nmi.c::nmi_watchdog_tick(), the variable `sum' is of type "int" but it's used to store the result of per_cpu(irq_stat, cpu).apic_timer_irqs which is an "unsigned int", it's also later compared to last_irq_sums[cpu] which is also an "unsigned int", so `sum' really ought to be unsigned itself. This small patch makes that change.
---
arch/i386/kernel/nmi.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletion(-)
arch/i386/kernel/nmi.c +2 -1
@@ -528,9 +528,10 @@ void nmi_watchdog_tick (struct pt_regs ** Since current_thread_info()-> is always on the stack, and we * always switch the stack NMI-atomically, it's safe to use * smp_processor_id(). */ -int sum, cpu =smp_processor_id (); +unsigned int sum; +int cpu =smp_processor_id (); sum =per_cpu (irq_stat, cpu).apic_timer_irqs;if (last_irq_sums[cpu] == sum) {
Quoted 15 lines · Jesper Juhl
In arch/i386/kernel/nmi.c::nmi_watchdog_tick(), the variable `sum' is of type "int" but it's used to store the result of
per_cpu(irq_stat, cpu).apic_timer_irqs which is an "unsigned int", it's also later compared to last_irq_sums[cpu] which is also an "unsigned int", so `sum' really ought to be unsigned itself. This small patch makes that change....
arch/i386/kernel/nmi.c +0
-0@@ -528,9 +528,10 @@ void nmi_watchdog_tick (struct pt_regs ** Since current_thread_info()-> is always on the stack, and we
arch/i386/kernel/nmi.c void nmi_watchdog_tick (struct pt_regs * · line 537
Quoted 10 lines
arch/i386/kernel/nmi.c +2
-1* always switch the stack NMI-atomically, it's safe to use * smp_processor_id(). */ -int sum, cpu =smp_processor_id (); +unsigned int sum; +int cpu =smp_processor_id (); sum =per_cpu (irq_stat, cpu).apic_timer_irqs;if (last_irq_sums[cpu] == sum) {
-ETOOTRIVIAL. The code as-is works OK, and we have these sorts of things all over the tee.
Quoted 22 lines · Andrew Morton
Quoted 21 lines · Jesper Juhl
In arch/i386/kernel/nmi.c::nmi_watchdog_tick(), the variable `sum' is of type "int" but it's used to store the result of
per_cpu(irq_stat, cpu).apic_timer_irqs which is an "unsigned int", it's also later compared to last_irq_sums[cpu] which is also an "unsigned int", so `sum' really ought to be unsigned itself. This small patch makes that change....
arch/i386/kernel/nmi.c 2005-11-19 23:58:17.000000000 +0100 renamed from arch/i386/kernel/nmi.c 2005-11-12 18:07:14.000000000 +0100 +2
-1@@ -528,9 +528,10 @@ void nmi_watchdog_tick (struct pt_regs * * Since current_thread_info()-> is always on the stack, and we * always switch the stack NMI-atomically, it's safe to use * smp_processor_id(). */ - int sum, cpu = smp_processor_id(); + unsigned int sum; + int cpu = smp_processor_id();
Quoted 8 lines
arch/i386/kernel/nmi.c 2005-11-19 23:58:17.000000000 +0100 void nmi_watchdog_tick (struct pt_regs * · line 537
Quoted 4 lines
arch/i386/kernel/nmi.c 2005-11-19 23:58:17.000000000 +0100 renamed from arch/i386/kernel/nmi.c 2005-11-12 18:07:14.000000000 +0100 +0
-0sum = per_cpu(irq_stat, cpu).apic_timer_irqs; if (last_irq_sums[cpu] == sum) {
-ETOOTRIVIAL. The code as-is works OK, and we have these sorts of things all over the tee.
Fair enough.
Would a patch to clean this sort of stuff up in bulk all over be of interrest or should I just leave it alone?
--
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
Quoted 8 lines · Jesper Juhl
Quoted 4 lines
-ETOOTRIVIAL. The code as-is works OK, and we have these sorts of things all over the tee.Fair enough.
Would a patch to clean this sort of stuff up in bulk all over be of interrest or should I just leave it alone?
Such a patchset would be pretty intrusive and it's not exactly trivial - at each site we need to decide whether we should be using signed or unsigned, then change one or the other, then do a full-scope check to see what the implications of that change are.
I think the two risks of signedness sloppiness are a) inadvertent or premature overflow and b) comparisons, where the signed quantity went negative.
Problem b) is more serious, and `gcc -Wsigned-compare' may be used to identify possible problems. There are quite a lot of places need checking, iirc.
Quoted 15 lines · Andrew Morton
Quoted 8 lines · Jesper Juhl
Quoted 4 lines
-ETOOTRIVIAL. The code as-is works OK, and we have these sorts of things all over the tee.Fair enough.
Would a patch to clean this sort of stuff up in bulk all over be of interrest or should I just leave it alone?
Such a patchset would be pretty intrusive and it's not exactly trivial - at each site we need to decide whether we should be using signed or unsigned, then change one or the other, then do a full-scope check to see what the implications of that change are.
Quoted 8 lines
I think the two risks of signedness sloppiness are a) inadvertent or premature overflow and b) comparisons, where the signed quantity went negative.
Problem b) is more serious, and `gcc -Wsigned-compare' may be used to identify possible problems. There are quite a lot of places need checking, iirc.
Ok, so does that mean that, if properly verified, patches for things that "gcc -Wsigned-compare" flags will be appreciated? I'll just restrict myself to that in that case.
--
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
Quoted 3 lines · Jesper Juhl
Ok, so does that mean that, if properly verified, patches for things
that "gcc -Wsigned-compare" flags will be appreciated?
All patches are appreciated, but not all are applied ;)
Sure, go for it - let's see what the patches end up looking like. We might find real bugs in there - I found a bunch of howlers back in 2.3.late. That was with `gcc -W' which turns on more than -Wsigned-compare.
Maybe you could prepare a quick overall summary first, see if you can work out the overall scope of the problem and then we can take a look at that, decide what bits to attack?
Quoted 7 lines · Andrew Morton
Quoted 3 lines · Jesper Juhl
Ok, so does that mean that, if properly verified, patches for things
that "gcc -Wsigned-compare" flags will be appreciated?All patches are appreciated, but not all are applied ;)
Quoted 8 lines
Sure, go for it - let's see what the patches end up looking like. We might find real bugs in there - I found a bunch of howlers back in 2.3.late. That was with `gcc -W' which turns on more than -Wsigned-compare.
Maybe you could prepare a quick overall summary first, see if you can work out the overall scope of the problem and then we can take a look at that, decide what bits to attack?
Sure, I'll do that tomorrow.
All these sign issues all over annoy me :)
I'll try to get a handle on the scope of the thing tomorrow, then send a mail with what I find, then we can talk about what would be useful to do.
--
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
Quoted 1 line · Jesper Juhl
On 11/20/05, Andrew Morton <[email protected]> wrote:
[snip]
Quoted 9 lines
Quoted 9 lines
Sure, go for it - let's see what the patches end up looking like. We might find real bugs in there - I found a bunch of howlers back in 2.3.late. That was with `gcc -W' which turns on more than -Wsigned-compare.
Maybe you could prepare a quick overall summary first, see if you can work out the overall scope of the problem and then we can take a look at that, decide what bits to attack?
Quoted 6 lines
Sure, I'll do that tomorrow.
All these sign issues all over annoy me :)
I'll try to get a handle on the scope of the thing tomorrow, then send a mail with what I find, then we can talk about what would be useful to do.
Ok, I did an allyesconfig build (x86 build) of 2.6.15-rc2 with -Wsign-compare and I have some numbers.
The total number of warnings for the build ended up at 14681 Of course some of the warnings are not related to sign issues, so let's look at just
comparison between signed and unsigned (12378 instances of this one)
signed and unsigned type in conditional expression (2109 instances
of this one)
comparison of promoted ~unsigned with constant (4 instances of this one)
So for the signedness related warnings we have a total of 14491 warnings. The 14491 warnings come from a total of 1701 files.
This looks like a lot, but it's really not as bad as it looks since a lot of the warnings are duplicates.
Let's take a look at the top 10 offenders :
1)
4078 counts of
warning: comparison between signed and unsigned
from include/asm/bitops.h:339
2)
1125 counts of
warning: comparison between signed and unsigned
from include/linux/netdevice.h:796
3)
256 counts of
warning: signed and unsigned type in conditional expression from drivers/scsi/aic7xxx/aic79xx_osm_pci.c:79
4)
256 counts of
warning: signed and unsigned type in conditional expression from drivers/scsi/aic7xxx/aic79xx_osm_pci.c:77
5)
120 counts of
warning: signed and unsigned type in conditional expression from fs/xfs/xfs_mount.h:458
6)
90 counts of
warning: comparison between signed and unsigned
from include/net/tcp_ecn.h:53
7)
90 counts of
warning: comparison between signed and unsigned
from include/net/tcp.h:866
8)
90 counts of
warning: signed and unsigned type in conditional expression from include/net/tcp.h:1148
9)
90 counts of
warning: signed and unsigned type in conditional expression from include/net/tcp.h:1143
10)
86 counts of
warning: comparison between signed and unsigned
from include/linux/fb.h:859
So, if we just cleaned up the top 10 offenders we'd get rid of 6281 out of the total 14491 warnings.
I've not yet looked at what would be involved in fixing these top 10, I'll look at that later, for now I just wanted to post the numbers I had.
--
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
Quoted 3 lines · Jesper Juhl
So, if we just cleaned up the top 10 offenders we'd get rid of 6281
out of the total 14491 warnings.
Yes, that's sane - it'll help make the warning output more manageable. I think the i386/bitops.h one is fixed in -mm.