This is the mail archive of the gdb-patches@sourceware.org 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]

small get_prev_frame cleanup (don't allow a NULL this_frame anymore), plus caller adjustment


I can only find one place currently where get_prev_frame can
be passed a NULL frame, so I think now is a good time to get rid of
the hack it has in place, the one of returning current_frame in that case.

Unfortunately, that place (``find_frame_addr_in_frame_chain'', which can be
read in the comments I'm moving around in the patch, is a hack itself
that should go, but it serves an MI interface, so, we have to leave that
cleanup for MI3), really expects that get_prev_frame would return NULL if the
target had no stack yet, and a valid current_frame otherwise.

To clean that up, the best seems to be to export ``has_stack_frames'',
which is also the predicate used by deprecated_get_selected_frame and
get_selected_frame, and use it at the get_prev_frame's and
deprecated_get_seleted_frame caller (varobj_create).  (This is not to
say that checks couldn't move yet closer to the top-level, but I'm
leaving that for another time --- focusing on get_prev_frame.)

WDYT?

-- 
Pedro Alves
2009-01-26  Pedro Alves  <pedro@codesourcery.com>

	* frame.c (has_stack_frames): Make public.
	(get_prev_frame): Don't allow a NULL this_frame anymore.
	* frame.h (has_stack_frames): Declare.
	* varobj.c (find_frame_addr_in_frame_chain): Don't ever pass NULL
	to get_prev_frame, instead start at get_current_frame.
	(varobj_create): Check has_stack_frames before getting any frame;
	eliminate one usage of deprecated_safe_get_selected_frame.

---
 gdb/frame.c  |   38 +-------------------------------------
 gdb/frame.h  |    5 +++++
 gdb/varobj.c |   39 +++++++++++++++++++++++----------------
 3 files changed, 29 insertions(+), 53 deletions(-)

Index: src/gdb/frame.c
===================================================================
--- src.orig/gdb/frame.c	2009-01-26 00:25:02.000000000 +0000
+++ src/gdb/frame.c	2009-01-26 00:25:04.000000000 +0000
@@ -997,7 +997,7 @@ get_current_frame (void)
 
 static struct frame_info *selected_frame;
 
-static int
+int
 has_stack_frames (void)
 {
   if (!target_has_registers || !target_has_stack || !target_has_memory)
@@ -1458,42 +1458,6 @@ get_prev_frame (struct frame_info *this_
 {
   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
-     get_selected_frame() and then called this code - only possibility
-     I can think of is code behaving badly.
-
-     NOTE: cagney/2003-01-10: Talk about code behaving badly.  Check
-     block_innermost_frame().  It does the sequence: frame = NULL;
-     while (1) { frame = get_prev_frame (frame); .... }.  Ulgh!  Why
-     it couldn't be written better, I don't know.
-
-     NOTE: cagney/2003-01-11: I suspect what is happening in
-     block_innermost_frame() is, when the target has no state
-     (registers, memory, ...), it is still calling this function.  The
-     assumption being that this function will return NULL indicating
-     that a frame isn't possible, rather than checking that the target
-     has state and then calling get_current_frame() and
-     get_prev_frame().  This is a guess mind.  */
-  if (this_frame == NULL)
-    {
-      /* NOTE: cagney/2002-11-09: There was a code segment here that
-	 would error out when CURRENT_FRAME was NULL.  The comment
-	 that went with it made the claim ...
-
-	 ``This screws value_of_variable, which just wants a nice
-	 clean NULL return from block_innermost_frame if there are no
-	 frames.  I don't think I've ever seen this message happen
-	 otherwise.  And returning NULL here is a perfectly legitimate
-	 thing to do.''
-
-         Per the above, this code shouldn't even be called with a NULL
-         THIS_FRAME.  */
-      frame_debug_got_null_frame (this_frame, "this_frame NULL");
-      return current_frame;
-    }
-
   /* There is always a frame.  If this assertion fails, suspect that
      something should be calling get_selected_frame() or
      get_current_frame().  */
Index: src/gdb/frame.h
===================================================================
--- src.orig/gdb/frame.h	2009-01-26 00:25:02.000000000 +0000
+++ src/gdb/frame.h	2009-01-26 00:25:04.000000000 +0000
@@ -204,6 +204,11 @@ enum frame_type
    error.  */
 extern struct frame_info *get_current_frame (void);
 
+/* Does the current target interface have enough state to be able to
+   query the current inferior for frame info, and is the inferior in a
+   state where that is possible?  */
+extern int has_stack_frames (void);
+
 /* Invalidates the frame cache (this function should have been called
    invalidate_cached_frames).
 
Index: src/gdb/varobj.c
===================================================================
--- src.orig/gdb/varobj.c	2009-01-26 00:25:02.000000000 +0000
+++ src/gdb/varobj.c	2009-01-26 00:25:04.000000000 +0000
@@ -431,14 +431,15 @@ find_frame_addr_in_frame_chain (CORE_ADD
   if (frame_addr == (CORE_ADDR) 0)
     return NULL;
 
-  while (1)
+  for (frame = get_current_frame ();
+       frame != NULL;
+       frame = get_prev_frame (frame))
     {
-      frame = get_prev_frame (frame);
-      if (frame == NULL)
-	return NULL;
       if (get_frame_base_address (frame) == frame_addr)
 	return frame;
     }
+
+  return NULL;
 }
 
 struct varobj *
@@ -462,20 +463,26 @@ varobj_create (char *objname,
       struct value *value = NULL;
       int expr_len;
 
-      /* Parse and evaluate the expression, filling in as much
-         of the variable's data as possible */
+      /* Parse and evaluate the expression, filling in as much of the
+         variable's data as possible.  */
 
-      /* Allow creator to specify context of variable */
-      if ((type == USE_CURRENT_FRAME) || (type == USE_SELECTED_FRAME))
-	fi = deprecated_safe_get_selected_frame ();
+      if (has_stack_frames ())
+	{
+	  /* Allow creator to specify context of variable */
+	  if ((type == USE_CURRENT_FRAME) || (type == USE_SELECTED_FRAME))
+	    fi = get_selected_frame (NULL);
+	  else
+	    /* FIXME: cagney/2002-11-23: This code should be doing a
+	       lookup using the frame ID and not just the frame's
+	       ``address''.  This, of course, means an interface
+	       change.  However, with out that interface change ISAs,
+	       such as the ia64 with its two stacks, won't work.
+	       Similar goes for the case where there is a frameless
+	       function.  */
+	    fi = find_frame_addr_in_frame_chain (frame);
+	}
       else
-	/* FIXME: cagney/2002-11-23: This code should be doing a
-	   lookup using the frame ID and not just the frame's
-	   ``address''.  This, of course, means an interface change.
-	   However, with out that interface change ISAs, such as the
-	   ia64 with its two stacks, won't work.  Similar goes for the
-	   case where there is a frameless function.  */
-	fi = find_frame_addr_in_frame_chain (frame);
+	fi = NULL;
 
       /* frame = -2 means always use selected frame */
       if (type == USE_SELECTED_FRAME)

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