Re: [PATCH] arm reversible : progress <phase_2_complete>

[Yao Qi <yao@codesourcery.com>]

On 04/10/2011 05:41 PM, paawan oza wrote:
> Hi,
> 
> phase2 (both arm and thumb insn implemenation is complete)
> 
> Hi Tom: 
> I have taken care of most of your comments: but could nit incorporate numercial 
> to be replaced by some meaningful symbolic names.
> as I dont know what sort of symbolic names are approproate.
> 
> 

This one looks much better than previous ones.  Thanks for working on
this.  I am not the people to approve this patch.  Some of my cents below.

> +
> +#define ARM_INSN_SIZE_BYTES 4    
> +#define THUMB_INSN_SIZE_BYTES 2
> +#define NO_OF_TYPE_OF_ARM_INSNS 8
> +#define NO_OF_TYPE_OF_THUMB_INSNS 8
> +
> +#define ARM_RECORD_ARCH_LIST_ADD_REG(regnum) \
> +    record_arch_list_add_reg (arm_record.regcache, regnum)
> +
> +#define GET_REG_VAL(REGCACHE,NO,BUF)  regcache_raw_read (REGCACHE, NO, BUF);
> +
> +#define IS_IT_ARM_INSN(X) ((X & 0x00000020) >> 5)
> +#define ARM_PARSE_INSN(X,BIT_POS,NO_OF_BITS) \
> +                ((X >> (BIT_POS-1)) & (0xFFFFFFFF >> ((sizeof(uint32_t)*8) - \
> +                  NO_OF_BITS)))


There are some existing macros you can use for parsing instructions.

/* Support routines for instruction parsing.  */
#define submask(x) ((1L << ((x) + 1)) - 1)
#define bit(obj,st) (((obj) >> (st)) & 1)
#define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
#define sbits(obj,st,fn) \
  ((long) (bits(obj,st,fn) | ((long) bit(obj,fn) * ~ submask (fn - st))))

> +
> +#define INSN_S_L_BIT_NUM 21
> +#define ARM_BIT_SET(X, NUM) (((X >> (NUM-1)) & 0x00000001) == 1)  
> +#define GET_BIT(X, NUM) (((X >> (NUM-1)) & 0x00000001))
> +

GET_BIT can be replaced by `bit' I posted above.

> +
> +static int 
> +handle_extension_space (insn_decode_record *arm_record)
> +{
> +  insn_decode_record *arm_insn_r = arm_record;
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (arm_insn_r->gdbarch);
> +  struct regcache *reg_cache = arm_insn_r->regcache;
> +    
> +  uint32_t reg_src1 = 0, reg_src2 = 0;
> +  uint32_t opcode1 = 0, opcode2 = 0;
> +
> +  opcode1 = ARM_PARSE_INSN (arm_insn_r->arm_insn,26,3);
> +  if ((3 == opcode1) && ARM_BIT_SET(arm_insn_r->arm_insn,5))
> +    {
> +      /* undefined instruction on ARM V5; need to handle if later versions
> +          define it.  */
> +    }
> +  
> +  opcode2 = ARM_PARSE_INSN (arm_insn_r->arm_insn,5,4);
> +  
> +  if ((!opcode1) && (9 == opcode2))
> +    {
> +      /* handle arithmetic insn extension space.  */
> +    }
> +
> +  opcode1 = ARM_PARSE_INSN (arm_insn_r->arm_insn,27,2);
                                                   ^  ^
You need an extra space after each comma.

> +  opcode2 = ARM_PARSE_INSN (arm_insn_r->arm_insn,24,2);
> +
> +  if ((!opcode1) && (2 == opcode2) && !ARM_BIT_SET(arm_insn_r->arm_insn,21))
> +    {
> +      /* handle control insn extension space.  */
> +    }
> +
> +  opcode1 = ARM_PARSE_INSN (arm_insn_r->arm_insn,26,3);
> +  if ((!opcode1) && (ARM_BIT_SET(arm_insn_r->arm_insn,8)) \
> +                 && (ARM_BIT_SET(arm_insn_r->arm_insn,5)))
> +    {
> +      /* handle load/store insn extension space.  */
> +    }
> +
> +  opcode1 = ARM_PARSE_INSN (arm_insn_r->arm_insn,24,5);
> +  if ((24 == opcode1) && ARM_BIT_SET(arm_insn_r->arm_insn,22))
> +    {
> +      /* handle coprocessor insn extension space.  */
> +    }
> +
> +  /* to be done for ARMv5 and later; as of now we return -1.  */
> +  return -1;
> +}
> +

> +
> +/* Parse the current instruction and record the values of the registers and
> +   memory that will be changed in current instruction to "record_arch_list".
> +   Return -1 if something is wrong..  */
> +
> +int 
> +arm_process_record (struct gdbarch *gdbarch, struct regcache *regcache,
> +                             CORE_ADDR insn_addr)
> +{
> +
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);  
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  uint32_t no_of_rec=0;
> +  uint32_t ret=0;
> +
> +  union
> +    {
> +      uint32_t s_word;
> +      gdb_byte buf[4];
> +    } u_buf;
> +
> +  insn_decode_record arm_record;
> +  memset (&u_buf, 0, sizeof(u_buf));
> +
> +  memset (&arm_record, 0, sizeof (insn_decode_record));
> +  arm_record.regcache = regcache;
> +  arm_record.this_addr = insn_addr;
> +  arm_record.gdbarch = gdbarch;
> +
> +
> +  if (record_debug > 1)
> +    {
> +      fprintf_unfiltered (gdb_stdlog, "Process record: arm_process_record "
> +                                      "addr = %s\n",
> +      paddress (gdbarch, arm_record.this_addr));
> +    }
> +
> +  /* check the insn, whether it is thumb or arm one.  */
> +  GET_REG_VAL (arm_record.regcache, ARM_PS_REGNUM, &u_buf.buf[0]);
> +  arm_record.cond = ARM_PARSE_INSN (arm_record.arm_insn,29,4); 
> +  
> +  if (!IS_IT_ARM_INSN (u_buf.s_word))

Please reference to `arm_frame_is_thumb' or `displaced_in_arm_mode' to
see how to check ARM mode or Thumb mode.

> +    {
> +      /* we are decoding arm insn.  */
> +      ret = decode_insn (&arm_record, ARM_INSN_SIZE_BYTES);      
> +    }
> +  else
> +    {
> +      /* we are decoding thumb insn.  */
> +      ret = decode_insn (&arm_record, THUMB_INSN_SIZE_BYTES);    

On some ARM arch, there are 32-bit Thumb instructions, called Thumb-2.
Do you plan to support Thumb-2 insn?

> +    }
> +
> +  /* record registers.  */
> +  ARM_RECORD_ARCH_LIST_ADD_REG(ARM_PC_REGNUM);
> +  if (arm_record.arm_regs)
> +    {
> +      for (no_of_rec=1;no_of_rec<=arm_record.arm_regs[0];no_of_rec++)
> +        {
> +          if (ARM_RECORD_ARCH_LIST_ADD_REG (arm_record.arm_regs[no_of_rec]))
> +          ret = -1;
> +        }
> +    }   
> +  /* record memories.  */
> +  if (arm_record.arm_mems)
> +    {
> +      for (no_of_rec=1;no_of_rec<=arm_record.arm_mems[0].len;no_of_rec++)
> +       {
> +          if (record_arch_list_add_mem \
> +            ((CORE_ADDR)arm_record.arm_mems[no_of_rec].addr,
> +            arm_record.arm_mems[no_of_rec].len))
> +            ret = -1;
> +       }
> +    }
> +
> +  if (record_arch_list_add_end ())
> +    ret = -1;
> +
> +  if (arm_record.arm_regs)
> +    xfree (arm_record.arm_regs);
> +  if (arm_record.arm_mems)
> +    xfree (arm_record.arm_mems);
> +  
> +  return ret; 
> +}

-- 
Yao (齐尧)