Re: [PULL 05/20] target/riscv: Check nanboxed inputs in trans_rvf.inc.c

[Alistair Francis]

On Thu, Aug 13, 2020 at 9:48 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/13/20 7:46 AM, Alistair Francis wrote:
> >> Hi Alistair,
> >>
> >> As Chih-Min said, it's wrong here.  He has given the correct patch code
> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg728540.html
> >>
> >> We can either  squash the code to this patch or add an separate patch
> >> later. I prefer the former.
> >> Thanks very much.
> >
> > Richard are you ok if I squash this diff into the patch and send a PR v2?
> >
> > diff --git a/target/riscv/insn_trans/trans_rvf.inc.c
> > b/target/riscv/insn_trans/trans_rvf.inc.c
> > index f9a9e0643a..76f281d275 100644
> > --- a/target/riscv/insn_trans/trans_rvf.inc.c
> > +++ b/target/riscv/insn_trans/trans_rvf.inc.c
> > @@ -201,7 +201,8 @@ static bool trans_fsgnjn_s(DisasContext *ctx,
> > arg_fsgnjn_s *a)
> >           * This formulation retains the nanboxing of rs1.
> >           */
> >          mask = tcg_const_i64(~MAKE_64BIT_MASK(31, 1));
> > -        tcg_gen_andc_i64(rs2, mask, rs2);
> > +        tcg_gen_not_i64(rs2, rs2);         // forget to inverse rs2
> > +        tcg_gen_andc_i64(rs2, rs2, mask);  //mask needs to be
>
> Ah, well.  Yes, it's a bug.  However,
>
>    ~rs2 & ~mask
> = ~(rs2 | mask)
>
> so a better fix could be
>
> -    tcg_gen_andc_i64(rs2, mask, rs2);
> +    tcg_gen_nor_i64(rs2, rs2, mask);

Fixed.

Alistair

>
>
> As an aside, I think perhaps I should have added a ppc-style rotate-and-insert
> primitive to handle this sort of bitfield insert, since the best set of host
> insns to perform this operation, when the start of the field is not bit 0, is
> difficult to predict from the translator.
>
>
> r~