Nother little one, this time in varobj.c

Nother little one, this time in varobj.c

[Jim Ingham]

Here is another little buglet.  varobj_update stores the current frame, 
and then resets it
when it is done.  This is great, because the varobj may be in a 
different frame than the current one.  Unfortunately, the intervening 
code can call c_value_of_root, which calls reinit_frame_cache, which 
blows away the frame cache, leaving varobj_update holding a pointer to a 
freed frame_info structure.

The patch below fixes this goof.

BTW, I am not sure why it is necessary to call reinit_frame_cache here.  
Keith, do you remember why this was necessary?  It is inefficient, 
especially if you are evaluating a bunch of variables that are fairly 
high up on the stack.  But since I don't remember why this was done, I 
am reluctant to just change it outright...

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.27
diff -c -w -r1.27 varobj.c
*** varobj.c    5 Apr 2002 22:04:42 -0000       1.27
--- varobj.c    10 Apr 2002 00:30:06 -0000
***************
*** 850,856 ****
     struct value *new;
     struct vstack *stack = NULL;
     struct vstack *result = NULL;
!   struct frame_info *old_fi;

     /* sanity check: have we been passed a pointer? */
     if (changelist == NULL)
--- 850,858 ----
     struct value *new;
     struct vstack *stack = NULL;
     struct vstack *result = NULL;
!   CORE_ADDR old_frame;
!   int old_level;
!

     /* sanity check: have we been passed a pointer? */
     if (changelist == NULL)
***************
*** 861,869 ****
       /* Not a root var */
       return -1;

!   /* Save the selected stack frame, since we will need to change it
!      in order to evaluate expressions. */
!   old_fi = 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
--- 863,875 ----
       /* Not a root var */
       return -1;

!   /* Save the selected stack frame, since we will need to change it in
!      order to evaluate expressions.  However, you have to hold onto
!      the address not the struct frame, because value_of_root calls
!      reinit_frame_cache for its own mysterious purposes, leaving you
!      holding onto garbage... */
!
!   record_selected_frame (&old_frame, &old_level);

     /* Update the root variable. value_of_root can return NULL
        if the variable is no longer around, i.e. we stepped out of
***************
*** 983,989 ****
       }

     /* Restore selected frame */
!   select_frame (old_fi, -1);

     if (type_changed)
       return -2;
--- 989,999 ----
       }

     /* Restore selected frame */
!   if (old_frame != 0)
!     {
!       old_fi = find_frame_addr_in_frame_chain (old_frame);
!       select_frame (old_fi, old_level);
!     }

     if (type_changed)
       return -2;

Jim
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer

Re: Nother little one, this time in varobj.c

[Keith Seitz]

On Tue, 9 Apr 2002, Jim Ingham wrote:

> BTW, I am not sure why it is necessary to call reinit_frame_cache here.
> Keith, do you remember why this was necessary?  It is inefficient,
> especially if you are evaluating a bunch of variables that are fairly
> high up on the stack.  But since I don't remember why this was done, I
> am reluctant to just change it outright...

I haven't a clue, actually. When I originally wrote varobj for Insight, it
did not allow varobjs to change frames dynamically, as it does now.
Presumably, this was all added at the same time. (Or I've forgotten all
about it...)

Keith

Re: Nother little one, this time in varobj.c

[Fernando Nasser]

Keith Seitz wrote:
> 
> On Tue, 9 Apr 2002, Jim Ingham wrote:
> 
> > BTW, I am not sure why it is necessary to call reinit_frame_cache here.
> > Keith, do you remember why this was necessary?  It is inefficient,
> > especially if you are evaluating a bunch of variables that are fairly
> > high up on the stack.  But since I don't remember why this was done, I
> > am reluctant to just change it outright...
> 
> I haven't a clue, actually. When I originally wrote varobj for Insight, it
> did not allow varobjs to change frames dynamically, as it does now.
> Presumably, this was all added at the same time. (Or I've forgotten all
> about it...)
> 

The frames code was already there when I converted it to gdbtk-varobj.c,
so it was added in between.  It seemed to work though.

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: Nother little one, this time in varobj.c

[Andrew Cagney]

> Keith Seitz wrote:
> 
>> 
>> On Tue, 9 Apr 2002, Jim Ingham wrote:
>> 
> 
>> > BTW, I am not sure why it is necessary to call reinit_frame_cache here.
>> > Keith, do you remember why this was necessary?  It is inefficient,
>> > especially if you are evaluating a bunch of variables that are fairly
>> > high up on the stack.  But since I don't remember why this was done, I
>> > am reluctant to just change it outright...
> 
>> 
>> I haven't a clue, actually. When I originally wrote varobj for Insight, it
>> did not allow varobjs to change frames dynamically, as it does now.
>> Presumably, this was all added at the same time. (Or I've forgotten all
>> about it...)
>> 
> 
> 
> The frames code was already there when I converted it to gdbtk-varobj.c,
> so it was added in between.  It seemed to work though.

Just FYI,

GDB is trying to move away from the global selected_frame switching 
technique.  The theory is that there is ``always'' a frame so that the 
selected frame parameter is made explicit to the code that uses it.  As 
the conversion goes through, there is going to be an intermediate mess.

As for reinit_frame_cache() who knows.  One reason for trying to push a 
``keep it simple stupid'' flush-on-write approach (just flush everything 
if the target changes) is that, as with the above, no one is sure what 
the existing strategy is.

enjoy,
Andrew

Re: Nother little one, this time in varobj.c

[Jim Ingham]

Keith,

Yeah, I added the ability to make the frames change (Syd thought that 
variables in the watch window should re-evaluate themselves every time 
that you changed the stack frame in the debugger - apparently that's 
what VC++ does).  But I added all this on 03-2000, and the 
reinit_frame_cache is already there in the initial import into 
sourceware.  Unfortunately, the sourceware repository history only goes 
back to Feb. 2000, and this was all checked in before that.  If you can 
look in the cygnus repository, you might at least be able to figure out 
who should remember why this is there - not that this actually helps 
that much...

Jim

On Tuesday, April 9, 2002, at 11:37  PM, Keith Seitz wrote:

> On Tue, 9 Apr 2002, Jim Ingham wrote:
>
>> BTW, I am not sure why it is necessary to call reinit_frame_cache here.
>> Keith, do you remember why this was necessary?  It is inefficient,
>> especially if you are evaluating a bunch of variables that are fairly
>> high up on the stack.  But since I don't remember why this was done, I
>> am reluctant to just change it outright...
>
> I haven't a clue, actually. When I originally wrote varobj for Insight, 
> it
> did not allow varobjs to change frames dynamically, as it does now.
> Presumably, this was all added at the same time. (Or I've forgotten all
> about it...)
>
> Keith
>
>
>
--
Jim Ingham                                   jingham@apple.com
Developer Tools - gdb
Apple Computer