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

[Luis Machado <luisgpm@linux.vnet.ibm.com>]

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