Skip to contentSkip to conversation

tmscsim: Fix big left-shifts of unsigned char

PATCH In review Roel Kluin 4 participants 8 messages Jun 18, 2009 SCSI

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 tmscsim: Fix big left-shifts of unsigned char

Shifting of u8 causes promotion to signed 'int'

Signed-off-by Roel Kluin <[email protected]>

---
I believe this was not intentionally?
see http://osdir.com/ml/git/2009-06/msg01347.html

drivers/scsi/tmscsim.c +1 -1
index 9a42734..09a3c12 100644
@@ -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) |
author
#1 lore raw

Op 18-06-09 14:14, Roel Kluin schreef:

Quoted 6 lines

Shifting of u8 causes promotion to signed 'int'

Signed-off-by Roel Kluin <[email protected]>

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

maintainer
#2 lore raw
In reply to Roel Kluin

drivers/scsi/tmscsim.c din_1: · line 897

Quoted 2 lines · Roel Kluin
drivers/scsi/tmscsim.c +1 -1
-           dc390_laststatus |= bval << 24;
+           dc390_laststatus |= (unsigned)bval << 24;

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

James

#3 lore raw
Quoted 5 lines · James Bottomley

drivers/scsi/tmscsim.c din_1: · line 897

Quoted 2 lines · Roel Kluin
drivers/scsi/tmscsim.c +1 -1
-           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 ;-)

Signature · 4 lines
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."
#4 lore raw
Quoted 11 lines · Matthew Wilcox
Quoted 4 lines · James Bottomley

drivers/scsi/tmscsim.c din_1: · line 897

Quoted 2 lines · Roel Kluin
drivers/scsi/tmscsim.c +1 -1
-           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'.

Quoted 7 lines

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.

Quoted 6 lines

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?

Quoted 2 lines

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

Are you sure. It looks we need the cast
Boaz

#5 lore raw
Quoted 5 lines · Boaz Harrosh
Quoted 5 lines

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:

Quoted 7 lines
Quoted 5 lines

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.

Quoted 9 lines
Quoted 5 lines

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.

Quoted 3 lines
Quoted 1 line

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 ;-)

Signature · 4 lines
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."
maintainer
#6 lore raw
Quoted 2 lines · Matthew Wilcox

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

#7 lore raw
Quoted 8 lines · James Bottomley
Quoted 2 lines · Matthew Wilcox

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.

Signature · 4 lines
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."