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

["Ulrich Weigand" <uweigand@de.ibm.com>]

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