Re: [PATCH 1/2] GDB process record and reverse debugging improvements for arm*-linux*

[Pedro Alves <palves at redhat dot com>]

On 11/24/2013 11:49 PM, Omair Javaid wrote:

> Patch has been updated after incorporating suggestions. Looking for a final
> commit verdict.
> 
> This patch adds support for recording thumb32 instructions. This patch also
> fixes bugs in arm process record instruction decoding.

It's not clear what this "also fixes bugs" refers to.  Could that part be
something that could be split into it's own patch?

> gdb:
> 
> 2013-11-08  Omair Javaid  <omair.javaid@linaro.org>
> 
> 	* arm-tdep.c (struct arm_mem_r): Use uint32_t for memory address.
> 	(arm_record_unsupported_insn): New function.
> 	(arm_record_coproc_data_proc): Removed.
> 	(thumb2_record_ld_st_multiple): New function.
> 	(thumb2_record_ld_st_dual_ex_tbb): New function.
> 	(thumb2_record_data_proc_sreg_mimm): New function.
> 	(thumb2_record_ps_dest_generic): New function.
> 	(thumb2_record_branch_misc_cntrl): New function.
> 	(thumb2_record_str_single_data): New function.
> 	(thumb2_record_ld_mem_hints): New function.
> 	(thumb2_record_ld_word): New function.
> 	(thumb2_record_lmul_lmla_div): New function.
> 	(thumb2_record_decode_inst_id): New function.
> 	(decode_insn): Add thumb32 instruction handlers.
> 


> +/* Handler for thumb2 load/store multiple instructions.  */
> +
> +static int
> +thumb2_record_ld_st_multiple (insn_decode_record *thumb2_insn_r)
> +{
> +  struct regcache *reg_cache = thumb2_insn_r->regcache;
> +
> +  uint32_t reg_rn, op;
> +  uint32_t register_bits = 0, register_count = 0;
> +  uint32_t index = 0, start_address = 0;
> +  uint32_t record_buf[24], record_buf_mem[48];
> +
> +  ULONGEST u_regval = 0;
> +
> +  reg_rn = bits (thumb2_insn_r->arm_insn, 16, 19);
> +  op = bits (thumb2_insn_r->arm_insn, 23, 24);
> +
> +  if (0 == op || 3 == op)
> +    {
> +      if (bit (thumb2_insn_r->arm_insn, INSN_S_L_BIT_NUM))
> +        {
> +          /* Handle RFE instruction.  */
> +          record_buf[0] = ARM_PS_REGNUM;
> +          thumb2_insn_r->reg_rec_count = 1;
> +        }
> +      else
> +        {
> +          /* Handle SRS instruction after reading banked SP.  */
> +          return arm_record_unsupported_insn (thumb2_insn_r);
> +        }
> +    }
> +  else if(1 == op || 2 == op)

Missing space before parens.

> +    {
> +      if (bit (thumb2_insn_r->arm_insn, INSN_S_L_BIT_NUM))
> +        {
> +          /* Handle LDM/LDMIA/LDMFD and LDMDB/LDMEA instructions.  */
> +          register_bits = bits (thumb2_insn_r->arm_insn, 0, 15);
> +          while (register_bits)
> +            {
> +              if (register_bits & 0x00000001)
> +                record_buf[index++] = register_count;
> +
> +              register_count++;
> +              register_bits = register_bits >> 1;
> +            }
> +          record_buf[index++] = reg_rn;
> +          record_buf[index++] = ARM_PS_REGNUM;
> +          thumb2_insn_r->reg_rec_count = index;
> +        }
> +      else
> +        {
> +          /* Handle STM/STMIA/STMEA and STMDB/STMFD.  */
> +          register_bits = bits (thumb2_insn_r->arm_insn, 0, 15);
> +          regcache_raw_read_unsigned (reg_cache, reg_rn, &u_regval);
> +          while (register_bits)
> +            {
> +              if (register_bits & 0x00000001)
> +                register_count++;
> +
> +              register_bits = register_bits >> 1;
> +            }
> +
> +          if (1 == op)
> +            {
> +              /* Start address calculation for LDMDB/LDMEA.  */
> +              start_address = u_regval;
> +            }
> +          else if (2 == op)
> +            {
> +              /* Start address calculation for LDMDB/LDMEA.  */
> +              start_address = (u_regval) - (register_count * 4);

Unnecessary parens.

> +            }
> +
> +          thumb2_insn_r->mem_rec_count = register_count;
> +          while (register_count)

          while (register_count > 0)

> +            {
> +              record_buf_mem[(register_count * 2) - 1] = start_address;
> +              record_buf_mem[(register_count * 2) - 2] = 4;

Unnecessary parens.

> +              start_address = start_address + 4;
> +              register_count--;
> +            }
> +          record_buf[0] = reg_rn;
> +          record_buf[1] = ARM_PS_REGNUM;
> +          thumb2_insn_r->reg_rec_count = 2;
> +        }
> +    }
> +
> +  MEM_ALLOC (thumb2_insn_r->arm_mems, thumb2_insn_r->mem_rec_count,
> +            record_buf_mem);
> +  REG_ALLOC (thumb2_insn_r->arm_regs, thumb2_insn_r->reg_rec_count,
> +            record_buf);
> +  return 0;
> +}
> +



> +/* Handler for thumb2 load memory hints instructions.  */
> +
> +static int
> +thumb2_record_ld_mem_hints (insn_decode_record *thumb2_insn_r)
> +{
> +  uint32_t record_buf[8];
> +  uint32_t reg_rt, reg_rn;
> +
> +  reg_rt = bits (thumb2_insn_r->arm_insn, 12, 15);
> +  reg_rn = bits (thumb2_insn_r->arm_insn, 16, 19);
> +
> +  if (ARM_PC_REGNUM != reg_rt)
> +    {
> +      record_buf[0] = reg_rt;
> +      record_buf[1] = reg_rn;
> +      record_buf[2] = ARM_PS_REGNUM;
> +      thumb2_insn_r->reg_rec_count = 3;
> +
> +      REG_ALLOC (thumb2_insn_r->arm_regs, thumb2_insn_r->reg_rec_count,
> +                record_buf);
> +      return 0;
> +    }
> +
> +  return -1;
> +}
> +

> +/* Handler for thumb2 long multiply, long multiply accumulate, and
> +   divide instructions.  Return value: -1:record failure ;  0:success.  */

"Returns -1 on failure, 0 on success."

Most of the other functions lack describing the return.  Please update
them too.

> +
> +static int
> +thumb2_record_lmul_lmla_div (insn_decode_record *thumb2_insn_r)
> +{
> +  uint32_t ret = 0;
> +  uint32_t opcode1 = 0, opcode2 = 0;
> +  uint32_t record_buf[8];
> +  uint32_t reg_src1 = 0;
> +
> +  opcode1 = bits (thumb2_insn_r->arm_insn, 20, 22);
> +  opcode2 = bits (thumb2_insn_r->arm_insn, 4, 7);
> +
> +  if (0 == opcode1 || 2 == opcode1 || (opcode1 >= 4 && opcode1 <= 6))
> +    {
> +      /* Handle SMULL, UMULL, SMULAL.  */
> +      /* Handle SMLAL(S), SMULL(S), UMLAL(S), UMULL(S).  */
> +      record_buf[0] = bits (thumb2_insn_r->arm_insn, 16, 19);
> +      record_buf[1] = bits (thumb2_insn_r->arm_insn, 12, 15);
> +      record_buf[2] = ARM_PS_REGNUM;
> +      thumb2_insn_r->reg_rec_count = 3;
> +    }
> +  else if (1 == opcode1 || 3 == opcode2)
> +    {
> +      /* Handle SDIV and UDIV.  */
> +      record_buf[0] = bits (thumb2_insn_r->arm_insn, 16, 19);
> +      record_buf[1] = bits (thumb2_insn_r->arm_insn, 12, 15);
> +      record_buf[2] = ARM_PS_REGNUM;
> +      thumb2_insn_r->reg_rec_count = 3;
> +    }
> +  else
> +    ret = -1;
> +
> +  REG_ALLOC (thumb2_insn_r->arm_regs, thumb2_insn_r->reg_rec_count,
> +            record_buf);
> +  return ret;
> +}
> +
> +/* Decodes thumb2 instruction type and return an instruction id.  */

Mention also the -1 on failure.  But, where are these instruction ids
described?  Can this be an enum?

> +
> +static unsigned int
> +thumb2_record_decode_inst_id (uint32_t thumb2_insn)
> +{

> +  return -1;
> +}
>  

> +  /* (Starting from numerical 0); bits 13,14,15 decodes type of thumb
> +     instruction.  */
> +  static const sti_arm_hdl_fp_t const thumb2_handle_insn[16] =
> +  { \

No need for that \.

> +    thumb2_record_ld_st_multiple,       /* 00.  */
> +    thumb2_record_ld_st_dual_ex_tbb,    /* 01.  */
> +    thumb2_record_data_proc_sreg_mimm,  /* 02.  */
> +    arm_record_unsupported_insn,        /* 03.  */
> +    thumb2_record_data_proc_sreg_mimm,  /* 04.  */
> +    thumb2_record_ps_dest_generic,      /* 05.  */
> +    thumb2_record_branch_misc_cntrl,    /* 06.  */
> +    thumb2_record_str_single_data,      /* 07.  */
> +    arm_record_unsupported_insn,        /* 08.  */
> +    thumb2_record_ld_mem_hints,         /* 09.  */
> +    thumb2_record_ld_mem_hints,         /* 10.  */
> +    thumb2_record_ld_word,              /* 11.  */
> +    thumb2_record_ps_dest_generic,      /* 12.  */
> +    thumb2_record_ps_dest_generic,      /* 13.  */
> +    thumb2_record_lmul_lmla_div,        /* 14.  */
> +    arm_record_unsupported_insn         /* 15.  */
> +  };

So, that number was an index into this table?  Why not simply
have thumb2_record_decode_inst_id return a sti_arm_hdl_fp_t directly?
Or better, just have thumb2_record_decode_inst_id call the function
directly (renamed to thumb2_record_handle_insn)?  AFAICS, this
is only used in one place:

> +
> +      insn_id = thumb2_record_decode_inst_id (arm_record->arm_insn);
> +
> +      if (insn_id >= 0)
> +        ret = thumb2_handle_insn[insn_id] (arm_record);
> +      else
> +        {
> +          arm_record_unsupported_insn(arm_record);
> +          ret = -1;
> +        }

That could then just be:

 	 ret = thumb2_record_handle_insn (arm_record->arm_insn);

BTW, "insn" is much more common in GDB as short for instruction
than inst.

> +
>    uint32_t ret = 0;    /* return value: negative:failure   0:success.  */

Please use full sentences instead of that ':' syntax.  Document the
return code in the function's intro instead, and then you don't need
to document it here.  A variable called 'ret' becomes self describing.

-- 
Pedro Alves