Re: [PATCH] [SH] Prologue skipping if there is none

[Kevin Buettner <kevinb@redhat.com>]

On Wed, 15 Feb 2012 07:54:13 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> On Wed, 15 Feb 2012 14:51:31 +0100
> Thomas Schwinge <thomas@codesourcery.com> wrote:
> 
> > The prologue skipping issue is that GDB fails to place breakpoints
> > correctly at the beginning of a function -- such as for ``break main'' --
> > for the case that there is no prologue in that function.
> 
> Hi Thomas,
> 
> I've been sitting on a patch which is similar to what you just posted,
> though I think it might provide more accurate results in some instances.
> Would you mind checking to see if it solves your problem?

Below is an updated patch which builds cleanly against current
sources.  I've verified that it produces better test results than not
having the patch.  I have not compared the test results to Thomas'
patch.

I ran my tests against the simulator.  I found that a sim patch is
necessary to do so.  I'll post that in a moment.

Kevin

	* sh-tdep.c (sh_analyze_prologue): Change loop to run to
	the limit PC.  Keep track of the PC value after frame
	related instructions; return this value.
	(after_prologue): Delete.
	(sh_skip_prologue):  Find the function limit and pass that
	as the limit address to sh_analyze_prologue().  Also use
	skip_prologue_using_sal() and return the lower result.

Index: sh-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sh-tdep.c,v
retrieving revision 1.236
diff -u -p -r1.236 sh-tdep.c
--- sh-tdep.c	28 Jan 2012 18:08:20 -0000	1.236
+++ sh-tdep.c	15 Feb 2012 23:55:14 -0000
@@ -518,39 +518,43 @@ sh_breakpoint_from_pc (struct gdbarch *g
 
 static CORE_ADDR
 sh_analyze_prologue (struct gdbarch *gdbarch,
-		     CORE_ADDR pc, CORE_ADDR current_pc,
+		     CORE_ADDR pc, CORE_ADDR limit_pc,
 		     struct sh_frame_cache *cache, ULONGEST fpscr)
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   ULONGEST inst;
-  CORE_ADDR opc;
+  CORE_ADDR after_last_frame_setup_insn = pc;
+  CORE_ADDR next_pc;
   int offset;
   int sav_offset = 0;
   int r3_val = 0;
   int reg, sav_reg = -1;
 
-  if (pc >= current_pc)
-    return current_pc;
-
   cache->uses_fp = 0;
-  for (opc = pc + (2 * 28); pc < opc; pc += 2)
+
+  for (;pc < limit_pc; pc = next_pc)
     {
       inst = read_memory_unsigned_integer (pc, 2, byte_order);
+      next_pc = pc + 2;
+
       /* See where the registers will be saved to.  */
       if (IS_PUSH (inst))
 	{
 	  cache->saved_regs[GET_SOURCE_REG (inst)] = cache->sp_offset;
 	  cache->sp_offset += 4;
+	  after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_STS (inst))
 	{
 	  cache->saved_regs[PR_REGNUM] = cache->sp_offset;
 	  cache->sp_offset += 4;
+	  after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_MACL_STS (inst))
 	{
 	  cache->saved_regs[MACL_REGNUM] = cache->sp_offset;
 	  cache->sp_offset += 4;
+	  after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_MOV_R3 (inst))
 	{
@@ -563,11 +567,14 @@ sh_analyze_prologue (struct gdbarch *gdb
       else if (IS_ADD_R3SP (inst))
 	{
 	  cache->sp_offset += -r3_val;
+	  after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_ADD_IMM_SP (inst))
 	{
 	  offset = ((inst & 0xff) ^ 0x80) - 0x80;
 	  cache->sp_offset -= offset;
+	  if (offset < 0)
+	    after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_MOVW_PCREL_TO_REG (inst))
 	{
@@ -626,6 +633,7 @@ sh_analyze_prologue (struct gdbarch *gdb
 	      sav_reg = -1;
 	    }
 	  cache->sp_offset += sav_offset;
+	  after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_FPUSH (inst))
 	{
@@ -637,17 +645,20 @@ sh_analyze_prologue (struct gdbarch *gdb
 	    {
 	      cache->sp_offset += 4;
 	    }
+	  after_last_frame_setup_insn = next_pc;
 	}
       else if (IS_MOV_SP_FP (inst))
 	{
+	  CORE_ADDR opc;
 	  cache->uses_fp = 1;
+	  after_last_frame_setup_insn = next_pc;
 	  /* At this point, only allow argument register moves to other
 	     registers or argument register moves to @(X,fp) which are
 	     moving the register arguments onto the stack area allocated
 	     by a former add somenumber to SP call.  Don't allow moving
 	     to an fp indirect address above fp + cache->sp_offset.  */
 	  pc += 2;
-	  for (opc = pc + 12; pc < opc; pc += 2)
+	  for (opc = pc + 12; pc < opc && pc < limit_pc; pc += 2)
 	    {
 	      inst = read_memory_integer (pc, 2, byte_order);
 	      if (IS_MOV_ARG_TO_IND_R14 (inst))
@@ -681,7 +692,10 @@ sh_analyze_prologue (struct gdbarch *gdb
 	     so, note that before returning the current pc.  */
 	  inst = read_memory_integer (pc + 2, 2, byte_order);
 	  if (IS_MOV_SP_FP (inst))
-	    cache->uses_fp = 1;
+	    {
+	      cache->uses_fp = 1;
+	      after_last_frame_setup_insn = pc;
+	    }
 	  break;
 	}
 #if 0		/* This used to just stop when it found an instruction
@@ -693,61 +707,30 @@ sh_analyze_prologue (struct gdbarch *gdb
 #endif
     }
 
-  return pc;
+  return after_last_frame_setup_insn;
 }
 
 /* Skip any prologue before the guts of a function.  */
 
-/* Skip the prologue using the debug information.  If this fails we'll
-   fall back on the 'guess' method below.  */
-static CORE_ADDR
-after_prologue (CORE_ADDR pc)
-{
-  struct symtab_and_line sal;
-  CORE_ADDR func_addr, func_end;
-
-  /* If we can not find the symbol in the partial symbol table, then
-     there is no hope we can determine the function's start address
-     with this code.  */
-  if (!find_pc_partial_function (pc, NULL, &func_addr, &func_end))
-    return 0;
-
-  /* Get the line associated with FUNC_ADDR.  */
-  sal = find_pc_line (func_addr, 0);
-
-  /* There are only two cases to consider.  First, the end of the source line
-     is within the function bounds.  In that case we return the end of the
-     source line.  Second is the end of the source line extends beyond the
-     bounds of the current function.  We need to use the slow code to
-     examine instructions in that case.  */
-  if (sal.end < func_end)
-    return sal.end;
-  else
-    return 0;
-}
-
 static CORE_ADDR
 sh_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
 {
-  CORE_ADDR pc;
+  CORE_ADDR pc, sal_end, func_addr, func_end;
   struct sh_frame_cache cache;
+  const char *name;
 
-  /* See if we can determine the end of the prologue via the symbol table.
-     If so, then return either PC, or the PC after the prologue, whichever
-     is greater.  */
-  pc = after_prologue (start_pc);
-
-  /* If after_prologue returned a useful address, then use it.  Else
-     fall back on the instruction skipping code.  */
-  if (pc)
-    return max (pc, start_pc);
+  /* Try to find the extent of the function that contains PC.  */
+  if (!find_pc_partial_function (start_pc, &name, &func_addr, &func_end))
+    return start_pc;
 
   cache.sp_offset = -4;
-  pc = sh_analyze_prologue (gdbarch, start_pc, (CORE_ADDR) -1, &cache, 0);
-  if (!cache.uses_fp)
-    return start_pc;
+  pc = sh_analyze_prologue (gdbarch, func_addr, func_end, &cache, 0);
 
-  return pc;
+  sal_end = skip_prologue_using_sal (gdbarch, start_pc);
+  if (sal_end != 0 && sal_end != start_pc && sal_end < pc)
+    return sal_end;
+  else
+    return pc;
 }
 
 /* The ABI says: