[PATCH] target/i386: Remove unnecessary checks of b2 in SSE decode

[PATCH] target/i386: Remove unnecessary checks of b2 in SSE decode

[Peter Maydell]

In the SSE decode function gen_sse(), we combine a byte
'b' and a value 'b1' which can be [0..3], and switch on them:
   b |= (b1 << 8);
   switch (b) {
   ...
   default:
   unknown_op:
       gen_unknown_opcode(env, s);
       return;
   }

In three cases inside this switch, we were then also checking for
 "if (b1 >= 2) { goto unknown_op; }".
However, this can never happen, because the 'case' values in each place
are 0x0nn or 0x1nn and the switch will have directed the b1 == (2, 3)
cases to the default already.

Delete the dead code.

This check was added in commit c045af25a52e9 in 2010; the added code
was unnecessary then as well.  this commit amounts to a revert of
c045af25a52e9.

Fixes: Coverity CID 1460207
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Somebody should double-check this, because one assumes Andi
added the code for a reason...
---
 target/i386/tcg/translate.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index aacb605eee4..3e7afd2620e 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3521,9 +3521,6 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b,
         case 0x171: /* shift xmm, im */
         case 0x172:
         case 0x173:
-            if (b1 >= 2) {
-                goto unknown_op;
-            }
             val = x86_ldub_code(env, s);
             if (is_xmm) {
                 tcg_gen_movi_tl(s->T0, val);
@@ -3772,9 +3769,6 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b,
             rm = modrm & 7;
             reg = ((modrm >> 3) & 7) | REX_R(s);
             mod = (modrm >> 6) & 3;
-            if (b1 >= 2) {
-                goto unknown_op;
-            }
 
             sse_fn_epp = sse_op_table6[b].op[b1];
             if (!sse_fn_epp) {
@@ -4202,9 +4196,6 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b,
             rm = modrm & 7;
             reg = ((modrm >> 3) & 7) | REX_R(s);
             mod = (modrm >> 6) & 3;
-            if (b1 >= 2) {
-                goto unknown_op;
-            }
 
             sse_fn_eppi = sse_op_table7[b].op[b1];
             if (!sse_fn_eppi) {
-- 
2.20.1

Re: [PATCH] target/i386: Remove unnecessary checks of b2 in SSE decode

[Peter Maydell]

On Fri, 20 Aug 2021 at 18:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In the SSE decode function gen_sse(), we combine a byte
> 'b' and a value 'b1' which can be [0..3], and switch on them:
>    b |= (b1 << 8);
>    switch (b) {
>    ...
>    default:
>    unknown_op:
>        gen_unknown_opcode(env, s);
>        return;
>    }
>
> In three cases inside this switch, we were then also checking for
>  "if (b1 >= 2) { goto unknown_op; }".
> However, this can never happen, because the 'case' values in each place
> are 0x0nn or 0x1nn and the switch will have directed the b1 == (2, 3)
> cases to the default already.
>
> Delete the dead code.
>
> This check was added in commit c045af25a52e9 in 2010; the added code
> was unnecessary then as well.  this commit amounts to a revert of
> c045af25a52e9.
>
> Fixes: Coverity CID 1460207
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Somebody should double-check this, because one assumes Andi
> added the code for a reason...

It occurred to me that maybe we'd be better just changing
these into
  assert(b1 < 2);

These are guarding all the places where we do a dereference
of an sse_op_table*[x][b1] and the inner table only has 2
elements. So asserting would be a sensible guard. I'll send out
a v2 patch that does that.

-- PMM