This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[patch/rfc] frame_id_unwind


Hello,

"infrun.c" contains the call:

get_frame_id (get_prev_frame (get_current_frame ()))

(and from memory others are pending). It is used when trying to catch an inferior returning from a function. A breakpoint is set at "current frame"'s return address, and the breakpoint's frame ID is set to the previous frame's ID (so that recursive calls are correctly handled).

Thing is, this apparently simple code has two nasty edge cases:

- get_prev_frame returns NULL for the outer most frame
That's ok though, get_frame_id turns a NULL frame into a null_frame_id.

- get_prev_frame might return NULL if "current frame" is "main"
Now that's a problem. get_prev_frame returns NULL not just when the outermost frame is reached, but also for a somewhat arbitrary set of conditions user visible conditions such as the current frame being "main". The breakpoint ents up incorrectly having a null_frame_id.


The attached introduces a new method, frame_unwind_id, which will return the frame ID for cases where the above would have returned a NULL frame (it skips some of the tests).

Now the challenge :-)

Can anyone think of a testcase?

I need to play a bit, but I'm not sure that I can come up with something.

Andrew
2004-03-25  Andrew Cagney  <cagney@redhat.com>

	* frame.h (frame_unwind_id): Declare.
	* frame.c (frame_unwind_id): New function.
	(get_prev_frame_1): New function.
	(frame_debug_got_null_frame): New function.
	(get_prev_frame): Use frame_debug_got_null_frame.  Move unwind
	code proper to prev_frame, update description.
	* infrun.c (step_over_function): Use frame_unwind_id.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.166
diff -u -r1.166 frame.c
--- frame.c	23 Mar 2004 14:47:55 -0000	1.166
+++ frame.c	25 Mar 2004 13:53:56 -0000
@@ -40,6 +40,8 @@
 #include "command.h"
 #include "gdbcmd.h"
 
+static struct frame_info *get_prev_frame_1 (struct frame_info *this_frame);
+
 /* We keep a cache of stack frames, each of which is a "struct
    frame_info".  The innermost one gets allocated (in
    wait_for_inferior) each time the inferior stops; current_frame
@@ -250,6 +252,16 @@
   return fi->this_id.value;
 }
 
+struct frame_id
+frame_unwind_id (struct frame_info *next_frame)
+{
+  /* Use prev_frame, and not get_prev_frame.  The latter will truncate
+     the frame chain, leading to this function unintentionally
+     returning a null_frame_id (e.g., when a caller requests the frame
+     ID of "main()"s caller.  */
+  return get_frame_id (get_prev_frame_1 (next_frame));
+}
+
 const struct frame_id null_frame_id; /* All zeros.  */
 
 struct frame_id
@@ -1720,23 +1732,22 @@
   return prev;
 }
 
-/* Return a structure containing various interesting information
-   about the frame that called THIS_FRAME.  Returns NULL
-   if there is no such frame.
-
-   This function tests some target-independent conditions that should
-   terminate the frame chain, such as unwinding past main().  It
-   should not contain any target-dependent tests, such as checking
-   whether the program-counter is zero.  */
+/* Return a "struct frame_info" corresponding to the frame that called
+   THIS_FRAME.  Returns NULL if there is no such frame.
 
-struct frame_info *
-get_prev_frame (struct frame_info *this_frame)
+   Unlike get_prev_frame, this function always tries to unwind the
+   frame.  */
+
+static struct frame_info *
+get_prev_frame_1 (struct frame_info *this_frame)
 {
   struct frame_info *prev_frame;
 
+  gdb_assert (this_frame != NULL);
+
   if (frame_debug)
     {
-      fprintf_unfiltered (gdb_stdlog, "{ get_prev_frame (this_frame=");
+      fprintf_unfiltered (gdb_stdlog, "{ get_prev_frame_1 (this_frame=");
       if (this_frame != NULL)
 	fprintf_unfiltered (gdb_stdlog, "%d", this_frame->level);
       else
@@ -1744,6 +1755,136 @@
       fprintf_unfiltered (gdb_stdlog, ") ");
     }
 
+  /* Only try to do the unwind once.  */
+  if (this_frame->prev_p)
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, this_frame->prev);
+	  fprintf_unfiltered (gdb_stdlog, " // cached \n");
+	}
+      return this_frame->prev;
+    }
+  this_frame->prev_p = 1;
+
+  /* If any of the old frame initialization methods are around, use
+     the legacy get_prev_frame method.  */
+  if (legacy_frame_p (current_gdbarch))
+    {
+      prev_frame = legacy_get_prev_frame (this_frame);
+      return prev_frame;
+    }
+
+  /* Check that this frame's ID was valid.  If it wasn't, don't try to
+     unwind to the prev frame.  Be careful to not apply this test to
+     the sentinel frame.  */
+  if (this_frame->level >= 0 && !frame_id_p (get_frame_id (this_frame)))
+    {
+      if (frame_debug)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "-> ");
+	  fprint_frame (gdb_stdlog, NULL);
+	  fprintf_unfiltered (gdb_stdlog, " // this ID is NULL }\n");
+	}
+      return NULL;
+    }
+
+  /* Check that this frame's ID isn't inner to (younger, below, next)
+     the next frame.  This happens when a frame unwind goes backwards.
+     Since the sentinel frame doesn't really exist, don't compare the
+     inner-most against that sentinel.  */
+  if (this_frame->level > 0
+      && frame_id_inner (get_frame_id (this_frame),
+			 get_frame_id (this_frame->next)))
+    error ("Previous frame inner to this frame (corrupt stack?)");
+
+  /* Check that this and the next frame are not identical.  If they
+     are, there is most likely a stack cycle.  As with the inner-than
+     test above, avoid comparing the inner-most and sentinel frames.  */
+  if (this_frame->level > 0
+      && frame_id_eq (get_frame_id (this_frame),
+		      get_frame_id (this_frame->next)))
+    error ("Previous frame identical to this frame (corrupt stack?)");
+
+  /* Allocate the new frame but do not wire it in to the frame chain.
+     Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along
+     frame->next to pull some fancy tricks (of course such code is, by
+     definition, recursive).  Try to prevent it.
+
+     There is no reason to worry about memory leaks, should the
+     remainder of the function fail.  The allocated memory will be
+     quickly reclaimed when the frame cache is flushed, and the `we've
+     been here before' check above will stop repeated memory
+     allocation calls.  */
+  prev_frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
+  prev_frame->level = this_frame->level + 1;
+
+  /* Don't yet compute ->unwind (and hence ->type).  It is computed
+     on-demand in get_frame_type, frame_register_unwind, and
+     get_frame_id.  */
+
+  /* Don't yet compute the frame's ID.  It is computed on-demand by
+     get_frame_id().  */
+
+  /* The unwound frame ID is validate at the start of this function,
+     as part of the logic to decide if that frame should be further
+     unwound, and not here while the prev frame is being created.
+     Doing this makes it possible for the user to examine a frame that
+     has an invalid frame ID.
+
+     Some very old VAX code noted: [...]  For the sake of argument,
+     suppose that the stack is somewhat trashed (which is one reason
+     that "info frame" exists).  So, return 0 (indicating we don't
+     know the address of the arglist) if we don't know what frame this
+     frame calls.  */
+
+  /* Link it in.  */
+  this_frame->prev = prev_frame;
+  prev_frame->next = this_frame;
+
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "-> ");
+      fprint_frame (gdb_stdlog, prev_frame);
+      fprintf_unfiltered (gdb_stdlog, " }\n");
+    }
+
+  return prev_frame;
+}
+
+/* Debug routine to print a NULL frame being returned.  */
+
+static void
+frame_debug_got_null_frame (struct ui_file *file,
+			    struct frame_info *this_frame,
+			    const char *reason)
+{
+  if (frame_debug)
+    {
+      fprintf_unfiltered (gdb_stdlog, "{ get_prev_frame (this_frame=");
+      if (this_frame != NULL)
+	fprintf_unfiltered (gdb_stdlog, "%d", this_frame->level);
+      else
+	fprintf_unfiltered (gdb_stdlog, "<NULL>");
+      fprintf_unfiltered (gdb_stdlog, ") -> // %s}\n", reason);
+    }
+}
+
+/* Return a structure containing various interesting information about
+   the frame that called THIS_FRAME.  Returns NULL if there is entier
+   no such frame or the frame fails any of a set of target-independent
+   condition that should terminate the frame chain (e.g., as unwinding
+   past main()).
+
+   This function should not contain target-dependent tests, such as
+   checking whether the program-counter is zero.  */
+
+struct frame_info *
+get_prev_frame (struct frame_info *this_frame)
+{
+  struct frame_info *prev_frame;
+
   /* Return the inner-most frame, when the caller passes in NULL.  */
   /* NOTE: cagney/2002-11-09: Not sure how this would happen.  The
      caller should have previously obtained a valid frame using
@@ -1776,6 +1917,7 @@
 
          Per the above, this code shouldn't even be called with a NULL
          THIS_FRAME.  */
+      frame_debug_got_null_frame (gdb_stdlog, this_frame, "this_frame NULL");
       return current_frame;
     }
 
@@ -1796,8 +1938,7 @@
        previously unwound.  That way if the user later decides to
        allow unwinds past main(), that just happens.  */
     {
-      if (frame_debug)
-	fprintf_unfiltered (gdb_stdlog, "-> NULL // inside main func }\n");
+      frame_debug_got_null_frame (gdb_stdlog, this_frame, "inside main func");
       return NULL;
     }
 
@@ -1836,28 +1977,10 @@
       && this_frame->type != DUMMY_FRAME && this_frame->level >= 0
       && inside_entry_func (this_frame))
     {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, "// inside entry func }\n");
-	}
+      frame_debug_got_null_frame (gdb_stdlog, this_frame, "inside entry func");
       return NULL;
     }
 
-  /* Only try to do the unwind once.  */
-  if (this_frame->prev_p)
-    {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, this_frame->prev);
-	  fprintf_unfiltered (gdb_stdlog, " // cached \n");
-	}
-      return this_frame->prev;
-    }
-  this_frame->prev_p = 1;
-
   /* If we're inside the entry file, it isn't valid.  Don't apply this
      test to a dummy frame - dummy frame PC's typically land in the
      entry file.  Don't apply this test to the sentinel frame.
@@ -1883,98 +2006,11 @@
       && this_frame->type != DUMMY_FRAME && this_frame->level >= 0
       && deprecated_inside_entry_file (get_frame_pc (this_frame)))
     {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, " // inside entry file }\n");
-	}
-      return NULL;
-    }
-
-  /* If any of the old frame initialization methods are around, use
-     the legacy get_prev_frame method.  */
-  if (legacy_frame_p (current_gdbarch))
-    {
-      prev_frame = legacy_get_prev_frame (this_frame);
-      return prev_frame;
-    }
-
-  /* Check that this frame's ID was valid.  If it wasn't, don't try to
-     unwind to the prev frame.  Be careful to not apply this test to
-     the sentinel frame.  */
-  if (this_frame->level >= 0 && !frame_id_p (get_frame_id (this_frame)))
-    {
-      if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame (gdb_stdlog, NULL);
-	  fprintf_unfiltered (gdb_stdlog, " // this ID is NULL }\n");
-	}
+      frame_debug_got_null_frame (gdb_stdlog, this_frame, "inside entry file");
       return NULL;
     }
 
-  /* Check that this frame's ID isn't inner to (younger, below, next)
-     the next frame.  This happens when a frame unwind goes backwards.
-     Since the sentinel frame doesn't really exist, don't compare the
-     inner-most against that sentinel.  */
-  if (this_frame->level > 0
-      && frame_id_inner (get_frame_id (this_frame),
-			 get_frame_id (this_frame->next)))
-    error ("Previous frame inner to this frame (corrupt stack?)");
-
-  /* Check that this and the next frame are not identical.  If they
-     are, there is most likely a stack cycle.  As with the inner-than
-     test above, avoid comparing the inner-most and sentinel frames.  */
-  if (this_frame->level > 0
-      && frame_id_eq (get_frame_id (this_frame),
-		      get_frame_id (this_frame->next)))
-    error ("Previous frame identical to this frame (corrupt stack?)");
-
-  /* Allocate the new frame but do not wire it in to the frame chain.
-     Some (bad) code in INIT_FRAME_EXTRA_INFO tries to look along
-     frame->next to pull some fancy tricks (of course such code is, by
-     definition, recursive).  Try to prevent it.
-
-     There is no reason to worry about memory leaks, should the
-     remainder of the function fail.  The allocated memory will be
-     quickly reclaimed when the frame cache is flushed, and the `we've
-     been here before' check above will stop repeated memory
-     allocation calls.  */
-  prev_frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
-  prev_frame->level = this_frame->level + 1;
-
-  /* Don't yet compute ->unwind (and hence ->type).  It is computed
-     on-demand in get_frame_type, frame_register_unwind, and
-     get_frame_id.  */
-
-  /* Don't yet compute the frame's ID.  It is computed on-demand by
-     get_frame_id().  */
-
-  /* The unwound frame ID is validate at the start of this function,
-     as part of the logic to decide if that frame should be further
-     unwound, and not here while the prev frame is being created.
-     Doing this makes it possible for the user to examine a frame that
-     has an invalid frame ID.
-
-     Some very old VAX code noted: [...]  For the sake of argument,
-     suppose that the stack is somewhat trashed (which is one reason
-     that "info frame" exists).  So, return 0 (indicating we don't
-     know the address of the arglist) if we don't know what frame this
-     frame calls.  */
-
-  /* Link it in.  */
-  this_frame->prev = prev_frame;
-  prev_frame->next = this_frame;
-
-  if (frame_debug)
-    {
-      fprintf_unfiltered (gdb_stdlog, "-> ");
-      fprint_frame (gdb_stdlog, prev_frame);
-      fprintf_unfiltered (gdb_stdlog, " }\n");
-    }
-
-  return prev_frame;
+  return get_prev_frame_1 (this_frame);
 }
 
 CORE_ADDR
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.122
diff -u -r1.122 frame.h
--- frame.h	23 Mar 2004 14:47:55 -0000	1.122
+++ frame.h	25 Mar 2004 13:53:56 -0000
@@ -312,6 +312,7 @@
    frame after a frame cache flush (and other similar operations).  If
    FI is NULL, return the null_frame_id.  */
 extern struct frame_id get_frame_id (struct frame_info *fi);
+extern struct frame_id frame_unwind_id (struct frame_info *next_frame);
 
 /* Assuming that a frame is `normal', return its base-address, or 0 if
    the information isn't available.  NOTE: This address is really only
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.142
diff -u -r1.142 infrun.c
--- infrun.c	23 Mar 2004 14:47:56 -0000	1.142
+++ infrun.c	25 Mar 2004 13:53:59 -0000
@@ -2959,7 +2959,7 @@
 	sr_id = get_frame_id (get_current_frame ());
     }
   else
-    sr_id = get_frame_id (get_prev_frame (get_current_frame ()));
+    sr_id = frame_unwind_id (get_current_frame ());
 
   step_resume_breakpoint = set_momentary_breakpoint (sr_sal, sr_id, bp_step_resume);
 
Index: rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.188
diff -u -r1.188 rs6000-tdep.c
--- rs6000-tdep.c	22 Mar 2004 17:07:08 -0000	1.188
+++ rs6000-tdep.c	25 Mar 2004 13:54:01 -0000
@@ -127,8 +127,6 @@
 			      CORE_ADDR safety);
 static CORE_ADDR skip_prologue (CORE_ADDR, CORE_ADDR,
                                 struct rs6000_framedata *);
-static void frame_get_saved_regs (struct frame_info * fi,
-				  struct rs6000_framedata * fdatap);
 
 /* Is REGNO an AltiVec register?  Return 1 if so, 0 otherwise.  */
 int
@@ -1372,128 +1370,6 @@
   ii = read_register (11);	/* r11 holds destination addr   */
   pc = read_memory_addr (ii, gdbarch_tdep (current_gdbarch)->wordsize); /* (r11) value */
   return pc;
-}
-
-/* If saved registers of frame FI are not known yet, read and cache them.
-   &FDATAP contains rs6000_framedata; TDATAP can be NULL,
-   in which case the framedata are read.  */
-
-static void
-frame_get_saved_regs (struct frame_info *fi, struct rs6000_framedata *fdatap)
-{
-  CORE_ADDR frame_addr;
-  struct rs6000_framedata work_fdata;
-  struct gdbarch_tdep * tdep = gdbarch_tdep (current_gdbarch);
-  int wordsize = tdep->wordsize;
-
-  if (deprecated_get_frame_saved_regs (fi))
-    return;
-
-  if (fdatap == NULL)
-    {
-      fdatap = &work_fdata;
-      (void) skip_prologue (get_frame_func (fi), get_frame_pc (fi), fdatap);
-    }
-
-  frame_saved_regs_zalloc (fi);
-
-  /* If there were any saved registers, figure out parent's stack
-     pointer.  */
-  /* The following is true only if the frame doesn't have a call to
-     alloca(), FIXME.  */
-
-  if (fdatap->saved_fpr == 0
-      && fdatap->saved_gpr == 0
-      && fdatap->saved_vr == 0
-      && fdatap->saved_ev == 0
-      && fdatap->lr_offset == 0
-      && fdatap->cr_offset == 0
-      && fdatap->vr_offset == 0
-      && fdatap->ev_offset == 0)
-    frame_addr = 0;
-  else
-    /* NOTE: cagney/2002-04-14: The ->frame points to the inner-most
-       address of the current frame.  Things might be easier if the
-       ->frame pointed to the outer-most address of the frame.  In the
-       mean time, the address of the prev frame is used as the base
-       address of this frame.  */
-    frame_addr = DEPRECATED_FRAME_CHAIN (fi);
-
-  /* if != -1, fdatap->saved_fpr is the smallest number of saved_fpr.
-     All fpr's from saved_fpr to fp31 are saved.  */
-
-  if (fdatap->saved_fpr >= 0)
-    {
-      int i;
-      CORE_ADDR fpr_addr = frame_addr + fdatap->fpr_offset;
-      for (i = fdatap->saved_fpr; i < 32; i++)
-	{
-	  deprecated_get_frame_saved_regs (fi)[FP0_REGNUM + i] = fpr_addr;
-	  fpr_addr += 8;
-	}
-    }
-
-  /* if != -1, fdatap->saved_gpr is the smallest number of saved_gpr.
-     All gpr's from saved_gpr to gpr31 are saved.  */
-
-  if (fdatap->saved_gpr >= 0)
-    {
-      int i;
-      CORE_ADDR gpr_addr = frame_addr + fdatap->gpr_offset;
-      for (i = fdatap->saved_gpr; i < 32; i++)
-	{
-	  deprecated_get_frame_saved_regs (fi)[tdep->ppc_gp0_regnum + i] = gpr_addr;
-	  gpr_addr += wordsize;
-	}
-    }
-
-  /* if != -1, fdatap->saved_vr is the smallest number of saved_vr.
-     All vr's from saved_vr to vr31 are saved.  */
-  if (tdep->ppc_vr0_regnum != -1 && tdep->ppc_vrsave_regnum != -1)
-    {
-      if (fdatap->saved_vr >= 0)
-	{
-	  int i;
-	  CORE_ADDR vr_addr = frame_addr + fdatap->vr_offset;
-	  for (i = fdatap->saved_vr; i < 32; i++)
-	    {
-	      deprecated_get_frame_saved_regs (fi)[tdep->ppc_vr0_regnum + i] = vr_addr;
-	      vr_addr += DEPRECATED_REGISTER_RAW_SIZE (tdep->ppc_vr0_regnum);
-	    }
-	}
-    }
-
-  /* if != -1, fdatap->saved_ev is the smallest number of saved_ev.
-	All vr's from saved_ev to ev31 are saved. ?????	*/
-  if (tdep->ppc_ev0_regnum != -1 && tdep->ppc_ev31_regnum != -1)
-    {
-      if (fdatap->saved_ev >= 0)
-	{
-	  int i;
-	  CORE_ADDR ev_addr = frame_addr + fdatap->ev_offset;
-	  for (i = fdatap->saved_ev; i < 32; i++)
-	    {
-	      deprecated_get_frame_saved_regs (fi)[tdep->ppc_ev0_regnum + i] = ev_addr;
-              deprecated_get_frame_saved_regs (fi)[tdep->ppc_gp0_regnum + i] = ev_addr + 4;
-	      ev_addr += DEPRECATED_REGISTER_RAW_SIZE (tdep->ppc_ev0_regnum);
-            }
-	}
-    }
-
-  /* If != 0, fdatap->cr_offset is the offset from the frame that holds
-     the CR.  */
-  if (fdatap->cr_offset != 0)
-    deprecated_get_frame_saved_regs (fi)[tdep->ppc_cr_regnum] = frame_addr + fdatap->cr_offset;
-
-  /* If != 0, fdatap->lr_offset is the offset from the frame that holds
-     the LR.  */
-  if (fdatap->lr_offset != 0)
-    deprecated_get_frame_saved_regs (fi)[tdep->ppc_lr_regnum] = frame_addr + fdatap->lr_offset;
-
-  /* If != 0, fdatap->vrsave_offset is the offset from the frame that holds
-     the VRSAVE.  */
-  if (fdatap->vrsave_offset != 0)
-    deprecated_get_frame_saved_regs (fi)[tdep->ppc_vrsave_regnum] = frame_addr + fdatap->vrsave_offset;
 }
 
 /* Return the size of register REG when words are WORDSIZE bytes long.  If REG

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]