infrun.c:restore_selected_frame???

infrun.c:restore_selected_frame???

[Jim Ingham]

Hi, all...

I am totally confused by restore_selected_frame.  Why is it calling 
find_relative_frame, passing in the current frame and level?  Remember, 
level comes from a stored value of selected_frame_level.  
selected_frame_level is set in select_frame, and is either an absolute 
frame level, or -1 if the caller of select_frame was just selecting by 
frame_info and doesn't care about the level (which is done in a number 
of places).  So it is NOT a relative level at all, certainly not 
relative to whatever the current frame happens to be except by 
accident...

Since you also have the frame address sitting around in the 
restore_selected_frame_args, why not use it to find the frame instead?  
The patch below works for me, and eliminates a bunch of errant kvetching 
about "Unable to restore selected frame"...

Does this seem right?

Jim

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.56
diff -c -w -r1.56 infrun.c
*** infrun.c    2002/03/18 02:26:31     1.56
--- infrun.c    2002/04/03 23:42:52
***************
*** 4004,4024 ****
     struct restore_selected_frame_args *fr =
     (struct restore_selected_frame_args *) args;
     struct frame_info *frame;
     int level = fr->level;

!   frame = find_relative_frame (get_current_frame (), &level);

     /* If inf_status->selected_frame_address is NULL, there was no
        previously selected frame.  */
!   if (frame == NULL ||
!   /*  FRAME_FP (frame) != fr->frame_address || */
!   /* elz: deleted this check as a quick fix to the problem that
!      for function called by hand gdb creates no internal frame
!      structure and the real stack and gdb's idea of stack are
!      different if nested calls by hands are made.
!
!      mvs: this worries me.  */
!       level != 0)
       {
         warning ("Unable to restore previously selected frame.\n");
         return 0;
--- 4004,4017 ----
     struct restore_selected_frame_args *fr =
     (struct restore_selected_frame_args *) args;
     struct frame_info *frame;
+   CORE_ADDR frame_address = fr->frame_address;
     int level = fr->level;

!   frame = find_frame_addr_in_frame_chain (frame_address);

     /* If inf_status->selected_frame_address is NULL, there was no
        previously selected frame.  */
!   if (frame == NULL)
       {
         warning ("Unable to restore previously selected frame.\n");
         return 0;

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

Re: infrun.c:restore_selected_frame???

[Andrew Cagney]

> Hi, all...

(Have you been sending me subliminal messages telling me to look at 
selected_frame_level?  I'd not looked at this e-mail when I posted the 
patch http://sources.redhat.com/ml/gdb-patches/2002-04/msg00241.html to 
add a frame->level.

> I am totally confused by restore_selected_frame.  Why is it calling find_relative_frame, passing in the current frame and level?  Remember, level comes from a stored value of selected_frame_level.  selected_frame_level is set in select_frame, and is either an absolute frame level, or -1 if the caller of select_frame was just selecting by frame_info and doesn't care about the level (which is done in a number of places).  So it is NOT a relative level at all, certainly not relative to whatever the current frame happens to be except by accident...

Selected_frame_level is relative (0 is inner most) not absolute.  The 
above patch demonstrates this.

The code appears to be assuming that, after an inferior function call, 
current frame has been restored to its pre-inferior-call and hence that 
current_frame+level gives the saved frame.

Robert Elz's comment suggests this isn't always the case and you can end 
up selecting a frame that doesn't match the expected (the commented out 
test).  I think there are two cases when this occures:

- the inferior function didn't exit and you've ended up with more than 
the expected number of frames to the saved frame.  The level test won't 
detect this and you'll just select the wrong frame

- the inferior function did exit along with a few other things and 
you've ended up with less frames then you expected.  The level test 
might detect this (if the number of levels takes you off the bottom of 
the stack).

> Since you also have the frame address sitting around in the restore_selected_frame_args, why not use it to find the frame instead?  The patch below works for me, and eliminates a bunch of errant kvetching about "Unable to restore selected frame"...

What are the exact circumstances under which this occures?  You're not N 
levels down from the current/inner-most stack frame?

> Does this seem right? 

I'm fairly sure it is.

--

Looking at find_frame_addr_in_frame_chain(), I think it needs to be made 
more robust - check that the loop hasn't gone past the specified frame. 
  The level code would have been covering this problem up.

--

Hmm, to more robustly identify a frame, should we save both the 
frame->frame and frame->pc (or containing function)?  This is separate / 
independant - I've always wondered if frame->frame was sufficient.

Andrew

Re: infrun.c:restore_selected_frame???

[Jim Ingham]

Andrew,

On Monday, April 8, 2002, at 04:26  PM, Andrew Cagney wrote:

>> Hi, all...
>
> (Have you been sending me subliminal messages telling me to look at 
> selected_frame_level?  I'd not looked at this e-mail when I posted the 
> patch http://sources.redhat.com/ml/gdb-patches/2002-04/msg00241.html to 
> add a frame->level.
>

I saw that and wondered if YOU had been reading MY mind...

>> I am totally confused by restore_selected_frame.  Why is it calling 
>> find_relative_frame, passing in the current frame and level?  
>> Remember, level comes from a stored value of selected_frame_level.  
>> selected_frame_level is set in select_frame, and is either an absolute 
>> frame level, or -1 if the caller of select_frame was just selecting by 
>> frame_info and doesn't care about the level (which is done in a number 
>> of places).  So it is NOT a relative level at all, certainly not 
>> relative to whatever the current frame happens to be except by 
>> accident...
>
> Selected_frame_level is relative (0 is inner most) not absolute.  The 
> above patch demonstrates this.
>

If this is right, the way it is set (and the comment) at the beginning 
of select_frame is confusing:

/* Select frame FI, and note that its stack level is LEVEL.
    LEVEL may be -1 if an actual level number is not known.  */

void
select_frame (struct frame_info *fi, int level)
{
   register struct symtab *s;

   selected_frame = fi;
   selected_frame_level = level;

Sounds like if I have a hold of the frame_info, it is sufficient to pass 
this and -1 in, but that is not right...

> The code appears to be assuming that, after an inferior function call, 
> current frame has been restored to its pre-inferior-call and hence that 
> current_frame+level gives the saved frame.
>
> Robert Elz's comment suggests this isn't always the case and you can 
> end up selecting a frame that doesn't match the expected (the commented 
> out test).  I think there are two cases when this occures:
>
> - the inferior function didn't exit and you've ended up with more than 
> the expected number of frames to the saved frame.  The level test won't 
> detect this and you'll just select the wrong frame
>
> - the inferior function did exit along with a few other things and 
> you've ended up with less frames then you expected.  The level test 
> might detect this (if the number of levels takes you off the bottom of 
> the stack).
>
>> Since you also have the frame address sitting around in the 
>> restore_selected_frame_args, why not use it to find the frame 
>> instead?  The patch below works for me, and eliminates a bunch of 
>> errant kvetching about "Unable to restore selected frame"...
>
> What are the exact circumstances under which this occures?  You're not 
> N levels down from the current/inner-most stack frame?

This is a collision with the varobj code.  varobj_create stashes away 
the current frame - in case the varobj that you are creating is in 
another frame, and then restores it by calling:

           select_frame (fi, -1);

According to the header comment for select frame, this is reasonable, 
because -1 is supposed to mean "I don't care about the level, but of 
course this doesn't work when you try to do restore_selected_frame with 
-1 from frame 0...

>
>> Does this seem right?
>
> I'm fairly sure it is.
>
> --
>
> Looking at find_frame_addr_in_frame_chain(), I think it needs to be 
> made more robust - check that the loop hasn't gone past the specified 
> frame.  The level code would have been covering this problem up.
>
> --
>
> Hmm, to more robustly identify a frame, should we save both the 
> frame->frame and frame->pc (or containing function)?  This is 
> separate / independant - I've always wondered if frame->frame was 
> sufficient.

We do this in Project Builder, but there we do it because a GUI has to 
present the WHOLE stack frame every time you step, which is slow, so we 
optimize this by sending frame+pc duples to PB using a special purpose 
command that just gets these and doesn't reconstruct the whole stack.

Don't know if you need this in gdb though.  When did you think this 
might be a problem?

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

Re: infrun.c:restore_selected_frame???

[Andrew Cagney]

>> Selected_frame_level is relative (0 is inner most) not absolute.  The above patch demonstrates this.
> 
> 
> If this is right, the way it is set (and the comment) at the beginning of select_frame is confusing:
> 
> /* Select frame FI, and note that its stack level is LEVEL.
>    LEVEL may be -1 if an actual level number is not known.  */
> 
> void
> select_frame (struct frame_info *fi, int level)
> {
>   register struct symtab *s;
> 
>   selected_frame = fi;
>   selected_frame_level = level;

I'm pretty sure it is.   My patch did two things;

- adds frame->level
- checks frame->level against selected_frame_level and throws an 
internal error if they clash

I then ran the testsuite until gdb stopped having internal errors.  As 
best I can tell, LEVEL is either fi->level or -1.  While probably not 
the best way, it was the only way I could think of to identify all cases :-)

I think LEVEL parameter is going to become redundant.

> Sounds like if I have a hold of the frame_info, it is sufficient to pass this and -1 in, but that is not right... 

Sounds like your existing patch to use frame->frame is best for now.  At 
least that way the correct level is restored.

> This is a collision with the varobj code.  varobj_create stashes away the current frame - in case the varobj that you are creating is in another frame, and then restores it by calling:
> 
>           select_frame (fi, -1);
> 
> According to the header comment for select frame, this is reasonable, because -1 is supposed to mean "I don't care about the level, but of course this doesn't work when you try to do restore_selected_frame with -1 from frame 0...

Ah!  Outch!

> We do this in Project Builder, but there we do it because a GUI has to present the WHOLE stack frame every time you step, which is slow, so we optimize this by sending frame+pc duples to PB using a special purpose command that just gets these and doesn't reconstruct the whole stack.
> 
> Don't know if you need this in gdb though.

If it hurts one GUI it is going to hurt others.

I think it might be better to try to modify GDB's frame code so that it 
isn't as heavy handed (fewer target accesses) when constructing the 
frame chain.  At present GDB typically fully analyzes one frame before 
going to construct a basic skeleton of the next one :-(

 >   When did you think this might be a problem?

I'm wondering if there can be false positives.  Consider a sequence like:

	o	in base
	o	save base's frame address
	o	do an inferior call to bar()
		which long-jumps over the return
		breakpoint
	o	eventually you hit a breakpoint
		in a random function

There is a chance that the frame will match base's frame giving a false 
positive.  I guess that is always there but the chance is so low it 
isn't worth worrying about.

enjoy,
Andrew

Re: infrun.c:restore_selected_frame???

[Andrew Cagney]

I wrote:

> Hmm, to more robustly identify a frame, should we save both the frame->frame and frame->pc (or containing function)?  This is separate / independant - I've always wondered if frame->frame was sufficient.
> 
> We do this in Project Builder, but there we do it because a GUI has to present the WHOLE stack frame every time you step, which is slow, so we optimize this by sending frame+pc duples to PB using a special purpose command that just gets these and doesn't reconstruct the whole stack.
> 
> Don't know if you need this in gdb though.  When did you think this might be a problem?

Jim wrote:

>>   When did you think this might be a problem?

I think I've I figured it out.

To safely identify a frame, I think the ->addr/base and the ->func/pc 
are needed.  The dwarf2cfi stuff drops the hint as to why.

Consider a function that doesn't create a frame.  A call through that 
function can result in two stack frames having the same ->frame value. 
They can only be differentiated by using the ->pc.

I think, the correct way to identify a frame is to save:

	->frame
		as before, a constant value that doesn't
		change through out the lifetime of the function
	->func
		i.e. function_containing(->pc)
		(It turns out that, whenever a frame is created
		this is computed anyway!)

I think ->func and not ->pc is needed so that it is possible to identify 
a frame at different points in its execution (eg stepping through the 
inner-most frame).

--

dwarf2cfi doesn't allow for a (mutually) recursive frameless function 
(where both ->frame and ->func would be identical).


enjoy,
Andrew