On 08/10/2011 02:46 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
> 
>> +  /* Rewrite instruction {pli/pld} PC imm12 into:
>> +     Preapre: tmp[0] <- r0, tmp[1] <- r1, r0 <- pc, r1 <- imm12
> 
> Typo: Prepare
> 

Fixed.

>> +     {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 ...

Yeah, we should only clear bits for register number.  Fixed.

>> +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 ?
> 

The else block is re-written like this to handle LDRB/LDRSB (literal),

  if (rn == 0xf) /* LDRB/LDRSB (literal) */
    return thumb2_copy_load_literal (gdbarch, insn1, insn2, regs, dsc,
				     1);
  else
    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
					"ldrb{reg, immediate}/ldrbt",
					dsc);

>> +    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).
> 

You are right.  Add a new argument `size'.

>> +	  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.
> 

OK.  Fixed.

>> +    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.
> 

Right.  Remove user_mode argument from thumb2_copy_load_reg_imm.

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

`size' argument is removed.

>> +  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 ...
> 

It is out of date.  Removed.

>> +    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.
> 

Yes, "else if" block is removed.

-- 
Yao (齐尧)