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

[Yao Qi <yao@codesourcery.com>]

[-- 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;