[patch/rfc] Flesh out struct frame_id

[Andrew Cagney <ac131313 at redhat dot com>]

Hello,

The attached fleshes out the `struct frame_id' object (lightweight).

It adds methods for:

- creating a frame_id from a stack/code address (hacky)
- comparing two frame_ids (equality and strictly innerthan)
- checking that a frame_id is valid

It also changes get_frame_id() to a function.  Turns out that when 
trying to use the comparison functions this is much easier (I've a few 
patches to follow this one that extend the frame_id's use).

I should note that the ia64 with its two stacks will need more work.  At 
least, though, there are methods methods that can be hooked.

I've three follow up patches to this:

- [experimentally] use the function instead of the PC address in a frame 
ID and then use that in the equality comparison (I've tried this on i386 
and it didn't show regressions -> not a good enough testcase).

- save the frame ID, instead of frame base in the `struct breakpoint' 
object; also pass in a frame ID, instead of a frame base, to the `new 
breakpoint' methods

- replace the global step_frame_address with step_frame_id

- eliminate set_current_frame() (ok for patches) this becomes possible 
with the above patches.


I'm looking to commit this in a few days,
Andrew
2002-11-30  Andrew Cagney  <ac131313@redhat.com>

	* frame.h (get_frame_id): Convert to a function.
	(null_frame_id, frame_id_p): Declare.
	(frame_id_eq, frame_id_inner): Declare.
	(frame_id_build): New function.
	* frame.c (get_frame_id): Update.  Use null_frame_id.
	(frame_find_by_id): Rewrite using frame_id_p, frame_id_eq and
	frame_id_inner.
	(null_frame_id, frame_id_p): Define.
	(frame_id_eq, frame_id_inner): Define.
	(frame_id_build): New function.
	
	* varobj.c (varobj_create): Update.
	(varobj_update): Update.
	* valops.c (value_assign): Update.
	(new_root_variable): Update.
	* infrun.c (save_inferior_status): Update.
	* breakpoint.c (watch_command_1): Update.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.98
diff -u -r1.98 breakpoint.c
--- breakpoint.c	1 Dec 2002 19:07:14 -0000	1.98
+++ breakpoint.c	1 Dec 2002 20:54:35 -0000
@@ -5400,7 +5400,7 @@
   if (frame)
     {
       prev_frame = get_prev_frame (frame);
-      get_frame_id (frame, &b->watchpoint_frame);
+      b->watchpoint_frame = get_frame_id (frame);
     }
   else
     {
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.36
diff -u -r1.36 frame.c
--- frame.c	1 Dec 2002 19:07:14 -0000	1.36
+++ frame.c	1 Dec 2002 21:03:14 -0000
@@ -35,24 +35,64 @@
 #include "annotate.h"
 #include "language.h"
 
-/* Return a frame uniq ID that can be used to, later re-find the
+/* 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)
+struct frame_id
+get_frame_id (struct frame_info *fi)
 {
   if (fi == NULL)
     {
-      id->base = 0;
-      id->pc = 0;
+      return null_frame_id;
     }
   else
     {
-      id->base = fi->frame;
-      id->pc = fi->pc;
+      struct frame_id id;
+      id.base = fi->frame;
+      id.pc = fi->pc;
+      return id;
     }
 }
 
+const struct frame_id null_frame_id; /* All zeros.  */
+
+struct frame_id
+frame_id_build (CORE_ADDR base, CORE_ADDR func_or_pc)
+{
+  struct frame_id id;
+  id.base = base;
+  id.pc = func_or_pc;
+  return id;
+}
+
+int
+frame_id_p (struct frame_id l)
+{
+  /* The .func can be NULL but the .base cannot.  */
+  return (l.base != 0);
+}
+
+int
+frame_id_eq (struct frame_id l, struct frame_id r)
+{
+  /* If .base is different, the frames are different.  */
+  if (l.base != r.base)
+    return 0;
+  /* Add a test to check that the frame ID's are for the same function
+     here.  */
+  return 1;
+}
+
+int
+frame_id_inner (struct frame_id l, struct frame_id r)
+{
+  /* Only return non-zero when strictly inner than.  Note that, per
+     comment in "frame.h", there is some fuzz here.  Frameless
+     functions are not strictly inner than (same .base but different
+     .func).  */
+  return INNER_THAN (l.base, r.base);
+}
+
 struct frame_info *
 frame_find_by_id (struct frame_id id)
 {
@@ -60,29 +100,24 @@
 
   /* 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)
+  if (!frame_id_p (id))
     return NULL;
 
   for (frame = get_current_frame ();
        frame != NULL;
        frame = get_prev_frame (frame))
     {
-      struct frame_id this;
-      get_frame_id (frame, &this);
-      if (INNER_THAN (this.base, id.base))
-	/* ``inner/current < frame < id.base''.  Keep looking along
-           the frame chain.  */
-	continue;
-      if (INNER_THAN (id.base, this.base))
-	/* ``inner/current < id.base < frame''.  Oops, gone past it.
-           Just give up.  */
+      struct frame_id this = get_frame_id (frame);
+      if (frame_id_eq (id, this))
+	/* An exact match.  */
+	return frame;
+      if (frame_id_inner (id, this))
+	/* Gone to far.  */
 	return NULL;
-      /* FIXME: cagney/2002-04-21: This isn't sufficient.  It should
-	 use id.pc / this.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;
+      /* Either, we're not yet gone far enough out along the frame
+         chain (inner(this,id), or we're comparing frameless functions
+         (same .base, different .func, no test available).  Struggle
+         on until we've definitly gone to far.  */
     }
   return NULL;
 }
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.43
diff -u -r1.43 frame.h
--- frame.h	1 Dec 2002 19:07:14 -0000	1.43
+++ frame.h	1 Dec 2002 21:04:21 -0000
@@ -31,8 +31,8 @@
 
 /* The frame object's ID.  This provides a per-frame unique identifier
    that can be used to relocate a `struct frame_info' after a target
-   resume or a frame cache destruct (assuming the target hasn't
-   unwound the stack past that frame - a problem handled elsewhere).  */
+   resume or a frame cache destruct.  It of course assumes that the
+   inferior hasn't unwound the stack past that frame.  */
 
 struct frame_id
 {
@@ -47,6 +47,38 @@
   CORE_ADDR pc;
 };
 
+/* Methods for constructing and comparing Frame IDs.
+
+   NOTE: Given frameless functions A and B, where A calls B (and hence
+   B is inner-to A).  The relationships: !eq(A,B); !eq(B,A);
+   !inner(A,B); !inner(B,A); all hold.  This is because, while B is
+   inner to A, B is not strictly inner to A (being frameless, they
+   have the same .base value).  */
+
+/* For convenience.  All fields are zero.  */
+extern const struct frame_id null_frame_id;
+
+/* Construct a frame ID.  The second parameter isn't yet well defined.
+   It might be the containing function, or the resume PC (see comment
+   above in `struct frame_id')?  A func/pc of zero indicates a
+   wildcard (i.e., do not use func in frame ID comparisons).  */
+extern struct frame_id frame_id_build (CORE_ADDR base,
+				       CORE_ADDR func_or_pc);
+
+/* Returns non-zero when L is a valid frame (a valid frame has a
+   non-zero .base).  */
+extern int frame_id_p (struct frame_id l);
+
+/* Returns non-zero when L and R identify the same frame, or, if
+   either L or R have a zero .func, then the same frame base.  */
+extern int frame_id_eq (struct frame_id l, struct frame_id r);
+
+/* Returns non-zero when L is strictly inner-than R (they have
+   different frame .bases).  Neither L, nor R can be `null'.  See note
+   above about frameless functions.  */
+extern int frame_id_inner (struct frame_id l, struct frame_id r);
+
+
 /* For every stopped thread, GDB tracks two frames: current and
    selected.  Current frame is the inner most frame of the selected
    thread.  Selected frame is the one being examined by the the GDB
@@ -176,8 +208,9 @@
 extern CORE_ADDR get_frame_base (struct frame_info *);
 
 /* Return the per-frame unique identifer.  Can be used to relocate a
-   frame after a frame cache flush (and other similar operations).  */
-extern void get_frame_id (struct frame_info *fi, struct frame_id *id);
+   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);
 
 /* The frame's level: 0 for innermost, 1 for its caller, ...; or -1
    for an invalid frame).  */
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.80
diff -u -r1.80 infrun.c
--- infrun.c	1 Dec 2002 19:07:15 -0000	1.80
+++ infrun.c	1 Dec 2002 21:25:18 -0000
@@ -3856,7 +3856,7 @@
 
   inf_status->registers = regcache_dup (current_regcache);
 
-  get_frame_id (deprecated_selected_frame, &inf_status->selected_frame_id);
+  inf_status->selected_frame_id = get_frame_id (deprecated_selected_frame);
   return inf_status;
 }
 
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.83
diff -u -r1.83 valops.c
--- valops.c	29 Nov 2002 19:15:15 -0000	1.83
+++ valops.c	1 Dec 2002 21:34:05 -0000
@@ -649,7 +649,7 @@
 	/* Since modifying a register can trash the frame chain, we
            save the old frame and then restore the new frame
            afterwards.  */
-	get_frame_id (deprecated_selected_frame, &old_frame);
+	old_frame = get_frame_id (deprecated_selected_frame);
 
 	/* Figure out which frame this is in currently.  */
 	if (VALUE_LVAL (toval) == lval_register)
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.37
diff -u -r1.37 varobj.c
--- varobj.c	29 Nov 2002 19:15:15 -0000	1.37
+++ varobj.c	1 Dec 2002 21:39:29 -0000
@@ -487,7 +487,7 @@
          Since select_frame is so benign, just call it for all cases. */
       if (fi != NULL)
 	{
-	  get_frame_id (fi, &var->root->frame);
+	  var->root->frame = get_frame_id (fi);
 	  old_fi = deprecated_selected_frame;
 	  select_frame (fi);
 	}
@@ -898,7 +898,7 @@
 
   /* Save the selected stack frame, since we will need to change it
      in order to evaluate expressions. */
-  get_frame_id (deprecated_selected_frame, &old_fid);
+  old_fid = get_frame_id (deprecated_selected_frame);
 
   /* Update the root variable. value_of_root can return NULL
      if the variable is no longer around, i.e. we stepped out of
@@ -1344,8 +1344,7 @@
   var->root->lang = NULL;
   var->root->exp = NULL;
   var->root->valid_block = NULL;
-  var->root->frame.base = 0;
-  var->root->frame.pc = 0;
+  var->root->frame = null_frame_id;
   var->root->use_selected_frame = 0;
   var->root->rootvar = NULL;