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

[Boaz Harrosh <bharrosh@panasas.com>]

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