On 02/11/2015 16:13, Peter Maydell wrote:
> On 2 November 2015 at 14:48, Paolo Bonzini <pbonz...@redhat.com> 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?
I don't know... That manual even says that GSR has no bit defined above
bit 6 (where scale_factor is 3 to 6).
It's possible that a newer processor defines a 5-bit scale factor; I
honestly have no idea.
Paolo
