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

[Daniel Jacobowitz <drow@mvista.com>]

On Thu, Jul 24, 2003 at 02:59:27PM -0500, Jim Blandy wrote:
> 
> For the trunk, put it in, definitely.
> 
> For 6.0, could you test it on a COFF toolchain, and on some non-GNU
> toolchain?  It would be nice to have those three PR's closed in 6.0.

I no longer have access to any non-GNU toolchains.  I'll try to build a
COFF simulator target and give it a whirl, though...

> 
> Daniel Jacobowitz <drow@mvista.com> writes:
> > Only the testsuite on i386-linux.  What would you recommend?
> > 
> > On Mon, Jul 21, 2003 at 02:13:55AM -0500, Jim Blandy wrote:
> > > 
> > > I think this is a great idea.  How widely have you tested it?
> > > 
> > > Daniel Jacobowitz <drow@mvista.com> writes:
> > > 
> > > > This patch fixes c++/1267, a bug where stepping over a function call that
> > > > went through the PLT (as happens when a -fPIC function makes a call to a
> > > > globally visible symbol) would lose control of the inferior.  I'll spare you
> > > > the complete debugging session, as it really doesn't make much sense.  But
> > > > here's the root of the problem:
> > > > 
> > > > When we called frame_pc_unwind on the sentinel frame, we got an address in
> > > > the PLT.  But when we called frame_func_unwind, we got "_init", in ".init",
> > > > which is generally located right before the PLT.  Then, we'd run the
> > > > new-and-improved prologue unwinder on _init, and get some completely bogus
> > > > information, since things weren't actually saved on the stack where it
> > > > thought they were.  This led to the unwound stack pointer being wrong for
> > > > the step_resume breakpoint, so when we hit the step_resume breakpoint we
> > > > kept going.
> > > > 
> > > > I fixed this by changing lookup_minimal_symbol_pc_section to be paranoid
> > > > about returning a minsym in the same section as the PC.  Technically, at
> > > > least on ELF targets, that doesn't _have_ to be true.  I've never
> > > > encountered an exception or a good reason for one, though.  Does anyone see
> > > > any pitfalls for this change?  Symtab maintainers, is this patch OK?
> > > > 
> > > > I believe this patch should also fix shlibs/1237, and may also fix
> > > > shlibs/1280.  Adam, could you check those?
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 
> > > > 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:
> > > > 
> > > >   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.
> > > > 
> > > > -- 
> > > > Daniel Jacobowitz
> > > > MontaVista Software                         Debian GNU/Linux Developer
> > > > 
> > > > 2003-07-19  Daniel Jacobowitz  <drow@mvista.com>
> > > > 
> > > > 	PR c++/1267
> > > > 	* minsyms.c (lookup_minimal_symbol_by_pc_section): If SECTION is
> > > > 	NULL, default to the section containing PC.
> > > > 
> > > > Index: minsyms.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/src/gdb/minsyms.c,v
> > > > retrieving revision 1.31
> > > > diff -u -p -r1.31 minsyms.c
> > > > --- minsyms.c	15 May 2003 22:23:24 -0000	1.31
> > > > +++ minsyms.c	19 Jul 2003 18:03:08 -0000
> > > > @@ -403,12 +403,22 @@ lookup_minimal_symbol_by_pc_section (COR
> > > >    struct objfile *objfile;
> > > >    struct minimal_symbol *msymbol;
> > > >    struct minimal_symbol *best_symbol = NULL;
> > > > +  struct obj_section *pc_section;
> > > >  
> > > >    /* pc has to be in a known section. This ensures that anything beyond
> > > >       the end of the last segment doesn't appear to be part of the last
> > > >       function in the last segment.  */
> > > > -  if (find_pc_section (pc) == NULL)
> > > > +  pc_section = find_pc_section (pc);
> > > > +  if (pc_section == NULL)
> > > >      return NULL;
> > > > +
> > > > +  /* If no section was specified, then just make sure that the PC is in
> > > > +     the same section as the minimal symbol we find.  */
> > > > +  if (section == NULL)
> > > > +    section = pc_section->the_bfd_section;
> > > > +
> > > > +  /* FIXME drow/2003-07-19: Should we also check that PC is in SECTION
> > > > +     if we were passed a non-NULL SECTION argument?  */
> > > >  
> > > >    for (objfile = object_files;
> > > >         objfile != NULL;
> > > 
> > 
> > -- 
> > Daniel Jacobowitz
> > MontaVista Software                         Debian GNU/Linux Developer
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer