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

[Peter Maydell <peter.maydell@linaro.org>]

On 2 November 2015 at 14:48, Paolo Bonzini <pbonzini@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?

thanks
-- PMM