Re: [PATCH] arm reversible : <phase_2_complete>

[Yao Qi <yao at codesourcery dot com>]

On 10/15/2011 11:45 AM, paawan oza wrote:
> please find the patch below.
> 

I noticed that test cases in gdb.reverse are *not* enabled in default.
You need to add the following two lines in your board file to turn them on,

  set_board_info gdb,use_precord 1
  set_board_info gdb,can_reverse 1

It is good if you can post the test result in gdb.reverse so that we are
more confidient to your patch.

I am not an expert on record, so I don't have much comment on your code
logic.  However, some of my cents below.

> 
> diff -urN arm_orig/arm-linux-tdep.c arm_new/arm-linux-tdep.c
> --- arm_orig/arm-linux-tdep.c    2011-07-28 09:40:19.000000000 +0530
> +++ arm_new/arm-linux-tdep.c    2011-07-28 09:41:06.000000000 +0530
> @@ -1025,6 +1025,9 @@
>    set_gdbarch_fetch_tls_load_module_address (gdbarch,
>                                               svr4_fetch_objfile_link_map);
>  
> +  /* Enable process record.  */
> +  set_gdbarch_process_record (gdbarch, arm_process_record);
> +
>    tramp_frame_prepend_unwinder (gdbarch,
>                  &arm_linux_sigreturn_tramp_frame);
>    tramp_frame_prepend_unwinder (gdbarch,
> @@ -1054,6 +1057,8 @@
>  
>  
>    tdep->syscall_next_pc = arm_linux_syscall_next_pc;
> +
> +  tdep->arm_swi_record = NULL;
>  }
>  
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
> diff -urN arm_orig/arm-tdep.c arm_new/arm-tdep.c
> --- arm_orig/arm-tdep.c    2011-07-28 09:40:19.000000000 +0530
> +++ arm_new/arm-tdep.c    2011-09-18 12:55:12.000000000 +0530
> @@ -55,6 +55,8 @@
>  #include "gdb_assert.h"
>  #include "vec.h"
>  
> +#include "record.h"
> +
>  #include "features/arm-with-m.c"
>  
>  static int arm_debug;
> @@ -8821,3 +8823,1769 @@
>                 NULL, /* FIXME: i18n: "ARM debugging is %s.  */
>                 &setdebuglist, &showdebuglist);
>  }

> +
> +/* 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() ).  */

These lines exceed the hard limit (80 chars).  The second line of
comment should start from column 4, like this:

/* ARM instruction ...
   and execution state ....  */

> +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;
> +}
> +
> +/* Handling ARM extension space insns.  */
> +
> +static int 
> +handle_extension_space (insn_decode_record *arm_insn_r)
> +{  
> +  uint32_t ret = 0;
> +  uint32_t opcode1 = 0, opcode2 = 0;
> +
> +  opcode1 = bits (arm_insn_r->arm_insn, 25, 27);
> +  if (3 == opcode1 && bit (arm_insn_r->arm_insn, 4))
> +    {
> +      ret = -1;
> +      /* Undefined instruction on ARM V5; need to handle if later versions
> +          define it.  */
            ^ one redundant space.
> +    }
> +  
> +  opcode2 = bits (arm_insn_r->arm_insn, 4, 7);
> +  
> +  if (!opcode1 && (9 == opcode2))
> +    {
> +      ret = -1;
> +      /* Handle arithmetic insn extension space.  */
> +    }
> +
> +  opcode1 = bits (arm_insn_r->arm_insn, 26, 27);
> +  opcode2 = bits (arm_insn_r->arm_insn, 23, 24);
> +
> +  if (!opcode1 && (2 == opcode2) && !bit (arm_insn_r->arm_insn, 20))
> +    {
> +      ret = -1;
> +      /* Handle control insn extension space.  */
> +    }
> +
> +  opcode1 = bits (arm_insn_r->arm_insn, 25, 27);
> +  if (!opcode1 && bit (arm_insn_r->arm_insn, 7) \
> +                 && bit (arm_insn_r->arm_insn, 4))
> +    {
> +      ret = -1;
> +      /* Handle load/store insn extension space.  */
> +    }
> +
> +  opcode1 = bits (arm_insn_r->arm_insn, 23, 27);
> +  if ((24 == opcode1) && bit (arm_insn_r->arm_insn, 21))
> +    {
> +      ret = -1;
> +      /* Handle coprocessor insn extension space.  */
> +    }
> +
> +  /* To be done for ARMv5 and later; as of now we return -1.  */
> +  if (-1 == ret)
> +    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 ret;
> +}
> +
> +/* Handling opcode 000 insns.  */
> +
> +static int 
> +arm_handle_data_proc_misc_ld_str_insn (insn_decode_record *arm_insn_r)
> +{
> +  struct regcache *reg_cache = arm_insn_r->regcache;
> +  uint32_t record_buf[8], record_buf_mem[8];
> +
> +  struct
> +    {
> +      ULONGEST unsigned_regval;
> +    } u_buf[2];
> +
> +
> +  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;
> +
> +  memset (&u_buf, 0, sizeof (u_buf));
> +
> +  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 preccedded 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.  */

Indentation here is wrong.

> +        reg_dest = bits (arm_insn_r->arm_insn, 12, 15);
> +        if (15 != reg_dest)
               ^^ magic number here.  Use ARM_PC_REGNUM.

> +          {
> +            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 insns, changes register and memory as well.  */
                               ^^ remove "s".

> +      /* SWP or SWPB insn.  */

This line of comment is useless.

> +      /* Get memory address given by Rn.  */
> +      reg_src1 = bits (arm_insn_r->arm_insn, 16, 19); 
> +      regcache_raw_read_unsigned (reg_cache, reg_src1
> +                                  , &u_buf[0].unsigned_regval);
                                     ^ "," should go to last line.

> +      /* 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_buf[0].unsigned_regval;
> +      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.  */
              ^^^^^^^^^^ Wrong indentation, and instances in other places.

> +        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.  */
> +      /* Accorindly to high vector configuration PC is set accordingly */
> +      /* 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) 
> +           let us begin according to addressing modes for store insns 
> +           STRH insn, addresing modes are taken following.  */
> +    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_buf[0].unsigned_regval);
                                       ^ "," should be put in previous line

> +        if (15 == reg_src1)
> +          {
> +            /* If R15 was used as Rn, hence current PC+8.  */
> +            u_buf[0].unsigned_regval = u_buf[0].unsigned_regval + 8;
> +          }            
> +        offset_8 = (immed_high << 4) | immed_low;
> +        /* Calculate target store address.  */
> +        if (14 == arm_insn_r->opcode)
> +          {
> +            tgt_mem_addr = u_buf[0].unsigned_regval + offset_8;
> +          }
> +        else
> +          {
> +            tgt_mem_addr = u_buf[0].unsigned_regval - offset_8;
> +          }
> +        record_buf_mem[0] = 2;
> +        record_buf_mem[1] = tgt_mem_addr;
> +        arm_insn_r->mem_rec_count = 1;
> +      }
> +    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_buf[0].unsigned_regval);
> +        regcache_raw_read_unsigned (reg_cache, reg_src2
> +                                    , &u_buf[1].unsigned_regval);                        
> +        if (15 == reg_src2)
> +          {
> +            /* If R15 was used as Rn, hence current PC+8.  */
> +            u_buf[0].unsigned_regval = u_buf[0].unsigned_regval + 8;
> +          }            
> +        /* Calculate target store address, Rn +/- Rm, register offset.  */
> +        if (12 == arm_insn_r->opcode)
> +          {
> +            tgt_mem_addr = u_buf[0].unsigned_regval + u_buf[1].unsigned_regval;
> +          }
> +        else
> +          {
> +            tgt_mem_addr = u_buf[1].unsigned_regval - u_buf[0].unsigned_regval;
> +          }            
> +        record_buf_mem[0] = 2;
> +        record_buf_mem[1] = tgt_mem_addr;
> +        arm_insn_r->mem_rec_count = 1;
> +      }
> +    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_buf[0].unsigned_regval);
> +        /* Calculate target store address, Rn +/- Rm, register offset.  */
> +        if ((15 == arm_insn_r->opcode) || (6 == arm_insn_r->opcode))
> +          {
> +            tgt_mem_addr = u_buf[0].unsigned_regval + offset_8;
> +          }
> +        else
> +          {
> +            tgt_mem_addr = u_buf[0].unsigned_regval - offset_8;
> +          }            
> +        record_buf_mem[0] = 2;
> +        record_buf_mem[1] = tgt_mem_addr;
> +        arm_insn_r->mem_rec_count = 1;
> +        /* Record Rn also as it changes.  */
> +        record_buf[0] = 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_buf[0].unsigned_regval);
> +        regcache_raw_read_unsigned (reg_cache, reg_src2
> +                                    , &u_buf[1].unsigned_regval);
> +        /* Calculate target store address, Rn +/- Rm, register offset.  */
> +        if ((13 == arm_insn_r->opcode) || (4 == arm_insn_r->opcode))
> +          {
> +            tgt_mem_addr = u_buf[0].unsigned_regval + u_buf[1].unsigned_regval;
> +          }
> +        else
> +          {
> +            tgt_mem_addr = u_buf[1].unsigned_regval - u_buf[0].unsigned_regval;
> +          }            
> +        record_buf_mem[0] = 2;
> +        record_buf_mem[1] = tgt_mem_addr;
> +        arm_insn_r->mem_rec_count = 1;
> +        /* Record Rn also as it changes.  */
> +        record_buf[0] = bits (arm_insn_r->arm_insn, 16, 19);              
> +        arm_insn_r->reg_rec_count = 1;
> +      }
> +    /* DSP insns  (e.g. LDRD)  TBD.  */
> +  }  
> +  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 101 insns.  */
> +
> +static int 
> +arm_handle_brn_insn (insn_decode_record *arm_insn_r)

"arm_handle_brn_insn" is confusing, because there is no insn "brn".  I
would name this function as "arm_record_b_bl_blx".

> +{
> +
> +  uint32_t record_buf[8];
> +
> +  /* Handle B, BL, BLX(1) insns.  */
> +  /* Wihtout optmization we save link register, 
> +        CSPR for the insn which changes T bit.  */
> +  record_buf[0] = ARM_PS_REGNUM;
> +  record_buf[1] = ARM_LR_REGNUM;
> +  arm_insn_r->reg_rec_count = 2;  
                                   ^^ trail spaces, and many other places.

We don't have to record PS and LR for these three kinds of insns.  For
B, we don't have to do anything here.  We only record LR for BL and
record LS and PS for BLX.

> +
> +  REG_ALLOC (arm_insn_r->arm_regs, arm_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_handle_data_proc_misc_ld_str_insn,    /* 000.  */
> +      arm_handle_data_proc_imm_insn,            /* 001.  */
> +      arm_handle_ld_st_imm_offset_insn,         /* 010.  */
> +      arm_handle_ld_st_reg_offset_insn,         /* 011.  */  
> +      arm_handle_ld_st_multiple_insn,           /* 100.  */
> +      arm_handle_brn_insn,                      /* 101.  */
> +      arm_handle_coproc_insn,                   /* 110.  */
> +      arm_handle_coproc_data_proc_insn          /* 111.  */

I don't like the name scheme "arm_handle_xxxx_insn" of these fucntions.
 In gdb backend, we "handle" insn for many different purposees, such as
software-single-step, displaced stepping, record, and etc.  I suggest
that we name them like "arm_record_xxx" (without "_insn").


> +  };
> +  
> +  /* (Starting from numerical 0); bits 13,14,15 decodes type of thumb instruction.  */
> +  static int (*const thumb_handle_insn[8]) 
> +                                          (insn_decode_record*) =
> +  { \
> +      thumb_handle_shift_add_sub_insn,         /* 000.  */
> +      thumb_handle_add_sub_cmp_mov_insn,       /* 001.  */
> +      thumb_handle_ld_st_reg_offset_insn,      /* 010.  */
> +      thumb_handle_ld_st_imm_offset_insn,      /* 011.  */  
> +      thumb_handle_ld_st_stack_insn,           /* 100.  */
> +      thumb_handle_misc_insn,                  /* 101.  */
> +      thumb_handle_swi_insn,                   /* 110.  */
> +      thumb_handle_branch_insn                 /* 111.  */  
> +  };

> +
> +/* 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);  
> +  uint32_t no_of_rec = 0;
> +  uint32_t ret = 0;
> +  ULONGEST t_bit = 0;
> +
> +  struct
> +    {
> +      ULONGEST unsigned_regval;
> +    } 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.  */
> +
> +  t_bit = arm_psr_thumb_bit (arm_record.gdbarch);
> +  regcache_raw_read_unsigned (arm_record.regcache, ARM_PS_REGNUM
> +                              , &u_buf.unsigned_regval);
> +    
> +  if (!(u_buf.unsigned_regval & t_bit))
> +    {
> +      /* 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);    

It is not correct to me.  Both thumb 16-bit and 32-bit insn are handled
here, so it is wrong to pass THUMB_INSN_SIZE_BYTES which is 2.

> +    }
> +
> +  if (0 == ret)
> +    {
> +      /* Record registers.  */
> +      record_arch_list_add_reg (arm_record.regcache, ARM_PC_REGNUM);
> +      if (arm_record.arm_regs)
> +        {
> +          for (no_of_rec = 0; no_of_rec < arm_record.reg_rec_count; no_of_rec++)
> +            {
> +              if (record_arch_list_add_reg (arm_record.regcache \
> +                                           , (arm_record.arm_regs[no_of_rec])))
> +              ret = -1;
> +            }
> +        }   
> +      /* Record memories.  */
> +      if (arm_record.arm_mems)
> +        {
> +          for (no_of_rec = 0; no_of_rec < arm_record.mem_rec_count; 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; 
> +}
> diff -urN arm_orig/arm-tdep.h arm_new/arm-tdep.h
> --- arm_orig/arm-tdep.h    2011-07-28 09:40:19.000000000 +0530
> +++ arm_new/arm-tdep.h    2011-07-28 09:41:06.000000000 +0530
> @@ -201,6 +201,9 @@
>    /* Return the expected next PC if FRAME is stopped at a syscall
>       instruction.  */
>    CORE_ADDR (*syscall_next_pc) (struct frame_info *frame);
> +
> +   /* Parse swi insn args, sycall record.  */
> +  int (*arm_swi_record) (struct regcache *regcache);
>  };
>  

I don't see any function is installed on this function pointer.

-- 
Yao (éå)