Re: [patch 2/3] Displaced stepping for 16-bit Thumb instructions

[Yao Qi <yao at codesourcery dot com>]

On 02/18/2011 03:22 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
> 
> 
>> -  unsigned long modinsn[DISPLACED_MODIFIED_INSNS];
>> +
>> +  struct insn
>> +  {
>> +    union
>> +    {
>> +      unsigned long a;
>> +      unsigned short t;
>> +    }insn;
>> +    unsigned short size;
>> +  }modinsns[DISPLACED_MODIFIED_INSNS];
>> +
> 
> I don't think this is the right way to go.  You cannot have a mixture of
> ARM and Thumb instructions in a single modinsn block, and if you have
> Thumb instructions, they all need to be transfered in 16-bit chunks,
> even the 32-bit Thumb2 instructions, to get the endian conversion right.
> 

I don't have a mixture of ARM and Thumb instructions in a single modinsn
block.  When displace stepping 16-bit instructions, modinsn[].insn.t is
used to record 16-bit instructions and all instructions in copy area are
16-bit also.  In 32-bit case, modinsn[].insn.a is used, and all
instructions in copy area are 32-bit.

> So I think you should rather keep a single modinsn array of unsigned long.
> When filling it in, ARM instructions are handled as today, 16-bit Thumb
> instructions are likewise just filled into one modinsn slot, and 32-bit
> Thumb instructions are filled into two modinsn slots.
> 
> When copying the modinsn array out to the target, each slot is transfered
> as 4 bytes in ARM mode, and as 2 bytes in Thumb mode.  To know in which
> mode you are, it is probably best to have a single flag in the struct
> displaced_step_closure that indicated whether it is ARM or Thumb; this
> flag would be set once at the start of arm_process_displaced_insn, and
> used throughout the code whereever we need to know the mode.

The reason I propose a union here is to try to avoid too-many byte
operations during recording instructions and copying to copy area.  The
union will waste some space in 16-bit instructions case, but IMO, it
doesn't matter too much.

I agree that we should a single flag for mode, and remove field size
from struct insn.

The changes in `struct displaced_step_closure' is like this,

 -  unsigned long modinsn[DISPLACED_MODIFIED_INSNS];
 +
 +  unsigned short flag; /* indicates the mode of instructions in
MODINSNS.  */
 +    union
 +    {
 +      unsigned long a;
 +      unsigned short t;
 +    }modinsns[DISPLACED_MODIFIED_INSNS];

Do you agree on this proposed data structure?  We need an agreement on
this basic data structure before I start to write/change the rest of
patches.

> 
> This approach would make most of the changes in this patch obsolete.
> 
>> 	(cleanup_branch): Replace magic number by macros.
> 
>> -      ULONGEST pc = displaced_read_reg (regs, from, 15);
>> -      displaced_write_reg (regs, dsc, 14, pc - 4, CANNOT_WRITE_PC);
>> +      ULONGEST pc = displaced_read_reg (regs, from, ARM_PC_REGNUM);
>> +      displaced_write_reg (regs, dsc, ARM_LR_REGNUM, pc - 4, CANNOT_WRITE_PC);
> 
> I'm not sure about this change -- other callers just pass in plain
> register numbers as well ...  Either those should all be changed,
> or none of them.  In any case, this is really an unrelated change,
> and should be done -if at all- in a separate patch.
> 

I'll remove this chunk from my patch, and create another patch specific
to this 'magic number' problem separately.


> 
>>    /* Put breakpoint afterwards.  */
>> -  write_memory (to + dsc->numinsns * 4, tdep->arm_breakpoint,
>> -		tdep->arm_breakpoint_size);
>> +  write_memory (to + arm_displaced_step_breakpoint_offset (dsc),
>> +		arm_breakpoint_from_pc (gdbarch, &from, &len),
>> +		len);
> 
> Calling arm_breakpoint_from_pc is not a good idea, since this calls
> arm_pc_is_thumb, which may end up getting a wrong result.  Since we
> already know whether we're in ARM or Thumb mode, you should just
> emit either tdep->arm_breakpoint or tdep->thumb_breakpoint.  (Since
> we're not *replacing* any instruction here, there is never a need
> to use the Thumb-2 breakpoint.)
> 

Yes, we've already known the mode.  We can use either
tdep->arm_breakpoint or tdep->thumb_breakpoint directly.

>> @@ -5960,7 +6001,11 @@ arm_displaced_step_fixup (struct gdbarch *gdbarch,
>>      dsc->cleanup (gdbarch, regs, dsc);
>>  
>>    if (!dsc->wrote_to_pc)
>> -    regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM, dsc->insn_addr + 4);
>> +    {
>> +      struct frame_info *fi = get_current_frame ();
>> +      regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM,
>> +				      arm_get_next_pc_raw(fi, dsc->insn_addr, 0));
>> +    }
> 
> Hmm, arm_get_next_pc_raw tries to follow branches etc, which is probably
> not what we want here.  Again, I'd rather just check ARM vs. Thumb state
> (in Thumb mode we could then check the instruction to see whether it is
> a 16-bit or 32-bit instruction --- or even better, the original decoding
> step could have just set a flag in dsc).

`if (!dsc->wrote_to_pc)' guard that we will not follow branch in this
case.  However, since we've known the mode, we can adjust pc directly,
without bothering complicated arm_get_next_pc_raw.

-- 
Yao (éå)