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

Ulrich Weigand
2011-02-25T19:22:20Z

Yao Qi wrote:

> This new patch implements what we discussed above. There is a minor
> difference on rule #3. "Thumb 32-bit instruction occupies *two* slots
> with flag `Thumb-2'", because we have to choose breakpoint type (thumb
> breakpoint or thumb-2 breakpoint) according to this flag.

Actually, there's no need to get complicated w.r.t. breakpoints.
The only reason for using a Thumb-2 breakpoint is if we *replace*
an existing 32-bit instruction and don't want to mess up instruction
stream parsing. (E.g. if the breakpoint is under an IT block and
happens to be skipped, instruction execution would continue with
the "second half" of the replaced instruction if we had used just
a regular Thumb breakpoint.)

However, with displaced stepping, we construct a full instruction
sequence from scratch. In this case, we can just always use a
16-bit Thumb breakpoint instruction. (In fact, throughout the
instruction sequence we construct, we can freely intermix 16-bit
and 32-bit instructions. We just cannot intermix ARM and Thumb,
of course.)

> + /* Compute pipeline offset:
> + - When executing an ARM instruction, PC reads as the address of the
> + current instruction plus 8.
> + - When executing a Thumb instruction, PC reads as the address of the
> + current instruction plus 4. */
> +
> + if (displaced_in_arm_mode (regs))
> + from += 8;
> + else
> + from += 4;
> +
> if (debug_displaced)
> fprintf_unfiltered (gdb_stdlog, "displaced: read pc value %.8lx\n",
> - (unsigned long) from + 8);
> - return (ULONGEST) from + 8; /* Pipeline offset. */
> + (unsigned long) from);
> + return (ULONGEST) from; /* Pipeline offset. */

Just remove the comment from that last line here; the offset is now
handled above.

> dsc->u.ldst.restore_r4 = 1;
> - dsc->modinsn[0] = 0xe92d8000; /* push {pc} */
> - dsc->modinsn[1] = 0xe8bd0010; /* pop {r4} */
> - dsc->modinsn[2] = 0xe044400f; /* sub r4, r4, pc. */
> - dsc->modinsn[3] = 0xe2844008; /* add r4, r4, #8. */
> - dsc->modinsn[4] = 0xe0800004; /* add r0, r0, r4. */
> + RECORD_ARM_MODE_INSN (0, 0xe92d8000); /* push {pc} */
> + RECORD_ARM_MODE_INSN (1, 0xe8bd0010); /* pop {r4} */
> + RECORD_ARM_MODE_INSN (2, 0xe044400f); /* sub r4, r4, pc. */
> + RECORD_ARM_MODE_INSN (3, 0xe2844008); /* add r4, r4, #8. */
> + RECORD_ARM_MODE_INSN (4, 0xe0800004); /* add r0, r0, r4. */
>
> /* As above. */
> if (immed)
> - dsc->modinsn[5] = (insn & 0xfff00fff) | 0x20000;
> + RECORD_ARM_MODE_INSN (5, ((insn & 0xfff00fff) | 0x20000));
> else
> - dsc->modinsn[5] = (insn & 0xfff00ff0) | 0x20003;
> -
> - dsc->modinsn[6] = 0x0; /* breakpoint location. */
> - dsc->modinsn[7] = 0x0; /* scratch space. */
> + RECORD_ARM_MODE_INSN (5, ((insn & 0xfff00ff0) | 0x20003));
>
> + RECORD_ARM_MODE_INSN (6, 0x00); /* breakpoint location. */
> + RECORD_ARM_MODE_INSN (7, 0x00); /* scratch space. */

This reminds me: after your latest patch in that area, we do not
actually use any scratch space in the instruction stream any more,
so this could be removed ...

> + {
> + CORE_ADDR next_pc;
> + if (dsc->insn_mode == ARM)
> + next_pc = dsc->insn_addr + 4;
> + else if (dsc->insn_mode == THUMB)
> + next_pc = dsc->insn_addr + 2;
> + else
> + {
> + struct frame_info *fi = get_current_frame ();
> + enum bfd_endian byte_order_for_code
> + = gdbarch_byte_order_for_code (gdbarch);
> + unsigned short inst1
> + = read_memory_unsigned_integer (dsc->insn_addr, 2,
> + byte_order_for_code);
> +
> + next_pc = dsc->insn_addr + thumb_insn_size (inst1);
> + }

Huh? Shouldn't we know this already? See below ... [*]

> +enum INSN_MODE {ARM, THUMB, THUMB_2};
> +
> /* Structures used for displaced stepping. */
>
> +/* Record an ARM mode instruction in one slot. */
> +#define RECORD_ARM_MODE_INSN(INDEX, INSN) do \
> +{\
> + dsc->modinsn[INDEX] = INSN;\
> + dsc->insn_mode = ARM;\
> + } while (0)

This doesn't really make sense to me; note how the insn_mode flag
gets overwritten every time one of the macros is used.

Rather, the insn_mode flag should be set *once*, at very top
of arm_process_displaced_insn, and it should indicate the mode
of the *original* instruction we're replacing (whether it is
ARM, 16-bit Thumb, or 32-bit Thumb).

If we do that, we don't have to re-examine the original
instruction as above at [*].

[ In fact, it might be even easier to replace insn_mode with
*two* separate fields:

* insn_size holds the size (4 or 2) of the *original* insn
* is_thumb is true if the original insn (and thus all
replacement insns) are Thumb instead of ARM ]

> + /* The mode of instructions copied in array MODINSN. */
> + enum INSN_MODE insn_mode;
> +
> + /* The slots in the array is used in this way below,
> + - 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'. */
> unsigned long modinsn[DISPLACED_MODIFIED_INSNS];

So this should rather say: "insn_mode" is the mode of the
original instruction. If insn_mode is ARM, then each entry of
modinsn holds 4 bytes corresponding to one ARM instruction;
if insn_mode is a Thumb mode, then each modinsn slot holds
2 bytes corresponding to either a 16-bit Thumb instruction
or one half of a 32-bit Thumb instruction.

Bye,
Ulrich

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