ARM gdb record-replay bug fixes

  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.

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.

gdb:

2015-02-03  Omair Javaid  <omair.javaid@linaro.org>

	* aarch64-tdep.c (arm_record_vdata_transfer_insn): Correct floating
	point register numbers handling.
	(arm_record_exreg_ld_st_insn): Fix offset calculation, memory recording
	and floating point register numbers handling.

---
 gdb/arm-tdep.c | 67 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 35 deletions(-)
  

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.
  

Hi Yao,

I am reposting these patches. You can take them forward from here.

(No change: Patch approved in previous iterations)
Patch#1  NEWS entry about aarch64-linux record/replay support
has been previously posted and approved here:
https://sourceware.org/ml/gdb-patches/2014-06/msg00185.html
https://sourceware.org/ml/gdb-patches/2014-09/msg00611.html

(Updated: Added suggested changes)
Patch#2  Implements aarch64 process record and reverse debugging support
has been discussed and updated in several iteration. Previous versions and
discussions can be found here:
https://sourceware.org/ml/gdb-patches/2014-10/msg00540.html
https://sourceware.org/ml/gdb-patches/2014-08/msg00662.html

(Updated: Added suggested changes)
Patch#3  Support for recording syscall on aarch64-linux
has been previously discussed and improved in this version too.
Previous discussions can be found here:
https://sourceware.org/ml/gdb-patches/2014-10/msg00541.html

(Updated: Added suggested changes)
Patch#4  Support for recording aarch64 advanced SIMD instructions
has also been previously posted and discussed here:
https://sourceware.org/ml/gdb-patches/2014-10/msg00345.html

(No change: Patch approved in previous iterations)
Patch#5  Enables gdb.reverse testsuite for aarch64*-linux targets
is a pretty trivial patch to enable gdb testsuite. Its previously posted and discussed here:
https://sourceware.org/ml/gdb-patches/2014-06/msg00242.html

(New Patch)
Patch#6  Adds testcases to test arm64 instruction set recording

Omair Javaid (6):
  NEWS entry about aarch64-linux record/replay support
  Implements aarch64 process record and reverse debugging support
  Support for recording syscall on aarch64-linux
  Support for recording aarch64 advanced SIMD instructions
  Enables gdb.reverse testsuite for aarch64*-linux targets
  Adds testcases to test arm64 instruction set recording

 gdb/NEWS                                       |    4 +
 gdb/aarch64-linux-tdep.c                       |  917 ++++++++++
 gdb/aarch64-linux-tdep.h                       |  266 +++
 gdb/aarch64-tdep.c                             |  862 ++++++++++
 gdb/aarch64-tdep.h                             |    6 +
 gdb/configure.tgt                              |    2 +-
 gdb/linux-record.h                             |    2 +
 gdb/testsuite/gdb.reverse/aarch64-test-rrp.exp |  234 +++
 gdb/testsuite/gdb.reverse/aarch64-test-rrp.txt | 2115 ++++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                      |    2 +
 10 files changed, 4409 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.reverse/aarch64-test-rrp.exp
 create mode 100644 gdb/testsuite/gdb.reverse/aarch64-test-rrp.txt
  

Omair Javaid <omair.javaid@linaro.org> writes:

> I am reposting these patches. You can take them forward from here.

Thanks Omair.

To be clear, this patch series V5 is still WIP, and I'll take them over
from Omair.  Here are still something I'll do:

 - complete the test case,
 - add changelog entry if missing, and complete commit log as well,
 - adjust code to address some previous review comments,