[rfc/rfa:breakpoint] Use a ``frame_id'' to track frames

[rfc/rfa:breakpoint] Use a ``frame_id'' to track frames

[Andrew Cagney]

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

Hello,

The attached patch introduces a ``struct frame_id'' for recording 
frames.  Previously (as JimI discovered) GDB was using a confused 
combination of a frame's relative level and its base/fp address.

A ``frame_id'' is a frame-{addr,base,fp} + frame-{pc,contaddr} pair. 
Two methods are provided:

	get_frame_id()
and 
frame_find_by_id()

I've also made this new function more robust then the current 
find_frame_addr...() code - it stops searching (I hope :-) as soon as it 
has gone past the relevant frame.

At present the frame_find_by_id() doesn't do a PC based test that is 
needed to tie-break frameless (?) frames (er, frames belonging to 
frameless function instances).  Can anyone come up with a testcase that 
demonstrates the need for the PC comparison test?

varobj, MI and insight should all be updated to use this more reliable 
frame handle.

I'm going to leave this on the table for a week or so.

Thoughts?  JimI, does it fix your bug?

enjoy,
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 10800 bytes --]

2002-04-21  Andrew Cagney  <ac131313@redhat.com>

	* infrun.c (struct inferior_status): Replace fields
	selected_frame_address and selected_level with field
	selected_frame_id.
	(save_inferior_status): Update.  Use get_frame_id.
	(struct restore_selected_frame_args): Delete.
	(restore_selected_frame): Update.  Use frame_find_by_id.
	(restore_inferior_status): Update.

	* breakpoint.h (struct breakpoint): Change type of
	watchpoint_frame to frame_id.
	* breakpoint.c (insert_breakpoints): Use frame_find_by_id.  Remove
	call to get_current_frame.
	(do_enable_breakpoint): Use frame_find_by_id.  Remove call to
	get_current_frame.
	(watchpoint_check): Use frame_find_by_id.

	* frame.h (record_selected_frame): Delete declaration.
	* stack.c (record_selected_frame): Delete function.
	
	* frame.h (struct frame_id): Define.
	(get_frame_id): Declare.
	(frame_find_by_id): Declare.
	* frame.c (frame_find_by_id): New function.
	(get_frame_id): New function.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.73
diff -u -r1.73 breakpoint.c
--- breakpoint.c	21 Apr 2002 20:23:32 -0000	1.73
+++ breakpoint.c	21 Apr 2002 23:11:07 -0000
@@ -909,13 +909,7 @@
 	else
 	  {
 	    struct frame_info *fi;
-
-	    /* There might be no current frame at this moment if we are
-	       resuming from a step over a breakpoint.
-	       Set up current frame before trying to find the watchpoint
-	       frame.  */
-	    get_current_frame ();
-	    fi = find_frame_addr_in_frame_chain (b->watchpoint_frame);
+	    fi = frame_find_by_id (b->watchpoint_frame);
 	    within_current_scope = (fi != NULL);
 	    if (within_current_scope)
 	      select_frame (fi, -1);
@@ -2320,7 +2314,7 @@
          any chance of handling watchpoints on local variables, we'll need
          the frame chain (so we can determine if we're in scope).  */
       reinit_frame_cache ();
-      fr = find_frame_addr_in_frame_chain (b->watchpoint_frame);
+      fr = frame_find_by_id (b->watchpoint_frame);
       within_current_scope = (fr != NULL);
       /* in_function_epilogue_p() returns a non-zero value if we're still
 	 in the function but the stack frame has already been invalidated.
@@ -5321,10 +5315,12 @@
   if (frame)
     {
       prev_frame = get_prev_frame (frame);
-      b->watchpoint_frame = frame->frame;
+      get_frame_id (frame, &b->watchpoint_frame);
     }
   else
-    b->watchpoint_frame = (CORE_ADDR) 0;
+    {
+      memset (&b->watchpoint_frame, 0, sizeof (b->watchpoint_frame));
+    }
 
   /* If the expression is "local", then set up a "watchpoint scope"
      breakpoint at the point where we've left the scope of the watchpoint
@@ -7266,12 +7262,7 @@
       if (bpt->exp_valid_block != NULL)
 	{
 	  struct frame_info *fr =
-
-	  /* Ensure that we have the current frame.  Else, this
-	     next query may pessimistically be answered as, "No,
-	     not within current scope". */
-	  get_current_frame ();
-	  fr = find_frame_addr_in_frame_chain (bpt->watchpoint_frame);
+	  fr = frame_find_by_id (bpt->watchpoint_frame);
 	  if (fr == NULL)
 	    {
 	      printf_filtered ("\
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.11
diff -u -r1.11 breakpoint.h
--- breakpoint.h	6 Feb 2002 18:31:07 -0000	1.11
+++ breakpoint.h	21 Apr 2002 23:11:10 -0000
@@ -270,10 +270,10 @@
        it the watchpoint_scope breakpoint or something like that. FIXME).  */
     struct breakpoint *related_breakpoint;
 
-    /* Holds the frame address which identifies the frame this watchpoint
-       should be evaluated in, or NULL if the watchpoint should be evaluated
-       on the outermost frame.  */
-    CORE_ADDR watchpoint_frame;
+    /* Holds the frame address which identifies the frame this
+       watchpoint should be evaluated in, or `null' if the watchpoint
+       should be evaluated on the outermost frame.  */
+    struct frame_id watchpoint_frame;
 
     /* Thread number for thread-specific breakpoint, or -1 if don't care */
     int thread;
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.9
diff -u -r1.9 frame.c
--- frame.c	21 Apr 2002 15:52:34 -0000	1.9
+++ frame.c	21 Apr 2002 23:11:10 -0000
@@ -27,6 +27,56 @@
 #include "inferior.h"	/* for inferior_ptid */
 #include "regcache.h"
 
+/* Return a frame uniq ID that can be used to, later re-find the
+   frame.  */
+
+void
+get_frame_id (struct frame_info *fi, struct frame_id *id)
+{
+  if (fi == NULL)
+    {
+      id->base = 0;
+      id->pc = 0;
+    }
+  else
+    {
+      id->base = FRAME_FP (fi);
+      id->pc = fi->pc;
+    }
+}
+
+struct frame_info *
+frame_find_by_id (struct frame_id id)
+{
+  struct frame_info *frame;
+
+  /* ZERO denotes the null frame, let the caller decide what to do
+     about it.  Should it instead return get_current_frame()?  */
+  if (id.base == 0 && id.pc == 0)
+    return NULL;
+
+  for (frame = get_current_frame ();
+       frame != NULL;
+       frame = get_prev_frame (frame))
+    {
+      if (INNER_THAN (FRAME_FP (frame), id.base))
+	/* ``inner/current < frame < id.base''.  Keep looking along
+           the frame chain.  */
+	continue;
+      if (INNER_THAN (id.base, FRAME_FP (frame)))
+	/* ``inner/current < id.base < frame''.  Oops, gone past it.
+           Just give up.  */
+	return NULL;
+      /* FIXME: cagney/2002-04-21: This isn't sufficient.  It should
+	 use id.pc to check that the two frames belong to the same
+	 function.  Otherwise we'll do things like match dummy frames
+	 or mis-match frameless functions.  However, until someone
+	 notices, stick with the existing behavour.  */
+      return frame;
+    }
+  return NULL;
+}
+
 /* FIND_SAVED_REGISTER ()
 
    Return the address in which frame FRAME's value of register REGNUM
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.13
diff -u -r1.13 frame.h
--- frame.h	21 Apr 2002 20:23:32 -0000	1.13
+++ frame.h	21 Apr 2002 23:11:10 -0000
@@ -251,7 +251,21 @@
 
 extern void select_frame (struct frame_info *, int);
 
-extern void record_selected_frame (CORE_ADDR *, int *);
+/* Return an ID that can be used to re-find a frame.  */
+
+struct frame_id
+{
+  /* The frame's address.  This should be constant through out the
+     lifetime of a frame.  */
+  CORE_ADDR base;
+  /* The frame's current PC.  While this changes, the function that
+     the PC falls into, does not.  */
+  CORE_ADDR pc;
+};
+
+extern void get_frame_id (struct frame_info *fi, struct frame_id *id);
+
+extern struct frame_info *frame_find_by_id (struct frame_id id);
 
 extern void select_and_print_frame (struct frame_info *, int);
 
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.56
diff -u -r1.56 infrun.c
--- infrun.c	18 Mar 2002 02:26:31 -0000	1.56
+++ infrun.c	21 Apr 2002 23:11:17 -0000
@@ -3910,7 +3910,6 @@
   CORE_ADDR step_resume_break_address;
   int stop_after_trap;
   int stop_soon_quietly;
-  CORE_ADDR selected_frame_address;
   char *stop_registers;
 
   /* These are here because if call_function_by_hand has written some
@@ -3918,7 +3917,9 @@
      any registers.  */
   char *registers;
 
-  int selected_level;
+  /* A frame unique identifier.  */
+  struct frame_id selected_frame_id;
+
   int breakpoint_proceeded;
   int restore_stack_info;
   int proceed_to_finish;
@@ -3987,44 +3988,27 @@
 
   read_register_bytes (0, inf_status->registers, REGISTER_BYTES);
 
-  record_selected_frame (&(inf_status->selected_frame_address),
-			 &(inf_status->selected_level));
+  get_frame_id (selected_frame, &inf_status->selected_frame_id);
   return inf_status;
 }
 
-struct restore_selected_frame_args
-{
-  CORE_ADDR frame_address;
-  int level;
-};
-
 static int
 restore_selected_frame (void *args)
 {
-  struct restore_selected_frame_args *fr =
-  (struct restore_selected_frame_args *) args;
+  struct frame_id *fid =  (struct frame_id *) args;
   struct frame_info *frame;
-  int level = fr->level;
 
-  frame = find_relative_frame (get_current_frame (), &level);
+  frame = frame_find_by_id (*fid);
 
   /* If inf_status->selected_frame_address is NULL, there was no
      previously selected frame.  */
-  if (frame == NULL ||
-  /*  FRAME_FP (frame) != fr->frame_address || */
-  /* elz: deleted this check as a quick fix to the problem that
-     for function called by hand gdb creates no internal frame
-     structure and the real stack and gdb's idea of stack are
-     different if nested calls by hands are made.
-
-     mvs: this worries me.  */
-      level != 0)
+  if (frame == NULL)
     {
       warning ("Unable to restore previously selected frame.\n");
       return 0;
     }
 
-  select_frame (frame, fr->level);
+  select_frame (frame, frame_relative_level (frame));
 
   return (1);
 }
@@ -4066,19 +4050,14 @@
 
   if (target_has_stack && inf_status->restore_stack_info)
     {
-      struct restore_selected_frame_args fr;
-      fr.level = inf_status->selected_level;
-      fr.frame_address = inf_status->selected_frame_address;
       /* The point of catch_errors is that if the stack is clobbered,
-         walking the stack might encounter a garbage pointer and error()
-         trying to dereference it.  */
-      if (catch_errors (restore_selected_frame, &fr,
+         walking the stack might encounter a garbage pointer and
+         error() trying to dereference it.  */
+      if (catch_errors (restore_selected_frame, &inf_status->selected_frame_id,
 			"Unable to restore previously selected frame:\n",
 			RETURN_MASK_ERROR) == 0)
-	/* Error in restoring the selected frame.  Select the innermost
-	   frame.  */
-
-
+	/* Error in restoring the selected frame.  Select the
+	   innermost frame.  */
 	select_frame (get_current_frame (), 0);
 
     }
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.34
diff -u -r1.34 stack.c
--- stack.c	21 Apr 2002 20:23:33 -0000	1.34
+++ stack.c	21 Apr 2002 23:11:22 -0000
@@ -1540,17 +1540,6 @@
     }
 }
 \f
-
-/* Store the selected frame and its level into *FRAMEP and *LEVELP.
-   If there is no selected frame, *FRAMEP is set to NULL.  */
-
-void
-record_selected_frame (CORE_ADDR *frameaddrp, int *levelp)
-{
-  *frameaddrp = selected_frame ? selected_frame->frame : 0;
-  *levelp = frame_relative_level (selected_frame);
-}
-
 /* Return the symbol-block in which the selected frame is executing.
    Can return zero under various legitimate circumstances.
 

Re: [rfc/rfa:breakpoint] Use a ``frame_id'' to track frames

[David S. Miller]

   From: Andrew Cagney <ac131313@cygnus.com>
   Date: Sun, 21 Apr 2002 19:30:16 -0400
   
   Can anyone come up with a testcase that 
   demonstrates the need for the PC comparison test?

There are various forms of trampolines (such as calling via PLT) that
would require such a check, but those need to be handled by the
target in other ways (e.g. with SOLIB_SKIP_TRAMPOLINE and friends).

Re: [rfc/rfa:breakpoint] Use a ``frame_id'' to track frames

[Andrew Cagney]

> 2002-04-21  Andrew Cagney  <ac131313@redhat.com>
> 
> 	* infrun.c (struct inferior_status): Replace fields
> 	selected_frame_address and selected_level with field
> 	selected_frame_id.
> 	(save_inferior_status): Update.  Use get_frame_id.
> 	(struct restore_selected_frame_args): Delete.
> 	(restore_selected_frame): Update.  Use frame_find_by_id.
> 	(restore_inferior_status): Update.
> 
> 	* breakpoint.h (struct breakpoint): Change type of
> 	watchpoint_frame to frame_id.
> 	* breakpoint.c (insert_breakpoints): Use frame_find_by_id.  Remove
> 	call to get_current_frame.
> 	(do_enable_breakpoint): Use frame_find_by_id.  Remove call to
> 	get_current_frame.
> 	(watchpoint_check): Use frame_find_by_id.
> 
> 	* frame.h (record_selected_frame): Delete declaration.
> 	* stack.c (record_selected_frame): Delete function.
> 	
> 	* frame.h (struct frame_id): Define.
> 	(get_frame_id): Declare.
> 	(frame_find_by_id): Declare.
> 	* frame.c (frame_find_by_id): New function.
> 	(get_frame_id): New function.
> 

(private ok from michaels) I've checked this in.

Andrew