Re: [try 2nd 5/8] Displaced stepping for Thumb 32-bit insns

Ulrich Weigand
2011-08-09T18:46:29Z

Yao Qi wrote:

> Support displaced stepping for Thumb 32-bit insns.

There's still a couple of issues I noticed, but overall it is
looking quite good now... Thanks!

> + /* Rewrite instruction {pli/pld} PC imm12 into:
> + Preapre: tmp[0] <- r0, tmp[1] <- r1, r0 <- pc, r1 <- imm12

Typo: Prepare

> + {pli/pld} [r0, r1]
> +
> + Cleanup: r0 <- tmp[0], r1 <- tmp[1]. */
> +
> + dsc->tmp[0] = displaced_read_reg (regs, dsc, 0);
> + dsc->tmp[1] = displaced_read_reg (regs, dsc, 1);
> +
> + pc_val = displaced_read_reg (regs, dsc, ARM_PC_REGNUM);
> +
> + displaced_write_reg (regs, dsc, 0, pc_val, CANNOT_WRITE_PC);
> + displaced_write_reg (regs, dsc, 1, imm12, CANNOT_WRITE_PC);
> + dsc->u.preload.immed = 0;
> +
> + /* {pli/pld} [r0, r1] */
> + dsc->modinsn[0] = insn1 & 0xff00;

Shouldn't this be something like 0xfff0 instead? We need to
keep bit 4 set ...

> +static int
> +decode_thumb_32bit_ld_mem_hints (struct gdbarch *gdbarch,
> + uint16_t insn1, uint16_t insn2,
> + struct regcache *regs,
> + struct displaced_step_closure *dsc)
> +{
> + int rt = bits (insn2, 12, 15);
> + int rn = bits (insn1, 0, 3);
> + int op1 = bits (insn1, 7, 8);
> + int err = 0;
> +
> + switch (bits (insn1, 5, 6))
> + {
> + case 0: /* Load byte and memory hints */
> + if (rt == 0xf) /* PLD/PLI */
> + {
> + if (rn == 0xf)
> + /* PLD literal or Encoding T3 of PLI(immediate, literal). */
> + return thumb2_copy_preload (gdbarch, insn1, insn2, regs, dsc);
> + else
> + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
> + "pli/pld", dsc);
> + }
> + else
> + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
> + "ldrb{reg, immediate}/ldrbt",
> + dsc);

Hmm. What about literal variants of LDRB/LDRSB ?

> + case 1: /* Load halfword and memory hints. */
> + if (rt == 0xf) /* PLD{W} and Unalloc memory hint. */
> + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
> + "pld/unalloc memhint", dsc);
> + else
> + {
> + int insn2_bit_8_11 = bits (insn2, 8, 11);
> +
> + if (rn == 0xf)
> + return thumb2_copy_load_literal (gdbarch, insn1, insn2, regs, dsc);

copy_load_literal currently only handles full-word loads ... this should
really be able to handle half-word loads as well (which means it probably
needs a size argument).

> + else
> + {
> + if (op1 == 0x1 || op1 == 0x3)
> + /* LDRH/LDRSH (immediate), in which bit 7 of insn1 is 1,
> + PC is not used. */
> + return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
> + "ldrh/ldrht", dsc);
> + else if (insn2_bit_8_11 == 0xc
> + || (insn2_bit_8_11 & 0x9) == 0x9)
> + /* LDRH/LDRSH (imediate), in which bit 7 of insn1 is 0, PC
> + can be used. */
> + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs,
> + dsc, 2, 0, bit (insn2, 8), 1);

Actually, it cannot ... if RT is PC, we have either UNPREDICTABLE or
an Unallocated memory hint; if RN is PC, we have the literal version.

It seems everything except literal can just be passed through unmodified,
and we do not need to call thumb2_copy_load_reg_imm at all.

> + case 2: /* Load word */
> + {
> + int insn2_bit_8_11 = bits (insn2, 8, 11);
> +
> + if (rn == 0xf)
> + return thumb2_copy_load_literal (gdbarch, insn1, insn2, regs, dsc);
> + else if (op1 == 0x1) /* Encoding T3 */
> + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs,
> + dsc, 4, 0, 0, 1);
> + else /* op1 == 0x0 */
> + {
> + if (insn2_bit_8_11 == 0xc || (insn2_bit_8_11 & 0x9) == 0x9)
> + /* LDR (immediate) */
> + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs,
> + dsc, 4, 0,
> + bit (insn2, 8), 1);
> + else
> + /* LDRT and LDR (register) */
> + return thumb2_copy_load_reg_imm (gdbarch, insn1, insn2, regs,
> + dsc, 4,
> + bits (insn2, 8, 11) == 0xe,
> + 0, 0);

LDRT also cannot use PC as target, so we really only need to check for
LDR (register) here. Also, this means that thumb2_copy_load_reg_imm
doesn't need a user_mode argument.

(It also seems that it doesn't need a size argument: loads into PC
are only allowed for the full-word instructions.)

> + switch (op1)
> + {
> + case 1:
> + {
> + switch (bits (insn1, 9, 10))
> + {
> + default: /* Coprocessor instructions. */
> + /* Thumb 32bit coprocessor instructions have the same encoding
> + as ARM's. */

The comment isn't really correct ...

> + case 2: /* op1 = 2 */
> + if (op) /* Branch and misc control. */
> + {
> + if (bit (insn2, 14) /* BLX/BL */
> + || bit (insn2, 12) /* Unconditional branch */
> + || (bits (insn1, 7, 9) != 0x7)) /* Conditional branch */
> + err = thumb2_copy_b_bl_blx (gdbarch, insn1, insn2, regs, dsc);
> + else if (!bit (insn2, 12) && bits (insn1, 7, 9) != 0x7)
> + /* Conditional Branch */
> + err = thumb2_copy_b_bl_blx (gdbarch, insn1, insn2, regs, dsc);

The else if is now superfluous: conditional branches are covered by
the first if condition.

Bye,
Ulrich

--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
Ulrich.Weigand@de.ibm.com