On 05/18/2011 01:14 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 

>> +static int
>> +thumb2_copy_preload_reg (struct gdbarch *gdbarch, uint16_t insn1,
>> +			 uint16_t insn2, struct regcache *regs,
>> +			 struct displaced_step_closure *dsc)
>> +{
>> +  unsigned int rn = bits (insn1, 0, 3);
>> +  unsigned int rm = bits (insn2, 0, 3);
>> +
>> +  if (rn != ARM_PC_REGNUM && rm != ARM_PC_REGNUM)
>> +    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, "preload reg",
>> +					dsc);
>> +
>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog, "displaced: copying preload insn %.4x%.4x\n",
>> +			insn1, insn1);
>> +
>> +  dsc->modinsn[0] = insn1 & 0xfff0;
>> +  dsc->modinsn[1] = (insn2 & 0xfff0) | 0x1;
>> +  dsc->numinsns = 2;
>> +
>> +  install_preload_reg (gdbarch, regs, dsc, rn, rm);
>> +  return 0;
>> +}
> 
> Handling of preload instructions seems wrong for a couple of reasons:
> 
> - In Thumb mode, PLD/PLI with register offset must not use PC as offset
>   register, so those can just be copied unmodified.  The only instructions
>   to be treated specially are the "literal" variants, which do encode
>   PC-relative offsets.
> 
>   This means a separate thumb2_copy_preload_reg shouldn't be needed.
> 

Right.  thumb2_copy_preload_reg is removed.

> - However, you cannot just transform a PLD/PLI "literal" (i.e. PC + immediate)
>   into an "immediate" (i.e. register + immediate) version, since in Thumb
>   mode the "literal" version supports a 12-bit immediate, while the immediate
>   version only supports an 8-bit immediate.
> 
>   I guess you could either add the immediate to the PC during preparation
>   stage and then use an "immediate" instruction with immediate zero, or
>   else load the immediate into a second register and use a "register"
>   version of the instruction.
> 

The former may not be correct.  PC should be set at the address of `copy
area' in displaced stepping, instead of any other arbitrary values.  The
alternative to the former approach is to compute the new immediate value
according to the new PC value we will set (new PC value is
dsc->scratch_base).  However, in this way, we have to worry about the
overflow of new computed 12-bit immediate.

The latter one sounds better, because we don't have to worry about
overflow problem, and cleanup_preload can be still used as cleanup
routine in this case.

> 
>> +static int
>> +thumb2_copy_copro_load_store (struct gdbarch *gdbarch, uint16_t insn1,
>> +			      uint16_t insn2, struct regcache *regs,
>> +			      struct displaced_step_closure *dsc)
>> +{
>> +  unsigned int rn = bits (insn1, 0, 3);
>> +
>> +  if (rn == ARM_PC_REGNUM)
>> +    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +					"copro load/store", dsc);
>> +
>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog, "displaced: copying coprocessor "
>> +			"load/store insn %.4x%.4x\n", insn1, insn2);
>> +
>> +  dsc->modinsn[0] = insn1 & 0xfff0;
>> +  dsc->modinsn[1] = insn2;
>> +  dsc->numinsns = 2;
> 
> This doesn't look right: you're replacing the RN register if it is anything
> *but* 15 -- but those cases do not need to be replaced!
> 

Oh, sorry, it is a logic error.  The code should be like

if (rn != ARM_PC_REGNUM)
  return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, "copro
load/store", dsc);

>> +static int
>> +thumb2_copy_b_bl_blx (struct gdbarch *gdbarch, uint16_t insn1,
>> +		      uint16_t insn2, struct regcache *regs,
>> +		      struct displaced_step_closure *dsc)
>> +
>> +  if (!link && !exchange) /* B */
>> +    {
>> +      cond = bits (insn1, 6, 9);
> 
> Only encoding T3 has condition bits, not T4.
> 

Oh, right.  Fixed.

>> +
>> +  dsc->modinsn[0] = THUMB_NOP;
>> +
>> +  install_b_bl_blx (gdbarch, regs, dsc, cond, exchange, 1, offset);
> 
> Why do you always pass 1 for link?  Shouldn't "link" be passed?
> 

"link" should be passed.  Fixed.

>> +static int
>> +thumb2_copy_alu_reg (struct gdbarch *gdbarch, uint16_t insn1,
>> +		     uint16_t insn2, struct regcache *regs,
>> +		     struct displaced_step_closure *dsc)
>> +{
>> +  unsigned int op2 = bits (insn2, 4, 7);
>> +  int is_mov = (op2 == 0x0);
>> +  unsigned int rn, rm, rd;
>> +
>> +  rn = bits (insn1, 0, 3); /* Rn */
>> +  rm = bits (insn2, 0, 3); /* Rm */
>> +  rd = bits (insn2, 8, 11); /* Rd */
>> +
>> +  /* In Thumb-2, rn, rm and rd can't be r15.  */
> This isn't quite true ... otherwise we wouldn't need the routine at all.

This line of comment is out of date.  Remove it.

>> +  if (rn != ARM_PC_REGNUM && rm != ARM_PC_REGNUM
>> +      && rd != ARM_PC_REGNUM)
>> +    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, "ALU reg", dsc);
>> +
>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog, "displaced: copying reg %s insn %.4x%.4x\n",
>> +			"ALU", insn1, insn2);
>> +
>> +  if (is_mov)
>> +    dsc->modinsn[0] = insn1;
>> +  else
>> +    dsc->modinsn[0] = ((insn1 & 0xfff0) | 0x1);
>> +
>> +  dsc->modinsn[1] = ((insn2 & 0xf0f0) | 0x2);
>> +  dsc->numinsns = 2;
> 
> This doesn't look right.  It looks like this function is called for all
> instructions in tables A6-22 through A6-26; those encodings differ
> significantly in how their fields are used.  Some of them have the
> Rn, Rm, Rd fields as above, but others just have some of them.  For
> some, a register field content of 15 does indeed refer to the PC and
> needs to be replaced; for others a register field content of 15 means
> instead that a different operation is to be performed (e.g. ADD vs TST,
> EOR vs TEQ ...) and so it must *not* be replaced; and for yet others,
> a register field content of 15 is unpredictable.
> 
> In fact, I think only a very small number of instructions in this
> category actually may refer to the PC (only MOV?), so there needs
> to the be more instruction decoding to actually identify those.
> 

thumb2_copy_alu_reg is called in for two groups of instructions,
1.  A6.3.11 Data-processing (shifted register)
2.  A6.3.12 Data-processing (register)

PC is not used in group #2.  Even in group #1, PC is only used in MOV.
This routine thumb2_copy_alu_reg is deleted, and
thumb2_decode_dp_shift_reg is added to decode group #2.

>>  static int
>> +thumb2_copy_ldr_str_ldrb_strb (struct gdbarch *gdbarch, uint16_t insn1,
>> +			       uint16_t insn2,  struct regcache *regs,
>> +			       struct displaced_step_closure *dsc,
>> +			       int load, int byte, int usermode, int writeback)
> 
> Hmmm ... this function is called for *halfwords* as well, not just for
> bytes and words.  This means the "byte" operand is no longer sufficient
> to uniquely determine the size -- note that when calling down to the
> install_ routine, xfersize is always set to 1 or 4.
> 

I thought "halfword" can be treated as "word" in this case, so I didn't
distinguish them.  I rename "thumb2_copy_ldr_str_ldrb_strb" to
"thumb2_copy_load_store", and change parameter BYTE to SIZE.  install_
routine and arm_ routine is updated as well.

>> +{
>> +  int immed = !bit (insn1, 9);
>> +  unsigned int rt = bits (insn2, 12, 15);
>> +  unsigned int rn = bits (insn1, 0, 3);
>> +  unsigned int rm = bits (insn2, 0, 3);  /* Only valid if !immed.  */
>> +
>> +  if (rt != ARM_PC_REGNUM && rn != ARM_PC_REGNUM && rm != ARM_PC_REGNUM)
> rm shouldn't be checked if immed is true

Fixed.

>> +    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, "load/store",
>> +					dsc);


>> +/* Decode extension register load/store.  Exactly the same as
>> +   arm_decode_ext_reg_ld_st.  */
>> +
>> +static int
>> +thumb2_decode_ext_reg_ld_st (struct gdbarch *gdbarch, uint16_t insn1,
>> +			     uint16_t insn2,  struct regcache *regs,
>> +			     struct displaced_step_closure *dsc)
>> +{
>> +
>> +    case 0x10: case 0x14: case 0x18: case 0x1c:  /* vstr.  */
>> +    case 0x11: case 0x15: case 0x19: case 0x1d:  /* vldr.  */
>> +      return thumb2_copy_copro_load_store (gdbarch, insn1, insn2, regs, dsc);
> 
> See the comment at thumb2_copy_copro_load_store: since that function will
> always copy the instruction unmodified, so can this function.
> 
> 

As we discussed VLDR may still use PC, so call
thumb_copy_unmodified_32bit for VSTR in my new patch.

>> +static int
>> +thumb2_decode_svc_copro (struct gdbarch *gdbarch, uint16_t insn1,
>> +			 uint16_t insn2, struct regcache *regs,
>> +			 struct displaced_step_closure *dsc)
>> +{
[...]
> 
> See above ... I don't think any of those instructions can ever use the PC
> in Thumb mode, so this can be simplified.
> 

It is simplified to some extent in new patch.

>> +	}
>> +    }
>> +  else
>> +    {
>> +      unsigned int op = bit (insn2, 4);
>> +      unsigned int bit_8 = bit (insn1, 8);
>> +
>> +      if (bit_8) /* Advanced SIMD */
>> +	return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +					    "neon", dsc);
>> +      else
>> +	{
>> +	  /*coproc is 101x.  */
>> +	  if ((coproc & 0xe) == 0xa)
>> +	    {
>> +	      if (op) /* 8,16,32-bit xfer.  */
>> +		return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						    "neon 8/16/32 bit xfer",
>> +						    dsc);
>> +	      else /* VFP data processing.  */
>> +		return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						    "vfp dataproc", dsc);
>> +	    }
>> +	  else
>> +	    {
>> +	      if (op)
>> +		{
>> +		  if (bit_4) /* MRC/MRC2 */
>> +		    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +							"mrc/mrc2", dsc);
>> +		  else /* MCR/MCR2 */
>> +		     return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +							"mcr/mcr2", dsc);
>> +		}
>> +	      else /* CDP/CDP 2 */
>> +		return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						    "cdp/cdp2", dsc);
>> +	    }
> 
> Likewise I'm not sure there is any need to decode to such depth, if the
> instruction in the end all can be copied unmodified.

OK.  Patch length can be reduced then.

>>  static int
>> +thumb_copy_pc_relative_32bit (struct gdbarch *gdbarch, struct regcache *regs,
>> +			      struct displaced_step_closure *dsc,
>> +			      int rd, unsigned int imm)
>> +{
>> +  /* Encoding T3: ADDS Rd, Rd, #imm */
> Why do you refer to ADDS?  The instruction you generate is ADD (with no S bit),
> which is actually correct -- so it seems just the comment is wrong.

It is a mistake in comment.  ADR doesn't update flags, we don't have S
bit in ADD.

>> +  dsc->modinsn[0] = (0xf100 | rd);
>> +  dsc->modinsn[1] = (0x0 | (rd << 8) | imm);
>> +
>> +  dsc->numinsns = 2;
>> +
>> +  install_pc_relative (gdbarch, regs, dsc, rd);
>> +
>> +  return 0;
>> +}
>> +
>> +static int
>> +thumb_decode_pc_relative_32bit (struct gdbarch *gdbarch, uint16_t insn1,
>> +				uint16_t insn2, struct regcache *regs,
>> +				struct displaced_step_closure *dsc)
>> +{
>> +  unsigned int rd = bits (insn2, 8, 11);
>> +  /* Since immeidate has the same encoding in both ADR and ADDS, so we simply
> typo
>> +     extract raw immediate encoding rather than computing immediate.  When
>> +     generating ADDS instruction, we can simply perform OR operation to set
>> +     immediate into ADDS.  */
> See above for ADDS vs. ADD.

s/ADDS/ADD/ in comments.

>> +  unsigned int imm = (insn2 & 0x70ff) | (bit (insn1, 10) << 26);
> 
> The last bit will get lost, since thumb_copy_pc_relative_32bit only or's
> the value to the second 16-bit halfword.

Then, we have separately set the bit 10 (i bit) in dsc->modinsn[0] per
original insn1's i bit.


>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog,
>> +			"displaced: copying thumb adr r%d, #%d insn %.4x%.4x\n",
>> +			rd, imm, insn1, insn2);
>> +
>> +  return thumb_copy_pc_relative_32bit (gdbarch, regs, dsc, rd, imm);
>> +}
> 
> B.t.w. I think the distinction between a _decode_ and a _copy_ routine is
> pointless in this case since the _decode_ routine is only ever called for
> one single instruction that matches ... it doesn't actually decode anything.
> 

thumb_decode_pc_relative_32bit is merged to thumb_copy_pc_relative_32bit.

> 
>> +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 rd = bits (insn2, 12, 15);
>> +  int user_mode = (bits (insn2, 8, 11) == 0xe);
>> +  int err = 0;
>> +  int writeback = 0;
>> +
>> +  switch (bits (insn1, 5, 6))
>> +    {
>> +    case 0: /* Load byte and memory hints */
>> +      if (rd == 0xf) /* PLD/PLI */
>> +	{
>> +	  if (bits (insn2, 6, 11))
> This check doesn't look right to me.

This part is re-written.

>> +	    return thumb2_copy_preload (gdbarch, insn1, insn2, regs, dsc);
>> +	  else
>> +	    return thumb2_copy_preload_reg (gdbarch, insn1, insn2, regs, dsc);
> 
> In any case, see the comments above on handling preload instructions.  You
> should only need to handle the "literal" variants.
> 

Right.  thumb2_copy_preload_reg is removed, and this part of code is
adjusted as well.

> 
> 
>>  static void
>>  thumb_process_displaced_32bit_insn (struct gdbarch *gdbarch, uint16_t insn1,
>>  				    uint16_t insn2, struct regcache *regs,
>>  				    struct displaced_step_closure *dsc)
>>  {
>> -  error (_("Displaced stepping is only supported in ARM mode and Thumb 16bit instructions"));
>> +  int err = 0;
>> +  unsigned short op = bit (insn2, 15);
>> +  unsigned int op1 = bits (insn1, 11, 12);
>> +
>> +  switch (op1)
>> +    {
>> +    case 1:
>> +      {
>> +	switch (bits (insn1, 9, 10))
>> +	  {
>> +	  case 0: /* load/store multiple */
>> +	    switch (bits (insn1, 7, 8))
>> +	      {
>> +	      case 0: case 3: /* SRS, RFE */
>> +		err = thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						   "srs/rfe", dsc);
>> +		break;
>> +	      case 1: case 2: /* LDM/STM/PUSH/POP */
>> +		/* These Thumb 32-bit insns have the same encodings as ARM
>> +		   counterparts.  */
> "same encodings" isn't quite true ...

This line of comment is out of date.  Removed.

>> +		err = thumb2_copy_block_xfer (gdbarch, insn1, insn2, regs, dsc);
>> +	      }
>> +	    break;
> 
> Hmm, it seems this case is missing code to handle the load/store dual,
> load/store exclusive, and table branch instructions (page A6-24 / table A6-17);
> there should be a check whether bit 6 is zero or one somewhere.
> 

routine thumb2_copy_table_branch is added to handle table branch
instructions.  load/store dual and load/store exclusive don't use PC, so
they are copy-unmodified.

>> +	  case 1:
>> +	    /* Data-processing (shift register).  In ARM archtecture reference
>> +	       manual, this entry is
>> +	       "Data-processing (shifted register) on page A6-31".  However,
>> +	    instructions in table A6-31 shows that they are `alu_reg'
>> +	    instructions.  There is no alu_shifted_reg instructions in
>> +	    Thumb-2.  */
> 
> Well ... they are not *register*-shifted register instructions like
> there are in ARM mode (i.e. register shifted by another register),
> but they are still *shifted* register instructions (i.e. register
> shifted by an immediate).
> 

Thanks for the clarification.  Only leave the 1st sentence of comment,
and remove the rest of them.

>> +	    err = thumb2_copy_alu_reg (gdbarch, insn1, insn2, regs,
>> +					       dsc);
> (see comments at that function ...)

Add a new function thumb2_decode_dp_shift_reg and call it here.

>> +	    break;
>> +	  default: /* Coprocessor instructions */
>> +	    /* Thumb 32bit coprocessor instructions have the same encoding
>> +	       as ARM's.  */
> (see above as to "same encoding" ... also, some ARM coprocessor instruction
> may in fact use the PC, while no Thumb coprocessor instruction can ... so
> there is probably no need to decode them further at this point)

As we discussed, STC/STC/VLDR may still use PC.  Leave it there.

> 
>> +    case 2: /* op1 = 2 */
>> +      if (op) /* Branch and misc control.  */
>> +	{
>> +	  if (bit (insn2, 14)) /* BLX/BL */
>> +	    err = thumb2_copy_b_bl_blx (gdbarch, insn1, insn2, regs, dsc);
>> +	  else if (!bits (insn2, 12, 14) && bits (insn1, 8, 10) != 0x7)
> I don't understand this condition, but it looks wrong to me ...
> 

This condition is about "Conditional Branch".  The 2nd half of condition
should be "bits (insn1, 7, 9) != 0x7", corresponding to the first line
of table A6-13 "op1 = 0x0, op is not x111xxx".

>> +	      else /* Store single data item */
>> +		{
>> +		  int user_mode = (bits (insn2, 8, 11) == 0xe);
>> +		  int byte = (bits (insn1, 5, 7) == 0
>> +			      || bits (insn1, 5, 7) == 4);
>> +		  int writeback = 0;
>> +
>> +		  if (bits (insn1, 5, 7) < 3 && bit (insn2, 11))
>> +		    writeback = bit (insn2, 8);
> 
> If things get this complicated, a decode routine might be appropriate.

OK, move these logics into a new function
"decode_thumb_32bit_store_single_data_item".

Note that patch sits on top of this patch,

  [patch] refactor arm-tdep.c:install_ldr_str_ldrb_strb to handle halfword
  http://sourceware.org/ml/gdb-patches/2011-07/msg00183.html

-- 
Yao