Re: [PATCH] ARM gdb record-replay bug fixes

[Yao Qi <qiyaoltc at gmail dot com>]

Hi Omair,
Sorry for the delayed review...

On 03/02/15 12:47, Omair Javaid wrote:
> This patch provides fixes for some gdb record-replay related testsuite failures
> on arm. The patch fixes instruction decoding errors in vector data transfer and
> extension register load/store instructions.

The patch has been in a good shape, but some bits are still missing.
Could you please split your patch to small pieces (one patch for each
instruction encoding, maybe?).  In each of them, give the details on
why current one is wrong, how is the encoding described in the ARM
ARM, and how does the patch fix it.

> This patch has been tested in remote/native mode on armv7 hardware and fixes
> failures in gdb.record gdb.mi testsuites. Fixes around 40 tests overall.

The result looks good.  Could you please add test cases for the
instructions affected by this patch?  These cases should fail before
your patch and pass with your patch applied.  ARM process record bits 
are not good, so we need more test cases to guarantee that it isn't
worse.

> gdb:
>
> 2015-02-03  Omair Javaid<omair.javaid@linaro.org>
>
> 	* aarch64-tdep.c (arm_record_vdata_transfer_insn): Correct floating
        ^^^^^^^^^^^^^^^^
typo: arm-tdep.c

> @@ -11961,12 +11960,7 @@ arm_record_vdata_transfer_insn (insn_decode_record *arm_insn_r)
>         /* Handle VMOV instruction.  */
>         if (bits_a == 0x00)
>           {
> -          if (bit (arm_insn_r->arm_insn, 20))
> -            record_buf[0] = reg_t;
> -          else
> -            record_buf[0] = num_regs + (bit (arm_insn_r->arm_insn, 7) |
> -                            (reg_v << 1));
> -
> +          record_buf[0] = reg_t;

Let us replace 8 spaces with tab here and somewhere else.  I know this
format issue was there when arm record/replay was added, but let us fix
it from now on.

>             arm_insn_r->reg_rec_count = 1;
>           }
>         /* Handle VMRS instruction.  */
> @@ -11984,12 +11978,9 @@ arm_record_vdata_transfer_insn (insn_decode_record *arm_insn_r)
>         /* Handle VMOV instruction.  */
>         if (bits_a == 0x00)
>           {
> -          if (bit (arm_insn_r->arm_insn, 20))
> -            record_buf[0] = reg_t;
> -          else
> -            record_buf[0] = num_regs + (bit (arm_insn_r->arm_insn, 7) |
> -                            (reg_v << 1));
> -
> +          uint32_t reg_single;
> +          reg_single = bit (arm_insn_r->arm_insn, 7) | (reg_v << 1);
> +          record_buf[0] = ARM_D0_REGNUM + reg_single / 2;

I think "reg_single / 2" should be equal to "reg_v".

> @@ -12064,9 +12054,17 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
>
>            if (!single_reg)
>              {
> -              record_buf[0] = num_regs + reg_m;
> -              record_buf[1] = num_regs + reg_m + 1;
> -              arm_insn_r->reg_rec_count = 2;
> +              if (reg_m & 0x01)
> +                {
> +                  record_buf[0] = ARM_D0_REGNUM + reg_m / 2;
> +                  record_buf[1] = ARM_D0_REGNUM + (reg_m + 1) / 2;
> +                  arm_insn_r->reg_rec_count = 2;
> +                }
> +              else
> +                {
> +                  record_buf[0] = ARM_D0_REGNUM + reg_m / 2;
> +                  arm_insn_r->reg_rec_count = 1;
> +                }
>              }
>            else
>              {

I am still confused that how gdb can go here, the code looks like:

arm_record_exreg_ld_st_insn
{
  ...
  if ((opcode & 0x1e) == 0x04)
    {
      if (bit (arm_insn_r->arm_insn, 4)) <-- [1]
        {}
      else
        {}
    }
  else
    {
    }
}

According to ARM ARM, A7.9 64-bit transfers between ARM core and 
extension registers, the bit 4 is 1, so condition on [1] is always
true.

> @@ -12103,19 +12101,17 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
>          {
>            if (!single_reg)

This block is about handling VSTM and VPUSH, so 'single_reg' (bit 8) is
always 1, isn't?

>              {
> -              record_buf_mem[memory_index] = start_address;
> -              record_buf_mem[memory_index + 1] = 4;
> +              record_buf_mem[memory_index++] = 4;
> +              record_buf_mem[memory_index++] = start_address;
>                start_address = start_address + 4;
> -              memory_index = memory_index + 2;
>              }
>            else
>              {
> -              record_buf_mem[memory_index] = start_address;
> -              record_buf_mem[memory_index + 1] = 4;
> -              record_buf_mem[memory_index + 2] = start_address + 4;
> -              record_buf_mem[memory_index + 3] = 4;
> +              record_buf_mem[memory_index++] = 4;
> +              record_buf_mem[memory_index++] = start_address;
> +              record_buf_mem[memory_index++] = 4;
> +              record_buf_mem[memory_index++] = start_address + 4;
>                start_address = start_address + 8;
> -              memory_index = memory_index + 4;
>              }
>            memory_count--;
>          }

although your changes in this block looks reasonable to me.

--
Yao (éå)