ARM gdb record-replay bug fixes
Commit Message
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(-)
Comments
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,
@@ -11943,7 +11943,6 @@ arm_record_vdata_transfer_insn (insn_decode_record *arm_insn_r)
uint32_t bits_a, bit_c, bit_l, reg_t, reg_v;
uint32_t record_buf[4];
- const int num_regs = gdbarch_num_regs (arm_insn_r->gdbarch);
reg_t = bits (arm_insn_r->arm_insn, 12, 15);
reg_v = bits (arm_insn_r->arm_insn, 21, 23);
bits_a = bits (arm_insn_r->arm_insn, 21, 23);
@@ -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;
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;
arm_insn_r->reg_rec_count = 1;
}
/* Handle VMSR instruction. */
@@ -12042,7 +12033,6 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
ULONGEST u_regval = 0;
struct regcache *reg_cache = arm_insn_r->regcache;
- const int num_regs = gdbarch_num_regs (arm_insn_r->gdbarch);
opcode = bits (arm_insn_r->arm_insn, 20, 24);
single_reg = bit (arm_insn_r->arm_insn, 8);
@@ -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
{
@@ -12085,7 +12083,7 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
reg_rn = bits (arm_insn_r->arm_insn, 16, 19);
regcache_raw_read_unsigned (reg_cache, reg_rn, &u_regval);
imm_off8 = bits (arm_insn_r->arm_insn, 0, 7);
- imm_off32 = imm_off8 << 24;
+ imm_off32 = imm_off8 << 2;
memory_count = imm_off8;
if (bit (arm_insn_r->arm_insn, 23))
@@ -12103,19 +12101,17 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
{
if (!single_reg)
{
- 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--;
}
@@ -12142,7 +12138,8 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
while (reg_count > 0)
{
if (single_reg)
- record_buf[reg_index++] = num_regs + reg_vd + reg_count - 1;
+ record_buf[reg_index++] = ARM_D0_REGNUM +
+ (reg_vd + reg_count - 1) / 2;
else
record_buf[reg_index++] = ARM_D0_REGNUM + reg_vd + reg_count - 1;
@@ -12159,7 +12156,7 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
reg_rn = bits (arm_insn_r->arm_insn, 16, 19);
regcache_raw_read_unsigned (reg_cache, reg_rn, &u_regval);
imm_off8 = bits (arm_insn_r->arm_insn, 0, 7);
- imm_off32 = imm_off8 << 24;
+ imm_off32 = imm_off8 << 2;
memory_count = imm_off8;
if (bit (arm_insn_r->arm_insn, 23))
@@ -12169,16 +12166,16 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
if (single_reg)
{
- 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;
arm_insn_r->mem_rec_count = 1;
}
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;
arm_insn_r->mem_rec_count = 2;
}
}
@@ -12195,7 +12192,7 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
else
{
reg_vd = (reg_vd << 1) | bit (arm_insn_r->arm_insn, 22);
- record_buf[0] = num_regs + reg_vd;
+ record_buf[0] = ARM_D0_REGNUM + reg_vd / 2;
}
arm_insn_r->reg_rec_count = 1;
}