On 02/26/2011 03:22 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
>> This new patch implements what we discussed above.  There is a minor
>> difference on rule #3.  "Thumb 32-bit instruction occupies *two* slots
>> with flag `Thumb-2'", because we have to choose breakpoint type (thumb
>> breakpoint or thumb-2 breakpoint) according to this flag.
> 
> Actually, there's no need to get complicated w.r.t. breakpoints.
> The only reason for using a Thumb-2 breakpoint is if we *replace*
> an existing 32-bit instruction and don't want to mess up instruction
> stream parsing.  (E.g. if the breakpoint is under an IT block and
> happens to be skipped, instruction execution would continue with
> the "second half" of the replaced instruction if we had used just
> a regular Thumb breakpoint.)
> 
> However, with displaced stepping, we construct a full instruction
> sequence from scratch.  In this case, we can just always use a
> 16-bit Thumb breakpoint instruction.  (In fact, throughout the
> instruction sequence we construct, we can freely intermix 16-bit
> and 32-bit instructions.  We just cannot intermix ARM and Thumb,
> of course.)
> 

Hmm, I think you are right.  Fix it in my new patch.

>> +      /* Compute pipeline offset:
>> +	 - When executing an ARM instruction, PC reads as the address of the
>> +	 current instruction plus 8.
>> +	 - When executing a Thumb instruction, PC reads as the address of the
>> +	 current instruction plus 4.  */
>> +
>> +      if (displaced_in_arm_mode (regs))
>> +	from += 8;
>> +      else
>> +	from += 4;
>> +
>>        if (debug_displaced)
>>  	fprintf_unfiltered (gdb_stdlog, "displaced: read pc value %.8lx\n",
>> -			    (unsigned long) from + 8);
>> -      return (ULONGEST) from + 8;  /* Pipeline offset.  */
>> +			    (unsigned long) from);
>> +      return (ULONGEST) from;  /* Pipeline offset.  */
> 
> Just remove the comment from that last line here; the offset is now
> handled above.
> 

Fixed.

>>        dsc->u.ldst.restore_r4 = 1;
>> -      dsc->modinsn[0] = 0xe92d8000;  /* push {pc} */
>> -      dsc->modinsn[1] = 0xe8bd0010;  /* pop  {r4} */
>> -      dsc->modinsn[2] = 0xe044400f;  /* sub r4, r4, pc.  */
>> -      dsc->modinsn[3] = 0xe2844008;  /* add r4, r4, #8.  */
>> -      dsc->modinsn[4] = 0xe0800004;  /* add r0, r0, r4.  */
>> +      RECORD_ARM_MODE_INSN (0, 0xe92d8000); /* push {pc} */
>> +      RECORD_ARM_MODE_INSN (1, 0xe8bd0010); /* pop  {r4} */
>> +      RECORD_ARM_MODE_INSN (2, 0xe044400f); /* sub r4, r4, pc.  */
>> +      RECORD_ARM_MODE_INSN (3, 0xe2844008); /* add r4, r4, #8.  */
>> +      RECORD_ARM_MODE_INSN (4, 0xe0800004);  /* add r0, r0, r4.  */
>>  
>>        /* As above.  */
>>        if (immed)
>> -	dsc->modinsn[5] = (insn & 0xfff00fff) | 0x20000;
>> +	RECORD_ARM_MODE_INSN (5, ((insn & 0xfff00fff) | 0x20000));
>>        else
>> -	dsc->modinsn[5] = (insn & 0xfff00ff0) | 0x20003;
>> -
>> -      dsc->modinsn[6] = 0x0;  /* breakpoint location.  */
>> -      dsc->modinsn[7] = 0x0;  /* scratch space.  */
>> +	RECORD_ARM_MODE_INSN (5, ((insn & 0xfff00ff0) | 0x20003));
>>  
>> +      RECORD_ARM_MODE_INSN (6, 0x00); /* breakpoint location.  */
>> +      RECORD_ARM_MODE_INSN (7, 0x00); /* scratch space.  */
> 
> This reminds me: after your latest patch in that area, we do not
> actually use any scratch space in the instruction stream any more,
> so this could be removed ...
> 

Oh, Yes.  I'll remove it by another patch.

>> +    {
>> +      CORE_ADDR next_pc;
>> +      if (dsc->insn_mode == ARM)
>> +	next_pc = dsc->insn_addr + 4;
>> +      else if (dsc->insn_mode == THUMB)
>> +	next_pc = dsc->insn_addr + 2;
>> +      else
>> +	{
>> +	  struct frame_info *fi = get_current_frame ();
>> +	  enum bfd_endian byte_order_for_code
>> +	    = gdbarch_byte_order_for_code (gdbarch);
>> +	  unsigned short inst1
>> +	    = read_memory_unsigned_integer (dsc->insn_addr, 2,
>> +					    byte_order_for_code);
>> +
>> +	  next_pc =  dsc->insn_addr + thumb_insn_size (inst1);
>> +	}
> 
> Huh?  Shouldn't we know this already?  See below ...  [*]
> 
> 
> [ In fact, it might be even easier to replace insn_mode with
>   *two* separate fields:
> 
>   * insn_size  holds the size (4 or 2) of the *original* insn
>   * is_thumb   is true if the original insn (and thus all
>                replacement insns) are Thumb instead of ARM ]
> 

OK, two new fields are added in struct displaced_step_closure.  The
computation of next_pc is simplified.

-- 
Yao (齐尧)