Re: [Qemu-devel] [PATCH] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

On 04/11/2015 11:12, Richard Henderson wrote:
> On 11/02/2015 04:13 PM, Peter Maydell wrote:
>> On 2 November 2015 at 14:48, Paolo Bonzini <address@hidden> wrote:
>>>
>>>
>>> On 02/11/2015 15:09, Peter Maydell wrote:
>>>>>> diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
>>>>>> index 383cc8b..45fc7db 100644
>>>>>> --- a/target-sparc/vis_helper.c
>>>>>> +++ b/target-sparc/vis_helper.c
>>>>>> @@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr,
>>>>>> uint64_t rs2)
>>>>>>       for (word = 0; word < 2; word++) {
>>>>>>           uint32_t val;
>>>>>>           int32_t src = rs2 >> (word * 32);
>>>>>> -        int64_t scaled = src << scale;
>>>>>> +        int64_t scaled = (int64_t)src << scale;
>>>>>>           int64_t from_fixed = scaled >> 16;
>>>> This will now shift left into the sign bit of a signed integer,
>>>> which is undefined behaviour.
>>>
>>> Why "now"?  It would have done the same before.
>>
>> True, but I was reviewing the new code rather than the
>> code you were taking away :-)
>>
>> Incidentally, that manual says the fpackfix and fpack32 insns
>> use a 4 bit GSR.scale_factor value, but our code is masking
>> by 0x1f in helper_fpack32 and helper_fpackfix. Which is right?
> 
> The 2011 manual has 5 bits for fpack32 and fpackfix; fpack16 uses only 4
> bits.
> 
> I do think we'd be better served by casting to uint64_t on that line. 
> Note that fpackfix requires the same correction.  And it wouldn't hurt
> to cast to uint32_t in fpack16, lest we anger the self-same shifting gods.

Hmmm.. say src = -0x80000000, scale = 1;

scaled     = (uint64_t)-0x8000000 << 1 = 0xffffffff00000000
from_fixed = 0xffffffff00000000 >> 16  = 0x0000ffffffff0000

Now from_fixed is positive and you get 32767 instead of -32768.  In
other words, we would have to cast to uint64_t on the scaled assignment,
and back to int64_t on the from_fixed assignment.  I must be
misunderstanding your suggestion.

Paolo