[RFA] Improve performance with lots of shared libraries

[RFA] Improve performance with lots of shared libraries

[Gary Benson]

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

Hi all,

Back in July I submitted a patch[1] that improved performance when
debugging inferiors that load lots of shared libraries.  It hinged on
avoiding calling find_pc_partial_function and skip_inline_frames in
handle_inferior_event when the event being handled was a stop at the
solib event breakpoint.

That patch was kind of hacky in that it relied on second-guessing some
of the logic that followed.  I started working on splitting the patch,
and committed a patch to make find_pc_partial_function lazy[2] in July.

Attached is a patch to make gdb avoid the call to skip_inline_frames.
With a small benchmark I put together (a little program that loads 1000
shared libraries) gdb ran the application in 36s with the patch
against 46s without.

This patch is superficially similar to the original patch, but much
simpler and different in scope.  The initial patch worked on the
premise that if the stop was specifially a stop for the solib event
breakpoint then the calls could be skipped (because I'd walked through
the code ahead and seen that their results were not used).  The result
was a fairly complex conditional that would have needed to be kept
updated if other parts of handle_inferior_event changed.

This patch works on the simpler premise that the solib event
breakpoint is by definition the address of a function, ie not inline,
so regardless of why the stop occurred the call to skip_inline_frames
can be omitted because there are no inline frames to skip.  The
heuristic is therefore simpler and very much less fragile.

I have tried a two of other approaches to this, but neither worked.
My preferred method was to make skip_inline_frames lazy, but this
caused problems because the skipping, when it occurred, invalidated
the frame cache.   handle_inferior_event has several places where
local "frame" variables are recreated because some call or another
has invalidated the frame cache and left the pointer dangling, and
making skip_inline_frames added many more.  It was looking like a
maintainence nightmaire long before I even got it to work.

The other approach I tried was to use bpstat_stop_status to
identify the solib event breakpoint, rather than writing my own
code to do it.  This looked really clean, but failed a huge number
of tests.  Apparently bpstat_stop_status has side effects!

How is this patch do you think?  Is it ok to commit?

Cheers,
Gary

[1] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00026.html
[2] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html

-- 
http://gbenson.net/

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

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.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 225034c..2e49470 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch,
     }
 }
 
+/* Helper for stopped_at_solib_event_breakpoint.  */
+
+static int
+stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg)
+{
+  struct execution_control_state *ecs
+    = (struct execution_control_state *) arg;
+
+  if (b->type == bp_shlib_event)
+    {
+      CORE_ADDR prev_pc = ecs->event_thread->prev_pc;
+      struct bp_location *loc;
+
+      for (loc = b->loc; loc; loc = loc->next)
+	{
+	  if (loc->pspace == current_program_space
+	      && (loc->address == stop_pc || loc->address == prev_pc))
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
+/* Nonzero if we are stopped at the solib event breakpoint.  */
+
+static int
+stopped_at_solib_event_breakpoint (struct execution_control_state *ecs)
+{
+  return iterate_over_breakpoints (stopped_at_solib_event_breakpoint_helper,
+				   ecs) != NULL;
+}
+
 /* Given an execution control state that has been freshly filled in
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
@@ -4010,9 +4043,14 @@ handle_inferior_event (struct execution_control_state *ecs)
   stopped_by_random_signal = 0;
 
   /* Hide inlined functions starting here, unless we just performed stepi or
-     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)
+     nexti, or we are at the solib event breakpoint.  After stepi and nexti,
+     always show the innermost frame (not any inline function call sites).
+     This call is expensive, and we avoid it if we are at the solib event
+     breakpoint which is defined as the address of a function (i.e., not
+     inline).  This improves performance with inferiors that load a lot of
+     shared libraries.  */
+  if (ecs->event_thread->control.step_range_end != 1
+      && !stopped_at_solib_event_breakpoint (ecs))
     skip_inline_frames (ecs->ptid);
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP

PING: [RFA] Improve performance with lots of shared libraries

[Gary Benson]

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

Hi all,

Back in July I submitted a patch[1] that improved performance when
debugging inferiors that load lots of shared libraries.  It hinged on
avoiding calling find_pc_partial_function and skip_inline_frames in
handle_inferior_event when the event being handled was a stop at the
solib event breakpoint.

That patch was kind of hacky in that it relied on second-guessing some
of the logic that followed.  I started working on splitting the patch,
and committed a patch to make find_pc_partial_function lazy[2] in July.

Attached is a patch to make gdb avoid the call to skip_inline_frames.
With a small benchmark I put together (a little program that loads 1000
shared libraries) gdb ran the application in 36s with the patch
against 46s without.

This patch is superficially similar to the original patch, but much
simpler and different in scope.  The initial patch worked on the
premise that if the stop was specifially a stop for the solib event
breakpoint then the calls could be skipped (because I'd walked through
the code ahead and seen that their results were not used).  The result
was a fairly complex conditional that would have needed to be kept
updated if other parts of handle_inferior_event changed.

This patch works on the simpler premise that the solib event
breakpoint is by definition the address of a function, ie not inline,
so regardless of why the stop occurred the call to skip_inline_frames
can be omitted because there are no inline frames to skip.  The
heuristic is therefore simpler and very much less fragile.

I have tried a two of other approaches to this, but neither worked.
My preferred method was to make skip_inline_frames lazy, but this
caused problems because the skipping, when it occurred, invalidated
the frame cache.   handle_inferior_event has several places where
local "frame" variables are recreated because some call or another
has invalidated the frame cache and left the pointer dangling, and
making skip_inline_frames added many more.  It was looking like a
maintainence nightmaire long before I even got it to work.

The other approach I tried was to use bpstat_stop_status to
identify the solib event breakpoint, rather than writing my own
code to do it.  This looked really clean, but failed a huge number
of tests.  Apparently bpstat_stop_status has side effects!

How is this patch do you think?  Is it ok to commit?

Cheers,
Gary

[1] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00026.html
[2] http://www.cygwin.com/ml/gdb-patches/2011-07/msg00460.html

-- 
http://gbenson.net/

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

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.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 225034c..2e49470 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch,
     }
 }
 
+/* Helper for stopped_at_solib_event_breakpoint.  */
+
+static int
+stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg)
+{
+  struct execution_control_state *ecs
+    = (struct execution_control_state *) arg;
+
+  if (b->type == bp_shlib_event)
+    {
+      CORE_ADDR prev_pc = ecs->event_thread->prev_pc;
+      struct bp_location *loc;
+
+      for (loc = b->loc; loc; loc = loc->next)
+	{
+	  if (loc->pspace == current_program_space
+	      && (loc->address == stop_pc || loc->address == prev_pc))
+	    return 1;
+	}
+    }
+
+  return 0;
+}
+
+/* Nonzero if we are stopped at the solib event breakpoint.  */
+
+static int
+stopped_at_solib_event_breakpoint (struct execution_control_state *ecs)
+{
+  return iterate_over_breakpoints (stopped_at_solib_event_breakpoint_helper,
+				   ecs) != NULL;
+}
+
 /* Given an execution control state that has been freshly filled in
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
@@ -4010,9 +4043,14 @@ handle_inferior_event (struct execution_control_state *ecs)
   stopped_by_random_signal = 0;
 
   /* Hide inlined functions starting here, unless we just performed stepi or
-     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)
+     nexti, or we are at the solib event breakpoint.  After stepi and nexti,
+     always show the innermost frame (not any inline function call sites).
+     This call is expensive, and we avoid it if we are at the solib event
+     breakpoint which is defined as the address of a function (i.e., not
+     inline).  This improves performance with inferiors that load a lot of
+     shared libraries.  */
+  if (ecs->event_thread->control.step_range_end != 1
+      && !stopped_at_solib_event_breakpoint (ecs))
     skip_inline_frames (ecs->ptid);
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP

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

[Tom Tromey]

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> How is this patch do you think?  Is it ok to commit?

Gary> 2011-09-22  Gary Benson  <gbenson@redhat.com>
Gary> 	* infrun.c (stopped_at_solib_event_breakpoint): New function.
Gary> 	(stopped_at_solib_event_breakpoint_helper): Likewise.
Gary> 	(handle_inferior_event): Avoid calling skip_inline_frames
Gary> 	when at the solib event breakpoint.

It looks reasonable to me, but I would much prefer a review from Pedro.

Tom

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

[Pedro Alves]

Hi Gary,

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.
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 225034c..2e49470 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch,
>      }
>  }
>  
> +/* Helper for stopped_at_solib_event_breakpoint.  */
> +
> +static int
> +stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg)

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.

> +{
> +  struct execution_control_state *ecs
> +    = (struct execution_control_state *) arg;
> +

You need to check that the breakpoint is enabled and installed.

> +  if (b->type == bp_shlib_event)
> +    {
> +      CORE_ADDR prev_pc = ecs->event_thread->prev_pc;
> +      struct bp_location *loc;
> +
> +      for (loc = b->loc; loc; loc = loc->next)
> +       {
> +         if (loc->pspace == current_program_space
> +             && (loc->address == stop_pc || loc->address == prev_pc))

I don't follow the prev_pc check.  What does that intend to do?

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.

-- 
Pedro Alves

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

[Gary Benson]

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.
> > 
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 225034c..2e49470 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -3102,6 +3102,39 @@ fill_in_stop_func (struct gdbarch *gdbarch,
> >      }
> >  }
> >  
> > +/* Helper for stopped_at_solib_event_breakpoint.  */
> > +
> > +static int
> > +stopped_at_solib_event_breakpoint_helper (struct breakpoint *b, void *arg)
> 

> 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.

Are you talking about a function like this:

  int non_inline_function (struct breakpoint *b)
  {
    return b->type == bp_shlib_event;
  }

or something more involved?

> 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.

Ok.

> > +{
> > +  struct execution_control_state *ecs
> > +    = (struct execution_control_state *) arg;
> > +
> 
> You need to check that the breakpoint is enabled and installed.

Does it matter if it isn't?  My thinking was that we are trying to
state that there cannot be inlined functions at the stop address, and
the existence of the shlib event breakpoint there allows you to state
that whether it is enabled or not.

(Am I correct in thinking "installed" means "not-pending"?)

> > +  if (b->type == bp_shlib_event)
> > +    {
> > +      CORE_ADDR prev_pc = ecs->event_thread->prev_pc;
> > +      struct bp_location *loc;
> > +
> > +      for (loc = b->loc; loc; loc = loc->next)
> > +       {
> > +         if (loc->pspace == current_program_space
> > +             && (loc->address == stop_pc || loc->address == prev_pc))
> 
> I don't follow the prev_pc check.  What does that intend to do?

You end up in handle_inferior_event twice for every breakpoint, once
when you hit it and once when you've single-stepped over it.  The
prev_pc check catches the latter.

> 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.

Ok, I will look into this.

Thank you,
Gary

-- 
http://gbenson.net/

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

[Gary Benson]

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

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?

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.

Thanks,
Gary

-- 
http://gbenson.net/

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

2011-10-11  Gary Benson  <gbenson@redhat.com>

	* breakpoint.h (stopped_at_non_inline_function): Declare.
	* breakpoint.c (is_non_inline_function,
	stopped_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.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 08ff69b..5782d3d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -13325,6 +13325,45 @@ iterate_over_breakpoints (int (*callback) (struct breakpoint *, void *),
   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
+stopped_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)
 {
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 5e5d1b9..a4e5a4f 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1357,6 +1357,12 @@ extern void end_rbreak_breakpoints (void);
 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 stopped_at_non_inline_function (struct address_space *aspace,
+					   CORE_ADDR pc);
+			 
 extern int user_breakpoint_p (struct breakpoint *);
 
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/infrun.c b/gdb/infrun.c
index cc2e29b..11ba700 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4044,7 +4044,23 @@ handle_inferior_event (struct execution_control_state *ecs)
      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 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);
+    }
 
   if (ecs->event_thread->suspend.stop_signal == TARGET_SIGNAL_TRAP
       && ecs->event_thread->control.trap_expected

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

[Pedro Alves]

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

[commit] Improve performance with lots of shared libraries

[Gary Benson]

[-- 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

Re: [commit] Improve performance with lots of shared libraries

[Pedro Alves]

On Wednesday 12 October 2011 16:59:18, Gary Benson wrote:
> Pedro Alves wrote:

> > 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?

Sure!  What I haven't thought about much is whether this
optimization would be indeed a general win.  :-)  It'd make a
difference if you tend to have planted breakpoints that 
don't cause a stop often (e.g., some python breakpoint),
and maybe it'd make a difference on software single-step
targets, and a tiny bit on handling step-resume
breakpoints on hardware step targets?  I don't have a clear
picture where time is being spent (other than roundtripping
to the target).  Thread event breakpoints sound like low
hang fruit though.

-- 
Pedro Alves