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

["Ulrich Weigand" <uweigand@de.ibm.com>]

Yao Qi wrote:
> On 02/18/2011 03:22 AM, Ulrich Weigand wrote:
> > 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.

I'm not sure if I understood you correctly, but 32-bit Thumb2 instructions
must be transfered in 2 16-bit chunks to get the endian conversion right,
just as is done everywhere else in arm-tdep.c.

But maybe this discussion can wait until we see the Thumb2 patch ...

> 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.

Are you talking about operations accessing the target, or computations
done on host?   If the former, the choice of data type for modinsns will
not affect that at all.   If the latter, I don't think there will be
any measurable difference either way ...
 
> 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.

Well, I just don't see the point.  The arm-tdep code usually does:

   dsc->modinsn[...] = <some integer expression>

Code generated from that will not be significantly different whether
the underlying type of dsc->modinsn is short, int, or long; on some
host platforms, having the destination type short will actually require
extra conversion steps ...

Because I don't see any actual performance benefit, I'd argue for keeping
the source code simple.

> I'll remove this chunk from my patch, and create another patch specific
> to this 'magic number' problem separately.
[snip]
> Yes, we've already known the mode.  We can use either
> tdep->arm_breakpoint or tdep->thumb_breakpoint directly.
[snip]
> `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.

OK, thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com