[RFA] Improve Sparc epilogue analysis

[RFA] Improve Sparc epilogue analysis

[David S. Miller]

Unlike other ports, Sparc only had a manual prologue discovery
mechanism.  Most other port have a manual prologue examiner, but they
also first try to use line number information.

To this end, we look for line number information (the first two ".loc"
entries for a function have the same line number, the second one
begins at the end of the prologue) and if found we use it.  Else
we just drop into the existing code.

Also, in sparc_init_extra_frame_info we were using grotty code to find
out if the save instruction in the prologue had executed yet.
Firstly, this relied on the debugging info being there.  Secondly
such things need to be done within the prologue range only, and
examine_prologue was created to figure this out.

Due to a bug in dwarf2 gas up until a week ago, gas would eliminate
these duplicate .loc entries used for prologue discovery by accident.

For those of you playing at home:

		sparc32			sparc64
failures before	88			124
failures after	83			111

More to come.

2002-04-19  David S. Miller  <davem@redhat.com>

	* sparc-tdep.c (examine_prologue): Put forward declaration before
	sparc_init_extra_frame_info.
	(sparc_init_extra_frame_info): Use it to determine if the save
	instruction has been executed yet instead of solely relying upon
	line number information.  Some versions of gas delete duplicates.
	(examine_prologue): Use line number information to find end of
	prologue, if available.  Else, use existing by-hand insn
	examination.

--- sparc-tdep.c.~1~	Fri Apr 19 23:03:54 2002
+++ sparc-tdep.c	Fri Apr 19 23:53:57 2002
@@ -279,6 +279,9 @@ struct frame_extra_info 
   int sp_offset;
 };
 
+static CORE_ADDR examine_prologue (CORE_ADDR, int, struct frame_info *,
+				   CORE_ADDR *);
+
 /* Call this for each newly created frame.  For SPARC, we need to
    calculate the bottom of the frame, and do some extra work if the
    prologue has been generated via the -mflat option to GCC.  In
@@ -393,25 +396,20 @@ sparc_init_extra_frame_info (int fromlea
 	     to the current value of the stack pointer and set
 	     the in_prologue flag.  */
 	  CORE_ADDR addr;
-	  struct symtab_and_line sal;
 
-	  sal = find_pc_line (prologue_start, 0);
-	  if (sal.line == 0)	/* no line info, use PC */
-	    prologue_end = fi->pc;
-	  else if (sal.end < prologue_end)
-	    prologue_end = sal.end;
+	  prologue_end = examine_prologue (prologue_start, 0, NULL, NULL);
 	  if (fi->pc < prologue_end)
 	    {
-	      for (addr = prologue_start; addr < fi->pc; addr += 4)
+	      for (addr = fi->pc; addr < prologue_end; addr += 4)
 		{
 		  insn = read_memory_integer (addr, 4);
 		  if (X_OP (insn) == 2 && X_OP3 (insn) == 0x3c)
 		    break;	/* SAVE seen, stop searching */
 		}
-	      if (addr >= fi->pc)
+	      if (addr < prologue_end)
 		{
 		  fi->extra_info->in_prologue = 1;
-		  fi->frame = read_register (SP_REGNUM);
+		  fi->frame = read_sp ();
 		}
 	    }
 	}
@@ -546,9 +544,6 @@ setup_arbitrary_frame (int argc, CORE_AD
    This routine should be more specific in its actions; making sure
    that it uses the same register in the initial prologue section.  */
 
-static CORE_ADDR examine_prologue (CORE_ADDR, int, struct frame_info *,
-				   CORE_ADDR *);
-
 static CORE_ADDR
 examine_prologue (CORE_ADDR start_pc, int frameless_p, struct frame_info *fi,
 		  CORE_ADDR *saved_regs)
@@ -557,7 +552,21 @@ examine_prologue (CORE_ADDR start_pc, in
   int dest = -1;
   CORE_ADDR pc = start_pc;
   int is_flat = 0;
+  struct symtab_and_line sal;
+  CORE_ADDR func_start, func_end;
+
+  /* This is the preferred method, find the end of the prologue by
+     using the debugging information.  */
+  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
+    {
+      sal = find_pc_line (func_start, 0);
+
+      if (sal.end < func_end
+	  && start_pc <= sal.end)
+	return sal.end;
+    }
 
+  /* Oh well, examine the code by hand.  */
   insn = fetch_instruction (pc);
 
   /* Recognize the `sethi' insn and record its destination.  */

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

"David S. Miller" wrote:
> 
> Unlike other ports, Sparc only had a manual prologue discovery
> mechanism.  Most other port have a manual prologue examiner, but they
> also first try to use line number information.
> 
> To this end, we look for line number information (the first two ".loc"
> entries for a function have the same line number, the second one
> begins at the end of the prologue) and if found we use it.  Else
> we just drop into the existing code.
> 
> Also, in sparc_init_extra_frame_info we were using grotty code to find
> out if the save instruction in the prologue had executed yet.
> Firstly, this relied on the debugging info being there.  Secondly
> such things need to be done within the prologue range only, and
> examine_prologue was created to figure this out.
> 
> Due to a bug in dwarf2 gas up until a week ago, gas would eliminate
> these duplicate .loc entries used for prologue discovery by accident.
> 
> For those of you playing at home:
> 
>                 sparc32                 sparc64
> failures before 88                      124
> failures after  83                      111
> 
> More to come.

No.  Examine prologue serves two roles.  One is simply
to skip to the end of the prologue, but the other is to
actually determine where the registers are saved by the 
prologue.  You've short-circuited the second usage.

Your idea is sound, though -- you've just implemented
it in the wrong place.  Instead of modifying examine_prologue, 
how about if you modify sparc_gdbarch_skip_prologue?


> 
> 2002-04-19  David S. Miller  <davem@redhat.com>
> 
>         * sparc-tdep.c (examine_prologue): Put forward declaration before
>         sparc_init_extra_frame_info.
>         (sparc_init_extra_frame_info): Use it to determine if the save
>         instruction has been executed yet instead of solely relying upon
>         line number information.  Some versions of gas delete duplicates.
>         (examine_prologue): Use line number information to find end of
>         prologue, if available.  Else, use existing by-hand insn
>         examination.
> 
> --- sparc-tdep.c.~1~    Fri Apr 19 23:03:54 2002
> +++ sparc-tdep.c        Fri Apr 19 23:53:57 2002
> @@ -279,6 +279,9 @@ struct frame_extra_info
>    int sp_offset;
>  };
> 
> +static CORE_ADDR examine_prologue (CORE_ADDR, int, struct frame_info *,
> +                                  CORE_ADDR *);
> +
>  /* Call this for each newly created frame.  For SPARC, we need to
>     calculate the bottom of the frame, and do some extra work if the
>     prologue has been generated via the -mflat option to GCC.  In
> @@ -393,25 +396,20 @@ sparc_init_extra_frame_info (int fromlea
>              to the current value of the stack pointer and set
>              the in_prologue flag.  */
>           CORE_ADDR addr;
> -         struct symtab_and_line sal;
> 
> -         sal = find_pc_line (prologue_start, 0);
> -         if (sal.line == 0)    /* no line info, use PC */
> -           prologue_end = fi->pc;
> -         else if (sal.end < prologue_end)
> -           prologue_end = sal.end;
> +         prologue_end = examine_prologue (prologue_start, 0, NULL, NULL);
>           if (fi->pc < prologue_end)
>             {
> -             for (addr = prologue_start; addr < fi->pc; addr += 4)
> +             for (addr = fi->pc; addr < prologue_end; addr += 4)
>                 {
>                   insn = read_memory_integer (addr, 4);
>                   if (X_OP (insn) == 2 && X_OP3 (insn) == 0x3c)
>                     break;      /* SAVE seen, stop searching */
>                 }
> -             if (addr >= fi->pc)
> +             if (addr < prologue_end)
>                 {
>                   fi->extra_info->in_prologue = 1;
> -                 fi->frame = read_register (SP_REGNUM);
> +                 fi->frame = read_sp ();
>                 }
>             }
>         }
> @@ -546,9 +544,6 @@ setup_arbitrary_frame (int argc, CORE_AD
>     This routine should be more specific in its actions; making sure
>     that it uses the same register in the initial prologue section.  */
> 
> -static CORE_ADDR examine_prologue (CORE_ADDR, int, struct frame_info *,
> -                                  CORE_ADDR *);
> -
>  static CORE_ADDR
>  examine_prologue (CORE_ADDR start_pc, int frameless_p, struct frame_info *fi,
>                   CORE_ADDR *saved_regs)
> @@ -557,7 +552,21 @@ examine_prologue (CORE_ADDR start_pc, in
>    int dest = -1;
>    CORE_ADDR pc = start_pc;
>    int is_flat = 0;
> +  struct symtab_and_line sal;
> +  CORE_ADDR func_start, func_end;
> +
> +  /* This is the preferred method, find the end of the prologue by
> +     using the debugging information.  */
> +  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
> +    {
> +      sal = find_pc_line (func_start, 0);
> +
> +      if (sal.end < func_end
> +         && start_pc <= sal.end)
> +       return sal.end;
> +    }
> 
> +  /* Oh well, examine the code by hand.  */
>    insn = fetch_instruction (pc);
> 
>    /* Recognize the `sethi' insn and record its destination.  */

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Tue, 23 Apr 2002 12:19:57 -0700

   No.  Examine prologue serves two roles.  One is simply
   to skip to the end of the prologue, but the other is to
   actually determine where the registers are saved by the 
   prologue.  You've short-circuited the second usage.
   
   Your idea is sound, though -- you've just implemented
   it in the wrong place.  Instead of modifying examine_prologue, 
   how about if you modify sparc_gdbarch_skip_prologue?
   
Right.  There is even extra confusion arising from the
fact that we have two functions for the same thing.

These are sparc_skip_prologue and sparc_gdbarch_skip_prologue.

I killed the latter, and we can "static" the former
when all the sparc targets are multi-arch'd.

How does this revised patch look?

2002-04-23  David S. Miller  <davem@redhat.com>

	* sparc-tdep.c (sparc_gdbarch_skip_prologue): Kill, duplicates
	sparc_skip_prologue.
	(sparc_skip_prologue): Kill frameless_p arg, and use line number
	information to find prologue when possible.
	(sparc_prologue_frameless_p): Use line number information.
	(sparc_gdbarch_init): Update set_gdbarch_skip_prologue call.
	(sparc_init_extra_frame_info): Use sparc_skip_prologue to find
	prologue bounds instead of looking at line number info by hand.
	* config/sparc/tm-sparc.h (sparc_skip_prologue): Update for killed
	second argument.
	(SKIP_PROLOGUE): Likewise.
	
--- ./config/sparc/tm-sparc.h.~1~	Sun Apr 21 19:18:59 2002
+++ ./config/sparc/tm-sparc.h	Tue Apr 23 22:38:43 2002
@@ -250,8 +250,8 @@ extern int sparc_intreg_size (void);
 /* Advance PC across any function entry prologue instructions
    to reach some "real" code.  */
 
-extern CORE_ADDR sparc_skip_prologue (CORE_ADDR, int);
-#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC, 0)
+extern CORE_ADDR sparc_skip_prologue (CORE_ADDR);
+#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC)
 
 /* Immediately after a function call, return the saved pc.
    Can't go through the frames for this because on some machines
--- ./sparc-tdep.c.~1~	Sun Apr 21 20:51:34 2002
+++ ./sparc-tdep.c	Tue Apr 23 23:00:46 2002
@@ -279,6 +279,8 @@ struct frame_extra_info 
   int sp_offset;
 };
 
+CORE_ADDR sparc_skip_prologue (CORE_ADDR);
+
 /* Call this for each newly created frame.  For SPARC, we need to
    calculate the bottom of the frame, and do some extra work if the
    prologue has been generated via the -mflat option to GCC.  In
@@ -393,25 +395,20 @@ sparc_init_extra_frame_info (int fromlea
 	     to the current value of the stack pointer and set
 	     the in_prologue flag.  */
 	  CORE_ADDR addr;
-	  struct symtab_and_line sal;
 
-	  sal = find_pc_line (prologue_start, 0);
-	  if (sal.line == 0)	/* no line info, use PC */
-	    prologue_end = fi->pc;
-	  else if (sal.end < prologue_end)
-	    prologue_end = sal.end;
+	  prologue_end = sparc_skip_prologue (prologue_start);
 	  if (fi->pc < prologue_end)
 	    {
-	      for (addr = prologue_start; addr < fi->pc; addr += 4)
+	      for (addr = fi->pc; addr < prologue_end; addr += 4)
 		{
 		  insn = read_memory_integer (addr, 4);
 		  if (X_OP (insn) == 2 && X_OP3 (insn) == 0x3c)
 		    break;	/* SAVE seen, stop searching */
 		}
-	      if (addr >= fi->pc)
+	      if (addr < prologue_end)
 		{
 		  fi->extra_info->in_prologue = 1;
-		  fi->frame = read_register (SP_REGNUM);
+		  fi->frame = read_sp ();
 		}
 	    }
 	}
@@ -685,18 +682,50 @@ examine_prologue (CORE_ADDR start_pc, in
   return pc;
 }
 
+/* Advance PC across any function entry prologue instructions to reach
+   some "real" code.  */
+
 CORE_ADDR
-sparc_skip_prologue (CORE_ADDR start_pc, int frameless_p)
+sparc_skip_prologue (CORE_ADDR start_pc)
 {
-  return examine_prologue (start_pc, frameless_p, NULL, NULL);
+  struct symtab_and_line sal;
+  CORE_ADDR func_start, func_end;
+
+  /* This is the preferred method, find the end of the prologue by
+     using the debugging information.  */
+  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
+    {
+      sal = find_pc_line (func_start, 0);
+
+      if (sal.end < func_end
+	  && start_pc <= sal.end)
+	return sal.end
+    }
+
+  /* Oh well, examine the code by hand.  */
+  return examine_prologue (start_pc, 0, NULL, NULL);
 }
 
 /* Is the prologue at IP frameless?  */
 
 int
-sparc_prologue_frameless_p (CORE_ADDR ip)
+sparc_prologue_frameless_p (CORE_ADDR start_pc)
 {
-  return ip == sparc_skip_prologue (ip, 1);
+  struct symtab_and_line sal;
+  CORE_ADDR func_start, func_end;
+
+  /* This is the preferred method, find the end of the prologue by
+     using the debugging information.  */
+  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
+    {
+      sal = find_pc_line (func_start, 0);
+
+      if (sal.end < func_end
+	  && start_pc <= sal.end)
+	return start_pc == sal.end;
+    }
+
+  return start_pc == examine_prologue (start_pc, 1, NULL, NULL);
 }
 
 /* Check instruction at ADDR to see if it is a branch.
@@ -2784,15 +2813,6 @@ sparc64_register_byte (int regno)
     return 64 * 8 + (regno - 80) * 8;
 }
 
-/* Advance PC across any function entry prologue instructions to reach
-   some "real" code.  */
-
-static CORE_ADDR
-sparc_gdbarch_skip_prologue (CORE_ADDR ip)
-{
-  return examine_prologue (ip, 0, NULL, NULL);
-}
-
 /* Immediately after a function call, return the saved pc.
    Can't go through the frames for this because on some machines
    the new frame is not set up until the new function executes
@@ -2993,7 +3013,7 @@ sparc_gdbarch_init (struct gdbarch_info 
   set_gdbarch_saved_pc_after_call (gdbarch, sparc_saved_pc_after_call);
   set_gdbarch_prologue_frameless_p (gdbarch, sparc_prologue_frameless_p);
   set_gdbarch_short_bit (gdbarch, 2 * TARGET_CHAR_BIT);
-  set_gdbarch_skip_prologue (gdbarch, sparc_gdbarch_skip_prologue);
+  set_gdbarch_skip_prologue (gdbarch, sparc_skip_prologue);
   set_gdbarch_sp_regnum (gdbarch, SPARC_SP_REGNUM);
   set_gdbarch_use_generic_dummy_frames (gdbarch, 0);
   set_gdbarch_write_pc (gdbarch, generic_target_write_pc);

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

"David S. Miller" wrote:
> 
>    From: Michael Snyder <msnyder@redhat.com>
>    Date: Tue, 23 Apr 2002 12:19:57 -0700
> 
>    No.  Examine prologue serves two roles.  One is simply
>    to skip to the end of the prologue, but the other is to
>    actually determine where the registers are saved by the
>    prologue.  You've short-circuited the second usage.
> 
>    Your idea is sound, though -- you've just implemented
>    it in the wrong place.  Instead of modifying examine_prologue,
>    how about if you modify sparc_gdbarch_skip_prologue?
> 
> Right.  There is even extra confusion arising from the
> fact that we have two functions for the same thing.
> 
> These are sparc_skip_prologue and sparc_gdbarch_skip_prologue.
> 
> I killed the latter, and we can "static" the former
> when all the sparc targets are multi-arch'd.
> 
> How does this revised patch look?

Not quite.  I think you're trying to do too much in one go.
Slow down, and make one change at a time.

In this case, I don't believe you got the frameless_p case 
right.  First, you don't seem to have changed the definition
of SKIP_PROLOGUE_FRAMELESS_P in tm-sparc.h.  Second, your
implementation of sparc_prologue_frameless_p is not 
functionally equivalent to the old one.  If it goes thru
the new, symbolic path, it will not return the same result.

Please try handling the simple rewrite of sparc_skip_prologue
first, and then work on combining the two functions in a
separate pass.  Smaller and simpler patches are also much
easier to review.

> 
> 2002-04-23  David S. Miller  <davem@redhat.com>
> 
>         * sparc-tdep.c (sparc_gdbarch_skip_prologue): Kill, duplicates
>         sparc_skip_prologue.
>         (sparc_skip_prologue): Kill frameless_p arg, and use line number
>         information to find prologue when possible.
>         (sparc_prologue_frameless_p): Use line number information.
>         (sparc_gdbarch_init): Update set_gdbarch_skip_prologue call.
>         (sparc_init_extra_frame_info): Use sparc_skip_prologue to find
>         prologue bounds instead of looking at line number info by hand.
>         * config/sparc/tm-sparc.h (sparc_skip_prologue): Update for killed
>         second argument.
>         (SKIP_PROLOGUE): Likewise.
> 
> --- ./config/sparc/tm-sparc.h.~1~       Sun Apr 21 19:18:59 2002
> +++ ./config/sparc/tm-sparc.h   Tue Apr 23 22:38:43 2002
> @@ -250,8 +250,8 @@ extern int sparc_intreg_size (void);
>  /* Advance PC across any function entry prologue instructions
>     to reach some "real" code.  */
> 
> -extern CORE_ADDR sparc_skip_prologue (CORE_ADDR, int);
> -#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC, 0)
> +extern CORE_ADDR sparc_skip_prologue (CORE_ADDR);
> +#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC)
> 
>  /* Immediately after a function call, return the saved pc.
>     Can't go through the frames for this because on some machines
> --- ./sparc-tdep.c.~1~  Sun Apr 21 20:51:34 2002
> +++ ./sparc-tdep.c      Tue Apr 23 23:00:46 2002
> @@ -279,6 +279,8 @@ struct frame_extra_info
>    int sp_offset;
>  };
> 
> +CORE_ADDR sparc_skip_prologue (CORE_ADDR);
> +
>  /* Call this for each newly created frame.  For SPARC, we need to
>     calculate the bottom of the frame, and do some extra work if the
>     prologue has been generated via the -mflat option to GCC.  In
> @@ -393,25 +395,20 @@ sparc_init_extra_frame_info (int fromlea
>              to the current value of the stack pointer and set
>              the in_prologue flag.  */
>           CORE_ADDR addr;
> -         struct symtab_and_line sal;
> 
> -         sal = find_pc_line (prologue_start, 0);
> -         if (sal.line == 0)    /* no line info, use PC */
> -           prologue_end = fi->pc;
> -         else if (sal.end < prologue_end)
> -           prologue_end = sal.end;
> +         prologue_end = sparc_skip_prologue (prologue_start);
>           if (fi->pc < prologue_end)
>             {
> -             for (addr = prologue_start; addr < fi->pc; addr += 4)
> +             for (addr = fi->pc; addr < prologue_end; addr += 4)
>                 {
>                   insn = read_memory_integer (addr, 4);
>                   if (X_OP (insn) == 2 && X_OP3 (insn) == 0x3c)
>                     break;      /* SAVE seen, stop searching */
>                 }
> -             if (addr >= fi->pc)
> +             if (addr < prologue_end)
>                 {
>                   fi->extra_info->in_prologue = 1;
> -                 fi->frame = read_register (SP_REGNUM);
> +                 fi->frame = read_sp ();
>                 }
>             }
>         }
> @@ -685,18 +682,50 @@ examine_prologue (CORE_ADDR start_pc, in
>    return pc;
>  }
> 
> +/* Advance PC across any function entry prologue instructions to reach
> +   some "real" code.  */
> +
>  CORE_ADDR
> -sparc_skip_prologue (CORE_ADDR start_pc, int frameless_p)
> +sparc_skip_prologue (CORE_ADDR start_pc)
>  {
> -  return examine_prologue (start_pc, frameless_p, NULL, NULL);
> +  struct symtab_and_line sal;
> +  CORE_ADDR func_start, func_end;
> +
> +  /* This is the preferred method, find the end of the prologue by
> +     using the debugging information.  */
> +  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
> +    {
> +      sal = find_pc_line (func_start, 0);
> +
> +      if (sal.end < func_end
> +         && start_pc <= sal.end)
> +       return sal.end
> +    }
> +
> +  /* Oh well, examine the code by hand.  */
> +  return examine_prologue (start_pc, 0, NULL, NULL);
>  }
> 
>  /* Is the prologue at IP frameless?  */
> 
>  int
> -sparc_prologue_frameless_p (CORE_ADDR ip)
> +sparc_prologue_frameless_p (CORE_ADDR start_pc)
>  {
> -  return ip == sparc_skip_prologue (ip, 1);
> +  struct symtab_and_line sal;
> +  CORE_ADDR func_start, func_end;
> +
> +  /* This is the preferred method, find the end of the prologue by
> +     using the debugging information.  */
> +  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
> +    {
> +      sal = find_pc_line (func_start, 0);
> +
> +      if (sal.end < func_end
> +         && start_pc <= sal.end)
> +       return start_pc == sal.end;
> +    }
> +
> +  return start_pc == examine_prologue (start_pc, 1, NULL, NULL);
>  }
> 
>  /* Check instruction at ADDR to see if it is a branch.
> @@ -2784,15 +2813,6 @@ sparc64_register_byte (int regno)
>      return 64 * 8 + (regno - 80) * 8;
>  }
> 
> -/* Advance PC across any function entry prologue instructions to reach
> -   some "real" code.  */
> -
> -static CORE_ADDR
> -sparc_gdbarch_skip_prologue (CORE_ADDR ip)
> -{
> -  return examine_prologue (ip, 0, NULL, NULL);
> -}
> -
>  /* Immediately after a function call, return the saved pc.
>     Can't go through the frames for this because on some machines
>     the new frame is not set up until the new function executes
> @@ -2993,7 +3013,7 @@ sparc_gdbarch_init (struct gdbarch_info
>    set_gdbarch_saved_pc_after_call (gdbarch, sparc_saved_pc_after_call);
>    set_gdbarch_prologue_frameless_p (gdbarch, sparc_prologue_frameless_p);
>    set_gdbarch_short_bit (gdbarch, 2 * TARGET_CHAR_BIT);
> -  set_gdbarch_skip_prologue (gdbarch, sparc_gdbarch_skip_prologue);
> +  set_gdbarch_skip_prologue (gdbarch, sparc_skip_prologue);
>    set_gdbarch_sp_regnum (gdbarch, SPARC_SP_REGNUM);
>    set_gdbarch_use_generic_dummy_frames (gdbarch, 0);
>    set_gdbarch_write_pc (gdbarch, generic_target_write_pc);

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Wed, 24 Apr 2002 16:14:56 -0700

   In this case, I don't believe you got the frameless_p case 
   right.  First, you don't seem to have changed the definition
   of SKIP_PROLOGUE_FRAMELESS_P in tm-sparc.h.  Second, your
   implementation of sparc_prologue_frameless_p is not 
   functionally equivalent to the old one.  If it goes thru
   the new, symbolic path, it will not return the same result.
   
SKIP_PROLOGUE_FRAMELESS_P died within the past day or two from the
whole GDB tree, perhaps you didn't notice :-)

Specifically, here:

2002-04-21  David S. Miller  <davem@redhat.com>

        * arch-utils.c (generic_prologue_frameless_p): Kill
        SKIP_PROLOGUE_FRAMELESS_P code.
        * config/arc/tm-arc.h (SKIP_PROLOGUE_FRAMELESS_P): Delete
        references.
        (PROLOGUE_FRAMELESS_P, arc_prologue_frameless_p): New.
        * arc-tdep.c (arc_prologue_frameless_p): Implement.
        * config/arc/tm-sparc.h (SKIP_PROLOGUE_FRAMELESS_P): Delete
        references.
        (PROLOGUE_FRAMELESS_P, sparc_prologue_frameless_p): New.
        * sparc-tdep.c (sparc_prologue_frameless_p): Implement.
        (sparc_gdbarch_init): Pass it to
        set_gdbarch_prologue_frameless_p.

Posted here:

	http://sources.redhat.com/ml/gdb-patches/2002-04/msg00727.html

I'd like to ask that you re-review this change, given this, please.

Thank you.

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

"David S. Miller" wrote:
> 
>    From: Michael Snyder <msnyder@redhat.com>
>    Date: Wed, 24 Apr 2002 16:14:56 -0700
> 
>    In this case, I don't believe you got the frameless_p case
>    right.  First, you don't seem to have changed the definition
>    of SKIP_PROLOGUE_FRAMELESS_P in tm-sparc.h.  Second, your
>    implementation of sparc_prologue_frameless_p is not
>    functionally equivalent to the old one.  If it goes thru
>    the new, symbolic path, it will not return the same result.
> 
> SKIP_PROLOGUE_FRAMELESS_P died within the past day or two from the
> whole GDB tree, perhaps you didn't notice :-)

No I didn't.  I don't read every message on this list.

> 
> I'd like to ask that you re-review this change, given this, please.

The part that's mentioned in the changelog entry is OK, except
you can remove sparc_skip_prologue_frameless_p (and say so in
the changelog) rather than rewrite it.

The parts that are NOT mentioned in the changelog (your changes
to the logic in sparc_init_extra_frame_info) are not OK, first
because you didn't mention them in the changelog, and second 
because I can't convince myself that they are right.

I'd like you to resubmit this patch with those modifications, please.

Michael

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Wed, 24 Apr 2002 17:36:02 -0700

   I'd like you to resubmit this patch with those modifications, please.

Ok, fair enough.

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Wed, 24 Apr 2002 17:36:02 -0700

   "David S. Miller" wrote:
   > I'd like to ask that you re-review this change, given this, please.
   
   The part that's mentioned in the changelog entry is OK, except
   you can remove sparc_skip_prologue_frameless_p (and say so in
   the changelog) rather than rewrite it.
   
Wait a second, how can I remove sparc_skip_prologue_frameless_p
when it isn't even there anymore? :-)

What exists is "sparc_prologue_frameless_p" which is a boolean.
That is what replaces the functionality for what I removed
(sparc_skip_prologue_frameless_p, SKIP_PROLOGUE_FRAMELESS_P,
et al.).

I think you're still a little confused on the current state of
the tree.

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Wed, 24 Apr 2002 17:36:02 -0700
   
   The parts that are NOT mentioned in the changelog (your changes
   to the logic in sparc_init_extra_frame_info) are not OK, first
   because you didn't mention them in the changelog, and second 
   because I can't convince myself that they are right.

Michael, I hate keeping on about this, but I did mention
them in my changes:

        (sparc_init_extra_frame_info): Use sparc_skip_prologue to find
        prologue bounds instead of looking at line number info by hand.

I know you're going to explode at me, but you seem to make a lot of
errors reviewing my changes.  Please settle down, take your time.  At
this point I'd much rather you delay than review things haphazardly.

Thank you.

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

Ok, broken apart.  He is a smaller chunk this time.

We use line debugging information, if possible, to determine
the end of the instructions that compose the prologue of a
function on Sparc.  examine_prologue is used as a backup in
this case, if we cannot fetch the necessary debugging information.

Here is how I modified sparc_skip_prologue:

	It is only called every with it's frameless_p
	argument set to zero, so this argument is removed.

	We check for a symtab_and_line, if it does not compose
	the whole function and the start_pc given to us is
	within the range, we return the end of the symtab_and_line
	which should be the end of the prologue.

	Else we fall back to the current behavior, which is to
	interrogate the instructions by hand to determine the
	bounds of the prologue.

Also, we had two functions doing the same thing, one for multi-arch
and one for the non-multiarch case of the same exact interface.
So I killed one of them.

Finally, I call examine_prologue directly from
sparc_prologue_frameless_p() instead of going through
sparc_skip_prologue().  This way there doesn't need to be an
"if (frameless_p)" test guarding the symbol table prologue stuff.  We
can tackle trying to use symbol table information in
sparc_prologue_frameless_p() in a future patch.

2002-04-24  David S. Miller  <davem@redhat.com>

	* sparc-tdep.c (sparc_gdbarch_skip_prologue): Kill, duplicates
	sparc_skip_prologue.
	(sparc_skip_prologue): Kill frameless_p arg, and use line number
	information to find prologue when possible.
	(sparc_prologue_frameless_p): Call examine_prologue directly.
	(sparc_gdbarch_init): Update set_gdbarch_skip_prologue call.
	* config/sparc/tm-sparc.h (sparc_skip_prologue): Update for killed
	second argument.
	(SKIP_PROLOGUE): Likewise.

--- ./config/sparc/tm-sparc.h.~1~	Sun Apr 21 19:18:59 2002
+++ ./config/sparc/tm-sparc.h	Wed Apr 24 17:54:53 2002
@@ -250,8 +250,8 @@ extern int sparc_intreg_size (void);
 /* Advance PC across any function entry prologue instructions
    to reach some "real" code.  */
 
-extern CORE_ADDR sparc_skip_prologue (CORE_ADDR, int);
-#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC, 0)
+extern CORE_ADDR sparc_skip_prologue (CORE_ADDR);
+#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC)
 
 /* Immediately after a function call, return the saved pc.
    Can't go through the frames for this because on some machines
--- ./sparc-tdep.c.~1~	Wed Apr 24 17:52:27 2002
+++ ./sparc-tdep.c	Wed Apr 24 19:04:51 2002
@@ -685,10 +685,28 @@ examine_prologue (CORE_ADDR start_pc, in
   return pc;
 }
 
+/* Advance PC across any function entry prologue instructions to reach
+   some "real" code.  */
+
 CORE_ADDR
-sparc_skip_prologue (CORE_ADDR start_pc, int frameless_p)
+sparc_skip_prologue (CORE_ADDR start_pc)
 {
-  return examine_prologue (start_pc, frameless_p, NULL, NULL);
+  struct symtab_and_line sal;
+  CORE_ADDR func_start, func_end;
+
+  /* This is the preferred method, find the end of the prologue by
+     using the debugging information.  */
+  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
+    {
+      sal = find_pc_line (func_start, 0);
+
+      if (sal.end < func_end
+	  && start_pc <= sal.end)
+	return sal.end;
+    }
+
+  /* Oh well, examine the code by hand.  */
+  return examine_prologue (start_pc, 0, NULL, NULL);
 }
 
 /* Is the prologue at IP frameless?  */
@@ -696,7 +714,7 @@ sparc_skip_prologue (CORE_ADDR start_pc,
 int
 sparc_prologue_frameless_p (CORE_ADDR ip)
 {
-  return ip == sparc_skip_prologue (ip, 1);
+  return ip == examine_prologue (ip, 1, NULL, NULL);
 }
 
 /* Check instruction at ADDR to see if it is a branch.
@@ -2784,15 +2802,6 @@ sparc64_register_byte (int regno)
     return 64 * 8 + (regno - 80) * 8;
 }
 
-/* Advance PC across any function entry prologue instructions to reach
-   some "real" code.  */
-
-static CORE_ADDR
-sparc_gdbarch_skip_prologue (CORE_ADDR ip)
-{
-  return examine_prologue (ip, 0, NULL, NULL);
-}
-
 /* Immediately after a function call, return the saved pc.
    Can't go through the frames for this because on some machines
    the new frame is not set up until the new function executes
@@ -2993,7 +3002,7 @@ sparc_gdbarch_init (struct gdbarch_info 
   set_gdbarch_saved_pc_after_call (gdbarch, sparc_saved_pc_after_call);
   set_gdbarch_prologue_frameless_p (gdbarch, sparc_prologue_frameless_p);
   set_gdbarch_short_bit (gdbarch, 2 * TARGET_CHAR_BIT);
-  set_gdbarch_skip_prologue (gdbarch, sparc_gdbarch_skip_prologue);
+  set_gdbarch_skip_prologue (gdbarch, sparc_skip_prologue);
   set_gdbarch_sp_regnum (gdbarch, SPARC_SP_REGNUM);
   set_gdbarch_use_generic_dummy_frames (gdbarch, 0);
   set_gdbarch_write_pc (gdbarch, generic_target_write_pc);

Re: [RFA] Improve Sparc epilogue analysis

[Eli Zaretskii]

On Wed, 24 Apr 2002, David S. Miller wrote:

> you seem to make a lot of
> errors reviewing my changes.  Please settle down, take your time.  At
> this point I'd much rather you delay than review things haphazardly.

Please try to refrain from such remarks.  Please trust the maintainers 
that they do their best when they review patches.  Errors do happen, so 
please by all means point them out, but please do so in a polite manner, 
and please assume that they are accidental, and that the maintainer did 
try to do his/her best job.

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

Eli Zaretskii wrote:
> 
> On Wed, 24 Apr 2002, David S. Miller wrote:
> 
> > you seem to make a lot of
> > errors reviewing my changes.  Please settle down, take your time.  At
> > this point I'd much rather you delay than review things haphazardly.
> 
> Please try to refrain from such remarks.  Please trust the maintainers
> that they do their best when they review patches.  Errors do happen, so
> please by all means point them out, but please do so in a polite manner,
> and please assume that they are accidental, and that the maintainer did
> try to do his/her best job.

Moreover, don't assume they are errors just because you don't understand
them.
We will speak more of this.

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

"David S. Miller" wrote:
> 
>    From: Michael Snyder <msnyder@redhat.com>
>    Date: Wed, 24 Apr 2002 17:36:02 -0700
> 
>    "David S. Miller" wrote:
>    > I'd like to ask that you re-review this change, given this, please.
> 
>    The part that's mentioned in the changelog entry is OK, except
>    you can remove sparc_skip_prologue_frameless_p (and say so in
>    the changelog) rather than rewrite it.
> 
> Wait a second, how can I remove sparc_skip_prologue_frameless_p
> when it isn't even there anymore? :-)
> 
> What exists is "sparc_prologue_frameless_p" which is a boolean.
> That is what replaces the functionality for what I removed
> (sparc_skip_prologue_frameless_p, SKIP_PROLOGUE_FRAMELESS_P,
> et al.).
> 
> I think you're still a little confused on the current state of
> the tree.

That comment was unnecessary, thank you.

Whether the function is in or out, it is wrong as written, 
or at the least you have to justify why you have changed
the behavior.

The existing function returns TRUE if and only if the input
address is equal to the address returned by examine_prologue, 
which in this case will be the address of one of a small set
of specific instructions (eg. a SAVE or an add sp).

Your function, in the presence of symbols, will return TRUE
if and only if the input address is equal to the last instruction
in the line (presumably of the prologue).  This is a change.
I don't know whether it was intentional or not, but you can't
make it without explaining it.

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

"David S. Miller" wrote:
> 
>    From: Michael Snyder <msnyder@redhat.com>
>    Date: Wed, 24 Apr 2002 17:36:02 -0700
> 
>    The parts that are NOT mentioned in the changelog (your changes
>    to the logic in sparc_init_extra_frame_info) are not OK, first
>    because you didn't mention them in the changelog, and second
>    because I can't convince myself that they are right.
> 
> Michael, I hate keeping on about this, but I did mention
> them in my changes:
> 
>         (sparc_init_extra_frame_info): Use sparc_skip_prologue to find
>         prologue bounds instead of looking at line number info by hand.

That entry says nothing about changing the logic or the functional
behavior.

> I know you're going to explode at me, 

Why would you say that?  Have I exploded at you up till now?

> but you seem to make a lot of errors reviewing my changes. 

If so, I would like to know about it.  Please detail 
one of my errors for the group.

> Please settle down, take your time. 

I'm going to remind you of this comment the next time you
rail about my not reviewing one of your changes on a Saturday.

> At
> this point I'd much rather you delay than review things haphazardly.

And I'd much rather you consider my reviews seriously, rather
than assume they are haphazard or erroneous.  I have been doing
this for a while, you know...

Your change to sparc_init_extra_frame_info changes its logic
and its black-box behavior.  You have said nothing about why
you made this change.  It's not my job to guess why you did it, 
it's my job to reject your change until you explain it.

It's also not my job to take shit from you.  Change your attitude, 
David.  Change it now.

Re: [RFA] Improve Sparc epilogue analysis

[Michael Snyder]

"David S. Miller" wrote:
> 
> Ok, broken apart.  He is a smaller chunk this time.
> 
> We use line debugging information, if possible, to determine
> the end of the instructions that compose the prologue of a
> function on Sparc.  examine_prologue is used as a backup in
> this case, if we cannot fetch the necessary debugging information.
> 
> Here is how I modified sparc_skip_prologue:
> 
>         It is only called every with it's frameless_p
>         argument set to zero, so this argument is removed.
> 
>         We check for a symtab_and_line, if it does not compose
>         the whole function and the start_pc given to us is
>         within the range, we return the end of the symtab_and_line
>         which should be the end of the prologue.
> 
>         Else we fall back to the current behavior, which is to
>         interrogate the instructions by hand to determine the
>         bounds of the prologue.
> 
> Also, we had two functions doing the same thing, one for multi-arch
> and one for the non-multiarch case of the same exact interface.
> So I killed one of them.
> 
> Finally, I call examine_prologue directly from
> sparc_prologue_frameless_p() instead of going through
> sparc_skip_prologue().  This way there doesn't need to be an
> "if (frameless_p)" test guarding the symbol table prologue stuff.  We
> can tackle trying to use symbol table information in
> sparc_prologue_frameless_p() in a future patch.

This patch is approved.  Thank you.
 
> 2002-04-24  David S. Miller  <davem@redhat.com>
> 
>         * sparc-tdep.c (sparc_gdbarch_skip_prologue): Kill, duplicates
>         sparc_skip_prologue.
>         (sparc_skip_prologue): Kill frameless_p arg, and use line number
>         information to find prologue when possible.
>         (sparc_prologue_frameless_p): Call examine_prologue directly.
>         (sparc_gdbarch_init): Update set_gdbarch_skip_prologue call.
>         * config/sparc/tm-sparc.h (sparc_skip_prologue): Update for killed
>         second argument.
>         (SKIP_PROLOGUE): Likewise.
> 
> --- ./config/sparc/tm-sparc.h.~1~       Sun Apr 21 19:18:59 2002
> +++ ./config/sparc/tm-sparc.h   Wed Apr 24 17:54:53 2002
> @@ -250,8 +250,8 @@ extern int sparc_intreg_size (void);
>  /* Advance PC across any function entry prologue instructions
>     to reach some "real" code.  */
> 
> -extern CORE_ADDR sparc_skip_prologue (CORE_ADDR, int);
> -#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC, 0)
> +extern CORE_ADDR sparc_skip_prologue (CORE_ADDR);
> +#define SKIP_PROLOGUE(PC) sparc_skip_prologue (PC)
> 
>  /* Immediately after a function call, return the saved pc.
>     Can't go through the frames for this because on some machines
> --- ./sparc-tdep.c.~1~  Wed Apr 24 17:52:27 2002
> +++ ./sparc-tdep.c      Wed Apr 24 19:04:51 2002
> @@ -685,10 +685,28 @@ examine_prologue (CORE_ADDR start_pc, in
>    return pc;
>  }
> 
> +/* Advance PC across any function entry prologue instructions to reach
> +   some "real" code.  */
> +
>  CORE_ADDR
> -sparc_skip_prologue (CORE_ADDR start_pc, int frameless_p)
> +sparc_skip_prologue (CORE_ADDR start_pc)
>  {
> -  return examine_prologue (start_pc, frameless_p, NULL, NULL);
> +  struct symtab_and_line sal;
> +  CORE_ADDR func_start, func_end;
> +
> +  /* This is the preferred method, find the end of the prologue by
> +     using the debugging information.  */
> +  if (find_pc_partial_function (start_pc, NULL, &func_start, &func_end))
> +    {
> +      sal = find_pc_line (func_start, 0);
> +
> +      if (sal.end < func_end
> +         && start_pc <= sal.end)
> +       return sal.end;
> +    }
> +
> +  /* Oh well, examine the code by hand.  */
> +  return examine_prologue (start_pc, 0, NULL, NULL);
>  }
> 
>  /* Is the prologue at IP frameless?  */
> @@ -696,7 +714,7 @@ sparc_skip_prologue (CORE_ADDR start_pc,
>  int
>  sparc_prologue_frameless_p (CORE_ADDR ip)
>  {
> -  return ip == sparc_skip_prologue (ip, 1);
> +  return ip == examine_prologue (ip, 1, NULL, NULL);
>  }
> 
>  /* Check instruction at ADDR to see if it is a branch.
> @@ -2784,15 +2802,6 @@ sparc64_register_byte (int regno)
>      return 64 * 8 + (regno - 80) * 8;
>  }
> 
> -/* Advance PC across any function entry prologue instructions to reach
> -   some "real" code.  */
> -
> -static CORE_ADDR
> -sparc_gdbarch_skip_prologue (CORE_ADDR ip)
> -{
> -  return examine_prologue (ip, 0, NULL, NULL);
> -}
> -
>  /* Immediately after a function call, return the saved pc.
>     Can't go through the frames for this because on some machines
>     the new frame is not set up until the new function executes
> @@ -2993,7 +3002,7 @@ sparc_gdbarch_init (struct gdbarch_info
>    set_gdbarch_saved_pc_after_call (gdbarch, sparc_saved_pc_after_call);
>    set_gdbarch_prologue_frameless_p (gdbarch, sparc_prologue_frameless_p);
>    set_gdbarch_short_bit (gdbarch, 2 * TARGET_CHAR_BIT);
> -  set_gdbarch_skip_prologue (gdbarch, sparc_gdbarch_skip_prologue);
> +  set_gdbarch_skip_prologue (gdbarch, sparc_skip_prologue);
>    set_gdbarch_sp_regnum (gdbarch, SPARC_SP_REGNUM);
>    set_gdbarch_use_generic_dummy_frames (gdbarch, 0);
>    set_gdbarch_write_pc (gdbarch, generic_target_write_pc);

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Thu, 25 Apr 2002 13:59:00 -0700

   "David S. Miller" wrote:
   > 
   >    From: Michael Snyder <msnyder@redhat.com>
   >    Date: Wed, 24 Apr 2002 17:36:02 -0700
   > 
   >    "David S. Miller" wrote:
   >    > I'd like to ask that you re-review this change, given this, please.
   > 
   >    The part that's mentioned in the changelog entry is OK, except
   >    you can remove sparc_skip_prologue_frameless_p (and say so in
   >    the changelog) rather than rewrite it.
   > 
   > Wait a second, how can I remove sparc_skip_prologue_frameless_p
   > when it isn't even there anymore? :-)
   > 
   > What exists is "sparc_prologue_frameless_p" which is a boolean.
   > That is what replaces the functionality for what I removed
   > (sparc_skip_prologue_frameless_p, SKIP_PROLOGUE_FRAMELESS_P,
   > et al.).
   > 
   > I think you're still a little confused on the current state of
   > the tree.
   
   That comment was unnecessary, thank you.
   
I think it was more than appropriate.

   Whether the function is in or out, it is wrong as written, 
   or at the least you have to justify why you have changed
   the behavior.
   
If it does not exist in the tree, it by definition can't be in any
other state than "not there".

This part of the thread centers around you telling me to remove
"sparc_skip_prologue_frameless_p".  And I'm trying to say "it isn't
there to begin with, what function are you even talking about?"

   The existing function returns TRUE if and only if the input
   address is equal to the address returned by examine_prologue, 
   which in this case will be the address of one of a small set
   of specific instructions (eg. a SAVE or an add sp).
   
   Your function, in the presence of symbols, will return TRUE
   if and only if the input address is equal to the last instruction
   in the line (presumably of the prologue).  This is a change.
   I don't know whether it was intentional or not, but you can't
   make it without explaining it.
   
Here you are making a point about my changes to
sparc_prologue_frameless_p, and those are quite valid.
And I have dealt with this in the followup patch I sent
last night.

But this has nothing to do with my what I am replying to,
which is you telling me to remove a differently named
function which no longer exists in the tree.

Re: [RFA] Improve Sparc epilogue analysis

[David S. Miller]

   From: Michael Snyder <msnyder@redhat.com>
   Date: Thu, 25 Apr 2002 14:04:21 -0700

   > I know you're going to explode at me, 
   
   Why would you say that?  Have I exploded at you up till now?
   
Yes you did.  You did it when I commented on the errors in your
reviewing of patches last time.  You said that you wouldn't have
reviewed any of my patches that day had you read the email in
question before doing so.  That, to me, is "blowing up".

   > but you seem to make a lot of errors reviewing my changes. 
   
   If so, I would like to know about it.  Please detail 
   one of my errors for the group.
   
The first patch of mine you reviewed you asked "is this a sparc
specific issue", when if you had really read the patch in it's
entirety, for the change in question, I added a long winded comment
that said that this was a sparc specific issue.

That was an error in patch review, and it showed to me that you
had only skimmed over my patch quickly instead of really having
a good look at it.

   Your change to sparc_init_extra_frame_info changes its logic
   and its black-box behavior.  You have said nothing about why
   you made this change.  It's not my job to guess why you did it, 
   it's my job to reject your change until you explain it.
   
You said I did not make any reference at all to the change
in my changelog entries, that is how you phrased it and that
is what I took issue with.

If you had told me to discuss the change "in more detail" that
would have been different.

   It's also not my job to take shit from you.
   
And it's not my job to take it from you either.