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

Luis Machado
2016-02-23T19:59:20Z

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..."?