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

[Yao Qi <yao@codesourcery.com>]

On 02/18/2011 08:17 PM, Ulrich Weigand wrote:
> 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 ...
> 

Yes, I did that in my Thumb-2 patch.

/* Combine two 16 bit instructions to a 32-bit format.  Some 32-bit Thumb
   instructions have the same encodings as ARM counterparts.  */
#define COMBINE_16BIT_TO_32BIT_INSN(INSN1, INSN2) \
  ((INSN1 << 16) | INSN2)


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

The latter one, I think.

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

Yes.  If I understand you correctly, modinsn is a 'unsigned long' array.
  * ARM instruction occupies one slot with flag `ARM',
  * Thumb 16 bit instruction occupies one slot with flag `Thumb'
  * Thumb 32-bit instruction occupies *two* slots with flag `Thumb',
That works, I think.

I just recall one extra benefit of my original approach is about sharing
some copy_* routines for Thumb 32-bit instructions and ARM instructions.
 In ARM manual, I noticed that some encodings of 32-bit Thumb-2
instructions are the same ARM counterparts (such as preload preload_reg,
and svc_copro), so that their copy routines can be shared.

In order to hide the difference of ARM instructions and 32-bit Thumb-2
instructions in these copy_* routines,  two parts of 32-bit Thumb
instructions are combined as a `32-bit instruction' via macro
COMBINE_16BIT_TO_32BIT_INSN.  Inside these copy_* routines, we don't
have to worry about they are ARM instructions or 32-bit Thumb instructions.

In my proposed approach, each instruction only occupies one slot,
  * ARM instruction, with instruction size `4-byte',
  * Thumb 16 bit instruction, with instruction size `2-byte'
  * Thumb 32-bit instruction, converted and with instruction size `4-byte',

Do you think this benefit is strong enough to convince us to adapt to my
original approach  to `complicate source code' to some extent?

-- 
Yao (齐尧)