This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 2/5] Implements aarch64 process record and reverse debugging support
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Omair Javaid <omair dot javaid at linaro dot org>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 19 Feb 2015 12:34:52 +0000
- Subject: Re: [PATCH v4 2/5] Implements aarch64 process record and reverse debugging support
- Authentication-results: sourceware.org; auth=none
- References: <1422926216-15740-1-git-send-email-omair dot javaid at linaro dot org> <1422926216-15740-3-git-send-email-omair dot javaid at linaro dot org>
Omair Javaid <omair.javaid@linaro.org> writes:
Hi Omair,
Thanks for your patience to submit and update your patches
continuously. I finally have a chance to review this series...
> 2015-02-02 Omair Javaid <omair.javaid@linaro.org>
>
> * aarch64-linux-tdep.c (aarch64_linux_init_abi): Install AArch64
> process record handler.
> * aarch64-tdep.c (record.h): Include.
> (record-full.h): Include.
> (submask): New macro.
> (bit): New macro.
> (bits): New macro.
> (REG_ALLOC): New macro.
> (MEM_ALLOC): New macro.
> (struct aarch64_mem_r): Define.
> (aarch64_record_result): New enum.
> (struct insn_decode_record): Define.
> (insn_decode_record): New typedef.
> (aarch64_record_data_proc_reg): Add record handler for data processing
> register insns.
"New function" should be fine here, some instances below.
> (aarch64_record_data_proc_imm): Add record handler for data processing
> immediate insns.
> (aarch64_record_branch_except_sys): Add record handler for branch,
> exception and system insns.
> (aarch64_record_load_store): Add record handler for load/store insns.
> (aarch64_record_decode_insn_handler): Add record insn decoding function.
> (deallocate_reg_mem): Add memory cleanup function for record
> data.
> (aarch64_process_record): Add gdbarch handler for AArch64 process
> record.
> * aarch64-tdep.h (aarch64_process_record): New extern declaration.
> +
> +/* AArch64 instruction record contains opcode of current insn and execution
> + state (before entry to decode_insn()), contains list of to-be-modified
> + registers and memory blocks (on return from decode_insn()). */
These comments are good, but please move them into each field below.
> +
> +typedef struct insn_decode_record_t
> +{
> + struct gdbarch *gdbarch;
> + struct regcache *regcache;
> + CORE_ADDR this_addr;
> + uint32_t aarch64_insn;
> + uint32_t mem_rec_count;
> + uint32_t reg_rec_count;
> + uint32_t *aarch64_regs;
> + struct aarch64_mem_r *aarch64_mems;
> +} insn_decode_record;
> +
> +/* Record handler for data processing - register instructions. */
A blank line is needed between the function definition and comment, here
and somewhere else.
> +static unsigned int
> +aarch64_record_data_proc_reg (insn_decode_record *aarch64_insn_r)
> +{
> + uint8_t reg_rd, insn_bits24_27, insn_bits21_23, setflags;
> + uint32_t record_buf[4];
> +
> + reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> + insn_bits24_27 = bits (aarch64_insn_r->aarch64_insn, 24, 27);
> + insn_bits21_23 = bits (aarch64_insn_r->aarch64_insn, 21, 23);
> +
> + if (!bit (aarch64_insn_r->aarch64_insn, 28))
> + {
Please move local setflags here, as it is only used inside this block.
> + /* Logical (shifted register). */
> + if (insn_bits24_27 == 0x0a)
> + setflags = (bits (aarch64_insn_r->aarch64_insn, 29, 30) == 0x03);
> + /* Add/subtract. */
> + else if (insn_bits24_27 == 0x0b)
> + setflags = bit (aarch64_insn_r->aarch64_insn, 29);
> + else
> + return AARCH64_RECORD_UNSUPPORTED;
According to the manual I have, there isn't such instruction encoding
goes to the "return AARCH64_RECORD_UNSUPPORTED" path. What is the
definition of AARCH64_RECORD_UNSUPPORTED? Its literal meaning to me
is that there are some instructions encoding available, but GDB doesn't
support them in process record. For the instructions will appear in
future, probably we can return AARCH64_RECORD_UNKNOWN or
AARCH64_RECORD_UNALLOCATED. In this way, we can easily do statistics
which instructions are not supported. What do you think?
> +
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + if (setflags)
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_CPSR_REGNUM;
> + }
> + else
> + {
> + if (insn_bits24_27 == 0x0b)
> + {
> + /* Data-processing (3 source). */
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + else if (insn_bits24_27 == 0x0a)
> + {
> + if (insn_bits21_23 == 0x00)
> + {
> + /* Add/subtract (with carry). */
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + if (bit (aarch64_insn_r->aarch64_insn, 29))
> + {
> + record_buf[1] = AARCH64_CPSR_REGNUM;
> + aarch64_insn_r->reg_rec_count = 2;
> + }
> + }
> + else if (insn_bits21_23 == 0x02)
> + {
> + /* Conditional compare (register) / Conditional compare (immediate). */
This line is too long.
> + record_buf[0] = AARCH64_CPSR_REGNUM;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + else if (insn_bits21_23 == 0x04 || insn_bits21_23 == 0x06)
> + {
> + /* CConditional select. */
> + /* Data-processing (2 source). */
> + /* Data-processing (1 source). */
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + else
> + return AARCH64_RECORD_UNSUPPORTED;
> + }
> + }
> +
> + REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
> + record_buf);
> + return AARCH64_RECORD_SUCCESS;
> +}
> +
> +/* Record handler for data processing - immediate instructions. */
> +static unsigned int
> +aarch64_record_data_proc_imm (insn_decode_record *aarch64_insn_r)
> +{
> + uint8_t reg_rd, insn_bit28, insn_bit23, insn_bits24_27, setflags;
> + uint32_t record_buf[4];
> +
> + reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> + insn_bit28 = bit (aarch64_insn_r->aarch64_insn, 28);
> + insn_bit23 = bit (aarch64_insn_r->aarch64_insn, 23);
> + insn_bits24_27 = bits (aarch64_insn_r->aarch64_insn, 24, 27);
> +
> + /* PC rel addressing / Move wide immediate / BitField / Extract. */
Let's split the comments into pieces and associate each to the condition below,
> + if (insn_bits24_27 == 0x00 || insn_bits24_27 == 0x03 ||
Move "||" to the next line.
> + (insn_bits24_27 == 0x02 && insn_bit23))
something like:
if (insn_bits24_27 == 0x00 /* PC rel addressing */
|| insn_bits24_27 == 0x03 /* Bitfield and Extract */
|| (insn_bits24_27 == 0x02 && insn_bit23)) /* Move wide (immediate) */
It's clear to read code and manual together in this way, IMO.
> + {
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + else if (insn_bits24_27 == 0x01)
> + {
> + /* Add/Subtract (immediate). */
> + setflags = bit (aarch64_insn_r->aarch64_insn, 29);
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + if (setflags)
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_CPSR_REGNUM;
> + }
> + else if (insn_bits24_27 == 0x02 && !insn_bit23)
> + {
> + /* Logical (immediate). */
> + setflags = bits (aarch64_insn_r->aarch64_insn, 29, 30) == 0x03;
> + record_buf[0] = reg_rd;
> + aarch64_insn_r->reg_rec_count = 1;
> + if (setflags)
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_CPSR_REGNUM;
> + }
> + else
> + return AARCH64_RECORD_UNSUPPORTED;
> +
> + REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
> + record_buf);
> + return AARCH64_RECORD_SUCCESS;
> +}
> +
> +/* Record handler for branch, exception generation and system instructions. */
> +static unsigned int
> +aarch64_record_branch_except_sys (insn_decode_record *aarch64_insn_r)
> +{
> + struct gdbarch_tdep *tdep = gdbarch_tdep (aarch64_insn_r->gdbarch);
tdep isn't used.
> + uint8_t insn_bits24_27, insn_bits28_31, insn_bits22_23;
> + uint32_t record_buf[4];
> +
> + insn_bits24_27 = bits (aarch64_insn_r->aarch64_insn, 24, 27);
> + insn_bits28_31 = bits (aarch64_insn_r->aarch64_insn, 28, 31);
> + insn_bits22_23 = bits (aarch64_insn_r->aarch64_insn, 22, 23);
> +
> + if (insn_bits28_31 == 0x0d)
> + {
> + /* Exception generation instructions. */
> + if (insn_bits24_27 == 0x04)
> + return AARCH64_RECORD_UNSUPPORTED;
> + /* System instructions. */
> + else if (insn_bits24_27 == 0x05 && insn_bits22_23 == 0x00)
> + {
> + record_buf[0] = AARCH64_CPSR_REGNUM;
> + record_buf[1] = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> + aarch64_insn_r->reg_rec_count = 2;
Some system instructions change Rt, such as "MSR (register)", but some
don't, such as ISB and DMB. We should handle them differently.
> + }
> + else if((insn_bits24_27 & 0x0e) == 0x06)
> + {
Add /* Unconditional branch (register) */
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_PC_REGNUM;
> + if (bits (aarch64_insn_r->aarch64_insn, 21, 22) == 0x01)
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_LR_REGNUM;
> + }
> + else
> + return AARCH64_RECORD_UNSUPPORTED;
return AARCH64_RECORD_UNKNOWN?
> + }
> + else if ((insn_bits28_31 & 0x07) == 0x01 && (insn_bits24_27 & 0x0c) == 0x04)
> + {
/* Unconditional branch (immediate) */
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_PC_REGNUM;
> + if (bit (aarch64_insn_r->aarch64_insn, 31))
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_LR_REGNUM;
> + }
> + else
> + /* All other types of branch instructions. */
Let us mention these branch instructions explicitly, like:
/* Compare & branch (immediate), Test & branch (immediate) and
Conditional branch (immediate) */
> + record_buf[aarch64_insn_r->reg_rec_count++] = AARCH64_PC_REGNUM;
> +
> + REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
> + record_buf);
> + return AARCH64_RECORD_SUCCESS;
> +}
> +
> +/* Record handler for load and store instructions. */
> +static unsigned int
> +aarch64_record_load_store (insn_decode_record *aarch64_insn_r)
> +{
> + uint8_t insn_bits24_27, insn_bits28_29, insn_bits10_11;
> + uint8_t insn_bit23, insn_bit21;
> + uint8_t opc, size_bits, ld_flag, vector_flag;
> + uint32_t reg_rn, reg_rt, reg_rt2;
> + uint64_t datasize, offset;
> + uint32_t record_buf[8];
> + uint64_t record_buf_mem[8];
> + CORE_ADDR address;
> +
> + insn_bits10_11 = bits (aarch64_insn_r->aarch64_insn, 10, 11);
> + insn_bits24_27 = bits (aarch64_insn_r->aarch64_insn, 24, 27);
> + insn_bits28_29 = bits (aarch64_insn_r->aarch64_insn, 28, 29);
> + insn_bit21 = bit (aarch64_insn_r->aarch64_insn, 21);
> + insn_bit23 = bit (aarch64_insn_r->aarch64_insn, 23);
> + ld_flag = bit (aarch64_insn_r->aarch64_insn, 22);
> + vector_flag = bit (aarch64_insn_r->aarch64_insn, 26);
> + reg_rt = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> + reg_rn = bits (aarch64_insn_r->aarch64_insn, 5, 9);
> + reg_rt2 = bits (aarch64_insn_r->aarch64_insn, 10, 14);
> + size_bits = bits (aarch64_insn_r->aarch64_insn, 30, 31);
> +
> + /* Load/store exclusive instructions decoding. */
The comment should consistent in style, better to be
/* Load/store exclusive */
> + if (insn_bits24_27 == 0x08 && insn_bits28_29 == 0x00)
> + {
> + if (ld_flag)
> + {
> + record_buf[0] = reg_rt;
> + aarch64_insn_r->reg_rec_count = 1;
> + if (insn_bit21)
> + {
> + record_buf[1] = reg_rt2;
> + aarch64_insn_r->reg_rec_count = 2;
> + }
> + }
> + else
> + {
> + if (insn_bit21)
> + datasize = (8 << size_bits) * 2;
> + else
> + datasize = (8 << size_bits);
> + regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rn,
> + &address);
> + record_buf_mem[0] = datasize / 8;
> + record_buf_mem[1] = address;
> + aarch64_insn_r->mem_rec_count = 1;
> + if (!insn_bit23)
> + {
> + /* Save register rs. */
> + record_buf[0] = bits (aarch64_insn_r->aarch64_insn, 16, 20);
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + }
> + }
> + /* Load register (literal) instructions decoding. */
> + else if ((insn_bits24_27 & 0x0b) == 0x08 && insn_bits28_29 == 0x01)
> + {
> + if (vector_flag)
> + record_buf[0] = reg_rt + AARCH64_V0_REGNUM;
> + else
> + record_buf[0] = reg_rt;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + /* All types of load/store pair instructions decoding. */
> + else if ((insn_bits24_27 & 0x0a) == 0x08 && insn_bits28_29 == 0x02)
> + {
> + if (ld_flag)
> + {
> + if (vector_flag)
> + {
> + record_buf[0] = reg_rt + AARCH64_V0_REGNUM;
> + record_buf[1] = reg_rt2 + AARCH64_V0_REGNUM;
> + }
> + else
> + {
> + record_buf[0] = reg_rt;
> + record_buf[1] = reg_rt2;
> + }
> + aarch64_insn_r->reg_rec_count = 2;
> + }
> + else
> + {
> + uint16_t imm7_off;
> + imm7_off = bits (aarch64_insn_r->aarch64_insn, 15, 21);
> + if (!vector_flag)
> + size_bits = size_bits >> 1;
> + datasize = 8 << (2 + size_bits);
> + offset = (imm7_off & 0x40) ? (~imm7_off & 0x007f) + 1 : imm7_off;
> + offset = offset << (2 + size_bits);
> + regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rn,
> + &address);
> + if (!((insn_bits24_27 & 0x0b) == 0x08 && insn_bit23))
> + {
> + if (imm7_off & 0x40)
> + address = address - offset;
> + else
> + address = address + offset;
> + }
> +
> + record_buf_mem[0] = datasize / 8;
> + record_buf_mem[1] = address;
> + record_buf_mem[2] = datasize / 8;
> + record_buf_mem[3] = address + (datasize / 8);
> + aarch64_insn_r->mem_rec_count = 2;
> + }
> + if (bit (aarch64_insn_r->aarch64_insn, 23))
> + record_buf[aarch64_insn_r->reg_rec_count++] = reg_rn;
> + }
> + /* Load/store register (unsigned immediate) instructions. */
> + else if ((insn_bits24_27 & 0x0b) == 0x09 && insn_bits28_29 == 0x03)
> + {
> + opc = bits (aarch64_insn_r->aarch64_insn, 22, 23);
> + if (!(opc >> 1))
> + if (opc & 0x01)
> + ld_flag = 0x01;
> + else
> + ld_flag = 0x0;
> + else
> + if (size_bits != 0x03)
> + ld_flag = 0x01;
> + else
> + return AARCH64_RECORD_UNSUPPORTED;
> +
> + if (!ld_flag)
> + {
> + offset = bits (aarch64_insn_r->aarch64_insn, 10, 21);
> + datasize = 8 << size_bits;
> + regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rn,
> + &address);
> + offset = offset << size_bits;
> + address = address + offset;
> +
> + record_buf_mem[0] = datasize >> 3;
> + record_buf_mem[1] = address;
> + aarch64_insn_r->mem_rec_count = 1;
> + }
> + else
> + {
> + if (vector_flag)
> + record_buf[0] = reg_rt + AARCH64_V0_REGNUM;
> + else
> + record_buf[0] = reg_rt;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + }
> + /* Load/store register (register offset) instructions. */
> + else if ((insn_bits24_27 & 0x0b) == 0x08 && insn_bits28_29 == 0x03 &&
> + insn_bits10_11 == 0x02 && insn_bit21)
> + {
> + opc = bits (aarch64_insn_r->aarch64_insn, 22, 23);
> + if (!(opc >> 1))
> + if (opc & 0x01)
> + ld_flag = 0x01;
> + else
> + ld_flag = 0x0;
> + else
> + if (size_bits != 0x03)
> + ld_flag = 0x01;
> + else
> + return AARCH64_RECORD_UNSUPPORTED;
> +
> + if (!ld_flag)
> + {
> + uint64_t reg_rm_val;
> + regcache_raw_read_unsigned (aarch64_insn_r->regcache,
> + bits (aarch64_insn_r->aarch64_insn, 16, 20), ®_rm_val);
> + if (bit (aarch64_insn_r->aarch64_insn, 12))
> + offset = reg_rm_val << size_bits;
> + else
> + offset = reg_rm_val;
> + datasize = 8 << size_bits;
> + regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rn,
> + &address);
> + address = address + offset;
> + record_buf_mem[0] = datasize >> 3;
> + record_buf_mem[1] = address;
> + aarch64_insn_r->mem_rec_count = 1;
> + }
> + else
> + {
> + if (vector_flag)
> + record_buf[0] = reg_rt + AARCH64_V0_REGNUM;
> + else
> + record_buf[0] = reg_rt;
> + aarch64_insn_r->reg_rec_count = 1;
> + }
> + }
> + /* Load/store register (immediate) instructions. */
To be clear, Load/store register (unprivileged) instruction goes into
this path too. Please update the comment to reflect this.
--
Yao (éå)