| Message ID | 1438630553-21240-1-git-send-email-rth@twiddle.net |
|---|---|
| State | New |
| Headers | show |
On 2015-08-03 12:35, Richard Henderson wrote: > The checks in dins is required to avoid triggering an assertion > in tcg_gen_deposit_tl. The check in dext is just for completeness. > Fold the other D cases in via fallthru. > > In this case the errant dins appears to be data, not code, as > translation failed to stop after a break insn. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target-mips/translate.c | 45 +++++++++++++++++++++++++-------------------- > 1 file changed, 25 insertions(+), 20 deletions(-) > > diff --git a/target-mips/translate.c b/target-mips/translate.c > index d1de35a..1cba415 100644 > --- a/target-mips/translate.c > +++ b/target-mips/translate.c > @@ -4750,48 +4750,53 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt, > gen_load_gpr(t1, rs); > switch (opc) { > case OPC_EXT: > - if (lsb + msb > 31) > + if (lsb + msb > 31) { > goto fail; > + } > tcg_gen_shri_tl(t0, t1, lsb); > if (msb != 31) { > - tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1); > + tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1); Is this change really needed? > } else { > tcg_gen_ext32s_tl(t0, t0); > } > break; > #if defined(TARGET_MIPS64) > - case OPC_DEXTM: > - tcg_gen_shri_tl(t0, t1, lsb); > - if (msb != 31) { > - tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1); > - } > - break; > case OPC_DEXTU: > - tcg_gen_shri_tl(t0, t1, lsb + 32); > - tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1); > - break; > + lsb += 32; > + goto do_dext; > + case OPC_DEXTM: > + msb += 32; > + goto do_dext; > case OPC_DEXT: > + do_dext: > + if (lsb + msb > 63) { > + goto fail; > + } Note that DEXT can't fail as both lsb and msb are in the range 0..31. DEXTU and DEXTM can. > tcg_gen_shri_tl(t0, t1, lsb); > - tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1); > + if (msb != 63) { > + tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1); > + } > break; > #endif > case OPC_INS: > - if (lsb > msb) > + if (lsb > msb) { > goto fail; > + } > gen_load_gpr(t0, rt); > tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1); > tcg_gen_ext32s_tl(t0, t0); > break; > #if defined(TARGET_MIPS64) > - case OPC_DINSM: > - gen_load_gpr(t0, rt); > - tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1); > - break; > case OPC_DINSU: > - gen_load_gpr(t0, rt); > - tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1); > - break; > + lsb += 32; > + /* FALLTHRU */ > + case OPC_DINSM: > + msb += 32; The same way DINSM can't fail. > + /* FALLTHRU */ > case OPC_DINS: > + if (lsb > msb) { > + goto fail; > + } > gen_load_gpr(t0, rt); > tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1); > break; Despite the above comments: Reviewed-by: Aurelien Jarno <aurelien@aurel32.net> Should we try to get this one into 2.4, if not already too late?
On 08/03/2015 02:31 PM, Aurelien Jarno wrote: > On 2015-08-03 12:35, Richard Henderson wrote: >> if (msb != 31) { >> - tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1); >> + tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1); > > Is this change really needed? msb == 30 means 1 << 31. Which officially must be unsigned to be correct. If we were to run under ubsan, this would trigger an error. > Note that DEXT can't fail as both lsb and msb are in the range 0..31. > DEXTU and DEXTM can. ... > The same way DINSM can't fail. Yes, I know. But it seems cleaner to do the checks always, unifying all of the code. > Should we try to get this one into 2.4, if not already too late? Perhaps. Otherwise via stable after the fact. r~
On 2015-08-03 14:41, Richard Henderson wrote: > On 08/03/2015 02:31 PM, Aurelien Jarno wrote: > >On 2015-08-03 12:35, Richard Henderson wrote: > >> if (msb != 31) { > >>- tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1); > >>+ tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1); > > > >Is this change really needed? > > msb == 30 means 1 << 31. Which officially must be unsigned to be correct. > If we were to run under ubsan, this would trigger an error. Ok. > >Note that DEXT can't fail as both lsb and msb are in the range 0..31. > >DEXTU and DEXTM can. > ... > >The same way DINSM can't fail. > > Yes, I know. But it seems cleaner to do the checks always, unifying all of > the code. Agreed. > >Should we try to get this one into 2.4, if not already too late? > > Perhaps. Otherwise via stable after the fact. Ok. Leon, do you have other pending patches for 2.4/2.4.1? The semihosting microMIPS R6 one maybe?
On 03/08/2015 23:09, Aurelien Jarno wrote: >>> Should we try to get this one into 2.4, if not already too late? >> >> Perhaps. Otherwise via stable after the fact. > > Ok. Leon, do you have other pending patches for 2.4/2.4.1? The > semihosting microMIPS R6 one maybe? Yes, it would be good also to include the semihosting microMIPS R6 fix in 2.4. Leon
On 03/08/2015 20:35, Richard Henderson wrote: > The checks in dins is required to avoid triggering an assertion > in tcg_gen_deposit_tl. The check in dext is just for completeness. > Fold the other D cases in via fallthru. > > In this case the errant dins appears to be data, not code, as > translation failed to stop after a break insn. > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > target-mips/translate.c | 45 +++++++++++++++++++++++++-------------------- > 1 file changed, 25 insertions(+), 20 deletions(-) Reviewed-by: Leon Alrae <leon.alrae@imgtec.com> I'll send out the pull request including this single patch from the series as well as semihosting microMIPS R6 fix soon (just waiting for tests to finish...). Thanks, Leon
diff --git a/target-mips/translate.c b/target-mips/translate.c index d1de35a..1cba415 100644 --- a/target-mips/translate.c +++ b/target-mips/translate.c @@ -4750,48 +4750,53 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt, gen_load_gpr(t1, rs); switch (opc) { case OPC_EXT: - if (lsb + msb > 31) + if (lsb + msb > 31) { goto fail; + } tcg_gen_shri_tl(t0, t1, lsb); if (msb != 31) { - tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1); + tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1); } else { tcg_gen_ext32s_tl(t0, t0); } break; #if defined(TARGET_MIPS64) - case OPC_DEXTM: - tcg_gen_shri_tl(t0, t1, lsb); - if (msb != 31) { - tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1); - } - break; case OPC_DEXTU: - tcg_gen_shri_tl(t0, t1, lsb + 32); - tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1); - break; + lsb += 32; + goto do_dext; + case OPC_DEXTM: + msb += 32; + goto do_dext; case OPC_DEXT: + do_dext: + if (lsb + msb > 63) { + goto fail; + } tcg_gen_shri_tl(t0, t1, lsb); - tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1); + if (msb != 63) { + tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1); + } break; #endif case OPC_INS: - if (lsb > msb) + if (lsb > msb) { goto fail; + } gen_load_gpr(t0, rt); tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1); tcg_gen_ext32s_tl(t0, t0); break; #if defined(TARGET_MIPS64) - case OPC_DINSM: - gen_load_gpr(t0, rt); - tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1); - break; case OPC_DINSU: - gen_load_gpr(t0, rt); - tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1); - break; + lsb += 32; + /* FALLTHRU */ + case OPC_DINSM: + msb += 32; + /* FALLTHRU */ case OPC_DINS: + if (lsb > msb) { + goto fail; + } gen_load_gpr(t0, rt); tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1); break;
The checks in dins is required to avoid triggering an assertion in tcg_gen_deposit_tl. The check in dext is just for completeness. Fold the other D cases in via fallthru. In this case the errant dins appears to be data, not code, as translation failed to stop after a break insn. Signed-off-by: Richard Henderson <rth@twiddle.net> --- target-mips/translate.c | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-)