Re: [Qemu-devel] [PATCH] target-arm: Fix UNDEF cases in Thumb load/store

[Aurelien Jarno]

On Thu, Mar 10, 2011 at 04:48:49PM +0000, Peter Maydell wrote:
> Decode of Thumb load/store was merging together the cases of 'bit 11==0'
> (reg+reg LSL imm) and 'bit 11==1' (reg+imm). This happens to work for
> valid instruction patterns but meant that we would not UNDEF for the
> cases the architecture mandates that we must. Make the decode actually
> look at bit 11 as well as [10..8] so that we UNDEF in the right places.
> 
> This change also removes what was a spurious unreachable 'case 8',
> and correctly frees TCG temporaries on the illegal-insn codepaths.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This patch was mostly prompted by that dodgy 'case 8' which I noted
> when doing the preload/hint space patches a month or so ago; I have
> finally added support for testing loads and stores to risu, so I
> can confirm that this patch doesn't break the non-UNDEF cases.
> 
>  target-arm/translate.c |   29 ++++++++++++++++++-----------
>  1 files changed, 18 insertions(+), 11 deletions(-)

Thanks, applied.

> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 062de5e..0afdbfb 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -8378,39 +8378,42 @@ static int disas_thumb2_insn(CPUState *env, 
> DisasContext *s, uint16_t insn_hw1)
>                  tcg_gen_addi_i32(addr, addr, imm);
>              } else {
>                  imm = insn & 0xff;
> -                switch ((insn >> 8) & 7) {
> -                case 0: case 8: /* Shifted Register.  */
> +                switch ((insn >> 8) & 0xf) {
> +                case 0x0: /* Shifted Register.  */
>                      shift = (insn >> 4) & 0xf;
> -                    if (shift > 3)
> +                    if (shift > 3) {
> +                        tcg_temp_free_i32(addr);
>                          goto illegal_op;
> +                    }
>                      tmp = load_reg(s, rm);
>                      if (shift)
>                          tcg_gen_shli_i32(tmp, tmp, shift);
>                      tcg_gen_add_i32(addr, addr, tmp);
>                      tcg_temp_free_i32(tmp);
>                      break;
> -                case 4: /* Negative offset.  */
> +                case 0xc: /* Negative offset.  */
>                      tcg_gen_addi_i32(addr, addr, -imm);
>                      break;
> -                case 6: /* User privilege.  */
> +                case 0xe: /* User privilege.  */
>                      tcg_gen_addi_i32(addr, addr, imm);
>                      user = 1;
>                      break;
> -                case 1: /* Post-decrement.  */
> +                case 0x9: /* Post-decrement.  */
>                      imm = -imm;
>                      /* Fall through.  */
> -                case 3: /* Post-increment.  */
> +                case 0xb: /* Post-increment.  */
>                      postinc = 1;
>                      writeback = 1;
>                      break;
> -                case 5: /* Pre-decrement.  */
> +                case 0xd: /* Pre-decrement.  */
>                      imm = -imm;
>                      /* Fall through.  */
> -                case 7: /* Pre-increment.  */
> +                case 0xf: /* Pre-increment.  */
>                      tcg_gen_addi_i32(addr, addr, imm);
>                      writeback = 1;
>                      break;
>                  default:
> +                    tcg_temp_free_i32(addr);
>                      goto illegal_op;
>                  }
>              }
> @@ -8423,7 +8426,9 @@ static int disas_thumb2_insn(CPUState *env, 
> DisasContext *s, uint16_t insn_hw1)
>              case 1: tmp = gen_ld16u(addr, user); break;
>              case 5: tmp = gen_ld16s(addr, user); break;
>              case 2: tmp = gen_ld32(addr, user); break;
> -            default: goto illegal_op;
> +            default:
> +                tcg_temp_free_i32(addr);
> +                goto illegal_op;
>              }
>              if (rs == 15) {
>                  gen_bx(s, tmp);
> @@ -8437,7 +8442,9 @@ static int disas_thumb2_insn(CPUState *env, 
> DisasContext *s, uint16_t insn_hw1)
>              case 0: gen_st8(tmp, addr, user); break;
>              case 1: gen_st16(tmp, addr, user); break;
>              case 2: gen_st32(tmp, addr, user); break;
> -            default: goto illegal_op;
> +            default:
> +                tcg_temp_free_i32(addr);
> +                goto illegal_op;
>              }
>          }
>          if (postinc)
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net