From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22682 invoked by alias); 12 Oct 2011 15:59:48 -0000 Received: (qmail 22667 invoked by uid 22791); 12 Oct 2011 15:59:46 -0000 X-SWARE-Spam-Status: No, hits=0.4 required=5.0 tests=AWL,BAYES_50,SARE_SUB_IMPROVE,SPF_FAIL X-Spam-Check-By: sourceware.org Received: from gbenson.demon.co.uk (HELO gbenson.demon.co.uk) (80.177.220.214) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 12 Oct 2011 15:59:21 +0000 Date: Wed, 12 Oct 2011 15:59:00 -0000 From: Gary Benson To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: [commit] Improve performance with lots of shared libraries Message-ID: <20111012155918.GA4216@redhat.com> Mail-Followup-To: Pedro Alves , gdb-patches@sourceware.org References: <20110922171206.GB5874@redhat.com> <201110071601.57025.pedro@codesourcery.com> <20111011155300.GB3411@redhat.com> <201110111753.27321.pedro@codesourcery.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="45Z9DzgjV8m4Oswq" Content-Disposition: inline In-Reply-To: <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/msg00361.txt.bz2 --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 2989 Pedro Alves wrote: > 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. Thanks, I made them and committed it (patch attached). > 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. Ah, nice! Would it be appropriate to file a bug containing this information? So it doesn't get lost before I have a chance to work on it? Thanks, Gary -- http://gbenson.net/ --45Z9DzgjV8m4Oswq Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=patch Content-length: 4827 Index: gdb/ChangeLog =================================================================== RCS file: /cvs/src/src/gdb/ChangeLog,v retrieving revision 1.13424 retrieving revision 1.13425 diff -u -r1.13424 -r1.13425 --- gdb/ChangeLog 12 Oct 2011 12:17:29 -0000 1.13424 +++ gdb/ChangeLog 12 Oct 2011 15:43:47 -0000 1.13425 @@ -1,3 +1,11 @@ +2011-10-12 Gary Benson + + * breakpoint.h (pc_at_non_inline_function): Declare. + * breakpoint.c (is_non_inline_function, + pc_at_non_inline_function): New functions. + * infrun.c (handle_inferior_event): Don't call skip_inline_frames + if the stop is at a location where functions cannot be inlined. + 2011-10-12 Pedro Alves * linux-nat.c (stop_and_resume_callback): Don't re-resume LWPs if Index: gdb/breakpoint.c =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.c,v retrieving revision 1.615 retrieving revision 1.616 diff -u -r1.615 -r1.616 --- gdb/breakpoint.c 2 Oct 2011 02:13:12 -0000 1.615 +++ gdb/breakpoint.c 12 Oct 2011 15:43:48 -0000 1.616 @@ -13325,6 +13325,45 @@ return NULL; } +/* Zero if any of the breakpoint's locations could be a location where + functions have been inlined, nonzero otherwise. */ + +static int +is_non_inline_function (struct breakpoint *b) +{ + /* The shared library event breakpoint is set on the address of a + non-inline function. */ + if (b->type == bp_shlib_event) + return 1; + + return 0; +} + +/* Nonzero if the specified PC cannot be a location where functions + have been inlined. */ + +int +pc_at_non_inline_function (struct address_space *aspace, CORE_ADDR pc) +{ + struct breakpoint *b; + struct bp_location *bl; + + ALL_BREAKPOINTS (b) + { + if (!is_non_inline_function (b)) + continue; + + for (bl = b->loc; bl != NULL; bl = bl->next) + { + if (!bl->shlib_disabled + && bpstat_check_location (bl, aspace, pc)) + return 1; + } + } + + return 0; +} + void initialize_breakpoint_ops (void) { Index: gdb/breakpoint.h =================================================================== RCS file: /cvs/src/src/gdb/breakpoint.h,v retrieving revision 1.160 retrieving revision 1.161 diff -u -r1.160 -r1.161 --- gdb/breakpoint.h 26 Aug 2011 09:28:27 -0000 1.160 +++ gdb/breakpoint.h 12 Oct 2011 15:43:49 -0000 1.161 @@ -1357,6 +1357,12 @@ extern struct breakpoint *iterate_over_breakpoints (int (*) (struct breakpoint *, void *), void *); +/* Nonzero if the specified PC cannot be a location where functions + have been inlined. */ + +extern int pc_at_non_inline_function (struct address_space *aspace, + CORE_ADDR pc); + extern int user_breakpoint_p (struct breakpoint *); #endif /* !defined (BREAKPOINT_H) */ Index: gdb/infrun.c =================================================================== RCS file: /cvs/src/src/gdb/infrun.c,v retrieving revision 1.508 retrieving revision 1.509 diff -u -r1.508 -r1.509 --- gdb/infrun.c 7 Oct 2011 12:06:46 -0000 1.508 +++ gdb/infrun.c 12 Oct 2011 15:43:49 -0000 1.509 @@ -4044,7 +4044,32 @@ nexti. After stepi and nexti, always show the innermost frame (not any inline function call sites). */ 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 off a breakpoint prior to reinstating it. + Note that we're assuming that the code we single-step to is + not inline, but that's not definitive: there's nothing + preventing the event breakpoint function from containing + 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. */ + if (!pc_at_non_inline_function (aspace, stop_pc) + && !(ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP + && ecs->event_thread->control.trap_expected + && pc_at_non_inline_function (aspace, + ecs->event_thread->prev_pc))) + skip_inline_frames (ecs->ptid); + } if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP && ecs->event_thread->control.trap_expected --45Z9DzgjV8m4Oswq--