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

Yao Qi
2011-02-18T06:06:12Z

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 (é½å°§)