Re: [PATCH 3/3] Fix various bugs in arm_record_exreg_ld_st_insn

[Luis Machado <lgustavo at codesourcery dot com>]

Just internal comment nits.

On 02/22/2016 01:53 PM, Yao Qi wrote:
> This patch fixes various bugs in arm_record_exreg_ld_st_insn, and use
> gdb.reverse/insn-reverse.c to test more arm instructions.
>
>   - Set flag SINGLE_REG correctly.  In the arch reference manual,
>     SING_REG is true when the bit 8 of instruction is zero.
>   - Record the right D registers for instructions changing S registers.
>   - Fix the order of length and address in record_buf_mem array.
>   - Shift the offset by 2 instead of by 24.
>
> This patch also fixes one internal error,
>
> (gdb) PASS: gdb.reverse/finish-precsave.exp: BP at end of main
> continue^M
> Continuing.^M
> ../../binutils-gdb/gdb/utils.c:1072: internal-error: virtual memory exhausted.^M
> A problem internal to GDB has been detected,FAIL: gdb.reverse/finish-precsave.exp: run to end of main (GDB internal error)

...

> @@ -10991,25 +11002,36 @@ arm_record_exreg_ld_st_insn (insn_decode_record *arm_insn_r)
>       {
>         uint32_t reg_count, reg_vd;
>         uint32_t reg_index = 0;
> +      uint32_t bit_d = bit (arm_insn_r->arm_insn, 22);
>
>         reg_vd = bits (arm_insn_r->arm_insn, 12, 15);
>         reg_count = bits (arm_insn_r->arm_insn, 0, 7);
>
> -      if (single_reg)
> -	reg_vd = reg_vd | (bit (arm_insn_r->arm_insn, 22) << 4);
> -      else
> -	reg_vd = (reg_vd << 1) | bit (arm_insn_r->arm_insn, 22);
> +      /* REG_VD is the first D register number.  If the instruction
> +	 loads memory to S registers (SINGLE_REG is TRUE), the register
> +	 number is (REG_VD << 1 | bit D), so the corresponding D
> +	 register number is (REG_VD << 1 | bit D) / 2 = REG_VD.  */
> +      if (!single_reg)
> +	reg_vd = reg_vd | (bit_d << 4);
>
> -      if (bit (arm_insn_r->arm_insn, 21))
> +      if (bit (arm_insn_r->arm_insn, 21) /* write back */)
>   	record_buf[reg_index++] = bits (arm_insn_r->arm_insn, 16, 19);
>
> -      while (reg_count > 0)
> +      /* If the instruction loads memory to D register, REG_COUNT should
> +	 divide 2, according to the ARM Architecture Reference Manual.

"...should be divided by 2..."?

> +	 If the instruction loads memory to S register, divide 2 as well

"... divide by 2..."

> +	 because two S registers are mapped to D register.  */
> +      reg_count  = reg_count / 2;
> +      if (single_reg && bit_d)
>   	{
> -	  if (single_reg)
> -	    record_buf[reg_index++] = num_regs + reg_vd + reg_count - 1;
> -	  else
> -	    record_buf[reg_index++] = ARM_D0_REGNUM + reg_vd + reg_count - 1;
> +	  /* Increase the register count if S register list starts from
> +	     odd number (bit d is one).  */

"...from an odd number..."?