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

[Matthew Wilcox <matthew@wil.cx>]

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