From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27097 invoked by alias); 11 Oct 2011 16:53:59 -0000 Received: (qmail 27083 invoked by uid 22791); 11 Oct 2011 16:53:56 -0000 X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 tests=AWL,BAYES_50,SARE_SUB_IMPROVE X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 11 Oct 2011 16:53:31 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RDfa6-0000OH-N4 from pedro_alves@mentor.com ; Tue, 11 Oct 2011 09:53:30 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Tue, 11 Oct 2011 17:53:28 +0100 From: Pedro Alves To: Gary Benson Subject: Re: [RFA take 2] Improve performance with lots of shared libraries Date: Tue, 11 Oct 2011 16:53:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.1; x86_64; ; ) Cc: gdb-patches@sourceware.org References: <20110922171206.GB5874@redhat.com> <201110071601.57025.pedro@codesourcery.com> <20111011155300.GB3411@redhat.com> In-Reply-To: <20111011155300.GB3411@redhat.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201110111753.27321.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-10/txt/msg00312.txt.bz2 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 > > > > > > * 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