[PATCH] PPC - Stepping off breakpoints in non-stop mode

[PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

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

Hi folks,

This is the ppc-specific code to step off breakpoints in non-stop mode.
The main code is the fixup function, responsible for making sure we have
the correct PC after a displaced stepping has been concluded.

It applies on top of Pedro and Jim's more general displaced stepping
patch.

Tested without regressions on PPC 32/64.

Is this OK? Comments?

Best regards,
Luis

[-- Attachment #2: ppc_displaced.diff --]
[-- Type: text/x-patch, Size: 5783 bytes --]

2008-05-02  Luis Machado  <luisgpm@br.ibm.com>

	* ppc-tdep.h: Define PPC_MAX_INSN_LEN, BRANCH_MASK, B_INSN, BC_INSN,
	LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BXL_INSN, BP_MASK and BP_INSN.
	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Remove LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK and BC_INSTRUCTION.

Index: HEAD/gdb/ppc-tdep.h
===================================================================
--- HEAD.orig/gdb/ppc-tdep.h	2008-05-01 14:00:40.000000000 -0700
+++ HEAD/gdb/ppc-tdep.h	2008-05-02 10:52:53.000000000 -0700
@@ -261,10 +261,28 @@
   PPC_NUM_REGS
 };
 
+/* The length of the longest ppc instruction.  */
+#define PPC_MAX_INSN_LEN (4)
 
 /* Instruction size.  */
 #define PPC_INSN_SIZE 4
 
+/* Instruction masks used during single-stepping of atomic sequences.  */
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
 /* Estimate for the maximum number of instrctions in a function epilogue.  */
 #define PPC_MAX_EPILOGUE_INSTRUCTIONS  52
 
Index: HEAD/gdb/rs6000-tdep.c
===================================================================
--- HEAD.orig/gdb/rs6000-tdep.c	2008-04-30 22:05:39.000000000 -0700
+++ HEAD/gdb/rs6000-tdep.c	2008-05-02 11:22:04.000000000 -0700
@@ -985,16 +985,66 @@
     return little_breakpoint;
 }
 
-
-/* Instruction masks used during single-stepping of atomic sequences.  */
-#define LWARX_MASK 0xfc0007fe
-#define LWARX_INSTRUCTION 0x7c000028
-#define LDARX_INSTRUCTION 0x7c0000A8
-#define STWCX_MASK 0xfc0007ff
-#define STWCX_INSTRUCTION 0x7c00012d
-#define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  unsigned int *insn  = (unsigned int *) closure;
+  unsigned int opcode = (*insn & BRANCH_MASK);
+  int offset = 4; /* Default offset for non PC-relative instructions.  */
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if ((opcode == B_INSN) || (opcode == BC_INSN) || (opcode == BXL_INSN))
+    {
+      /* LK bit Indicates whether we should set the link register to point
+	 to the next instruction or not.  */
+      char link_register_bit = (char) (*insn & 0x1);
+      unsigned long current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch), &current_pc);
+      offset = current_pc - to;
+
+      if (debug_displaced)
+	fprintf_unfiltered (gdb_stdlog,
+			    "displaced: (ppc) branch instruction: 0x%x\n"
+			    "displaced: (ppc) adjusted PC from %s to %s\n",
+			    *insn, paddr_nz (current_pc), paddr_nz (from + offset));
+
+      if (opcode != BXL_INSN)
+	{
+	  /* AA bit indicating whether this is an absolute addressing or
+	     PC-relative. */
+	  char absolute_addr_bit = (char) (*insn & 0x2);
+
+	  if (!absolute_addr_bit)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from + offset);
+	}
+
+      if (link_register_bit)
+	regcache_cooked_write_unsigned (regs, 67, from + 4);
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((*insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from + offset);
+}
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -1032,7 +1082,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           if (bc_insn_count >= 1)
             return 0; /* More than one conditional branch found, fallback 
@@ -3821,6 +3871,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_MAX_INSN_LEN);
+
   return gdbarch;
 }
 

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Pedro Alves]

A Friday 02 May 2008 19:30:18, Luis Machado wrote:

+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+                          struct displaced_step_closure *closure,
+                          CORE_ADDR from, CORE_ADDR to,
+                          struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  unsigned int *insn  = (unsigned int *) closure;
+  unsigned int opcode = (*insn & BRANCH_MASK);

...
+      char link_register_bit = (char) (*insn & 0x1);
+      unsigned long current_pc;

Using unsigned int, char and unsigned long in a tdep file isn't
safe.  Can you switch to gdb_bytes and CORE_ADDR's?  This file
is used for cross-debugging.

-- 
Pedro Alves

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

Ping? Anyone had a chance to look into this?

Best regards,
Luis

On Fri, 2008-05-02 at 15:30 -0300, Luis Machado wrote:
> Hi folks,
> 
> This is the ppc-specific code to step off breakpoints in non-stop mode.
> The main code is the fixup function, responsible for making sure we have
> the correct PC after a displaced stepping has been concluded.
> 
> It applies on top of Pedro and Jim's more general displaced stepping
> patch.
> 
> Tested without regressions on PPC 32/64.
> 
> Is this OK? Comments?
> 
> Best regards,
> Luis

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

On Mon, 2008-05-19 at 14:22 +0100, Pedro Alves wrote:
> A Friday 02 May 2008 19:30:18, Luis Machado wrote:
> 
> +void
> +ppc_displaced_step_fixup (struct gdbarch *gdbarch,
> +                          struct displaced_step_closure *closure,
> +                          CORE_ADDR from, CORE_ADDR to,
> +                          struct regcache *regs)
> +{
> +  /* Since we use simple_displaced_step_copy_insn, our closure is a
> +     copy of the instruction.  */
> +  unsigned int *insn  = (unsigned int *) closure;
> +  unsigned int opcode = (*insn & BRANCH_MASK);
> 
> ...
> +      char link_register_bit = (char) (*insn & 0x1);
> +      unsigned long current_pc;
> 
> Using unsigned int, char and unsigned long in a tdep file isn't
> safe.  Can you switch to gdb_bytes and CORE_ADDR's?  This file
> is used for cross-debugging.

Yes, that's true. I'll get this fixed. Thanks!

Regards,
Luis

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

> > Using unsigned int, char and unsigned long in a tdep file isn't
> > safe.  Can you switch to gdb_bytes and CORE_ADDR's?  This file
> > is used for cross-debugging.
> 
> Yes, that's true. I'll get this fixed. Thanks!

Attached the updated patch with the types fixed and some additional
comments.

Best regards,
Luis


2008-05-28  Luis Machado  <luisgpm@br.ibm.com>

	* ppc-tdep.h: Define PPC_MAX_INSN_LEN, BRANCH_MASK, B_INSN, BC_INSN,
	LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BXL_INSN, BP_MASK and BP_INSN.
	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Remove LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK and BC_INSTRUCTION.

Index: gdb/ppc-tdep.h
===================================================================
--- gdb.orig/ppc-tdep.h	2008-05-28 15:06:16.000000000 -0700
+++ gdb/ppc-tdep.h	2008-05-28 15:09:03.000000000 -0700
@@ -260,10 +260,28 @@
   PPC_NUM_REGS
 };
 
+/* The length of the longest ppc instruction.  */
+#define PPC_MAX_INSN_LEN (4)
 
 /* Instruction size.  */
 #define PPC_INSN_SIZE 4
 
+/* Instruction masks used during single-stepping of atomic sequences.  */
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
 /* Estimate for the maximum number of instrctions in a function epilogue.  */
 #define PPC_MAX_EPILOGUE_INSTRUCTIONS  52
 
Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2008-05-28 15:06:16.000000000 -0700
+++ gdb/rs6000-tdep.c	2008-05-28 17:17:06.000000000 -0700
@@ -841,16 +841,97 @@
     return little_breakpoint;
 }
 
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  ULONGEST *insn  = (ULONGEST *) closure;
+  ULONGEST opcode = 0;
+  LONGEST offset = 0x4; /* Default offset for non PC-relative instructions.  */
+
+  *insn = (*insn >> 32) & 0x00000000ffffffffUL;
+  opcode = *insn & BRANCH_MASK;
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if ((opcode == B_INSN) || (opcode == BC_INSN) || (opcode == BXL_INSN))
+    {
+      /* LK bit Indicates whether we should set the link register to point
+	 to the next instruction or not.  */
+      gdb_byte link_register_bit = (gdb_byte) (*insn & 0x1);
+      CORE_ADDR current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);
+      offset = current_pc - to;
+
+      if (opcode != BXL_INSN)
+	{
+	  /* AA bit indicating whether this is an absolute addressing or
+	     PC-relative. */
+	  gdb_byte absolute_addr_bit = (gdb_byte) (*insn & 0x2);
+
+	  if (!absolute_addr_bit)
+	    {
 
-/* Instruction masks used during single-stepping of atomic sequences.  */
-#define LWARX_MASK 0xfc0007fe
-#define LWARX_INSTRUCTION 0x7c000028
-#define LDARX_INSTRUCTION 0x7c0000A8
-#define STWCX_MASK 0xfc0007ff
-#define STWCX_INSTRUCTION 0x7c00012d
-#define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
+	      if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+			    "displaced: (ppc) branch instruction: 0x%s\n"
+			    "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
+			    paddr_nz (*insn), paddr_nz (current_pc),
+			    paddr_nz (from + offset));
+
+	      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					      from + offset);
+	    }
+	}
+      else
+	{
+	  /* If we're here, it means we have a branch to LR or CTR.  If the
+	     branch was taken, the offset is probably greater than 4 (the next
+	     instruction), so it's safe to assume that a offset of 4 means we
+	     did not take the branch.  */
+	  if (offset == 4)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					    from + 0x4);
+	}
+
+      if (link_register_bit)
+	{
+
+	  regcache_cooked_write_unsigned (regs,
+					  gdbarch_tdep (gdbarch)->ppc_lr_regnum,
+					  from + 0x4);
+	  if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+				    "displaced: (ppc) adjusted LR to 0x%s\n",
+				    paddr_nz (from + 0x4));
+
+	}
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((*insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+  /* Handle any other instructions that do not fit in the categories above.  */
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				    from + offset);
+}
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -887,7 +968,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & ~3) << 16) >> 16;
           int absolute = ((insn >> 1) & 1);
@@ -3214,6 +3295,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_MAX_INSN_LEN);
+
   return gdbarch;
 }
 

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Daniel Jacobowitz]

On Wed, May 28, 2008 at 09:18:12PM -0300, Luis Machado wrote:
> > > Using unsigned int, char and unsigned long in a tdep file isn't
> > > safe.  Can you switch to gdb_bytes and CORE_ADDR's?  This file
> > > is used for cross-debugging.
> > 
> > Yes, that's true. I'll get this fixed. Thanks!
> 
> Attached the updated patch with the types fixed and some additional
> comments.

Sorry, the problem Pedro spotted is still there :-(

> +  /* Since we use simple_displaced_step_copy_insn, our closure is a
> +     copy of the instruction.  */
> +  ULONGEST *insn  = (ULONGEST *) closure;

For instance, what's at closure is four bytes in target byte order.
It may not be the size of a ULONGEST.  The >> 32 is also a hint; you
can see that won't work well on a 32-bit host.

About the rest I'll just have to trust you; I don't know enough about
PowerPC to be sure you got everything.

> +  /* Check for breakpoints in the inferior.  If we've found one, place the PC
> +     right at the breakpoint instruction.  */
> +  else if ((*insn & BP_MASK) == BP_INSN)
> +    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);

I'm not sure this is the sensible thing to do in a GDB context but
it's what we do for i386 so it seems fine.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

On Thu, 2008-06-05 at 16:02 -0400, Daniel Jacobowitz wrote:
> Sorry, the problem Pedro spotted is still there :-(

How about the attached one?

Best regards,
Luis

2008-06-06  Luis Machado  <luisgpm@br.ibm.com>

	* ppc-tdep.h: Define PPC_MAX_INSN_LEN, BRANCH_MASK, B_INSN, BC_INSN,
	LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BXL_INSN, BP_MASK and BP_INSN.
	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Remove LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK and BC_INSTRUCTION.

Index: gdb/ppc-tdep.h
===================================================================
--- gdb.orig/ppc-tdep.h	2008-05-29 06:57:15.000000000 -0700
+++ gdb/ppc-tdep.h	2008-06-05 13:20:46.000000000 -0700
@@ -260,10 +260,28 @@
   PPC_NUM_REGS
 };
 
+/* The length of the longest ppc instruction.  */
+#define PPC_MAX_INSN_LEN (4)
 
 /* Instruction size.  */
 #define PPC_INSN_SIZE 4
 
+/* Instruction masks used during single-stepping of atomic sequences.  */
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
 /* Estimate for the maximum number of instrctions in a function epilogue.  */
 #define PPC_MAX_EPILOGUE_INSTRUCTIONS  52
 
Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2008-05-29 06:57:15.000000000 -0700
+++ gdb/rs6000-tdep.c	2008-06-06 07:20:28.000000000 -0700
@@ -841,16 +841,97 @@
     return little_breakpoint;
 }
 
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  ULONGEST insn  = extract_unsigned_integer ((gdb_byte *) closure,
+					      PPC_MAX_INSN_LEN);
+  ULONGEST opcode = 0;
+  LONGEST offset = PPC_MAX_INSN_LEN; /* Default offset for non PC-relative instructions.  */
+
+  opcode = insn & BRANCH_MASK;
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if ((opcode == B_INSN) || (opcode == BC_INSN) || (opcode == BXL_INSN))
+    {
+      /* LK bit Indicates whether we should set the link register to point
+	 to the next instruction or not.  */
+      gdb_byte link_register_bit = (gdb_byte) (insn & 0x1);
+      CORE_ADDR current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);
+      offset = current_pc - to;
+
+      if (opcode != BXL_INSN)
+	{
+	  /* AA bit indicating whether this is an absolute addressing or
+	     PC-relative.  */
+	  gdb_byte absolute_addr_bit = (gdb_byte) (insn & 0x2);
+
+	  if (!absolute_addr_bit)
+	    {
 
-/* Instruction masks used during single-stepping of atomic sequences.  */
-#define LWARX_MASK 0xfc0007fe
-#define LWARX_INSTRUCTION 0x7c000028
-#define LDARX_INSTRUCTION 0x7c0000A8
-#define STWCX_MASK 0xfc0007ff
-#define STWCX_INSTRUCTION 0x7c00012d
-#define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
+	      if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+			    "displaced: (ppc) branch instruction: 0x%s\n"
+			    "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
+			    paddr_nz (insn), paddr_nz (current_pc),
+			    paddr_nz (from + offset));
+
+	      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					      from + offset);
+	    }
+	}
+      else
+	{
+	  /* If we're here, it means we have a branch to LR or CTR.  If the
+	     branch was taken, the offset is probably greater than 4 (the next
+	     instruction), so it's safe to assume that a offset of 4 means we
+	     did not take the branch.  */
+	  if (offset == PPC_MAX_INSN_LEN)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					    from + PPC_MAX_INSN_LEN);
+	}
+
+      if (link_register_bit)
+	{
+
+	  regcache_cooked_write_unsigned (regs,
+					  gdbarch_tdep (gdbarch)->ppc_lr_regnum,
+					  from + PPC_MAX_INSN_LEN);
+	  if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+				    "displaced: (ppc) adjusted LR to 0x%s\n",
+				    paddr_nz (from + PPC_MAX_INSN_LEN));
+
+	}
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+  /* Handle any other instructions that do not fit in the categories above.  */
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				    from + offset);
+}
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -887,7 +968,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & ~3) << 16) >> 16;
           int absolute = ((insn >> 1) & 1);
@@ -3214,6 +3295,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_MAX_INSN_LEN);
+
   return gdbarch;
 }
 

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

ping?

http://sourceware.org/ml/gdb-patches/2008-06/msg00113.html

Regards,
Luis

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Pedro Alves]

A Friday 06 June 2008 17:00:14, Luis Machado wrote:
> On Thu, 2008-06-05 at 16:02 -0400, Daniel Jacobowitz wrote:
> > Sorry, the problem Pedro spotted is still there :-(
>
> How about the attached one?
>

FWIW, I'm happy with this version.



> +  if ((opcode == B_INSN) || (opcode == BC_INSN) || (opcode == BXL_INSN))

Strictly speaking, the extra parens violate the GNU coding
conventions, as they aren't really needed here.

> +      if (link_register_bit)
> +	{
> +
> +	  regcache_cooked_write_unsigned (regs,

Unneeded extra blank line here.

-- 
Pedro Alves

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

Thanks Pedro.

> > +  if ((opcode == B_INSN) || (opcode == BC_INSN) || (opcode == BXL_INSN))
> 
> Strictly speaking, the extra parens violate the GNU coding
> conventions, as they aren't really needed here.
> 
> > +      if (link_register_bit)
> > +	{
> > +
> > +	  regcache_cooked_write_unsigned (regs,
> 
> Unneeded extra blank line here.

Fixed in the attached one.

Regards,
Luis

2008-06-24  Luis Machado  <luisgpm@br.ibm.com>

	* ppc-tdep.h: Define PPC_MAX_INSN_LEN, BRANCH_MASK, B_INSN, BC_INSN,
	LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BXL_INSN, BP_MASK and BP_INSN.
	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Remove LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK and BC_INSTRUCTION.

Index: gdb/ppc-tdep.h
===================================================================
--- gdb.orig/ppc-tdep.h	2008-06-23 05:13:22.000000000 -0700
+++ gdb/ppc-tdep.h	2008-06-23 05:13:52.000000000 -0700
@@ -260,10 +260,28 @@
   PPC_NUM_REGS
 };
 
+/* The length of the longest ppc instruction.  */
+#define PPC_MAX_INSN_LEN (4)
 
 /* Instruction size.  */
 #define PPC_INSN_SIZE 4
 
+/* Instruction masks used during single-stepping of atomic sequences.  */
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
 /* Estimate for the maximum number of instrctions in a function epilogue.  */
 #define PPC_MAX_EPILOGUE_INSTRUCTIONS  52
 
Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2008-06-23 05:13:22.000000000 -0700
+++ gdb/rs6000-tdep.c	2008-06-24 07:28:56.000000000 -0700
@@ -841,16 +841,95 @@
     return little_breakpoint;
 }
 
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  ULONGEST insn  = extract_unsigned_integer ((gdb_byte *) closure,
+					      PPC_MAX_INSN_LEN);
+  ULONGEST opcode = 0;
+  LONGEST offset = PPC_MAX_INSN_LEN; /* Default offset for non PC-relative instructions.  */
+
+  opcode = insn & BRANCH_MASK;
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if (opcode == B_INSN || opcode == BC_INSN || opcode == BXL_INSN)
+    {
+      /* LK bit Indicates whether we should set the link register to point
+	 to the next instruction or not.  */
+      gdb_byte link_register_bit = (gdb_byte) (insn & 0x1);
+      CORE_ADDR current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);
+      offset = current_pc - to;
+
+      if (opcode != BXL_INSN)
+	{
+	  /* AA bit indicating whether this is an absolute addressing or
+	     PC-relative.  */
+	  gdb_byte absolute_addr_bit = (gdb_byte) (insn & 0x2);
+
+	  if (!absolute_addr_bit)
+	    {
+	      if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+			    "displaced: (ppc) branch instruction: 0x%s\n"
+			    "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
+			    paddr_nz (insn), paddr_nz (current_pc),
+			    paddr_nz (from + offset));
+
+	      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					      from + offset);
+	    }
+	}
+      else
+	{
+	  /* If we're here, it means we have a branch to LR or CTR.  If the
+	     branch was taken, the offset is probably greater than 4 (the next
+	     instruction), so it's safe to assume that a offset of 4 means we
+	     did not take the branch.  */
+	  if (offset == PPC_MAX_INSN_LEN)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					    from + PPC_MAX_INSN_LEN);
+	}
+
+      if (link_register_bit)
+	{
+	  regcache_cooked_write_unsigned (regs,
+					  gdbarch_tdep (gdbarch)->ppc_lr_regnum,
+					  from + PPC_MAX_INSN_LEN);
+	  if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+				    "displaced: (ppc) adjusted LR to 0x%s\n",
+				    paddr_nz (from + PPC_MAX_INSN_LEN));
 
-/* Instruction masks used during single-stepping of atomic sequences.  */
-#define LWARX_MASK 0xfc0007fe
-#define LWARX_INSTRUCTION 0x7c000028
-#define LDARX_INSTRUCTION 0x7c0000A8
-#define STWCX_MASK 0xfc0007ff
-#define STWCX_INSTRUCTION 0x7c00012d
-#define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
+	}
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+  /* Handle any other instructions that do not fit in the categories above.  */
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				    from + offset);
+}
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -887,7 +966,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & ~3) << 16) >> 16;
           int absolute = ((insn >> 1) & 1);
@@ -3214,6 +3293,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_MAX_INSN_LEN);
+
   return gdbarch;
 }

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Joel Brobecker]

Hi Luis,

> 2008-06-24  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* ppc-tdep.h: Define PPC_MAX_INSN_LEN, BRANCH_MASK, B_INSN, BC_INSN,
> 	LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
> 	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BXL_INSN, BP_MASK and BP_INSN.
> 	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
> 	(deal_with_atomic_sequence): Update BC masks.
> 	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
> 	Remove LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION, STWCX_MASK,
> 	STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK and BC_INSTRUCTION.

Overall, this looks good to me, I just have a few questions before we
commit. Thanks much to Pedro for his informal review, btw.

> +/* The length of the longest ppc instruction.  */
> +#define PPC_MAX_INSN_LEN (4)
>  /* Instruction size.  */
>  #define PPC_INSN_SIZE 4

Can we ditch the PPC_MAX_INSN_LEN macro and only use PPC_INSN_SIZE?
Right now, you define the latter but never use it. Right now, you only
use PPC_MAX_INSN_LEN, which I like less than PPC_INSN_SIZE, because
AFAIK all instructions on powerpc are the same size.

> +/* Instruction masks used during single-stepping of atomic sequences.  */
> +#define LWARX_MASK 0xfc0007fe
> +#define LWARX_INSTRUCTION 0x7c000028
> +#define LDARX_INSTRUCTION 0x7c0000A8
> +#define STWCX_MASK 0xfc0007ff
> +#define STWCX_INSTRUCTION 0x7c00012d
> +#define STDCX_INSTRUCTION 0x7c0001ad

Why did you move these macros to the .h file? Right now, they are only
used inside rs6000-tdep.c, so we could keep them there.

> +/* Instruction masks for displaced stepping.  */
> +#define BRANCH_MASK 0xfc000000
> +#define BP_MASK 0xFC0007FE
> +#define B_INSN 0x48000000
> +#define BC_INSN 0x40000000
> +#define BXL_INSN 0x4c000000
> +#define BP_INSN 0x7C000008

Same for these macros, we could certainly keep them inside rs6000-tdep.c.

I'm not opposed to having them in the .h file, if you prefer it that
way. But I generally prefer to keep definitions inside the .c file if
they are not used elsewhere. That way, I instantly know that this macro
is only used within that unit.

> +  ULONGEST opcode = 0;
> +  LONGEST offset = PPC_MAX_INSN_LEN; /* Default offset for non PC-relative instructions.  */

The comment is a little bit too long. I know it would have been nice to
keep the comment on the same line in this case, but I think you'll have
to put it above. You'll problably want to reword it a bit to better fit
with the new order (comment-code vs code-comment). For instance:

    /* The default offset for non PC-relative instructions.  */
    LONGEST offset = PPC_INSN_SIZE;

> +      /* LK bit Indicates whether we should set the link register to point
> +	 to the next instruction or not.  */
> +      gdb_byte link_register_bit = (gdb_byte) (insn & 0x1);

I don't think you need to use a gdb_byte in this case. Wouldn't it
be simpler to use an int? Hopefully, that will allow you to avoid
this cast. Otherwise, it looks like this constant is only used once
in your code:

        if (link_register_bit)

So another alternative is is to embed it inside the "if" statement
with a comment. Something like this:

        if (insn & 0x1)
          {
            /* The LK bit is set. It indicates that we should set
               the link register to point to the next instruction.  */

> +	  /* AA bit indicating whether this is an absolute addressing or
> +	     PC-relative.  */
> +	  gdb_byte absolute_addr_bit = (gdb_byte) (insn & 0x2);

Same here.

> +	  /* If we're here, it means we have a branch to LR or CTR.  If the
> +	     branch was taken, the offset is probably greater than 4 (the next
> +	     instruction), so it's safe to assume that a offset of 4 means we
                                                       ^ "an offset"
> +	     did not take the branch.  */

-- 
Joel

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

> Overall, this looks good to me, I just have a few questions before we
> commit. Thanks much to Pedro for his informal review, btw.

Thanks for looking into this guys.

> Can we ditch the PPC_MAX_INSN_LEN macro and only use PPC_INSN_SIZE?
> Right now, you define the latter but never use it. Right now, you only
> use PPC_MAX_INSN_LEN, which I like less than PPC_INSN_SIZE, because
> AFAIK all instructions on powerpc are the same size.

Yes, this makes sense. I've changed the code to use only PPC_INSN_SIZE. 

> Why did you move these macros to the .h file? Right now, they are only
> used inside rs6000-tdep.c, so we could keep them there.

> Same for these macros, we could certainly keep them inside rs6000-tdep.c.

> I'm not opposed to having them in the .h file, if you prefer it that
> way. But I generally prefer to keep definitions inside the .c file if
> they are not used elsewhere. That way, I instantly know that this macro
> is only used within that unit.

Defines and macros looked better for me in header files since they're
almost always constant values, but i didn't think of it the way you
mentioned. It's a good point. They're back to the .c file.

> I don't think you need to use a gdb_byte in this case. Wouldn't it
> be simpler to use an int? Hopefully, that will allow you to avoid
> this cast. Otherwise, it looks like this constant is only used once
> in your code:
> 
>         if (link_register_bit)
> 
> So another alternative is is to embed it inside the "if" statement
> with a comment. Something like this:

Fixed as well.

How does it look now?

Thanks,
Luis


2008-06-25  Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2008-06-23 05:13:22.000000000 -0700
+++ gdb/rs6000-tdep.c	2008-06-25 06:20:02.000000000 -0700
@@ -841,6 +841,101 @@
     return little_breakpoint;
 }
 
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  ULONGEST insn  = extract_unsigned_integer ((gdb_byte *) closure,
+					      PPC_INSN_SIZE);
+  ULONGEST opcode = 0;
+  /* Offset for non-PC relative instructions.  */
+  LONGEST offset = PPC_INSN_SIZE;
+
+  opcode = insn & BRANCH_MASK;
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if (opcode == B_INSN || opcode == BC_INSN || opcode == BXL_INSN)
+    {
+      CORE_ADDR current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);
+      offset = current_pc - to;
+
+      if (opcode != BXL_INSN)
+	{
+	  if (!(insn & 0x2))
+	    {
+	      /* AA bit indicating whether this is an absolute addressing or
+	      PC-relative.  */
+	      if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+			    "displaced: (ppc) branch instruction: 0x%s\n"
+			    "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
+			    paddr_nz (insn), paddr_nz (current_pc),
+			    paddr_nz (from + offset));
+
+	      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					      from + offset);
+	    }
+	}
+      else
+	{
+	  /* If we're here, it means we have a branch to LR or CTR.  If the
+	     branch was taken, the offset is probably greater than 4 (the next
+	     instruction), so it's safe to assume that an offset of 4 means we
+	     did not take the branch.  */
+	  if (offset == PPC_INSN_SIZE)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					    from + PPC_INSN_SIZE);
+	}
+
+      if (insn & 0x1)
+	{
+	  /* LK bit Indicates whether we should set the link register to point
+	 to the next instruction or not.  */
+	  regcache_cooked_write_unsigned (regs,
+					  gdbarch_tdep (gdbarch)->ppc_lr_regnum,
+					  from + PPC_INSN_SIZE);
+	  if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+				    "displaced: (ppc) adjusted LR to 0x%s\n",
+				    paddr_nz (from + PPC_INSN_SIZE));
+
+	}
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+  /* Handle any other instructions that do not fit in the categories above.  */
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				    from + offset);
+}
 
 /* Instruction masks used during single-stepping of atomic sequences.  */
 #define LWARX_MASK 0xfc0007fe
@@ -849,8 +944,6 @@
 #define STWCX_MASK 0xfc0007ff
 #define STWCX_INSTRUCTION 0x7c00012d
 #define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -887,7 +980,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & ~3) << 16) >> 16;
           int absolute = ((insn >> 1) & 1);
@@ -3214,6 +3307,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
+
   return gdbarch;
 }

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Joel Brobecker]

> 2008-06-25  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
> 	(deal_with_atomic_sequence): Update BC masks.
> 	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
> 	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.

Yep, this looks great. Could you just double-check the formatting of
a couple of comments before you commit? These darn tabs sometimes
confuse me when they are combined with diff markers...

> +  /* Offset for non-PC relative instructions.  */

Should the hyphen be between "PC" and "relative"?
(I-am-not-an-English-native-speaker alert!).

> +	      /* AA bit indicating whether this is an absolute addressing or
> +	      PC-relative.  */

I think there are a couple of spaces missing at the beginning of the
second line. Also, perhaps a comment that says that the bit isn't set
and therefore the addressing is [...] might be helpful? (just an idea,
you don't have to followup if you disagree).

> +	      if (debug_displaced)
> +		fprintf_unfiltered (gdb_stdlog,
> +			    "displaced: (ppc) branch instruction: 0x%s\n"
> +			    "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
> +			    paddr_nz (insn), paddr_nz (current_pc),
> +			    paddr_nz (from + offset));

I missed that part in my previous review. The formatting should be:

		fprintf_unfiltered
		  (gdb_stdlog,
		   "displaced: (ppc) branch instruction: 0x%s\n"
		   "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
		   paddr_nz (insn), paddr_nz (current_pc),
		   paddr_nz (from + offset));



> +	  /* LK bit Indicates whether we should set the link register to point
> +	 to the next instruction or not.  */

Formatting as well on the start of the second line.

-- 
Joel

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

> > +  /* Offset for non-PC relative instructions.  */
> 
> Should the hyphen be between "PC" and "relative"?
> (I-am-not-an-English-native-speaker alert!).

Oops. I just introduced this broken bit.

Hopefully the formatting issues are fixed now. I've added a few bits of
comment to the AA/LK bit portion of code to make thing a bit more clear
as to what each state means exactly.

Thanks,
Luis


2008-06-25  Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2008-06-23 05:13:22.000000000 -0700
+++ gdb/rs6000-tdep.c	2008-06-25 07:03:59.000000000 -0700
@@ -841,6 +841,105 @@
     return little_breakpoint;
 }
 
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  ULONGEST insn  = extract_unsigned_integer ((gdb_byte *) closure,
+					      PPC_INSN_SIZE);
+  ULONGEST opcode = 0;
+  /* Offset for non PC-relative instructions.  */
+  LONGEST offset = PPC_INSN_SIZE;
+
+  opcode = insn & BRANCH_MASK;
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if (opcode == B_INSN || opcode == BC_INSN || opcode == BXL_INSN)
+    {
+      CORE_ADDR current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);
+      offset = current_pc - to;
+
+      if (opcode != BXL_INSN)
+	{
+	  /* Check for AA bit indicating whether this is an absolute
+	     addressing or PC-relative (1: absolute, 0: relative).  */
+	  if (!(insn & 0x2))
+	    {
+	      /* PC-relative addressing is being used in the branch.  */
+	      if (debug_displaced)
+		fprintf_unfiltered
+		  (gdb_stdlog,
+		   "displaced: (ppc) branch instruction: 0x%s\n"
+		   "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
+		   paddr_nz (insn), paddr_nz (current_pc),
+		   paddr_nz (from + offset));
+
+	      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					      from + offset);
+	    }
+	}
+      else
+	{
+	  /* If we're here, it means we have a branch to LR or CTR.  If the
+	     branch was taken, the offset is probably greater than 4 (the next
+	     instruction), so it's safe to assume that an offset of 4 means we
+	     did not take the branch.  */
+	  if (offset == PPC_INSN_SIZE)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					    from + PPC_INSN_SIZE);
+	}
+
+      /* Check for LK bit indicating whether we should set the link
+	 register to point to the next instruction
+	 (1: Set, 0: Don't set).  */
+      if (insn & 0x1)
+	{
+	  /* Link register needs to be set to the next instruction's PC.  */
+	  regcache_cooked_write_unsigned (regs,
+					  gdbarch_tdep (gdbarch)->ppc_lr_regnum,
+					  from + PPC_INSN_SIZE);
+	  if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+				    "displaced: (ppc) adjusted LR to 0x%s\n",
+				    paddr_nz (from + PPC_INSN_SIZE));
+
+	}
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+  /* Handle any other instructions that do not fit in the categories above.  */
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				    from + offset);
+}
 
 /* Instruction masks used during single-stepping of atomic sequences.  */
 #define LWARX_MASK 0xfc0007fe
@@ -849,8 +948,6 @@
 #define STWCX_MASK 0xfc0007ff
 #define STWCX_INSTRUCTION 0x7c00012d
 #define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -887,7 +984,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & ~3) << 16) >> 16;
           int absolute = ((insn >> 1) & 1);
@@ -3214,6 +3311,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
+
   return gdbarch;
 }

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Joel Brobecker]

> 2008-06-25  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
> 	(deal_with_atomic_sequence): Update BC masks.
> 	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
> 	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.

Just in case you were waiting for my re-approval, this is good to go.

Cheers,
-- 
Joel

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

On Wed, 2008-06-25 at 11:51 -0400, Joel Brobecker wrote:
> > 2008-06-25  Luis Machado  <luisgpm@br.ibm.com>
> > 
> > 	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
> > 	(deal_with_atomic_sequence): Update BC masks.
> > 	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
> > 	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.
> 
> Just in case you were waiting for my re-approval, this is good to go.

Thanks for reviewing this one Joel.

Checked in the following.

Regards,
Luis


2008-06-30  Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
	(deal_with_atomic_sequence): Update BC masks.
	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2008-06-23 05:13:22.000000000 -0700
+++ gdb/rs6000-tdep.c	2008-06-25 07:03:59.000000000 -0700
@@ -841,6 +841,105 @@
     return little_breakpoint;
 }
 
+/* Instruction masks for displaced stepping.  */
+#define BRANCH_MASK 0xfc000000
+#define BP_MASK 0xFC0007FE
+#define B_INSN 0x48000000
+#define BC_INSN 0x40000000
+#define BXL_INSN 0x4c000000
+#define BP_INSN 0x7C000008
+
+/* Fix up the state of registers and memory after having single-stepped
+   a displaced instruction.  */
+void
+ppc_displaced_step_fixup (struct gdbarch *gdbarch,
+			   struct displaced_step_closure *closure,
+			   CORE_ADDR from, CORE_ADDR to,
+			   struct regcache *regs)
+{
+  /* Since we use simple_displaced_step_copy_insn, our closure is a
+     copy of the instruction.  */
+  ULONGEST insn  = extract_unsigned_integer ((gdb_byte *) closure,
+					      PPC_INSN_SIZE);
+  ULONGEST opcode = 0;
+  /* Offset for non PC-relative instructions.  */
+  LONGEST offset = PPC_INSN_SIZE;
+
+  opcode = insn & BRANCH_MASK;
+
+  if (debug_displaced)
+    fprintf_unfiltered (gdb_stdlog,
+			"displaced: (ppc) fixup (0x%s, 0x%s)\n",
+			paddr_nz (from), paddr_nz (to));
+
+
+  /* Handle PC-relative branch instructions.  */
+  if (opcode == B_INSN || opcode == BC_INSN || opcode == BXL_INSN)
+    {
+      CORE_ADDR current_pc;
+
+      /* Read the current PC value after the instruction has been executed
+	 in a displaced location.  Calculate the offset to be applied to the
+	 original PC value before the displaced stepping.  */
+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);
+      offset = current_pc - to;
+
+      if (opcode != BXL_INSN)
+	{
+	  /* Check for AA bit indicating whether this is an absolute
+	     addressing or PC-relative (1: absolute, 0: relative).  */
+	  if (!(insn & 0x2))
+	    {
+	      /* PC-relative addressing is being used in the branch.  */
+	      if (debug_displaced)
+		fprintf_unfiltered
+		  (gdb_stdlog,
+		   "displaced: (ppc) branch instruction: 0x%s\n"
+		   "displaced: (ppc) adjusted PC from 0x%s to 0x%s\n",
+		   paddr_nz (insn), paddr_nz (current_pc),
+		   paddr_nz (from + offset));
+
+	      regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					      from + offset);
+	    }
+	}
+      else
+	{
+	  /* If we're here, it means we have a branch to LR or CTR.  If the
+	     branch was taken, the offset is probably greater than 4 (the next
+	     instruction), so it's safe to assume that an offset of 4 means we
+	     did not take the branch.  */
+	  if (offset == PPC_INSN_SIZE)
+	    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+					    from + PPC_INSN_SIZE);
+	}
+
+      /* Check for LK bit indicating whether we should set the link
+	 register to point to the next instruction
+	 (1: Set, 0: Don't set).  */
+      if (insn & 0x1)
+	{
+	  /* Link register needs to be set to the next instruction's PC.  */
+	  regcache_cooked_write_unsigned (regs,
+					  gdbarch_tdep (gdbarch)->ppc_lr_regnum,
+					  from + PPC_INSN_SIZE);
+	  if (debug_displaced)
+		fprintf_unfiltered (gdb_stdlog,
+				    "displaced: (ppc) adjusted LR to 0x%s\n",
+				    paddr_nz (from + PPC_INSN_SIZE));
+
+	}
+    }
+  /* Check for breakpoints in the inferior.  If we've found one, place the PC
+     right at the breakpoint instruction.  */
+  else if ((insn & BP_MASK) == BP_INSN)
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch), from);
+  else
+  /* Handle any other instructions that do not fit in the categories above.  */
+    regcache_cooked_write_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				    from + offset);
+}
 
 /* Instruction masks used during single-stepping of atomic sequences.  */
 #define LWARX_MASK 0xfc0007fe
@@ -849,8 +948,6 @@
 #define STWCX_MASK 0xfc0007ff
 #define STWCX_INSTRUCTION 0x7c00012d
 #define STDCX_INSTRUCTION 0x7c0001ad
-#define BC_MASK 0xfc000000
-#define BC_INSTRUCTION 0x40000000
 
 /* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
    instruction and ending with a STWCX/STDCX instruction.  If such a sequence
@@ -887,7 +984,7 @@
       /* Assume that there is at most one conditional branch in the atomic
          sequence.  If a conditional branch is found, put a breakpoint in 
          its destination address.  */
-      if ((insn & BC_MASK) == BC_INSTRUCTION)
+      if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & ~3) << 16) >> 16;
           int absolute = ((insn >> 1) & 1);
@@ -3214,6 +3311,17 @@
     /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
     tdep->ppc_dl0_regnum += 32;
 
+  /* Setup displaced stepping.  */
+  set_gdbarch_displaced_step_copy_insn (gdbarch,
+					simple_displaced_step_copy_insn);
+  set_gdbarch_displaced_step_fixup (gdbarch, ppc_displaced_step_fixup);
+  set_gdbarch_displaced_step_free_closure (gdbarch,
+					   simple_displaced_step_free_closure);
+  set_gdbarch_displaced_step_location (gdbarch,
+				       displaced_step_at_entry_point);
+
+  set_gdbarch_max_insn_length (gdbarch, PPC_INSN_SIZE);
+
   return gdbarch;
 }

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

On Tue, 2008-07-08 at 15:58 +0100, Jonathan Larmour wrote:
> Luis Machado wrote:
> > Hi Jonathan,
> > 
> > Is this an embedded target?
> 
> Yes, powerpc-eabi

Ok. Changing "current_pc" from CORE_ADDR to ULONGEST fixes the problem.
I'll check it in if folks don't have any objections about this.

Thanks for letting me know.

Luis

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Jonathan Larmour]

Luis Machado wrote:
> Hi Jonathan,
> 
> Is this an embedded target?

Yes, powerpc-eabi

Jifl

>> Unfortunately this fails to build due to this warning (in combination with
>> -Werror):
>> .../rs6000-tdep.c: In function ‘ppc_displaced_step_fixup’:
>> .../rs6000-tdep.c:885: warning: passing argument 3 of
>> ‘regcache_cooked_read_unsigned’ from incompatible pointer type
>> make[2]: *** [rs6000-tdep.o] Error 1
>>
>> This is because in the following line, current_pc is CORE_ADDR, but
>> regcache_cooked_read_unsigned's last arg is meant to be a ULONGEST *.
>>
>> +      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
>> +				      &current_pc);
>>
>> I was thinking of just casting it, but in fact would it be better to be
>> calling regcache_read_pc() here as well as regcache_write_pc() in the rest
>> of the patch when operating on the pc? From earlier in this thread, there
>> was a little discussion about address sizes, so I thought I'd best ask
>> before submitting a patch, as there could be portability implications and
>> it might have been done this way for a reason.
>>
>> Jifl
> 
> 


-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Luis Machado]

Hi Jonathan,

Is this an embedded target?

> Unfortunately this fails to build due to this warning (in combination with
> -Werror):
> .../rs6000-tdep.c: In function ‘ppc_displaced_step_fixup’:
> .../rs6000-tdep.c:885: warning: passing argument 3 of
> ‘regcache_cooked_read_unsigned’ from incompatible pointer type
> make[2]: *** [rs6000-tdep.o] Error 1
> 
> This is because in the following line, current_pc is CORE_ADDR, but
> regcache_cooked_read_unsigned's last arg is meant to be a ULONGEST *.
> 
> +      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
> +				      &current_pc);
> 
> I was thinking of just casting it, but in fact would it be better to be
> calling regcache_read_pc() here as well as regcache_write_pc() in the rest
> of the patch when operating on the pc? From earlier in this thread, there
> was a little discussion about address sizes, so I thought I'd best ask
> before submitting a patch, as there could be portability implications and
> it might have been done this way for a reason.
> 
> Jifl

Re: [PATCH] PPC - Stepping off breakpoints in non-stop mode

[Jonathan Larmour]

Luis Machado wrote:
> [...]
> Checked in the following.
> [...]
> 2008-06-30  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* rs6000-tdep.c (ppc_displaced_step_fixup): New function.
> 	(deal_with_atomic_sequence): Update BC masks.
> 	(rs6000_gdbarch_init): Init displaced stepping infra-structure.
> 	Define BRANCH_MASK, B_INSN, BC_INSN, BXL_INSN, BP_MASK and BP_INSN.

Unfortunately this fails to build due to this warning (in combination with
-Werror):
.../rs6000-tdep.c: In function ‘ppc_displaced_step_fixup’:
.../rs6000-tdep.c:885: warning: passing argument 3 of
‘regcache_cooked_read_unsigned’ from incompatible pointer type
make[2]: *** [rs6000-tdep.o] Error 1

This is because in the following line, current_pc is CORE_ADDR, but
regcache_cooked_read_unsigned's last arg is meant to be a ULONGEST *.

+      regcache_cooked_read_unsigned (regs, gdbarch_pc_regnum (gdbarch),
+				      &current_pc);

I was thinking of just casting it, but in fact would it be better to be
calling regcache_read_pc() here as well as regcache_write_pc() in the rest
of the patch when operating on the pc? From earlier in this thread, there
was a little discussion about address sizes, so I thought I'd best ask
before submitting a patch, as there could be portability implications and
it might have been done this way for a reason.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine