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

Yao Qi
2011-02-21T06:31:41Z

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