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

[Richard Henderson]

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.


r~