Re: RFA symtab: Fix for PR c++/1267 ("next" and shared libraries)

[Daniel Jacobowitz <drow at mvista dot com>]

On Mon, Jul 21, 2003 at 12:18:28PM -0400, Andrew Cagney wrote:
> 
> >By the way, I'm convinced that all is not well in step_over_function.  This
> >comment,
> >
> >  /* NOTE: cagney/2003-04-06:
> >
> >     The intent of DEPRECATED_SAVED_PC_AFTER_CALL was to:
> >
> >     - provide a very light weight equivalent to frame_unwind_pc()
> >     (nee FRAME_SAVED_PC) that avoids the prologue analyzer
> >
> >     - avoid handling the case where the PC hasn't been saved in the
> >     prologue analyzer
> >
> >     Unfortunatly, not five lines further down, is a call to
> >     get_frame_id() and that is guarenteed to trigger the prologue
> >     analyzer.
> >
> >is either incorrect or has gotten out of sync with the code:
> 
> Nope (it pays to look at the archives).
> 
> >  if (DEPRECATED_SAVED_PC_AFTER_CALL_P ())
> >    sr_sal.pc = ADDR_BITS_REMOVE (DEPRECATED_SAVED_PC_AFTER_CALL 
> >    (get_current_frame ()));
> >  else
> >    sr_sal.pc = ADDR_BITS_REMOVE (frame_pc_unwind (get_current_frame ()));
> >  sr_sal.section = find_pc_overlay (sr_sal.pc);
> >
> >  check_for_old_step_resume_breakpoint ();
> >  step_resume_breakpoint =
> >    set_momentary_breakpoint (sr_sal, get_frame_id (get_current_frame ()),
> >                              bp_step_resume);
> >
> >
> >Note that get_frame_id unwinds from the NEXT frame, and
> >frame_pc_unwind/DEPRECATED_SAVED_PC_AFTER_CALL unwind from THIS frame.
> >This throws me a loop every time I have to work in this function.  Also, I
> >have the nagging feeling we're saving the wrong frame.  I have an old MIPS
> >patch where I needed to use get_prev_frame in step_over_function.  As soon
> >as I have time to revisit that patch I'll be back to clean this up some
> >more.
> 
> The complete code body is:
> 
>   if (DEPRECATED_SAVED_PC_AFTER_CALL_P ())
>     sr_sal.pc = ADDR_BITS_REMOVE (DEPRECATED_SAVED_PC_AFTER_CALL 
> (get_current_frame ()));
>   else
>     sr_sal.pc = ADDR_BITS_REMOVE (frame_pc_unwind (get_current_frame ()));
>   sr_sal.section = find_pc_overlay (sr_sal.pc);
> 
>   check_for_old_step_resume_breakpoint ();
>   step_resume_breakpoint =
>     set_momentary_breakpoint (sr_sal, get_frame_id (get_current_frame ()),
> 			      bp_step_resume);
> 
>   if (frame_id_p (step_frame_id)
>       && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc))
>     step_resume_breakpoint->frame_id = step_frame_id;
> 
> while the original code looks like:
> 
>   struct symtab_and_line sr_sal;
> 
>   init_sal (&sr_sal);		/* initialize to zeros */
>   sr_sal.pc = ADDR_BITS_REMOVE (SAVED_PC_AFTER_CALL (get_current_frame 
> ()));
>   sr_sal.section = find_pc_overlay (sr_sal.pc);
> 
>   check_for_old_step_resume_breakpoint ();
>   step_resume_breakpoint =
>     set_momentary_breakpoint (sr_sal, get_current_frame (), 
> bp_step_resume);
> 
>   if (step_frame_address && !IN_SOLIB_DYNSYM_RESOLVE_CODE (sr_sal.pc))
>     step_resume_breakpoint->frame = step_frame_address;
> 
>   if (breakpoints_inserted)
>     insert_breakpoints ();

No, really, I still think that the comment is wrong.  The get_frame_id
call triggers the prologue analyzer to analyze the NEXT frame.  I.E.
this_id is called with the NEXT frame, and THIS cache.

The call to frame_pc_unwind uses THIS frame and creates the PREV cache. 
It's one frame off on the stack.  The frame it would be saving from
prologue analysis is not the one we'd need to analyze for the
get_frame_id call.

What am I missing?  Certainly you converted the code correctly, I just
don't understand the comment.

> It would appear that the get_frame_id() call has been wrong for a long 
> long time but, at a guess, was worked around by picking up step_frame_id 
> / step_frame_address.

Maybe...

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer