Re: PING: [RFA] Improve performance with lots of shared libraries

[Gary Benson <gbenson@redhat.com>]

Hi Pedro,

Pedro Alves wrote:
> On Tuesday 04 October 2011 11:25:18, Gary Benson wrote:
> >   2011-09-22  Gary Benson  <gbenson@redhat.com>
> > 
> >         * infrun.c (stopped_at_solib_event_breakpoint): New function.
> >         (stopped_at_solib_event_breakpoint_helper): Likewise.
> >         (handle_inferior_event): Avoid calling skip_inline_frames
> >         when at the solib event breakpoint.
> > 
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 225034c..2e49470 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> >      }
> >  }
> >  
> > +/* Helper for stopped_at_solib_event_breakpoint.  */
> > +
> > +static int
> > +stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg)
> 

> As I mentioned before, better would for this to be a
> property/function of the breakpoint itself (and its locations) --
> meaning "I am / am not an inlined location".  There's no reason to
> limit this just to the solib event breakpoint.  An obvious example
> without even that generalization is the thread event breakpoint.

Are you talking about a function like this:

  int non_inline_function (struct breakpoint *b)
  {
    return b->type == bp_shlib_event;
  }

or something more involved?

> Even if you don't do that, please add this new function (that checks
> whether we stopped at an address that can't be inlined) to
> breakpoint.c instead, and call it `stopped_at_non_inline_function'
> or something along those lines.  We can later rework its internals
> keeping its interface.

Ok.

> > +{
> > +  struct execution_control_state *ecs
> > +    = (struct execution_control_state *) arg;
> > +
> 
> You need to check that the breakpoint is enabled and installed.

Does it matter if it isn't?  My thinking was that we are trying to
state that there cannot be inlined functions at the stop address, and
the existence of the shlib event breakpoint there allows you to state
that whether it is enabled or not.

(Am I correct in thinking "installed" means "not-pending"?)

> > +  if (b->type == bp_shlib_event)
> > +    {
> > +      CORE_ADDR prev_pc = ecs->event_thread->prev_pc;
> > +      struct bp_location *loc;
> > +
> > +      for (loc = b->loc; loc; loc = loc->next)
> > +       {
> > +         if (loc->pspace == current_program_space
> > +             && (loc->address == stop_pc || loc->address == prev_pc))
> 
> I don't follow the prev_pc check.  What does that intend to do?

You end up in handle_inferior_event twice for every breakpoint, once
when you hit it and once when you've single-stepped over it.  The
prev_pc check catches the latter.

> When working in the direction of matching from an event back to our
> tables and symbolic info, you start from the address_space instead.
> Get at the address space of the current PC using
> `get_regcache_aspace (get_thread_regcache (ecs->ptid))'.  But better
> yet, since you'd end up rewritting bkpt_breakpoint_hit, it's better
> instead to reuse bpstat_check_location (despite the name, it doesn't
> work with a bpstat).  See
> bpstop_status->bpstat_check_location->bkpt_breakpoint_hit, and
> factor out the necessary bits.

Ok, I will look into this.

Thank you,
Gary

-- 
http://gbenson.net/