Powerpc skip prologue

Powerpc skip prologue

[Aleksandar Ristovski]

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

Hello,

GDB assumes the gpr registers will be saved starting from a rN register up to r31. This assumption doesn't seem to be right. See this: http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html 
and this: http://sourceware.org/ml/gdb/2008-07/msg00279.html

So I devised a micro-patch for handling the saved gprs. I based it on the Daniel's observations and my own, by disassembling several functions - they appear to have prologue that will save several general purpose registers in the ascending register index order, but not up to r31. For example, r30 only, or r28,r29 etc.

Unfortunately, I can only test this on our (Neutrino) powerpc targets.

(No ChangeLog since I can not claim this is a final and correct solution in compliance with the ABI. If it turns out that ABI allows for saving registers non-sequentially or out-of order, e.g. r28, r30, r29, then this is not good and we need a more thorough patch that would allow for such situations).


Thanks,

Aleksandar Ristovski
QNX Software Systems


[-- Attachment #2: rs6000-tdep.c.prologueissue.diff --]
[-- Type: text/plain, Size: 1533 bytes --]

Index: gdb/rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.318
diff -u -p -r1.318 rs6000-tdep.c
--- gdb/rs6000-tdep.c	15 Jul 2008 18:32:06 -0000	1.318
+++ gdb/rs6000-tdep.c	28 Jul 2008 20:49:24 -0000
@@ -117,6 +117,7 @@ struct rs6000_framedata
 				   by which we decrement sp to allocate
 				   the frame */
     int saved_gpr;		/* smallest # of saved gpr */
+    int saved_gpr_max;		/* Largest # of saved gpr */
     int saved_fpr;		/* smallest # of saved fpr */
     int saved_vr;               /* smallest # of saved vr */
     int saved_ev;               /* smallest # of saved ev */
@@ -1197,6 +1198,7 @@ skip_prologue (struct gdbarch *gdbarch, 
 
   memset (fdata, 0, sizeof (struct rs6000_framedata));
   fdata->saved_gpr = -1;
+  fdata->saved_gpr_max = -1;
   fdata->saved_fpr = -1;
   fdata->saved_vr = -1;
   fdata->saved_ev = -1;
@@ -1282,6 +1284,8 @@ skip_prologue (struct gdbarch *gdbarch, 
 		op &= ~3UL;
 	      fdata->gpr_offset = SIGNED_SHORT (op) + offset;
 	    }
+	  if (fdata->saved_gpr_max < reg)
+	    fdata->saved_gpr_max = reg;
 	  continue;
 
 	}
@@ -2571,7 +2575,7 @@ rs6000_frame_cache (struct frame_info *t
     {
       int i;
       CORE_ADDR gpr_addr = cache->base + fdata.gpr_offset;
-      for (i = fdata.saved_gpr; i < ppc_num_gprs; i++)
+      for (i = fdata.saved_gpr; i <= fdata.saved_gpr_max; i++)
 	{
 	  cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
 	  gpr_addr += wordsize;

Re: Powerpc skip prologue

[Joel Brobecker]

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

> GDB assumes the gpr registers will be saved starting from a rN register up 
> to r31. This assumption doesn't seem to be right. See this: 
> http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html and this: 
> http://sourceware.org/ml/gdb/2008-07/msg00279.html
> 
> So I devised a micro-patch for handling the saved gprs.

I just realized that we faced the same problem and forgot to submit
the patch to the FSF. The ABI doesn't say anything about requiring
that a sequence of registers be saved, which is why we introduced
the use of a map rather than just save the lowest/highest register
number.

I propose to commit the following in a week unless there are comments/
objections:

2008-08-09  Jerome Guitton  <guitton@adacore.com>

        * rs6000-tdep.c (rs6000_framedata): Add new field.
        (SET_REG_IN_MAP, GET_REG_IN_MAP): New macros.
        (skip_prologue): Update register map when a register is saved.
        (rs6000_frame_cache): Only set register address if the corresponding
        register has been saved.

Tested on ppc-aix.

Cheers,
-- 
Joel

[-- Attachment #2: rs6000-tdep.diff --]
[-- Type: text/plain, Size: 2563 bytes --]

Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.318
diff -u -p -r1.318 rs6000-tdep.c
--- rs6000-tdep.c	15 Jul 2008 18:32:06 -0000	1.318
+++ rs6000-tdep.c	9 Aug 2008 14:12:58 -0000
@@ -117,6 +117,9 @@ struct rs6000_framedata
 				   by which we decrement sp to allocate
 				   the frame */
     int saved_gpr;		/* smallest # of saved gpr */
+    unsigned int saved_gpr_map; /* saved gpr map; for any reg in gprs,
+                                   iff bit "# of reg" of saved_gprs_map is 1
+                                   then reg is saved */
     int saved_fpr;		/* smallest # of saved fpr */
     int saved_vr;               /* smallest # of saved vr */
     int saved_ev;               /* smallest # of saved ev */
@@ -1042,6 +1045,9 @@ ppc_deal_with_atomic_sequence (struct fr
 
 #define GET_SRC_REG(x) (((x) >> 21) & 0x1f)
 
+#define SET_REG_IN_MAP(map,x) ((map |= 1 << x))
+#define GET_REG_IN_MAP(map,x) ((map & (1 << x)))
+
 /* Limit the number of skipped non-prologue instructions, as the examining
    of the prologue is expensive.  */
 static int max_skip_non_prologue_insns = 10;
@@ -1155,6 +1161,7 @@ bl_to_blrl_insn_p (CORE_ADDR pc, int ins
    - offset is the initial size of this stack frame --- the amount by
    which we decrement the sp to allocate the frame.
    - saved_gpr is the number of the first saved gpr.
+   - saved_gpr_map is the map of saved gprs.
    - saved_fpr is the number of the first saved fpr.
    - saved_vr is the number of the first saved vr.
    - saved_ev is the number of the first saved ev.
@@ -1197,6 +1204,7 @@ skip_prologue (struct gdbarch *gdbarch, 
 
   memset (fdata, 0, sizeof (struct rs6000_framedata));
   fdata->saved_gpr = -1;
+  fdata->saved_gpr_map = 0;
   fdata->saved_fpr = -1;
   fdata->saved_vr = -1;
   fdata->saved_ev = -1;
@@ -1275,6 +1283,7 @@ skip_prologue (struct gdbarch *gdbarch, 
 	{
 
 	  reg = GET_SRC_REG (op);
+          SET_REG_IN_MAP (fdata->saved_gpr_map, reg);
 	  if (fdata->saved_gpr == -1 || fdata->saved_gpr > reg)
 	    {
 	      fdata->saved_gpr = reg;
@@ -2573,7 +2582,8 @@ rs6000_frame_cache (struct frame_info *t
       CORE_ADDR gpr_addr = cache->base + fdata.gpr_offset;
       for (i = fdata.saved_gpr; i < ppc_num_gprs; i++)
 	{
-	  cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
+          if (GET_REG_IN_MAP (fdata.saved_gpr_map,i))
+            cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
 	  gpr_addr += wordsize;
 	}
     }

Re: Powerpc skip prologue

[Daniel Jacobowitz]

On Sat, Aug 09, 2008 at 06:16:51PM +0400, Joel Brobecker wrote:
> I just realized that we faced the same problem and forgot to submit
> the patch to the FSF. The ABI doesn't say anything about requiring
> that a sequence of registers be saved, which is why we introduced
> the use of a map rather than just save the lowest/highest register
> number.
> 
> I propose to commit the following in a week unless there are comments/
> objections:
> 
> 2008-08-09  Jerome Guitton  <guitton@adacore.com>
> 
>         * rs6000-tdep.c (rs6000_framedata): Add new field.
>         (SET_REG_IN_MAP, GET_REG_IN_MAP): New macros.
>         (skip_prologue): Update register map when a register is saved.
>         (rs6000_frame_cache): Only set register address if the corresponding
>         register has been saved.

Could you test an alternative for me?  The patch from my December
message, but without rs6000_force_frame_sniffer, the
rs6000_gdbarch_init change, or the new testcase.  I believe that will
cover the same cases as your patch, plus several others.  This is a
slightly larger portion of the patch than Aleksandar reposted earlier.

Ref:
  http://sourceware.org/ml/gdb-patches/2007-12/msg00111.html

Patch attached.

-- 
Daniel Jacobowitz
CodeSourcery

2008-08-09  Daniel Jacobowitz  <dan@codesourcery.com>

	* rs6000-tdep.c (struct rs6000_framedata): Add gpr_mask, used_bl,
	lr_register.
	(rs6000_in_function_epilogue_p): Check for bctr.
	(skip_prologue): Initialize lr_register.  Set lr_reg to a register
	number.  Set gpr_mask and used_bl.  Continue scanning while some
	expected registers are not saved.  Set lr_register if LR is not
	stored.
	(rs6000_frame_cache): Handle gpr_mask and lr_register.

	* gdb.arch/powerpc-prologue.exp: Correct saved registers.

Index: rs6000-tdep.c
===================================================================
RCS file: /scratch/gcc/repos/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.300
diff -u -p -r1.300 rs6000-tdep.c
--- rs6000-tdep.c	19 Nov 2007 05:06:24 -0000	1.300
+++ rs6000-tdep.c	6 Dec 2007 20:09:33 -0000
@@ -118,17 +118,20 @@ struct rs6000_framedata
 				   by which we decrement sp to allocate
 				   the frame */
     int saved_gpr;		/* smallest # of saved gpr */
+    unsigned int gpr_mask;	/* Each bit is an individual saved GPR.  */
     int saved_fpr;		/* smallest # of saved fpr */
     int saved_vr;               /* smallest # of saved vr */
     int saved_ev;               /* smallest # of saved ev */
     int alloca_reg;		/* alloca register number (frame ptr) */
     char frameless;		/* true if frameless functions. */
     char nosavedpc;		/* true if pc not saved. */
+    char used_bl;		/* true if link register clobbered */
     int gpr_offset;		/* offset of saved gprs from prev sp */
     int fpr_offset;		/* offset of saved fprs from prev sp */
     int vr_offset;              /* offset of saved vrs from prev sp */
     int ev_offset;              /* offset of saved evs from prev sp */
     int lr_offset;		/* offset of saved lr */
+    int lr_register;		/* register of saved lr, if trustworthy */
     int cr_offset;		/* offset of saved cr */
     int vrsave_offset;          /* offset of saved vrsave register */
   };
@@ -838,6 +841,7 @@ insn_changes_sp_or_jumps (unsigned long 
 static int
 rs6000_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   bfd_byte insn_buf[PPC_INSN_SIZE];
   CORE_ADDR scan_pc, func_start, func_end, epilogue_start, epilogue_end;
   unsigned long insn;
@@ -865,6 +869,17 @@ rs6000_in_function_epilogue_p (struct gd
       insn = extract_unsigned_integer (insn_buf, PPC_INSN_SIZE);
       if (insn == 0x4e800020)
         break;
+      /* Assume a bctr is a tail call unless it points strictly within
+	 this function.  */
+      if (insn == 0x4e800420)
+	{
+	  CORE_ADDR ctr = get_frame_register_unsigned (curfrm,
+						       tdep->ppc_ctr_regnum);
+	  if (ctr > func_start && ctr < func_end)
+	    return 0;
+	  else
+	    break;
+	}
       if (insn_changes_sp_or_jumps (insn))
         return 0;
     }
@@ -1283,6 +1298,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
   fdata->alloca_reg = -1;
   fdata->frameless = 1;
   fdata->nosavedpc = 1;
+  fdata->lr_register = -1;
 
   for (;; pc += 4)
     {
@@ -1324,7 +1340,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	     remember just the first one, but skip over additional
 	     ones.  */
 	  if (lr_reg == -1)
-	    lr_reg = (op & 0x03e00000);
+	    lr_reg = (op & 0x03e00000) >> 21;
           if (lr_reg == 0)
             r0_contains_arg = 0;
 	  continue;
@@ -1355,6 +1371,10 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	{
 
 	  reg = GET_SRC_REG (op);
+	  if ((op & 0xfc1f0000) == 0xbc010000)
+	    fdata->gpr_mask |= ~((1U << reg) - 1);
+	  else
+	    fdata->gpr_mask |= 1U << reg;
 	  if (fdata->saved_gpr == -1 || fdata->saved_gpr > reg)
 	    {
 	      fdata->saved_gpr = reg;
@@ -1446,6 +1466,7 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
       else if (op == 0x48000005)
 	{			/* bl .+4 used in 
 				   -mrelocatable */
+	  fdata->used_bl = 1;
 	  continue;
 
 	}
@@ -1470,7 +1491,10 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	  /* If the return address has already been saved, we can skip
 	     calls to blrl (for PIC).  */
           if (lr_reg != -1 && bl_to_blrl_insn_p (pc, op))
-	    continue;
+	    {
+	      fdata->used_bl = 1;
+	      continue;
+	    }
 
 	  /* Don't skip over the subroutine call if it is not within
 	     the first three instructions of the prologue and either
@@ -1496,8 +1520,9 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 	  if (op == 0x4def7b82 || op == 0)	/* crorc 15, 15, 15 */
 	    break;		/* don't skip over 
 				   this branch */
-	  continue;
 
+	  fdata->used_bl = 1;
+	  continue;
 	}
       /* update stack pointer */
       else if ((op & 0xfc1f0000) == 0x94010000)
@@ -1758,11 +1783,15 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
 
       else
 	{
+	  unsigned int all_mask = ~((1U << fdata->saved_gpr) - 1);
+
 	  /* Not a recognized prologue instruction.
 	     Handle optimizer code motions into the prologue by continuing
 	     the search if we have no valid frame yet or if the return
-	     address is not yet saved in the frame.  */
-	  if (fdata->frameless == 0 && fdata->nosavedpc == 0)
+	     address is not yet saved in the frame.  Also skip instructions
+	     if some of the GPRs expected to be saved are not yet saved.  */
+	  if (fdata->frameless == 0 && fdata->nosavedpc == 0
+	      && (fdata->gpr_mask & all_mask) == all_mask)
 	    break;
 
 	  if (op == 0x4e800020		/* blr */
@@ -1815,6 +1844,9 @@ skip_prologue (CORE_ADDR pc, CORE_ADDR l
     }
 #endif /* 0 */
 
+  if (pc == lim_pc && lr_reg >= 0)
+    fdata->lr_register = lr_reg;
+
   fdata->offset = -fdata->offset;
   return last_prologue_pc;
 }
@@ -2954,7 +2986,8 @@ rs6000_frame_cache (struct frame_info *n
     }
 
   /* if != -1, fdata.saved_gpr is the smallest number of saved_gpr.
-     All gpr's from saved_gpr to gpr31 are saved.  */
+     All gpr's from saved_gpr to gpr31 are saved (except during the
+     prologue).  */
 
   if (fdata.saved_gpr >= 0)
     {
@@ -2962,7 +2995,8 @@ rs6000_frame_cache (struct frame_info *n
       CORE_ADDR gpr_addr = cache->base + fdata.gpr_offset;
       for (i = fdata.saved_gpr; i < ppc_num_gprs; i++)
 	{
-	  cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
+	  if (fdata.gpr_mask & (1U << i))
+	    cache->saved_regs[tdep->ppc_gp0_regnum + i].addr = gpr_addr;
 	  gpr_addr += wordsize;
 	}
     }
@@ -3009,6 +3043,8 @@ rs6000_frame_cache (struct frame_info *n
      holds the LR.  */
   if (fdata.lr_offset != 0)
     cache->saved_regs[tdep->ppc_lr_regnum].addr = cache->base + fdata.lr_offset;
+  else if (fdata.lr_register != -1)
+    cache->saved_regs[tdep->ppc_lr_regnum].realreg = fdata.lr_register;
   /* The PC is found in the link register.  */
   cache->saved_regs[gdbarch_pc_regnum (gdbarch)] =
     cache->saved_regs[tdep->ppc_lr_regnum];
Index: testsuite/gdb.arch/powerpc-prologue.exp
===================================================================
RCS file: /scratch/gcc/repos/src/src/gdb/testsuite/gdb.arch/powerpc-prologue.exp,v
retrieving revision 1.4
diff -u -p -r1.4 powerpc-prologue.exp
--- testsuite/gdb.arch/powerpc-prologue.exp	23 Aug 2007 18:14:16 -0000	1.4
+++ testsuite/gdb.arch/powerpc-prologue.exp	6 Dec 2007 21:39:53 -0000
@@ -84,5 +84,5 @@ gdb_test "backtrace 10" \
 	"backtrace in optimized"
 
 gdb_test "info frame" \
-	".*Saved registers:.*r30 at.*r31 at.*pc at.*lr at.*" \
+	".*Saved registers:.*r30 at.*pc at.*lr at.*" \
 	"saved registers in optimized"

Re: Powerpc skip prologue

[Joel Brobecker]

> Could you test an alternative for me?  The patch from my December
> message, but without rs6000_force_frame_sniffer, the
> rs6000_gdbarch_init change, or the new testcase.  I believe that will
> cover the same cases as your patch, plus several others.  This is a
> slightly larger portion of the patch than Aleksandar reposted earlier.

Sure!

> 2008-08-09  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* rs6000-tdep.c (struct rs6000_framedata): Add gpr_mask, used_bl,
> 	lr_register.
> 	(rs6000_in_function_epilogue_p): Check for bctr.
> 	(skip_prologue): Initialize lr_register.  Set lr_reg to a register
> 	number.  Set gpr_mask and used_bl.  Continue scanning while some
> 	expected registers are not saved.  Set lr_register if LR is not
> 	stored.
> 	(rs6000_frame_cache): Handle gpr_mask and lr_register.
> 
> 	* gdb.arch/powerpc-prologue.exp: Correct saved registers.

This tested OK on ppc-linux :-).

-- 
Joel

Re: Powerpc skip prologue

[Joel Brobecker]

Hi Daniel,

> > 2008-08-09  Daniel Jacobowitz  <dan@codesourcery.com>
> > 
> > 	* rs6000-tdep.c (struct rs6000_framedata): Add gpr_mask, used_bl,
> > 	lr_register.
> > 	(rs6000_in_function_epilogue_p): Check for bctr.
> > 	(skip_prologue): Initialize lr_register.  Set lr_reg to a register
> > 	number.  Set gpr_mask and used_bl.  Continue scanning while some
> > 	expected registers are not saved.  Set lr_register if LR is not
> > 	stored.
> > 	(rs6000_frame_cache): Handle gpr_mask and lr_register.
> > 
> > 	* gdb.arch/powerpc-prologue.exp: Correct saved registers.
> 
> This tested OK on ppc-linux :-).

Can we commit this change? I don't mind at all doing it for you, but
I thought that doing it unilaterally was bad form :).

-- 
Joel

Re: Powerpc skip prologue

[Daniel Jacobowitz]

On Mon, Aug 18, 2008 at 02:09:54PM +0400, Joel Brobecker wrote:
> Can we commit this change? I don't mind at all doing it for you, but
> I thought that doing it unilaterally was bad form :).

Checked in now.  I haven't decided what to do with the remainder of
the original patch - I'll think about it.

-- 
Daniel Jacobowitz
CodeSourcery