Re: [Qemu-devel] [PULL v4 29/46] target/mips: Add emulation of DSP ASE for nanoMIPS - part 1

[Stefan Markovic]

On 16.10.18. 16:00, Peter Maydell wrote:
> On 23 August 2018 at 14:34, Aleksandar Markovic
> <address@hidden> wrote:
> > From: Stefan Markovic <address@hidden>
> >
> > Add emulation of DSP ASE instructions for nanoMIPS - part 1.
> >
> > Reviewed-by: Aleksandar Markovic <address@hidden>
> > Signed-off-by: Aleksandar Markovic <address@hidden>
> > Signed-off-by: Stefan Markovic <address@hidden>
> Hi. Coverity points out a bug in this patch (CID 1395627):
>
> > ---
> >   target/mips/translate.c | 554 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 554 insertions(+)
> >
> > diff --git a/target/mips/translate.c b/target/mips/translate.c
> > index 95632dd..d3635e7 100644
> > --- a/target/mips/translate.c
> > +++ b/target/mips/translate.c
> > @@ -18061,6 +18061,554 @@ static void gen_pool32f_nanomips_insn(DisasContext 
> > *ctx)
> >       }
> >   }
> >
> > +static void gen_pool32a5_nanomips_insn(DisasContext *ctx, int opc,
> > +                                       int rd, int rs, int rt)
> > +{
> [...]
>
> > +    case NM_SHRA_R_PH:
> > +        check_dsp(ctx);
> > +        tcg_gen_movi_tl(t0, rd >> 1);
> > +        switch (extract32(ctx->opcode, 10, 1)) {
> > +        case 0:
> > +            /* SHRA_PH */
> > +            gen_helper_shra_ph(v1_t, t0, v1_t);
> > +            break;
> > +            gen_store_gpr(v1_t, rt);
> This gen_store_gpr() call is unreachable because it
> is after the 'break'. Should the two lines be in the
> other order?


Yes, those two lines should be in the other order:

+        case 0:
+            /* SHRA_PH */
+            gen_helper_shra_ph(v1_t, t0, v1_t);
+            gen_store_gpr(v1_t, rt);
+            break;


> > +        case 1:
> > +            /* SHRA_R_PH */
> > +            gen_helper_shra_r_ph(v1_t, t0, v1_t);
> > +            gen_store_gpr(v1_t, rt);
> > +            break;
> > +        }
> > +        break;
> thanks
> -- PMM