This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
small get_prev_frame cleanup (don't allow a NULL this_frame anymore), plus caller adjustment
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Date: Mon, 26 Jan 2009 00:26:58 +0000
- Subject: 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)