RFC: selected frame in read_var_value

RFC: selected frame in read_var_value

[Daniel Jacobowitz]

Thoughts on this?  I observed one case where the selected frame hadn't been
initialized yet: when checking watchpoint values.  Oops!  The condition
matches the case in which get_current_frame () can be reasonably expected to
succeed.  There are a couple other places that need this same check, mostly
functions which will work on globals as well as locals.

An alternative would be to create a lifeless_frame and have
get_current_frame return that when there is no running target; have it call
error if a register is read, its base is requested, et cetera.  I suspect
that would require some serious surgery.  We'd also need to pass
lifeless_frame everywhere we could be passing a NULL frame now.  For
instance, several callers of read_var_value will give it no frame.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-08-01  Daniel Jacobowitz  <drow@mvista.com>

	* findvar.c (read_var_value): If the target is running, call
	get_selected_frame instead of checking deprecated_selected_frame.

--- gdb-5.3.20030801/gdb/findvar.c.orig	2003-08-01 14:46:12.000000000 -0400
+++ gdb-5.3.20030801/gdb/findvar.c	2003-08-01 14:50:27.000000000 -0400
@@ -388,7 +388,7 @@ symbol_read_needs_frame (struct symbol *
    and a stack frame id, read the value of the variable
    and return a (pointer to a) struct value containing the value. 
    If the variable cannot be found, return a zero pointer.
-   If FRAME is NULL, use the deprecated_selected_frame.  */
+   If FRAME is NULL, use the selected frame.  */
 
 struct value *
 read_var_value (register struct symbol *var, struct frame_info *frame)
@@ -404,8 +404,9 @@ read_var_value (register struct symbol *
 
   len = TYPE_LENGTH (type);
 
-  if (frame == NULL)
-    frame = deprecated_selected_frame;
+  if (frame == NULL && target_has_registers && target_has_stack
+      && target_has_memory)
+    frame = get_selected_frame ();
 
   switch (SYMBOL_CLASS (var))
     {

Re: RFC: selected frame in read_var_value

[Andrew Cagney]

> Thoughts on this?  I observed one case where the selected frame hadn't been
> initialized yet: when checking watchpoint values.  Oops!  The condition
> matches the case in which get_current_frame () can be reasonably expected to
> succeed.  There are a couple other places that need this same check, mostly
> functions which will work on globals as well as locals.

I think the caller, and not this code, should be the one deciding 
if/when a get_selected_frame should be called.  Otherwize the dependance 
on global state remains.

> An alternative would be to create a lifeless_frame and have
> get_current_frame return that when there is no running target; have it call
> error if a register is read, its base is requested, et cetera.  I suspect
> that would require some serious surgery.  We'd also need to pass
> lifeless_frame everywhere we could be passing a NULL frame now.  For
> instance, several callers of read_var_value will give it no frame.

Which exact case are you thinking of?  A core file should have a frame. 
  An exec file?  A lifeless frame (throw an error if any register is 
unwound) should be straight forward.

Andrew



> --- gdb-5.3.20030801/gdb/findvar.c.orig	2003-08-01 14:46:12.000000000 -0400
> +++ gdb-5.3.20030801/gdb/findvar.c	2003-08-01 14:50:27.000000000 -0400
> @@ -388,7 +388,7 @@ symbol_read_needs_frame (struct symbol *
>     and a stack frame id, read the value of the variable
>     and return a (pointer to a) struct value containing the value. 
>     If the variable cannot be found, return a zero pointer.
> -   If FRAME is NULL, use the deprecated_selected_frame.  */
> +   If FRAME is NULL, use the selected frame.  */
>  
>  struct value *
>  read_var_value (register struct symbol *var, struct frame_info *frame)
> @@ -404,8 +404,9 @@ read_var_value (register struct symbol *
>  
>    len = TYPE_LENGTH (type);
>  
> -  if (frame == NULL)
> -    frame = deprecated_selected_frame;
> +  if (frame == NULL && target_has_registers && target_has_stack
> +      && target_has_memory)
> +    frame = get_selected_frame ();

Re: RFC: selected frame in read_var_value

[Daniel Jacobowitz]

On Thu, Aug 07, 2003 at 01:45:28PM -0400, Andrew Cagney wrote:
> >Thoughts on this?  I observed one case where the selected frame hadn't been
> >initialized yet: when checking watchpoint values.  Oops!  The condition
> >matches the case in which get_current_frame () can be reasonably expected 
> >to
> >succeed.  There are a couple other places that need this same check, mostly
> >functions which will work on globals as well as locals.
> 
> I think the caller, and not this code, should be the one deciding 
> if/when a get_selected_frame should be called.  Otherwize the dependance 
> on global state remains.

The callers don't know any more than we do.  Take a look at some of
them - several callers don't take frame arguments, and work both when
the program is running and when it isn't.  If you want to cut down on
the global state dependence, I guess you'd have to push this up through
every caller, so that it could eventually be pushed even higher?

Me, I was just fixing the regression caused by lazily selecting a
frame.  I wasn't planning on redesigning interfaces all through the
block and variable layers; and fixing this really does go quite far up.
For instance it works back to block_innermost_frame, which can return
NULL.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: selected frame in read_var_value

[Andrew Cagney]

> On Thu, Aug 07, 2003 at 01:45:28PM -0400, Andrew Cagney wrote:
> 
>> >Thoughts on this?  I observed one case where the selected frame hadn't been
>> >initialized yet: when checking watchpoint values.  Oops!  The condition
>> >matches the case in which get_current_frame () can be reasonably expected 
>> >to
>> >succeed.  There are a couple other places that need this same check, mostly
>> >functions which will work on globals as well as locals.
> 
>> 
>> I think the caller, and not this code, should be the one deciding 
>> if/when a get_selected_frame should be called.  Otherwize the dependance 
>> on global state remains.
> 
> 
> The callers don't know any more than we do.  Take a look at some of
> them - several callers don't take frame arguments, and work both when
> the program is running and when it isn't.  If you want to cut down on
> the global state dependence, I guess you'd have to push this up through
> every caller, so that it could eventually be pushed even higher?

Yep, welcome to my world :-)

> Me, I was just fixing the regression caused by lazily selecting a
> frame.  I wasn't planning on redesigning interfaces all through the
> block and variable layers; and fixing this really does go quite far up.
> For instance it works back to block_innermost_frame, which can return
> NULL.

What about something like this as a step sideways:

> if (frame == NULL)
>   frame = deprecated_safe_get_selected_frame ();

where deprecated_safe_get_selected_frame() would contain those sanity 
checks?

Andrew

Re: RFC: selected frame in read_var_value

[Daniel Jacobowitz]

On Fri, Sep 05, 2003 at 01:09:46PM -0400, Andrew Cagney wrote:
> >On Thu, Aug 07, 2003 at 01:45:28PM -0400, Andrew Cagney wrote:
> >
> >>>Thoughts on this?  I observed one case where the selected frame hadn't 
> >>been
> >>>initialized yet: when checking watchpoint values.  Oops!  The condition
> >>>matches the case in which get_current_frame () can be reasonably 
> >>expected >to
> >>>succeed.  There are a couple other places that need this same check, 
> >>mostly
> >>>functions which will work on globals as well as locals.
> >
> >>
> >>I think the caller, and not this code, should be the one deciding 
> >>if/when a get_selected_frame should be called.  Otherwize the dependance 
> >>on global state remains.
> >
> >
> >The callers don't know any more than we do.  Take a look at some of
> >them - several callers don't take frame arguments, and work both when
> >the program is running and when it isn't.  If you want to cut down on
> >the global state dependence, I guess you'd have to push this up through
> >every caller, so that it could eventually be pushed even higher?
> 
> Yep, welcome to my world :-)

Your world is a very strange place :)

> >Me, I was just fixing the regression caused by lazily selecting a
> >frame.  I wasn't planning on redesigning interfaces all through the
> >block and variable layers; and fixing this really does go quite far up.
> >For instance it works back to block_innermost_frame, which can return
> >NULL.
> 
> What about something like this as a step sideways:
> 
> >if (frame == NULL)
> >  frame = deprecated_safe_get_selected_frame ();
> 
> where deprecated_safe_get_selected_frame() would contain those sanity 
> checks?

Do you think this would interfere with fixing it later?  If not, I'll
put together a patch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: selected frame in read_var_value

[Andrew Cagney]

> What about something like this as a step sideways:
>> 
> 
>> >if (frame == NULL)
>> >  frame = deprecated_safe_get_selected_frame ();
> 
>> 
>> where deprecated_safe_get_selected_frame() would contain those sanity 
>> checks?
> 
> 
> Do you think this would interfere with fixing it later?  If not, I'll
> put together a patch.

No, just delay it.  It's also ok for 6.0.

Andrew

Re: RFC: selected frame in read_var_value

[Daniel Jacobowitz]

On Fri, Sep 05, 2003 at 01:09:46PM -0400, Andrew Cagney wrote:
> >On Thu, Aug 07, 2003 at 01:45:28PM -0400, Andrew Cagney wrote:
> >
> >>>Thoughts on this?  I observed one case where the selected frame hadn't 
> >>been
> >>>initialized yet: when checking watchpoint values.  Oops!  The condition
> >>>matches the case in which get_current_frame () can be reasonably 
> >>expected >to
> >>>succeed.  There are a couple other places that need this same check, 
> >>mostly
> >>>functions which will work on globals as well as locals.
> >
> >>
> >>I think the caller, and not this code, should be the one deciding 
> >>if/when a get_selected_frame should be called.  Otherwize the dependance 
> >>on global state remains.
> >
> >
> >The callers don't know any more than we do.  Take a look at some of
> >them - several callers don't take frame arguments, and work both when
> >the program is running and when it isn't.  If you want to cut down on
> >the global state dependence, I guess you'd have to push this up through
> >every caller, so that it could eventually be pushed even higher?
> 
> Yep, welcome to my world :-)
> 
> >Me, I was just fixing the regression caused by lazily selecting a
> >frame.  I wasn't planning on redesigning interfaces all through the
> >block and variable layers; and fixing this really does go quite far up.
> >For instance it works back to block_innermost_frame, which can return
> >NULL.
> 
> What about something like this as a step sideways:
> 
> >if (frame == NULL)
> >  frame = deprecated_safe_get_selected_frame ();
> 
> where deprecated_safe_get_selected_frame() would contain those sanity 
> checks?
> 
> Andrew

Is this about what you wanted, and are my comments on the mark?

If so, how do you feel about a mass replacement of the one deprecated
construct (deprecated_selected_frame) with the new deprecated construct
(deprecated_safe_get_selected_frame) in the places in GDB which use
this "if frame arg is NULL, get selected frame" idiom?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-09-06  Daniel Jacobowitz  <drow@mvista.com>

	* frame.c (deprecated_safe_get_selected_frame): New function.
	* frame.h (deprecated_safe_get_selected_frame): Add prototype.
	* findvar.c (read_var_value): Call it.

Index: findvar.c
===================================================================
RCS file: /cvs/src/src/gdb/findvar.c,v
retrieving revision 1.62
diff -u -p -r1.62 findvar.c
--- findvar.c	22 Jul 2003 15:41:59 -0000	1.62
+++ findvar.c	7 Sep 2003 03:46:36 -0000
@@ -404,8 +404,11 @@ read_var_value (register struct symbol *
 
   len = TYPE_LENGTH (type);
 
+
+  /* FIXME drow/2003-09-06: this call to the selected frame should be
+     pushed upwards to the callers.  */
   if (frame == NULL)
-    frame = deprecated_selected_frame;
+    frame = deprecated_safe_get_selected_frame ();
 
   switch (SYMBOL_CLASS (var))
     {
Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.137
diff -u -p -r1.137 frame.c
--- frame.c	12 Aug 2003 17:45:13 -0000	1.137
+++ frame.c	7 Sep 2003 03:46:36 -0000
@@ -917,6 +917,18 @@ get_selected_frame (void)
   return deprecated_selected_frame;
 }
 
+/* This is a variant of get_selected_frame which can be called when the
+   inferior is not running; in that case it will return NULL instead of
+   calling error ().  */
+
+struct frame_info *
+deprecated_safe_get_selected_frame (void)
+{
+  if (!target_has_registers || !target_has_stack || !target_has_memory)
+    return NULL;
+  return get_selected_frame ();
+}
+
 /* Select frame FI (or NULL - to invalidate the current frame).  */
 
 void
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.106
diff -u -p -r1.106 frame.h
--- frame.h	11 Jul 2003 15:31:43 -0000	1.106
+++ frame.h	7 Sep 2003 03:46:36 -0000
@@ -639,6 +639,19 @@ extern void return_command (char *, int)
 
 extern struct frame_info *deprecated_selected_frame;
 
+/* NOTE: drow/2003-09-06:
+
+   This function is "a step sideways" for uses of deprecated_selected_frame.
+   They should be fixed as above, but meanwhile, we needed a solution for
+   cases where functions are called with a NULL frame meaning either "the
+   program is not running" or "use the selected frame".  Lazy building of
+   deprecated_selected_frame confuses the situation, because now
+   deprecated_selected_frame can be NULL even when the inferior is running.
+
+   This function calls get_selected_frame if the inferior is running, or
+   returns NULL otherwise.  */
+
+extern struct frame_info *deprecated_safe_get_selected_frame (void);
 
 /* Create a frame using the specified BASE and PC.  */
 

Re: RFC: selected frame in read_var_value

[Andrew Cagney]

> Is this about what you wanted, and are my comments on the mark?

Yes, just one pedantic tweak.

> If so, how do you feel about a mass replacement of the one deprecated
> construct (deprecated_selected_frame) with the new deprecated construct
> (deprecated_safe_get_selected_frame) in the places in GDB which use
> this "if frame arg is NULL, get selected frame" idiom?

For 6.0, scares the peverbial out of me -> got a convincing argument? 
:-)  For the mainline, not phased.

Andrew


> +/* This is a variant of get_selected_frame which can be called when the
> +   inferior is not running; in that case it will return NULL instead of
> +   calling error ().  */

Almost.  Those are also true for a core file.

> +struct frame_info *
> +deprecated_safe_get_selected_frame (void)
> +{
> +  if (!target_has_registers || !target_has_stack || !target_has_memory)
> +    return NULL;
> +  return get_selected_frame ();
> +}

Re: RFC: selected frame in read_var_value

[Daniel Jacobowitz]

On Mon, Sep 08, 2003 at 09:33:33AM -0400, Andrew Cagney wrote:
> 
> >Is this about what you wanted, and are my comments on the mark?
> 
> Yes, just one pedantic tweak.
> 
> >If so, how do you feel about a mass replacement of the one deprecated
> >construct (deprecated_selected_frame) with the new deprecated construct
> >(deprecated_safe_get_selected_frame) in the places in GDB which use
> >this "if frame arg is NULL, get selected frame" idiom?
> 
> For 6.0, scares the peverbial out of me -> got a convincing argument? 
> :-)  For the mainline, not phased.

I don't know if it's convincing, but I've had one bug reported that
would be fixed by it (assertion failure in -data-list-changed-registers).
I've got another one to look at now that might be similar.  We could
just do 'em as they're discovered.

> 
> Andrew
> 
> 
> >+/* This is a variant of get_selected_frame which can be called when the
> >+   inferior is not running; in that case it will return NULL instead of
> >+   calling error ().  */
> 
> Almost.  Those are also true for a core file.

Ooh, that's right of course.  I'll revise and commit.

> 
> >+struct frame_info *
> >+deprecated_safe_get_selected_frame (void)
> >+{
> >+  if (!target_has_registers || !target_has_stack || !target_has_memory)
> >+    return NULL;
> >+  return get_selected_frame ();
> >+}
> 
> 
> 
> 
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: selected frame in read_var_value

[Daniel Jacobowitz]

On Mon, Sep 08, 2003 at 09:33:33AM -0400, Andrew Cagney wrote:
> 
> >Is this about what you wanted, and are my comments on the mark?
> 
> Yes, just one pedantic tweak.

Patch checked in to HEAD and 6.0, with the comment fixed.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: selected frame in read_var_value

[Daniel Jacobowitz]

On Mon, Sep 08, 2003 at 09:33:33AM -0400, Andrew Cagney wrote:
> 
> >Is this about what you wanted, and are my comments on the mark?
> 
> Yes, just one pedantic tweak.
> 
> >If so, how do you feel about a mass replacement of the one deprecated
> >construct (deprecated_selected_frame) with the new deprecated construct
> >(deprecated_safe_get_selected_frame) in the places in GDB which use
> >this "if frame arg is NULL, get selected frame" idiom?
> 
> For 6.0, scares the peverbial out of me -> got a convincing argument? 
> :-)  For the mainline, not phased.

Hey Andrew,

This is something I dropped the ball on.  Of course, we're a lot closer
to releasing 6.1 now than we were when I asked you about this in
September.  How would you feel about doing it now?  If you're
uncomfortable, I'll try to dig up the specific instances Debian users
have reported as causing crashes, instead.

For reference, here's the function:

struct frame_info *
deprecated_safe_get_selected_frame (void)
{
  if (!target_has_registers || !target_has_stack || !target_has_memory)
    return NULL;
  return get_selected_frame ();
}

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFC: selected frame in read_var_value

[Andrew Cagney]

>> For 6.0, scares the peverbial out of me -> got a convincing argument? 
>> :-)  For the mainline, not phased.
> 
> 
> Hey Andrew,
> 
> This is something I dropped the ball on.  Of course, we're a lot closer
> to releasing 6.1 now than we were when I asked you about this in
> September.  How would you feel about doing it now?  If you're
> uncomfortable, I'll try to dig up the specific instances Debian users
> have reported as causing crashes, instead.
> 
> For reference, here's the function:
> 
> struct frame_info *
> deprecated_safe_get_selected_frame (void)
> {
>   if (!target_has_registers || !target_has_stack || !target_has_memory)
>     return NULL;
>   return get_selected_frame ();
> }

Like I said, not phased (MichaelC might be though :-).

Suggest adding a note mentioning that the underlying problem is that 
some code requires both the static (or compile time) scope (identifiable 
by the SAL or a fake static-frame) when there is no dynamic (or runtime) 
scope available.  There's a discussion of this on gdb@ but my link is 
down :-(

Andrew