[PATCH] tmscsim: Fix big left-shifts of unsigned char

[PATCH] tmscsim: Fix big left-shifts of unsigned char

[Roel Kluin]

Shifting of u8 causes promotion to signed 'int'

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
I believe this was not intentionally?
see http://osdir.com/ml/git/2009-06/msg01347.html

diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index 9a42734..09a3c12 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -894,7 +894,7 @@ din_1:
 	    if (!i) printk (KERN_ERR "DC390: DMA Blast aborted unfinished!\n");
 	    //DC390_write8 (DMA_Cmd, READ_DIRECTION+DMA_IDLE_CMD);
 	    dc390_laststatus &= ~0xff000000;
-	    dc390_laststatus |= bval << 24;
+	    dc390_laststatus |= (unsigned)bval << 24;
 
 	    DEBUG1(printk (KERN_DEBUG "Blast: Read %i times DMA_Status %02x", 0xa000-i, bval));
 	    ResidCnt = (((u32) DC390_read8 (CtcReg_High) << 16) |

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[James Bottomley]

On Thu, 2009-06-18 at 14:14 +0200, Roel Kluin wrote:
> -           dc390_laststatus |= bval << 24;
> +           dc390_laststatus |= (unsigned)bval << 24;

bval is already u8, thus unsigned, so the cast is a nop.

James

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[Matthew Wilcox]

On Thu, Jun 18, 2009 at 09:03:24AM -0400, James Bottomley wrote:
> On Thu, 2009-06-18 at 14:14 +0200, Roel Kluin wrote:
> > -           dc390_laststatus |= bval << 24;
> > +           dc390_laststatus |= (unsigned)bval << 24;
> 
> bval is already u8, thus unsigned, so the cast is a nop.

Err ... I believe you're pedantically uncorrect, though right in practice.

The (unsigned) cast is short for (unsigned int) -- it doesn't mean
'cast to an unsigned version of the type that it already has'.

Now, in practice what happens is:

6.5.7:

The integer promotions are performed on each of the operands. The type
of the result is that of the promoted left operand.

so bval would be promoted from an unsigned char to a signed int, and
then shifted left by 24 bits, resulting in a potentially negative number.
But dc390_laststatus is an u32, so the |= converts this negative number
into a positive one, leading to the same answer that would have been
carried out in unsigned arithmetic.

So you're right (the cast isn't needed) for the wrong reason ;-)

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[Boaz Harrosh]

On 06/18/2009 04:16 PM, Matthew Wilcox wrote:
> On Thu, Jun 18, 2009 at 09:03:24AM -0400, James Bottomley wrote:
>> On Thu, 2009-06-18 at 14:14 +0200, Roel Kluin wrote:
>>> -           dc390_laststatus |= bval << 24;
>>> +           dc390_laststatus |= (unsigned)bval << 24;
>> bval is already u8, thus unsigned, so the cast is a nop.
> 
> Err ... I believe you're pedantically uncorrect, though right in practice.
> 
> The (unsigned) cast is short for (unsigned int) -- it doesn't mean
> 'cast to an unsigned version of the type that it already has'.
> 
> Now, in practice what happens is:
> 
> 6.5.7:
> 
> The integer promotions are performed on each of the operands. The type
> of the result is that of the promoted left operand.
> 

Note the left operand, not the lvalue.

> so bval would be promoted from an unsigned char to a signed int, and
> then shifted left by 24 bits, resulting in a potentially negative number.
> But dc390_laststatus is an u32, so the |= converts this negative number
> into a positive one, leading to the same answer that would have been
> carried out in unsigned arithmetic.
> 

It could get dangerous in 64bit integer machines if the negative number
gets truncated to 32bit while converted. So if the bval was > 127
and we get a 64bit signed negative number what will happen then?

> So you're right (the cast isn't needed) for the wrong reason ;-)
> 

Are you sure. It looks we need the cast
Boaz

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[Matthew Wilcox]

On Thu, Jun 18, 2009 at 04:27:37PM +0300, Boaz Harrosh wrote:
> > The (unsigned) cast is short for (unsigned int) -- it doesn't mean
> > 'cast to an unsigned version of the type that it already has'.
> > 
> > Now, in practice what happens is:
> > 
> > 6.5.7:
> > 
> > The integer promotions are performed on each of the operands. The type
> > of the result is that of the promoted left operand.
> > 
> 
> Note the left operand, not the lvalue.

Yes, but the *promoted* left operand.  Look at 6.3.1 again, and you'll
see an unsigned char is promoted to int.

> > so bval would be promoted from an unsigned char to a signed int, and
> > then shifted left by 24 bits, resulting in a potentially negative number.
> > But dc390_laststatus is an u32, so the |= converts this negative number
> > into a positive one, leading to the same answer that would have been
> > carried out in unsigned arithmetic.
> 
> It could get dangerous in 64bit integer machines if the negative number
> gets truncated to 32bit while converted. So if the bval was > 127
> and we get a 64bit signed negative number what will happen then?

For one, Linux doesn't support an ILP64 model.  I32LP64 and ILP32 are
the only two models supported.

If we assume support for I64, then the value 255 gets promoted to be a
64-bit signed integer, and then shifted left by 24 bits.  The sign bit
is way up at 63, so it doesn't become negative.

Then it'll be ORed with the u32.  The u32 gets promoted to an s64 for
the operation, the OR is done in 64-bit arithmetic, then truncated back
down to u32 for the assignment.  Again, I don't see the problem.

> > So you're right (the cast isn't needed) for the wrong reason ;-)
> 
> Are you sure. It looks we need the cast

I'm going to need to see a better argument than that before I'm convinced ;-)

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[Roel Kluin]

Op 18-06-09 14:14, Roel Kluin schreef:
> Shifting of u8 causes promotion to signed 'int'
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> I believe this was not intentionally?
> see http://osdir.com/ml/git/2009-06/msg01347.html
 ResidCnt = (((u32) DC390_read8 (CtcReg_High) << 16) |

Maybe my patch is not right. I cannot reproduce it with:

#include <stdio.h>

int main()
{
	unsigned long size = 0;
	unsigned char c = 1;

	size += c << 24;

	printf("%lu\n", size);
	printf("%lu\n", 1 << 24);
	printf("%u\n", c);
	return 0;
}

[roel@zoinx linux-git]$ ./a.out
16777216
16777216
1

Roel

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[James Bottomley]

On Thu, 2009-06-18 at 07:36 -0600, Matthew Wilcox wrote:
> For one, Linux doesn't support an ILP64 model.  I32LP64 and ILP32 are
> the only two models supported.

Heh ... embedded is easy to forget.  I believe, for reasons I have had
explained at length but understandably don't wish to go through again,
Sony has an IP32L64 model for the PS3 which causes all sorts of erm
problems.

James

Re: [PATCH] tmscsim: Fix big left-shifts of unsigned char

[Matthew Wilcox]

On Thu, Jun 18, 2009 at 02:45:42PM +0000, James Bottomley wrote:
> On Thu, 2009-06-18 at 07:36 -0600, Matthew Wilcox wrote:
> > For one, Linux doesn't support an ILP64 model.  I32LP64 and ILP32 are
> > the only two models supported.
> 
> Heh ... embedded is easy to forget.  I believe, for reasons I have had
> explained at length but understandably don't wish to go through again,
> Sony has an IP32L64 model for the PS3 which causes all sorts of erm
> problems.

And they compile the kernel in that mode?  I'd not heard of that before.
Not that it makes any difference to this case ... but I suppose we
mostly assume that a pointer fits in an unsigned long, and rarely assume
vice-versa.

I know Windows supports an IL32P64 mode, and IRIX had an ILP64 mode,
but this is the first I've heard of an IP32L64 mode.  There's no limits
to the craziness some people will do, I suppose.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."