This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA take 2] Improve performance with lots of shared libraries
- From: Pedro Alves <pedro at codesourcery dot com>
- To: Gary Benson <gbenson at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 11 Oct 2011 17:53:27 +0100
- Subject: Re: [RFA take 2] Improve performance with lots of shared libraries
- References: <20110922171206.GB5874@redhat.com> <201110071601.57025.pedro@codesourcery.com> <20111011155300.GB3411@redhat.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