Re: [PATCH, v2] Fix ARM machine state testcase failures

[Yao Qi <yao@codesourcery.com>]

Luis Machado <lgustavo@codesourcery.com> writes:

> While at it, i noticed the original functions had some issues like
> unused variables and broken identation, so i went ahead and completely
> rewrote the functions handling ARM mode opcodes 010 and 100.

It's great to make code cleaner, but better to put code refactor and bug
fixing into separate patches.

> I see the same results as before, with no regressions.
>
> How does it look now?
>
> Luis
>
> 2014-10-21  Luis Machado  <lgustavo@codesourcery.com>
>
> 	* arm-tdep.c (INSN_S_L_BIT_NUM): Document.
> 	(arm_record_ld_st_imm_offset): Reimplement to cover all
> 	load/store cases for ARM opcode 010.
> 	(arm_record_ld_st_multiple): Reimplement to cover all
> 	load/store cases for ARM opcode 100.
>

With these comments below addressed, this patch is OK to me.

>  
> -/* Handling opcode 100 insns.  */
> +/* Handle ARM mode instructions with opcode 100.  */
>  
>  static int
>  arm_record_ld_st_multiple (insn_decode_record *arm_insn_r)
>  {
>    struct regcache *reg_cache = arm_insn_r->regcache;
> -
> -  uint32_t register_list[16] = {0}, register_count = 0, register_bits = 0;
> -  uint32_t reg_src1 = 0, addr_mode = 0, no_of_regs = 0;
> -  uint32_t start_address = 0, index = 0;
> +  uint32_t register_count = 0, register_bits;
> +  uint32_t reg_base, addr_mode;
>    uint32_t record_buf[24], record_buf_mem[48];
> +  uint32_t wback;
> +  ULONGEST u_regval;
>  
> -  ULONGEST u_regval[2] = {0};
> +  /* Fetch the list of registers.  */
> +  register_bits = bits (arm_insn_r->arm_insn, 0, 15);
> +  arm_insn_r->reg_rec_count = 0;
> +
> +  /* Fetch the base register that contains the address we are loading data
> +     to.  */
> +  reg_base = bits (arm_insn_r->arm_insn, 16, 19);
>  
> -  /* This mode is exclusively for load and store multiple.  */
> -  /* Handle incremenrt after/before and decrment after.before mode;
> -     Rn is changing depending on W bit, but as of now we store Rn too
> -     without optimization.  */
> +  /* Calculate wback.  */
> +  wback = (bit (arm_insn_r->arm_insn, 24) == 0)
> +	  || (bit (arm_insn_r->arm_insn, 21) == 1);

Bit 24 is zero, so we only need to check bit 21.

>  
>    if (bit (arm_insn_r->arm_insn, INSN_S_L_BIT_NUM))
>      {
> -      /* LDM  (1,2,3) where LDM  (3) changes CPSR too.  */
> +      /* LDM/LDMIA/LDMFD, LDMDA/LDMFA, LDMDB and LDMIB.  */
>  
> -      if (bit (arm_insn_r->arm_insn, 20) && !bit (arm_insn_r->arm_insn, 22))
> -        {
> -          register_bits = bits (arm_insn_r->arm_insn, 0, 15);
> -          no_of_regs = 15;
> -        }
> -      else
> -        {
> -          register_bits = bits (arm_insn_r->arm_insn, 0, 14);
> -          no_of_regs = 14;
> -        }
> -      /* Get Rn.  */
> -      reg_src1 = bits (arm_insn_r->arm_insn, 16, 19);
> +      /* Find out which registers are going to be loaded from memory.  */
>        while (register_bits)
> -      {
> -        if (register_bits & 0x00000001)
> -          record_buf[index++] = register_count;
> -        register_bits = register_bits >> 1;
> -        register_count++;
> -      }
> +	{
> +	  if (register_bits & 0x00000001)
> +	    record_buf[arm_insn_r->reg_rec_count++] = register_count;
> +	  register_bits = register_bits >> 1;
> +	  register_count++;
> +	}
>  
> -        /* Extra space for Base Register and CPSR; wihtout optimization.  */
> -        record_buf[index++] = reg_src1;
> -        record_buf[index++] = ARM_PS_REGNUM;
> -        arm_insn_r->reg_rec_count = index;
> +      /* Extra space for Base Register and CPSR; wihtout optimization.  */

s/wihtout/without/

-- 
Yao (齐尧)