[commit] Improve performance with lots of shared libraries

[Gary Benson <gbenson@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

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/

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 4827 bytes --]

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  <gbenson@redhat.com>
+
+	* 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  <pedro@codesourcery.com>
 
 	* 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