On 07/16/2011 02:47 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
>> On 05/18/2011 01:14 AM, Ulrich Weigand wrote:
>>> - 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.
> 
> OK, this looks good to me now.
> 
>>> 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);
> 
> Hmm, it's still the wrong way in this patch?
> 

Sorry, fixed.

> 
>>>> +    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".
> 
> But "!bits (insn2, 12, 14)" doesn't say "op1 = 0x0" either ...  Since we
> already know bit 14 is 0, this should probably just check for bit 12.

OK, the condition checking is changed from "!bits (insn2, 12, 14)" to
"!bit (insn2, 12)".

> Some more comments on the latest patch.  There's a couple of issues I
> had overlooked in the previous review, in particular handling of the
> load/store instructions.  Most of the rest is just minor things ...
> 
> 
>> +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);
> 
> This still needs to be rn != ARM_PC_REGNUM
> 

Oh, sorry for missing this one in last patch.  Fixed.

>> +  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;
>> +
>> +  install_copro_load_store (gdbarch, regs, dsc, bit (insn1, 9), rn);
> 
> Why bit 9?  Isn't the writeback bit bit 5 here?  But anyway, those
> instructions we support here in Thumb mode (LDC/LDC2, VLDR) don't
> support writeback anyway.  It's probably best to just pass 0.
> 

Right, let us pass 0 for writeback in this function.

>> +static int
>> +thumb2_copy_b_bl_blx (struct gdbarch *gdbarch, uint16_t insn1,
>> +		      uint16_t insn2, struct regcache *regs,
>> +		      struct displaced_step_closure *dsc)
>> +{
>> +  int link = bit (insn2, 14);
>> +  int exchange = link && !bit (insn2, 12);
>> +  int cond = INST_AL;
>> +  long offset =0;
> Space after =
> 

Fixed.

> 
>> +thumb2_copy_load_store (struct gdbarch *gdbarch, uint16_t insn1, uint16_t insn2,
>> +			struct regcache *regs,
>> +			struct displaced_step_closure *dsc, int load, int size,
>> +			int usermode, int writeback)
> 
> Looking at the store instructions (STR/STRB/STRH[T]), it would appear that none
> of them may use PC in Thumb mode.  Therefore, it seems that this routine should
> just handle loads (i.e. rename to thumb2_copy_load and remove the load argument).
> 
> There is another fundamental problem: The LDR "literal" Thumb encodings provide
> a "long-form" 12-bit immediate *and* an U bit.  However, the non-PC-relative
> "immediate" Thumb encodings only provide a U bit with the short 8-bit immediates;
> the 12-bit immediate form does not have a U bit (instead, bit 7 encodes whether
> the 12-bit or 8-bit form is in use).
> 
> This means that you cannot simply translate LDR literal into LDR immediate
> forms, but probably need to handle by loading the immediate into a register,
> similar to the preload case.
> 

OK, thumb2_copy_load_store is renamed to thumb2_copy_load_reg_imm, which
is to handle non-literal case, and thumb2_copy_load_literal is a new
function to handle LDR "literal" instruction.

>> +{
>> +  int immed = !bit (insn1, 9);
> 
> This check looks incorrect.  E.g. LDR (register) also has bit 9 equals zero.
> There needs to be a more complex decoding step somewhere, either in the caller,
> or directly in here.  (Note that decoding immediate, usermode, and writeback
> flags are closely coupled, so this should probably be done in the same location.)
> 
>> +  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
>> +      && (immed || rm != ARM_PC_REGNUM))
>> +    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, "load/store",
>> +					dsc);
>> +
>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog,
>> +			"displaced: copying %s%s r%d [r%d] insn %.4x%.4x\n",
>> +			load ? (size == 1 ? "ldrb" : (size == 2 ? "ldrh" : "ldr"))
>> +			: (size == 1 ? "strb" : (size == 2 ? "strh" : "str")),
>> +			usermode ? "t" : "",
>> +			rt, rn, insn1, insn2);
>> +
>> +  install_load_store (gdbarch, regs, dsc, load, immed, writeback, size,
>> +		      usermode, rt, rm, rn);
>> +
>> +  if (load || rt != ARM_PC_REGNUM)
>> +    {
>> +      dsc->u.ldst.restore_r4 = 0;
>> +
>> +      if (immed)
>> +	/* {ldr,str}[b]<cond> rt, [rn, #imm], etc.
>> +	   ->
>> +	   {ldr,str}[b]<cond> r0, [r2, #imm].  */
>> +	{
>> +	  dsc->modinsn[0] = (insn1 & 0xfff0) | 0x2;
>> +	  dsc->modinsn[1] = insn2 & 0x0fff;
>> +	}
>> +      else
>> +	/* {ldr,str}[b]<cond> rt, [rn, rm], etc.
>> +	   ->
>> +	   {ldr,str}[b]<cond> r0, [r2, r3].  */
>> +	{
>> +	  dsc->modinsn[0] = (insn1 & 0xfff0) | 0x2;
>> +	  dsc->modinsn[1] = (insn2 & 0x0ff0) | 0x3;
>> +	}
>> +
>> +      dsc->numinsns = 2;
>> +    }
>> +  else
>> +    {
>> +      /* In Thumb-32 instructions, the behavior is unpredictable when Rt is
>> +	 PC, while the behavior is undefined when Rn is PC.  Shortly, neither
>> +	 Rt nor Rn can be PC.  */
>> +
>> +      gdb_assert (0);
>> +    }
> 
> See above, this should only be used for loads.
> 

This is removed.

> 
>> +static int
>> +thumb2_copy_block_xfer (struct gdbarch *gdbarch, uint16_t insn1, uint16_t insn2,
>> +			struct regcache *regs,
>> +			struct displaced_step_closure *dsc)
>> +{
>> +  int rn = bits (insn1, 0, 3);
>> +  int load = bit (insn1, 4);
>> +  int writeback = bit (insn1, 5);
>> +
>> +  /* Block transfers which don't mention PC can be run directly
>> +     out-of-line.  */
>> +  if (rn != ARM_PC_REGNUM && (insn2 & 0x8000) == 0)
>> +    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2, "ldm/stm", dsc);
>> +
>> +  if (rn == ARM_PC_REGNUM)
>> +    {
>> +      warning (_("displaced: Unpredictable LDM or STM with "
>> +		 "base register r15"));
>> +      return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +					  "unpredictable ldm/stm", dsc);
>> +    }
>> +
>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog, "displaced: copying block transfer insn "
>> +			"%.4x%.4x\n", insn1, insn2);
>> +
>> +  /* Clear bit 13, since it should be always zero.  */
>> +  dsc->u.block.regmask = (insn2 & 0xdfff);
>> +  dsc->u.block.rn = rn;
>> +
>> +  dsc->u.block.load = bit (insn1, 4);
> We've already read that bit into "load".
> 

Fixed.

>> +  dsc->u.block.user = bit (insn1, 6);
> This must always be 0 -- we're never called otherwise.
> 

Fixed.

>> +static int
>> +thumb2_decode_dp_shift_reg (struct gdbarch *gdbarch, uint16_t insn1,
>> +			    uint16_t insn2,  struct regcache *regs,
>> +			    struct displaced_step_closure *dsc)
>> +{
>> +  /* Data processing (shift register) instructions can be grouped according to
>> +     their encondings:
> Typo.
>> +
>> +     1. Insn X Rn :inst1,3-0 Rd: insn2,8-11, Rm: insn2,3-0. Rd=15 & S=1, Insn Y.
>> +     Rn != PC, Rm ! = PC.
>> +     X: AND, Y: TST (REG)
>> +     X: EOR, Y: TEQ (REG)
>> +     X: ADD, Y: CMN (REG)
>> +     X: SUB, Y: CMP (REG)
>> +
>> +     2. Insn X Rn : ins1,3-0, Rm: insn2, 3-0; Rm! = PC, Rn != PC
>> +     Insn X: TST, TEQ, PKH, CMN, and CMP.
>> +
>> +     3. Insn X Rn:inst1,3-0 Rd:insn2,8-11, Rm:insn2, 3-0. Rn != PC, Rd != PC,
>> +     Rm != PC.
>> +     X: BIC, ADC, SBC, and RSB.
>> +
>> +     4. Insn X Rn:inst1,3-0 Rd:insn2,8-11, Rm:insn2,3-0.  Rd = 15, Insn Y.
>> +     X: ORR, Y: MOV (REG).
>> +     X: ORN, Y: MVN (REG).
>> +
>> +     5.  Insn X Rd: insn2, 8-11, Rm: insn2, 3-0.
>> +     X: MVN, Rd != PC, Rm != PC
>> +     X: MOV: Rd/Rm can be PC.
>> +
>> +     PC is only allowed to be used in instruction MOV.
>> +*/
> 
> Do we need this comment at all (except for the last sentence)?
> 
> 

I leave this comment there to help me to remind what instruction is
still missing here.  It is somewhat redundant when it is done.  Removed
except for the last one.

>>  static int
>> +thumb_copy_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 ADD, so we simply
> Typo: immediate

Fixed.

>> +     extract raw immediate encoding rather than computing immediate.  When
>> +     generating ADD instruction, we can simply perform OR operation to set
>> +     immediate into ADD.  */
>> +  unsigned int imm_3_8 = insn2 & 0x70ff;
>> +  unsigned int imm_i = insn1 & 0x0400; /* Clear all bits except bit 10.  */
>> +
>> +  if (debug_displaced)
>> +    fprintf_unfiltered (gdb_stdlog,
>> +			"displaced: copying thumb adr r%d, #%d:%d insn %.4x%.4x\n",
>> +			rd, imm_i, imm_3_8, insn1, insn2);
>> +
>> +  /* Encoding T3: ADD Rd, Rd, #imm */
>> +  dsc->modinsn[0] = (0xf100 | rd | imm_i);
>> +  dsc->modinsn[1] = ((rd << 8) | imm_3_8);
> 
> Hmm.  So this handles the T3 encoding of ADR correctly.  However, in the T2
> encoding, we need to *subtract* the immediate from PC, so we really need to
> generate a SUB instead of an ADD as replacement ...
> 
> 

We generate SUB (immediate) Encoding T3 for ADR Encoding T2 in new patch.

>> +/* Copy Table Brach Byte/Halfword */
> Type: Branch
> 
> 

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 user_mode = (bits (insn2, 8, 11) == 0xe);
> 
> This is too simplistic.  The "long immediate" forms may just accidentally
> have 0xe in those bits -- they're part of the immediate there.   See above
> for the comments about computing immediate/writeback/usermode flags at
> the same location.
> 

This part is re-written to decode immediate/writeback/usermode bits.

>> +  int err = 0;
>> +  int writeback = 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
>> +	    {
>> +	      switch (op1)
>> +		{
>> +		case 0: case 2:
>> +		  if (bits (insn2, 8, 11) == 0x1110
>> +		      || (bits (insn2, 8, 11) & 0x6) == 0x9)
>> +		    return thumb_32bit_copy_unpred (gdbarch, insn1, insn2, dsc);
>> +		  else
>> +		    /* PLI/PLD (reigster, immediate) doesn't use PC.  */
>> +		    return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +							"pli/pld", dsc);
>> +		  break;
>> +		case 1: /* PLD/PLDW (immediate) */
>> +		case 3: /* PLI (immediate, literal) */
>> +		  return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						      "pli/pld", dsc);
>> +		  break;
>> +
>> +		}
> 
> I'd just make the whole block use copy_unmodified ...  That's some complexity
> here for no real gain.
> 

OK.  Replace them all with a single copy_unmodified routine.

>> +	    }
>> +	}
> 
> 
>> +      else
>> +	{
>> +	  if ((op1 == 0 || op1 == 2) && bit (insn2, 11))
>> +	    writeback = bit (insn2, 8);
>> +
>> +	  return thumb2_copy_load_store (gdbarch, insn1, insn2, regs, dsc, 1, 1,
>> +					 user_mode, writeback);
> 
> As discussed above, we'll have to distiguish the "literal" forms from
> the immediate forms.
> 

Fixed.

>> +	}
> 
> 
>> +    case 1: /* Load halfword and memory hints.  */
>> +      if (rt == 0xf) /* PLD{W} and Unalloc memory hint.  */
>> +	{
>> +	  if (rn == 0xf)
>> +	    {
>> +	      if (op1 == 0 || op1 == 1)
>> +		return thumb_32bit_copy_unpred (gdbarch, insn1, insn2, dsc);
>> +	      else
>> +		return thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						    "unalloc memhint", dsc);
>> +	    }
>> +	  else
>> +	    {
>> +	      if ((op1 == 0 || op1 == 2)
>> +		  && (bits (insn2, 8, 11) == 0xe
>> +		      || ((bits (insn2, 8, 11) & 0x9) == 0x9)))
>> +		return thumb_32bit_copy_unpred (gdbarch, insn1, insn2, dsc);
>> +	      else thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						"pld/unalloc memhint", dsc);
>> +	    }
> 
> See above, it's probably not worth to make such fine-grained distinctions
> when the result is effectively the same anyway.
> 

OK.  They are combined into a single call to copy_unmodified routine.

>> +	}
>> +      else
>> +	{
>> +	  int op1 = bits (insn1, 7, 8);
>> +
>> +	  if ((op1 == 0 || op1 == 2) && bit (insn2, 11))
>> +	    writeback = bit (insn2, 8);
>> +	  return thumb2_copy_load_store (gdbarch, insn1, insn2, regs, dsc, 1,
>> +					 2, user_mode, writeback);
> 
> See above for literal forms; computation of writeback etc. flags.
> 
>> +	}
>> +      break;
>> +    case 2: /* Load word */
>> +      {
>> +	int op1 = bits (insn1, 7, 8);
>> +
>> +	  if ((op1 == 0 || op1 == 2) && bit (insn2, 11))
>> +	    writeback = bit (insn2, 8);
>> +
>> +	return thumb2_copy_load_store (gdbarch, insn1, insn2, regs, dsc, 1, 4,
>> +				       user_mode, writeback);
>> +	break;
> 
> Likewise.
> 
> 
>> +static int
>> +decode_thumb_32bit_store_single_data_item (struct gdbarch *gdbarch,
>> +					   uint16_t insn1, uint16_t insn2,
>> +					   struct regcache *regs,
>> +					   struct displaced_step_closure *dsc)
>> +{
>> +  int user_mode = (bits (insn2, 8, 11) == 0xe);
>> +  int size = 0;
>> +  int writeback = 0;
>> +  int op1 = bits (insn1, 5, 7);
>> +
>> +  switch (op1)
>> +    {
>> +    case 0: case 4: size = 1; break;
>> +    case 1: case 5: size = 2; break;
>> +    case 2: case 6: size = 4; break;
>> +    }
>> +  if (bits (insn1, 5, 7) < 3 && bit (insn2, 11))
>> +    writeback = bit (insn2, 8);
>> +
>> +  return thumb2_copy_load_store (gdbarch, insn1, insn2, regs,
>> +				 dsc, 0, size, user_mode,
>> +				 writeback);
>> +
>> +}
> 
> As per the discussion above, this function is probably unnecessary,
> since stores cannot use the PC in Thumb mode.
> 
> 

Yes, it is removed.

>>  static void
>>  thumb_process_displaced_32bit_insn (struct gdbarch *gdbarch, uint16_t insn1,
>>  				    uint16_t insn2, struct regcache *regs,
>>  				    struct displaced_step_closure *dsc)
>>  {
> 
>> +    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, 7, 9) != 0x7)
>> +	    /* Conditional Branch */
>> +	    err = thumb2_copy_b_bl_blx (gdbarch, insn1, insn2, regs, dsc);
> 
> See above for the problems with this condition.  Also, you're missing
> (some) *unconditional* branch instructions (B) here; those have bit 12
> equal to 1.
> 
> Maybe the checks should be combined into:
>   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);
> 

Yeah, it looks right.

>> +    case 3: /* op1 = 3 */
>> +      switch (bits (insn1, 9, 10))
>> +	{
>> +	case 0:
>> +	  if ((bits (insn1, 4, 6) & 0x5) == 0x1)
>> +	    err = decode_thumb_32bit_ld_mem_hints (gdbarch, insn1, insn2,
>> +						   regs, dsc);
> 
> This check misses the "Load word" instructions.  It should probably
> just be "if (bit (insn1, 4))" at this point.
> 

Fixed.

>> +	  else
>> +	    {
>> +	      if (bit (insn1, 8)) /* NEON Load/Store */
>> +		err = thumb_copy_unmodified_32bit (gdbarch, insn1, insn2,
>> +						   "neon elt/struct load/store",
>> +						   dsc);
>> +	      else /* Store single data item */
>> +		err = decode_thumb_32bit_store_single_data_item (gdbarch,
>> +								 insn1, insn2,
>> +								 regs, dsc);
> 
> As discussed above, I think those can all be copied unmodified.
> 

Done.

-- 
Yao (齐尧)