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

Yao Qi
2011-03-01T09:01:47Z

[-- Attachment #1: Type: text/plain, Size: 1978 bytes --]

On 03/01/2011 01:07 AM, Ulrich Weigand wrote:
> OK, so at this point I think it's really not necessary to have those
> as macros in the first place. Instead, code should just continue to
> fill in dsc->modinsn, which will shorten this patch significantly :-)
>

I am hesitant to remove these macros, because my following patches are
using these macros. Now, since most of my following patches should be
re-written, I am fine to remove these macros.

>> > @@ -5117,10 +5119,21 @@ displaced_read_reg (struct regcache *regs, CORE_ADDR from, int regno)
>> >
>> > if (regno == 15)
>> > {
>> > + /* 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))
> It would be somewhat nicer here to use dsc->is_thumb instead of re-computing
> its value. However, the displaced_read_reg function doesn't have the dsc
> argument, which is annoying (and asymmetrical to displaced_write_reg ...).
>
> So if you want to make the effort to change all call sites to pass in dsc,
> this would be nice, but I guess I'm also OK with doing it as above.
>

Let us move this change to another patch.

>> > @@ -6904,23 +6919,49 @@ arm_displaced_init_closure (struct gdbarch *gdbarch, CORE_ADDR from,
>> > CORE_ADDR to, struct displaced_step_closure *dsc)
>> > {
>> > struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> > - unsigned int i;
>> > + unsigned int i, len, offset;
>> > enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
>> > + int size = dsc->insn_size;
> Ah, this is wrong: it needs to be "dsc->is_thumb? 2 : 4". Note that if the
> original instruction was 32-bit Thumb2, insn_size will be 4, but we still
> need to copy 2-byte chunks here.

Right.

--
Yao (é½å°§)

[-- Attachment #2: 0000-handle-both-32-bit-and-16-bit.patch --]
[-- Type: text/x-patch, Size: 4954 bytes --]

gdb/
* arm-tdep.h (struct displaced_step_closure): Add two new fields
is_thumb and insn_size.
* arm-tdep.c (displaced_read_reg): Adjust correct pipeline offset
on both ARM and Thumb mode.
(arm_process_displaced_insn): Set is_thumb and insn_size.
(arm_displaced_init_closure): Handle both 16-bit and 32-bit.
(arm_displaced_step_fixup): Likewise.

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index f0e9435..555a6eb 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -5106,6 +5106,8 @@ arm_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
/* NOP instruction (mov r0, r0). */
#define ARM_NOP 0xe1a00000

+static int displaced_in_arm_mode (struct regcache *regs);
+
/* Helper for register reads for displaced stepping. In particular, this
returns the PC as it would be seen by the instruction at its original
location. */
@@ -5117,10 +5119,21 @@ displaced_read_reg (struct regcache *regs, CORE_ADDR from, int regno)

if (regno == 15)
{
+ /* 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;
}
else
{
@@ -6861,6 +6874,8 @@ arm_process_displaced_insn (struct gdbarch *gdbarch, CORE_ADDR from,
if (!displaced_in_arm_mode (regs))
return thumb_process_displaced_insn (gdbarch, from, to, regs, dsc);

+ dsc->is_thumb = 0;
+ dsc->insn_size = 4;
insn = read_memory_unsigned_integer (from, 4, byte_order_for_code);
if (debug_displaced)
fprintf_unfiltered (gdb_stdlog, "displaced: stepping insn %.8lx "
@@ -6904,23 +6919,49 @@ arm_displaced_init_closure (struct gdbarch *gdbarch, CORE_ADDR from,
CORE_ADDR to, struct displaced_step_closure *dsc)
{
struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
- unsigned int i;
+ unsigned int i, len, offset;
enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+ int size = dsc->is_thumb? 2 : 4;
+ const unsigned char *bkp_insn;

+ offset = 0;
/* Poke modified instruction(s). */
for (i = 0; i < dsc->numinsns; i++)
{
if (debug_displaced)
- fprintf_unfiltered (gdb_stdlog, "displaced: writing insn %.8lx at "
- "%.8lx\n", (unsigned long) dsc->modinsn[i],
- (unsigned long) to + i * 4);
- write_memory_unsigned_integer (to + i * 4, 4, byte_order_for_code,
+ {
+ fprintf_unfiltered (gdb_stdlog, "displaced: writing insn ");
+ if (size == 4)
+ fprintf_unfiltered (gdb_stdlog, "%.8lx",
+ dsc->modinsn[i]);
+ else if (size == 2)
+ fprintf_unfiltered (gdb_stdlog, "%.4x",
+ (unsigned short)dsc->modinsn[i]);
+
+ fprintf_unfiltered (gdb_stdlog, " at %.8lx\n",
+ (unsigned long) to + offset);
+
+ }
+ write_memory_unsigned_integer (to + offset, size,
+ byte_order_for_code,
dsc->modinsn[i]);
+ offset += size;
+ }
+
+ /* Choose the correct breakpoint instruction. */
+ if (dsc->is_thumb)
+ {
+ bkp_insn = tdep->thumb_breakpoint;
+ len = tdep->thumb_breakpoint_size;
+ }
+ else
+ {
+ bkp_insn = tdep->arm_breakpoint;
+ len = tdep->arm_breakpoint_size;
}

/* Put breakpoint afterwards. */
- write_memory (to + dsc->numinsns * 4, tdep->arm_breakpoint,
- tdep->arm_breakpoint_size);
+ write_memory (to + offset, bkp_insn, len);

if (debug_displaced)
fprintf_unfiltered (gdb_stdlog, "displaced: copy %s->%s: ",
@@ -6956,7 +6997,9 @@ 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);
+ regcache_cooked_write_unsigned (regs, ARM_PC_REGNUM,
+ dsc->insn_addr + dsc->insn_size);
+
}

#include "bfd-in2.h"
diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
index ef02002..a2293ba 100644
--- a/gdb/arm-tdep.h
+++ b/gdb/arm-tdep.h
@@ -262,6 +262,17 @@ struct displaced_step_closure
struct displaced_step_closure *dsc);
} svc;
} u;
+
+ /* The size of original instruction, 2 or 4. */
+ unsigned int insn_size;
+ /* True if the original insn (and thus all replacement insns) are Thumb
+ instead of ARM. */
+ unsigned int is_thumb;
+
+ /* The slots in the array is used in this way below,
+ - ARM instruction occupies one slot,
+ - Thumb 16 bit instruction occupies one slot,
+ - Thumb 32-bit instruction occupies *two* slots, one part for each. */
unsigned long modinsn[DISPLACED_MODIFIED_INSNS];
int numinsns;
CORE_ADDR insn_addr;