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

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 11 October 2011 16:53:01, Gary Benson wrote:
> 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.
> > 
> > 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.
> > 
> > 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.
> ...
> > You need to check that the breakpoint is enabled and installed.
> ...
> > 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.
> 
> Further to my mail yesterday with questions/explanations, attached is
> a patch which does what I think you are asking.  Is this what you had
> in mind?

Eh, sorry, you were too fast.  :-)  Yeah, something like that.

> Note that I haven't extended it to recognise anything other than the
> shared library event breakpoint as a non-inline location.  If possible
> I'd like to handle other cases in a separate patch as it would require
> further discussion.

Yes, a good idea.

>    if (ecs->event_thread->control.step_range_end != 1)
> -    skip_inline_frames (ecs->ptid);
> +    {
> +      struct address_space *aspace = 
> +       get_regcache_aspace (get_thread_regcache (ecs->ptid));
> +
> +      /* skip_inline_frames is expensive, so we avoid it if we can
> +        determine that the address is one where functions cannot have
> +        been inlined.  This improves performance with inferiors that
> +        load a lot of shared libraries, because the solib event
> +        breakpoint is defined as the address of a function (i.e. not
> +        inline).  Note that we have to check the previous PC as well
> +        as the current one to catch cases when we have just
> +        single-stepped over a breakpoint prior to reinstating it.  */
> +      if (!stopped_at_non_inline_function (aspace, stop_pc)
> +         && !stopped_at_non_inline_function (aspace,
> +                                             ecs->event_thread->prev_pc))
> +       skip_inline_frames (ecs->ptid);
> +    }

Thanks for extending the comment.  The first check is quite definitive.
The second is heuristic.  There's nothing preventing the event breakpoint
function to contain a bit of inlined code, and the single-step ending up
there.  If the user had set a breakpoint on that inlined code, the missing
skip_inline_frames call would break things.  Fortunately, that's an extremely
unlikely scenario.  But please do adjust the comment to mention that we
assume the code the single-step takes us to after single-stepping is also
not inlined.

Also, let's not trust prev_pc unless we did finish a single-step:

      if (!stopped_at_non_inline_function (aspace, stop_pc)
          && !(ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
               && ecs->event_thread->control.trap_expected
               && stopped_at_non_inline_function (aspace,
                                                  ecs->event_thread->prev_pc))

Please drop the "stopped" from the function's name.  The prev_pc case
clearly isn't about having stopped at prev_pc.  Something like
pc_at_non_inline_function would be better.  It is just concerned with
returning whether its passed in PC is known to point at a non-inlined
location, whether the thread is currently stopped there or not.

Okay with these changes.

To make this generic for all breakpoints/stops, what I have in mind would be:

 - at breakpoint creation or re-set time, check if the locations we've created
   point at inlined code, and set a flag in the breakpoint's locations.
   We know the location is inlined or not from the debug info.  Breakpoint
   creation is the slow path, so that's okay.

 - given that we need to single-step over those breakpoints, we also need
   to know whether the PC after stepping over those breakpoints points at
   inlined code.  I think we can still do that at breakpoint creation or
   re-set time.  We'd need to reuse the software single-step machinery to
   know where the single-step would take us, and record somewhere
   that those locations point to inline code or not.  We'd also check this
   list in stopped_at_non_inline_function.  The software single-step machinery
   would need some cleaning up to make this possible.  It's interface,
   gdbarch_software_single_step, isn't fit for this.  The gdbarch hook should return
   a list of locations where to put the breakpoint, instead of implementations
   planting the breakpoints themselves, which would be a nice cleanup for other
   things too.  We'd also need to implement this hook for x86.  It's not
   implemented currently because x86 can do hardware single-stepping.

Thanks.

-- 
Pedro Alves