[PATCH] i386, nmi: signed vs unsigned mixup

[PATCH] i386, nmi: signed vs unsigned mixup

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


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 arch/i386/kernel/nmi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.15-rc1-git7-orig/arch/i386/kernel/nmi.c	2005-11-12 18:07:14.000000000 +0100
+++ linux-2.6.15-rc1-git7/arch/i386/kernel/nmi.c	2005-11-19 23:58:17.000000000 +0100
@@ -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) {

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Andrew Morton]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> 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.
> 
> ...
> 
> --- linux-2.6.15-rc1-git7-orig/arch/i386/kernel/nmi.c	2005-11-12 18:07:14.000000000 +0100
> +++ linux-2.6.15-rc1-git7/arch/i386/kernel/nmi.c	2005-11-19 23:58:17.000000000 +0100
> @@ -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) {
> 

-ETOOTRIVIAL.  The code as-is works OK, and we have these sorts of things
all over the tee.

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Jesper Juhl]

On 11/20/05, Andrew Morton <akpm@osdl.org> wrote:
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
> >
> > 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.
> >
> > ...
> >
> > --- linux-2.6.15-rc1-git7-orig/arch/i386/kernel/nmi.c 2005-11-12 18:07:14.000000000 +0100
> > +++ linux-2.6.15-rc1-git7/arch/i386/kernel/nmi.c      2005-11-19 23:58:17.000000000 +0100
> > @@ -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) {
> >
>
> -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 <jesper.juhl@gmail.com>
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

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Andrew Morton]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> > -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.

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Jesper Juhl]

On 11/20/05, Andrew Morton <akpm@osdl.org> wrote:
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
> >
> > > -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.
>
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 <jesper.juhl@gmail.com>
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

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Andrew Morton]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> 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?

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Jesper Juhl]

On 11/20/05, Andrew Morton <akpm@osdl.org> wrote:
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
> >
> > 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?
>
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 <jesper.juhl@gmail.com>
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

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Jesper Juhl]

On 11/20/05, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> On 11/20/05, Andrew Morton <akpm@osdl.org> wrote:
[snip]
> >
> > 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.
>

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 <jesper.juhl@gmail.com>
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

Re: [PATCH] i386, nmi: signed vs unsigned mixup

[Andrew Morton]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> 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.