From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55950) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMNKV-0001eK-Th for qemu-devel@nongnu.org; Mon, 03 Aug 2015 17:31:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZMNKU-00010T-K6 for qemu-devel@nongnu.org; Mon, 03 Aug 2015 17:31:31 -0400 Received: from hall.aurel32.net ([2001:bc8:30d7:100::1]:36955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMNKU-0000yt-EN for qemu-devel@nongnu.org; Mon, 03 Aug 2015 17:31:30 -0400 Date: Mon, 3 Aug 2015 23:31:28 +0200 From: Aurelien Jarno Message-ID: <20150803213128.GA19612@aurel32.net> References: <1438627752-19903-2-git-send-email-rth@twiddle.net> <1438630553-21240-1-git-send-email-rth@twiddle.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1438630553-21240-1-git-send-email-rth@twiddle.net> Subject: Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: leon.alrae@imgtec.com, qemu-devel@nongnu.org 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 > --- > 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 Should we try to get this one into 2.4, if not already too late? -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net