Re: [PATCH] arm reversible : <phase_2_complete>

Yao Qi
2011-10-25T02:19:52Z

On 10/22/2011 11:41 PM, oza Pawandeep wrote:

>
> @Yao: I have tried to take care of formatting as much as possible in
> xemacs editor, execuse me with identation problems and trailing space
> problems.
> I ahev tried to correct it; but not sure it has fallen righlty.

Yeah, it looks much better. I still have some comments below.

>
>
> PATCH STARTS
> -----------------------
> diff -wurN arm_orig/arm-linux-tdep.c arm_new/arm-linux-tdep.c
> --- arm_orig/arm-tdep.c 2011-07-28 09:40:19.000000000 +0530
> +++ arm_new/arm-tdep.c 2011-10-22 17:48:05.000000000 +0530
> @@ -8821,3 +8823,2005 @@
> NULL, /* FIXME: i18n: "ARM debugging is %s. */
> &setdebuglist, &showdebuglist);
> }
> +
> +
> +
> +/* ARM-reversible process record data structures. */
> +
> +#define ARM_INSN_SIZE_BYTES 4
> +#define THUMB_INSN_SIZE_BYTES 2
> +#define THUMB2_INSN_SIZE_BYTES 4
> +

macro THUMB2_INSN_SIZE_BYTES is never used. Please remove it.

> +#define INSN_S_L_BIT_NUM 20
> +
> +#define REG_ALLOC(REGS,LENGTH,RECORD_BUF) \
^ space before "(" and ",".
> +do \
> + { \
> + unsigned int reg_len = LENGTH; \
> + if (reg_len) \
> + { \
> + REGS = XNEWVEC (uint32_t, reg_len); \
> + memcpy(&REGS[0],&RECORD_BUF[0],sizeof(uint32_t)*LENGTH); \
^ ^ spaces are needed before "(" and ",".

> + } \
> + } \
> +while (0)
> +
> +#define MEM_ALLOC(MEMS,LENGTH,RECORD_BUF) \
> +do \
> + { \
> + unsigned int mem_len = LENGTH; \
> + if (mem_len) \
> + { \
> + MEMS = XNEWVEC (struct arm_mem_r, mem_len); \
> + memcpy(&MEMS->len,&RECORD_BUF[0],sizeof(struct arm_mem_r) * LENGTH); \
> + } \
> + } \
> +while (0)
> +
> +#define INSN_RECORDED(ARM_RECORD) \
> + ((ARM_RECORD->arm_regs || ARM_RECORD->arm_mems))
> +
> +/* ARM 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() ). */
> +
> +typedef struct insn_decode_record_t
> +{
> + struct gdbarch *gdbarch;
> + struct regcache *regcache;
> + CORE_ADDR this_addr; /* Address of the insn being decoded. */
> + uint32_t arm_insn; /* Should accommodate thumb. */
> + uint32_t cond; /* Condition code. */
> + uint32_t opcode; /* Insn opcode. */
> + uint32_t decode; /* Insn decode bits. */
> + uint32_t mem_rec_count; /* No of mem records */
> + uint32_t reg_rec_count; /* No of reg records */
> + uint32_t *arm_regs; /* Registers to be saved for this record. */
> + struct arm_mem_r *arm_mems; /* Memory to be saved for this record. */
> +} insn_decode_record;
> +
> +
> +/* Checks ARM SBZ and SBO mandatory fields. */
> +
> +static int
> +sbo_sbz (uint32_t insn, uint32_t bit_num, uint32_t len, uint32_t sbo)
> +{
> + uint32_t ones = bits (insn, bit_num - 1, (bit_num -1) + (len - 1));
> +
> + if (!len)
> + return 1;
> +
> + if (!sbo)
> + ones = ~ones;
> +
> + while (ones)
> + {
> + if (!(ones & sbo))
> + {
> + return 0;
> + }
> + ones = ones >> 1;
> + }
> + return 1;
> +}
> +

This function is used to match opcode for instructions. Why don't
you use bit operation (AND and OR) and logic operation to match
instruction? Bit operation and logic operation are widely used in
gdb. It is efficient and easy to read. I suggest to replace
sbo_sbz by bit/logical operation when matching instruction.

> +typedef enum
> +{
> + ARM_RECORD_STRH=1,
> + ARM_RECORD_STRD
> +}arm_record_strx_t;
> +
> +static int
> +arm_record_strx(insn_decode_record *arm_insn_r, uint32_t *record_buf,
^ space is missing

> + uint32_t *record_buf_mem, arm_record_strx_t str_type)
^ wrong indentation.

> +{
> +
> + struct regcache *reg_cache = arm_insn_r->regcache;
> + ULONGEST u_regval[2]= {0};
> +
> + uint32_t reg_src1 = 0, reg_src2 = 0;
> + uint32_t immed_high = 0, immed_low = 0,offset_8 = 0, tgt_mem_addr = 0;
> + uint32_t opcode1 = 0;
> +
> + arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 21, 24);
> + arm_insn_r->decode = bits (arm_insn_r->arm_insn, 4, 7);
> + opcode1 = bits (arm_insn_r->arm_insn, 20, 24);
> +
> +
> + if ((14 == arm_insn_r->opcode) || (10 == arm_insn_r->opcode))
> + {
> + /* 1) Handle misc store, immediate offset. */
> + immed_low = bits (arm_insn_r->arm_insn, 0, 3);
> + immed_high = bits (arm_insn_r->arm_insn, 8, 11);
> + reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
> + regcache_raw_read_unsigned (reg_cache, reg_src1
> + , &u_regval[0]);

"," should be in previous line here and somewhere else.

> + if (15 == reg_src1)
^^ 15 is a magic number here and somewhere else.
Please use ARM_PC_REGNUM.

> + {
> + /* If R15 was used as Rn, hence current PC+8. */
> + u_regval[0] = u_regval[0] + 8;
> + }
> + offset_8 = (immed_high << 4) | immed_low;
> + /* Calculate target store address. */
> + if (14 == arm_insn_r->opcode)
> + {
> + tgt_mem_addr = u_regval[0] + offset_8;
> + }
> + else
> + {
> + tgt_mem_addr = u_regval[0] - offset_8;
> + }
> + if (ARM_RECORD_STRH == str_type)
> + {
> + *(record_buf_mem) = 2;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + arm_insn_r->mem_rec_count = 1;
> + }
> + else if (ARM_RECORD_STRD == str_type)
> + {
> + *(record_buf_mem) = 4;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + *(record_buf_mem + 2) = 4;
> + *(record_buf_mem + 3) = tgt_mem_addr + 4;
> + arm_insn_r->mem_rec_count = 2;
> + }
> + }
> + else if ((12 == arm_insn_r->opcode) || (8 == arm_insn_r->opcode))
> + {
> + /* 2) Store, register offset. */
> + /* Get Rm. */
> + reg_src1 = bits (arm_insn_r->arm_insn, 0, 3);
> + /* Get Rn. */
> + reg_src2 = bits (arm_insn_r->arm_insn, 16, 19);
> + regcache_raw_read_unsigned (reg_cache, reg_src1
> + , &u_regval[0]);
> + regcache_raw_read_unsigned (reg_cache, reg_src2
> + , &u_regval[1]);
> + if (15 == reg_src2)
> + {
> + /* If R15 was used as Rn, hence current PC+8. */
> + u_regval[0] = u_regval[0] + 8;
> + }
> + /* Calculate target store address, Rn +/- Rm, register offset. */
> + if (12 == arm_insn_r->opcode)
> + {
> + tgt_mem_addr = u_regval[0] + u_regval[1];
> + }
> + else
> + {
> + tgt_mem_addr = u_regval[1] - u_regval[0];
> + }
> + if (ARM_RECORD_STRH == str_type)
> + {
> + *(record_buf_mem) = 2;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + arm_insn_r->mem_rec_count = 1;
> + }
> + else if (ARM_RECORD_STRD == str_type)
> + {
> + *(record_buf_mem) = 4;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + *(record_buf_mem + 2) = 4;
> + *(record_buf_mem + 3) = tgt_mem_addr + 4;
> + arm_insn_r->mem_rec_count = 2;
> + }
> + }
> + else if ((11 == arm_insn_r->opcode) || (15 == arm_insn_r->opcode)
> + || (2 == arm_insn_r->opcode) || (6 == arm_insn_r->opcode))
> + {
> + /* 3) Store, immediate pre-indexed. */
> + /* 5) Store, immediate post-indexed. */
> + immed_low = bits (arm_insn_r->arm_insn, 0, 3);
> + immed_high = bits (arm_insn_r->arm_insn, 8, 11);
> + offset_8 = (immed_high << 4) | immed_low;
> + reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
> + regcache_raw_read_unsigned (reg_cache, reg_src1
> + , &u_regval[0]);
> + /* Calculate target store address, Rn +/- Rm, register offset. */
> + if ((15 == arm_insn_r->opcode) || (6 == arm_insn_r->opcode))
> + {
> + tgt_mem_addr = u_regval[0] + offset_8;
> + }
> + else
> + {
> + tgt_mem_addr = u_regval[0] - offset_8;
> + }
> + if (ARM_RECORD_STRH == str_type)
> + {
> + *(record_buf_mem) = 2;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + arm_insn_r->mem_rec_count = 1;
> + }
> + else if (ARM_RECORD_STRD == str_type)
> + {
> + *(record_buf_mem) = 4;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + *(record_buf_mem + 2) = 4;
> + *(record_buf_mem + 3) = tgt_mem_addr + 4;
> + arm_insn_r->mem_rec_count = 2;
> + }
> + /* Record Rn also as it changes. */
> + *(record_buf) = bits (arm_insn_r->arm_insn, 16, 19);
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else if ((9 == arm_insn_r->opcode) || (13 == arm_insn_r->opcode)
> + || (0 == arm_insn_r->opcode) || (4 == arm_insn_r->opcode))
> + {
> + /* 4) Store, register pre-indexed. */
> + /* 6) Store, register post -indexed. */
> + reg_src1 = bits (arm_insn_r->arm_insn, 0, 3);
> + reg_src2 = bits (arm_insn_r->arm_insn, 16, 19);
> + regcache_raw_read_unsigned (reg_cache, reg_src1
> + , &u_regval[0]);
> + regcache_raw_read_unsigned (reg_cache, reg_src2
> + , &u_regval[1]);
> + /* Calculate target store address, Rn +/- Rm, register offset. */
> + if ((13 == arm_insn_r->opcode) || (4 == arm_insn_r->opcode))
> + {
> + tgt_mem_addr = u_regval[0] + u_regval[1];
> + }
> + else
> + {
> + tgt_mem_addr = u_regval[1] - u_regval[0];
> + }
> + if (ARM_RECORD_STRH == str_type)
> + {
> + *(record_buf_mem) = 2;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + arm_insn_r->mem_rec_count = 1;
> + }
> + else if (ARM_RECORD_STRD == str_type)
> + {
> + *(record_buf_mem) = 4;
> + *(record_buf_mem + 1) = tgt_mem_addr;
> + *(record_buf_mem + 2) = 4;
> + *(record_buf_mem + 3) = tgt_mem_addr + 4;
> + arm_insn_r->mem_rec_count = 2;
> + }
> + /* Record Rn also as it changes. */
> + *(record_buf) = bits (arm_insn_r->arm_insn, 16, 19);
> + arm_insn_r->reg_rec_count = 1;
> + }
> + return 0;
> +}

[...]
> +
> +/* Handling opcode 000 insns. */
> +
> +static int
> +arm_record_data_proc_misc_ld_str (insn_decode_record *arm_insn_r)
> +{
> + struct regcache *reg_cache = arm_insn_r->regcache;
> + uint32_t record_buf[8], record_buf_mem[8];
> + ULONGEST u_regval[2] = {0};
> +
> + uint32_t reg_src1 = 0, reg_src2 = 0, reg_dest = 0;
> + uint32_t immed_high = 0, immed_low = 0,offset_8 = 0, tgt_mem_addr = 0;
> + uint32_t opcode1 = 0;
> +
> + arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 21, 24);
> + arm_insn_r->decode = bits (arm_insn_r->arm_insn, 4, 7);
> + opcode1 = bits (arm_insn_r->arm_insn, 20, 24);
> +
> + /* Data processing insn /multiply insn. */
> + if ((9 == arm_insn_r->decode)
> + && (((4 <= arm_insn_r->opcode) && (7 >= arm_insn_r->opcode))
> + || ((0 == arm_insn_r->opcode) || (1 == arm_insn_r->opcode))))
> + {
> + /* Handle multiply instructions. */
> + /* MLA, MUL, SMLAL, SMULL, UMLAL, UMULL. */
> + if ((0 == arm_insn_r->opcode) || (1 == arm_insn_r->opcode))
> + {
> + /* Handle MLA and MUL. */
> + record_buf[0] = bits (arm_insn_r->arm_insn, 16, 19);
> + record_buf[1] = ARM_PS_REGNUM;
> + arm_insn_r->reg_rec_count = 2;
> + }
> + else if ((4 <= arm_insn_r->opcode) && (7 >= arm_insn_r->opcode))
> + {
> + /* Handle SMLAL, SMULL, UMLAL, UMULL. */
> + record_buf[0] = bits (arm_insn_r->arm_insn, 16, 19);
> + record_buf[1] = bits (arm_insn_r->arm_insn, 12, 15);
> + record_buf[2] = ARM_PS_REGNUM;
> + arm_insn_r->reg_rec_count = 3;
> + }
> + }
> + else if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM)
> + && ((11 == arm_insn_r->decode) || (13 == arm_insn_r->decode)))
> + {
> + /* Handle misc load insns, as 20th bit (L = 1). */
> + /* LDR insn has a capability to do branching, if
> + MOV LR, PC is precceded by LDR insn having Rn as R15
> + in that case, it emulates branch and link insn, and hence we
> + need to save CSPR and PC as well. I am not sure this is right
> + place as opcode = 010 LDR insn make this happen, if R15 was
> + used. */
> + reg_dest = bits (arm_insn_r->arm_insn, 12, 15);
> + if (15 != reg_dest)
> + {
> + record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else
> + {
> + record_buf[0] = reg_dest;
> + record_buf[1] = ARM_PS_REGNUM;
> + arm_insn_r->reg_rec_count = 2;
> + }
> + }
> + else if (((9 == arm_insn_r->opcode) || (11 == arm_insn_r->opcode))
> + && sbo_sbz (arm_insn_r->arm_insn, 5, 12, 0)
> + && sbo_sbz (arm_insn_r->arm_insn, 13, 4, 1)
> + && 2 == bits (arm_insn_r->arm_insn, 20, 21))
> + {
> + /* Handle MSR insn. */
> + if (9 == arm_insn_r->opcode)
> + {
> + /* CSPR is going to be changed. */
> + record_buf[0] = ARM_PS_REGNUM;
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else
> + {
> + /* SPSR is going to be changed. */
> + /* How to read SPSR value? */
> + printf_unfiltered (_("Process record does not support instruction "
> + "0x%0x at address %s.\n"),
> + arm_insn_r->arm_insn,
> + paddress (arm_insn_r->gdbarch,
> arm_insn_r->this_addr));
> + return -1;
> + }
> + }
> + else if ((9 == arm_insn_r->decode)
> + && ((8 == arm_insn_r->opcode) || (10 == arm_insn_r->opcode))
> + && !bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
> + {
> + /* Handling SWP, SWPB. */
> + /* These insn, changes register and memory as well. */
> + /* SWP or SWPB insn. */
> +
> + reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
> + regcache_raw_read_unsigned (reg_cache, reg_src1
> + , &u_regval[0]);
> + /* SWP insn ?, swaps word. */
> + if (8 == arm_insn_r->opcode)
> + {
> + record_buf_mem[0] = 4;
> + }
> + else
> + {
> + /* SWPB insn, swaps only byte. */
> + record_buf_mem[0] = 1;
> + }
> + record_buf_mem[1] = u_regval[0];
> + arm_insn_r->mem_rec_count = 1;
> + record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else if ((3 == arm_insn_r->decode) && (0x12 == opcode1)
> + && sbo_sbz (arm_insn_r->arm_insn, 9, 12, 1))
> + {
> + /* Handle BLX, branch and link/exchange. */
> + if (9 == arm_insn_r->opcode)
> + {
> + /* Branch is chosen by setting T bit of CSPR, bitp[0] of Rm,
> + and R14 stores the return address. */
> + record_buf[0] = ARM_PS_REGNUM;
> + record_buf[1] = ARM_LR_REGNUM;
> + arm_insn_r->reg_rec_count = 2;
> + }
> + }
> + else if ((7 == arm_insn_r->decode) && (0x12 == opcode1))
> + {
> + /* Handle enhanced software breakpoint insn, BKPT. */
> + /* CPSR is changed to be executed in ARM state, disabling normal
> + interrupts, entering abort mode. */
> + /* According to high vector configuration PC is set. */
> + /* What if user hit breakpoint and type reverse, in
> + that case, we need to go back with previous CPSR and
> + Program Counter. */
> + record_buf[0] = ARM_PS_REGNUM;
> + record_buf[1] = ARM_LR_REGNUM;
> + arm_insn_r->reg_rec_count = 2;
> +
> + /* Save SPSR also; how? */
> + printf_unfiltered (_("Process record does not support instruction "
> + "0x%0x at address %s.\n"),
> + arm_insn_r->arm_insn,
> + paddress (arm_insn_r->gdbarch, arm_insn_r->this_addr));
> + return -1;
> + }
> + else if ((11 == arm_insn_r->decode)
> + && !bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
> + {
> + /* Handle enhanced store insns and DSP insns (e.g. LDRD). */
> + arm_record_strx(arm_insn_r, &record_buf[0], &record_buf_mem[0],
^ space is required.

> + ARM_RECORD_STRH);
> + /* DSP insns (e.g. LDRD) TBD. */

Comment here is confusing. STRH (register) and STRH (immediate) are
handled here, but comment doesn't reflect this.

> + }
> + else if ((1 == arm_insn_r->decode) && (0x12 == opcode1)
> + && sbo_sbz (arm_insn_r->arm_insn, 9, 12, 1))
> + {
> + /* Handle BX, branch and link/exchange. */
> + /* Branch is chosen by setting T bit of CSPR, bitp[0] of Rm. */
> + record_buf[0] = ARM_PS_REGNUM;
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else if ((1 == arm_insn_r->decode) && (0x16 == opcode1)
> + && sbo_sbz (arm_insn_r->arm_insn, 9, 4, 1)
> + && sbo_sbz (arm_insn_r->arm_insn, 17, 4, 1))
> + {
> + /* Count leading zeros: CLZ. */
> + record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else if (!bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM)
> + && ((8 == arm_insn_r->opcode) || (10 == arm_insn_r->opcode))
> + && sbo_sbz (arm_insn_r->arm_insn, 17, 4, 1)
> + && sbo_sbz (arm_insn_r->arm_insn, 1, 12, 0)
> + )
> + {
> + /* Handle MRS insn. */
> + record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
> + arm_insn_r->reg_rec_count = 1;
> + }
> + else if (arm_insn_r->opcode <= 15)
> + {
> + /* Normal data processing insns. */
> + /* Out of 11 shifter operands mode, all the insn modifies destination
> + register, which is specified by 13-16 decode. */
> + record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
> + record_buf[1] = ARM_PS_REGNUM;
> + arm_insn_r->reg_rec_count = 2;
> + }
> + else
> + {
> + return -1;
> + }
> +
> + REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, record_buf);
> + MEM_ALLOC (arm_insn_r->arm_mems, arm_insn_r->mem_rec_count, record_buf_mem);
> + return 0;
> +}
> +

> +/* Handling opcode 110 insns. */
> +
> +static int
> +thumb_record_swi (insn_decode_record *thumb_insn_r)

The name of function is incorrect. Better to name it
"thumb_record_ldm_stm_swi", or something similar.

> +{
> + struct gdbarch_tdep *tdep = gdbarch_tdep (thumb_insn_r->gdbarch);
> + struct regcache *reg_cache = thumb_insn_r->regcache;
> +
> + uint32_t ret = 0;
> + uint32_t reg_src1 = 0;
> + uint32_t opcode1 = 0, opcode2 = 0, register_bits = 0, register_count = 0;
> + uint32_t register_list[8] = {0}, index = 0, start_address = 0;
> + uint32_t record_buf[24], record_buf_mem[48];
> +
> + ULONGEST u_regval = 0;
> +
> + opcode1 = bits (thumb_insn_r->arm_insn, 8, 12);
> + opcode2 = bits (thumb_insn_r->arm_insn, 11, 12);
> +
> + if (1 == opcode2)
> + {
> +
> + /* LDMIA. */
> + register_bits = bits (thumb_insn_r->arm_insn, 0, 7);
> + /* Get Rn. */
> + reg_src1 = bits (thumb_insn_r->arm_insn, 8, 10);
> + while (register_bits)
> + {
> + if (register_bits & 0x00000001)
> + register_list[register_count++] = 1;
> + register_bits = register_bits >> 1;
> + }
> + record_buf[register_count] = reg_src1;
> + thumb_insn_r->reg_rec_count = register_count + 1;
> + for (register_count = 0; register_count < 8; register_count++)
> + {
> + if (register_list[register_count])
> + {
> + record_buf[index] = register_count;
> + index++;
> + }
> + }
> + }
> + else if (0 == opcode2)
> + {
> + /* It handles both STMIA. */
> + register_bits = bits (thumb_insn_r->arm_insn, 0, 7);
> + /* Get Rn. */
> + reg_src1 = bits (thumb_insn_r->arm_insn, 8, 10);
> + regcache_raw_read_unsigned (reg_cache, reg_src1, &u_regval);
> + while (register_bits)
> + {
> + if (register_bits & 0x00000001)
> + register_count++;
> + register_bits = register_bits >> 1;
> + }
> + start_address = u_regval;
> + thumb_insn_r->mem_rec_count = register_count;
> + while (register_count)
> + {
> + record_buf_mem[(register_count * 2) - 1] = start_address;
> + record_buf_mem[(register_count * 2) - 2] = 4;
> + start_address = start_address + 4;
> + register_count--;
> + }
> + }
> + else if (0x1F == opcode1)
> + {
> + /* Handle arm syscall insn. */
> + if (tdep->arm_swi_record != NULL)
> + {
> + ret = tdep->arm_swi_record(reg_cache);
> + }
> + else
> + {
> + printf_unfiltered (_("no syscall record support\n"));
> + return -1;
> + }
> + }
> +
> + /* B(1), conditional branch is automatically taken care in process_record,
> + as PC is saved there. */

Redundant comments?

> +
> + REG_ALLOC (thumb_insn_r->arm_regs, thumb_insn_r->reg_rec_count, record_buf);
> + MEM_ALLOC (thumb_insn_r->arm_mems, thumb_insn_r->mem_rec_count,
> + record_buf_mem);
> +
> + return ret;
> +}
> +
> +/* Handling opcode 111 insns. */
> +
> +static int
> +thumb_record_branch (insn_decode_record *thumb_insn_r)
> +{
> + uint32_t record_buf[8];
> +
> + /* BL , BLX(1). */
> + record_buf[0] = ARM_PS_REGNUM;
> + record_buf[1] = ARM_LR_REGNUM;
> + thumb_insn_r->reg_rec_count = 2;
> +

Again, don't have to store register PS for BL.

> + /* B(2) is automatically taken care in process_record, as PC is saved
> + there. */
> +
> + REG_ALLOC (thumb_insn_r->arm_regs, thumb_insn_r->reg_rec_count, record_buf);
> +
> + return 0;
> +}
> +
> +
> +/* Decode arm/thumb insn depending on condition cods and opcodes; and
> dispatch it. */
> +
> +static int
> +decode_insn (insn_decode_record *arm_record, uint32_t insn_size)
> +{
> +
> + /* (Starting from numerical 0); bits 25, 26, 27 decodes type of arm
> instruction. */
> + static int (*const arm_handle_insn[8])
> + (insn_decode_record*) =
> + {
> + arm_record_data_proc_misc_ld_str, /* 000. */
> + arm_record_data_proc_imm, /* 001. */
> + arm_record_ld_st_imm_offset, /* 010. */
> + arm_record_ld_st_reg_offset, /* 011. */
> + arm_record_ld_st_multiple, /* 100. */
> + arm_record_b_bl, /* 101. */
> + arm_record_coproc, /* 110. */
> + arm_record_coproc_data_proc /* 111. */
> + };
> +
> + /* (Starting from numerical 0); bits 13,14,15 decodes type of thumb
> instruction. */
> + static int (*const thumb_handle_insn[8])
> + (insn_decode_record*) =
> + { \
> + thumb_record_shift_add_sub, /* 000. */
> + thumb_record_add_sub_cmp_mov, /* 001. */
> + thumb_record_ld_st_reg_offset, /* 010. */
> + thumb_record_ld_st_imm_offset, /* 011. */
> + thumb_record_ld_st_stack, /* 100. */
> + thumb_record_misc, /* 101. */
> + thumb_record_swi, /* 110. */
> + thumb_record_branch /* 111. */
> + };
> +
> + struct
> + {
> + gdb_byte buf[insn_size];
> + } u_buf;
> +
> + uint32_t ret=0, insn_id = 0;
> +
> + memset (&u_buf, 0, sizeof(u_buf));
> + if (target_read_memory (arm_record->this_addr, &u_buf.buf[0], insn_size))
> + {
> + if (record_debug)
> + {
> + printf_unfiltered (_("Process record: error reading memory at "
> + "addr %s len = %d.\n"),
> + paddress (arm_record->gdbarch, arm_record->this_addr), insn_size);
> + return -1;
> + }
> + }
> + else if (ARM_INSN_SIZE_BYTES == insn_size)
> + {
> + arm_record->arm_insn = (uint32_t) extract_unsigned_integer (&u_buf.buf[0]
> + , ARM_INSN_SIZE_BYTES , gdbarch_byte_order (arm_record->gdbarch));
> + arm_record->cond = bits (arm_record->arm_insn, 28, 31);
> + insn_id = bits (arm_record->arm_insn, 25, 27);
> + arm_record_extension_space (arm_record);
> + /* if this insn has fallen into extension space then we need
> not decode it anymore. */
> + if (!INSN_RECORDED(arm_record))
> + {
> + arm_handle_insn[insn_id] (arm_record);
> + }
> + }
> + else if (THUMB_INSN_SIZE_BYTES == insn_size)
> + {
> + /* As thumb does not have condition codes, we set negatiive. */
> + arm_record->cond = -1;
> + arm_record->arm_insn = (uint32_t) extract_unsigned_integer (&u_buf.buf[0]
> + , THUMB_INSN_SIZE_BYTES , gdbarch_byte_order
> (arm_record->gdbarch));
> +
> + insn_id = bits (arm_record->arm_insn, 11, 15);
> + /* is it thumb2 insn? */
> + if ((0x1D == insn_id) || (0x1E == insn_id) || (0x1F == insn_id))
> + {
> + printf_unfiltered (_("Process record does not support thumb32 "
> + "instruction 0x%0x at address %s.\n"),
> + arm_record->arm_insn,
> + paddress (arm_record->gdbarch,
> arm_record->this_addr));
> + ret = -1;
> + }
> + insn_id = bits (arm_record->arm_insn, 13, 15);
> + ret = thumb_handle_insn[insn_id] (arm_record);
> + }

This kind of design leaves thumb2-32 bit support difficult in the
future. We can let the caller, arm_process_record, to differentiate
arm/thumb 16bit/thumb 32bit, and call different decode routines
respectively, like this,

int
arm_process_record
{
if (!(u_regval & t_bit))
{
/* We are decoding arm insn. */
ret = arm_record_decode_arm (&arm_record);
}
else
{
insn = read_memory_unsigned_integer (start, 2, byte_order_for_code);
if (thumb_insn_size (insn) == 4)
ret = arm_record_decode_thumb_32bit (&arm_record);
else
ret = arm_record_decode_thumb_16bit (&arm_record);
}

....
}

In arm_record_decode_thumb_32bit, just print a warning like what we
have now. It makes future work easier.

> + else
> + {
> + /* Throw assertion. */
> + gdb_assert (0);
> + }
> +
> + return ret;
> +}
> +

--
Yao (é½å°§)