From: Yao Qi <qiyaoltc@gmail.com>
To: Omair Javaid <omair.javaid@linaro.org>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v4 2/5] Implements aarch64 process record and reverse debugging support
Date: Thu, 19 Feb 2015 12:35:00 -0000 [thread overview]
Message-ID: <86fva2yt8z.fsf@gmail.com> (raw)
In-Reply-To: <1422926216-15740-3-git-send-email-omair.javaid@linaro.org> (Omair Javaid's message of "Tue, 3 Feb 2015 06:16:53 +0500")
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 (齐尧)
next prev parent reply other threads:[~2015-02-19 12:35 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-03 1:17 [PATCH v4 0/5] Process record and reverse debugging support on aarch64-linux Omair Javaid
2015-02-03 1:17 ` [PATCH v4 5/5] Enables gdb.reverse testsuite for aarch64*-linux targets Omair Javaid
2015-02-20 9:57 ` Yao Qi
2015-02-03 1:17 ` [PATCH v4 2/5] Implements aarch64 process record and reverse debugging support Omair Javaid
2015-02-19 12:35 ` Yao Qi [this message]
2015-02-03 1:17 ` [PATCH v4 3/5] Support for recording syscall on aarch64-linux Omair Javaid
2015-02-19 16:55 ` Yao Qi
2015-02-03 1:17 ` [PATCH v4 4/5] Support for recording aarch64 advanced SIMD instructions Omair Javaid
2015-02-20 9:53 ` Yao Qi
2015-02-03 1:17 ` [PATCH v4 1/5] NEWS entry about aarch64-linux record/replay support Omair Javaid
2015-02-03 3:38 ` Eli Zaretskii
2015-02-03 1:32 ` [PATCH v4 0/5] Process record and reverse debugging support on aarch64-linux Omair Javaid
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86fva2yt8z.fsf@gmail.com \
--to=qiyaoltc@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=omair.javaid@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).