RFC: lazily call save_current_space_and_thread

RFC: lazily call save_current_space_and_thread

[Tom Tromey]

I would appreciate comments on this.

I have been working on making the "make check" multi-inferior scenario
work better in gdb.  The premise in this situation is that most
inferiors are not actually interesting, so we want to read debuginfo as
lazily as possible.

While debugging causes of debuginfo loading, I found that calling
save_current_space_and_thread is actually quite expensive.  This is
because it calls make_cleanup_restore_current_thread, which looks
up the selected frame, which can cause all kinds of work.

This patch changes the breakpoint insertion code to lazily call
save_current_space_and_thread.

Built and regtested on x86-64 (compile farm).


It has occurred to me that perhaps this is just a hack, and that a
better fix would be to somehow maintain the selected frame per-thread.
I'd appreciate comments on this as well.

A more general point here is that sometimes there are hints about future
directions of gdb -- in the comments, in bugzilla -- but it is hard to
evaluate whether those directions still make sense.  I think we'd all
benefit from more clarity about the design wish-list.  I hear complaints
about this quite a bit, and I think this lack of clarity makes potential
contributors more reluctant to attempt bigger projects; nobody wants to
invest a lot of time on an idea that might be rejected.

Tom

2011-03-04  Tom Tromey  <tromey@redhat.com>

	* breakpoint.c (insert_breakpoint_locations): Lazily call
	save_current_space_and_thread.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 623effa..5cf3fcd 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1842,13 +1842,18 @@ insert_breakpoint_locations (void)
 
   struct ui_file *tmp_error_stream = mem_fileopen ();
   struct cleanup *cleanups = make_cleanup_ui_file_delete (tmp_error_stream);
-  
+
+  /* We lazily save the current program space.  Saving the current
+     program space is reasonably expensive in the multi-inferior case,
+     and in many situations we don't need to do it.  Comparing a
+     cleanup against NULL is usually forbidden, but in this case we
+     have already made a cleanup, so we know it is safe.  */
+  struct cleanup *current_space_cleanup = NULL;
+
   /* Explicitly mark the warning -- this will only be printed if
      there was an error.  */
   fprintf_unfiltered (tmp_error_stream, "Warning:\n");
 
-  save_current_space_and_thread ();
-
   ALL_BP_LOCATIONS (bl, blp_tmp)
     {
       if (!should_be_inserted (bl) || bl->inserted)
@@ -1861,6 +1866,8 @@ insert_breakpoint_locations (void)
 	  && !valid_thread_id (bl->owner->thread))
 	continue;
 
+      if (current_space_cleanup == NULL)
+	current_space_cleanup = save_current_space_and_thread ();
       switch_to_program_space_and_thread (bl->pspace);
 
       /* For targets that support global breakpoints, there's no need

Re: RFC: lazily call save_current_space_and_thread

[Jan Kratochvil]

On Fri, 04 Mar 2011 20:52:41 +0100, Tom Tromey wrote:
> +  /* We lazily save the current program space.  Saving the current
> +     program space is reasonably expensive in the multi-inferior case,
> +     and in many situations we don't need to do it.  Comparing a
> +     cleanup against NULL is usually forbidden, but in this case we
> +     have already made a cleanup, so we know it is safe.  */
> +  struct cleanup *current_space_cleanup = NULL;
> +
[...]
> +      if (current_space_cleanup == NULL)
> +	current_space_cleanup = save_current_space_and_thread ();

save_current_space_and_thread - like any make_cleanup derivation - may return
NULL even for successfully pushed cleanup function.  For example inside
TRY_CATCH, due to its save_cleanups.

GDB does need more C++.


Thanks,
Jan

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

>> +      if (current_space_cleanup == NULL)
>> +	current_space_cleanup = save_current_space_and_thread ();

Jan> save_current_space_and_thread - like any make_cleanup derivation -
Jan> may return NULL even for successfully pushed cleanup function.  For
Jan> example inside TRY_CATCH, due to its save_cleanups.

In this case I still think it is safe -- but it is silly to rely on that
when a flag is just as easy.

I think I can actually make make_cleanup_restore_current_thread more
efficient.  But if not, I will reimplement this with a flag.

Tom

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

Tom> I think I can actually make make_cleanup_restore_current_thread more
Tom> efficient.  But if not, I will reimplement this with a flag.

This turned out to be pretty easy; I'm not sure why I didn't do it
before.

This patch makes make_cleanup_restore_current_thread more efficient, by
having it not compute the current frame when there is no selected frame.
This approach let me remove another hack I had in my tree.

Built and regtested on x86-64 (compile farm).

Let me know what you think.

Tom

2011-03-08  Tom Tromey  <tromey@redhat.com>

	* thread.c (restore_selected_frame): Handle frame_level == -1.
	(make_cleanup_restore_current_thread): Use
	get_selected_frame_if_set.
	* frame.h (get_selected_frame_if_set): Declare.
	* frame.c (get_selected_frame_if_set): New function.

diff --git a/gdb/frame.c b/gdb/frame.c
index 36fcefe..8ed20b1 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1247,6 +1247,16 @@ get_selected_frame (const char *message)
   return selected_frame;
 }
 
+/* If there is a selected frame, return it.  Otherwise, return NULL.  */
+
+struct frame_info *
+get_selected_frame_if_set (void)
+{
+  if (selected_frame == NULL)
+    return NULL;
+  return get_selected_frame (NULL);
+}
+
 /* This is a variant of get_selected_frame() which can be called when
    the inferior does not have a frame; in that case it will return
    NULL instead of calling error().  */
diff --git a/gdb/frame.h b/gdb/frame.h
index 2c5276e..252b75e 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -260,6 +260,9 @@ extern void reinit_frame_cache (void);
    and then return that thread's previously selected frame.  */
 extern struct frame_info *get_selected_frame (const char *message);
 
+/* If there is a selected frame, return it.  Otherwise, return NULL.  */
+extern struct frame_info *get_selected_frame_if_set (void);
+
 /* Select a specific frame.  NULL, apparently implies re-select the
    inner most frame.  */
 extern void select_frame (struct frame_info *);
diff --git a/gdb/thread.c b/gdb/thread.c
index 7d8f6da..f7eccfe 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1019,6 +1019,13 @@ restore_selected_frame (struct frame_id a_frame_id, int frame_level)
   struct frame_info *frame = NULL;
   int count;
 
+  /* This means there was no selected frame.  */
+  if (frame_level == -1)
+    {
+      select_frame (NULL);
+      return;
+    }
+
   gdb_assert (frame_level >= 0);
 
   /* Restore by level first, check if the frame id is the same as
@@ -1137,7 +1144,13 @@ make_cleanup_restore_current_thread (void)
 	  && target_has_registers
 	  && target_has_stack
 	  && target_has_memory)
-	frame = get_selected_frame (NULL);
+	{
+	  /* If we naively call get_selected_frame here, then we can
+	     end up reading debuginfo for the current frame, even
+	     though the user has not selected a frame and we don't
+	     actually need the debuginfo at this point.  */
+	  frame = get_selected_frame_if_set ();
+	}
       else
 	frame = NULL;
 

Re: RFC: lazily call save_current_space_and_thread

[Jan Kratochvil]

On Tue, 08 Mar 2011 17:51:56 +0100, Tom Tromey wrote:
> +/* If there is a selected frame, return it.  Otherwise, return NULL.  */
> +
> +struct frame_info *
> +get_selected_frame_if_set (void)
> +{
> +  if (selected_frame == NULL)
> +    return NULL;
> +  return get_selected_frame (NULL);
> +}

Maybe it could be just:
{
  return selected_frame;
}

It seems safe to me.


Thanks,
Jan

Re: RFC: lazily call save_current_space_and_thread

[Pedro Alves]

On Tuesday 08 March 2011 16:51:56, Tom Tromey wrote:
> +         /* If we naively call get_selected_frame here, then we can
> +            end up reading debuginfo for the current frame, even
> +            though the user has not selected a frame and we don't
> +            actually need the debuginfo at this point.  */
> +         frame = get_selected_frame_if_set ();

The comment doesn't seem to make much sense as is.  I think
that when the user has the prompt, selected_frame is not going
to be NULL, as normal_stop does:

  select_frame (get_current_frame ());

I think the case at hand is while we're handling internal
events, before giving the prompt to the user.  Is that so?

-- 
Pedro Alves

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> I think the case at hand is while we're handling internal
Pedro> events, before giving the prompt to the user.  Is that so?

Yes, that is what is happening.
I will try to reword the comment to be more clear.

Tom

Re: RFC: lazily call save_current_space_and_thread

[Pedro Alves]

On Tuesday 08 March 2011 17:20:43, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> I think the case at hand is while we're handling internal
> Pedro> events, before giving the prompt to the user.  Is that so?
> 
> Yes, that is what is happening.
> I will try to reword the comment to be more clear.

I'm guessing you're trying all-stop.  In non-stop, I _think_
you'll much more rarely see a NULL selected frame here.

-- 
Pedro Alves

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

Pedro> I'm guessing you're trying all-stop.  In non-stop, I _think_
Pedro> you'll much more rarely see a NULL selected frame here.

Nope, I do this before my test:

  set target-async on
  set schedule-multiple on
  set pagination off
  set detach-on-fork off
  set non-stop on

I can't say that I understand everything that happens in this mode.

"run" appears to work in the background, which is mysterious to me.

And, if I C-c gdb, I just get "Quit." back, and then gdb is in a weird
state (I can't "continue" any inferior and if I try "run" I get an
internal error).

I am planning to debug these next; but if you know what is going on
already...

Tom

Re: RFC: lazily call save_current_space_and_thread

[Pedro Alves]

On Tuesday 08 March 2011 17:49:29, Tom Tromey wrote:
> Pedro> I'm guessing you're trying all-stop.  In non-stop, I _think_
> Pedro> you'll much more rarely see a NULL selected frame here.
> 
> Nope, I do this before my test:
> 
>   set target-async on
>   set schedule-multiple on
>   set pagination off
>   set detach-on-fork off
>   set non-stop on


Okay.  In any case, what I was aluding to, is that
in non-stop mode gdb is making sure the user selected
frame is constant between events (fetch_inferior_event),
so in that case, there's a selected frame to restore back
to.  But thinking a bit more, that obviously doesn't matter
because if the user had a frame selected, then the debug info
for that frame has already been read anyway, and there's nothing
to optimize.

> I can't say that I understand everything that happens in this mode.
> 
> "run" appears to work in the background, which is mysterious to me.

Hmm.  It shouldn't.  Well, behind the scenes it does run
asynchronously, but only "run&" should work in the background,
from the user's perpective.

Boo, something's seriously broken :-/ :

[Thread debugging using libthread_db enabled]
process 2857 is executing new program: /usr/bin/iconv
Error in re-setting breakpoint 1: Cannot access memory at address 0x45e22a
Error in re-setting breakpoint 2: Cannot access memory at address 0x4e2e5a
warning: Error removing breakpoint 1
warning: Error removing breakpoint 2
warning: Error removing breakpoint 1
warning: Error removing breakpoint 2
warning: Error removing breakpoint 1
warning: Error removing breakpoint 2
warning: Error removing breakpoint 1
warning: Error removing breakpoint 2
warning: Error removing breakpoint 1
...

I suspect it's related to spawning iconv (or rather, that's
triggering some bug).

> 
> And, if I C-c gdb, I just get "Quit." back,

That just means you have the prompt, and the inferior is either
backgrounded, or not running at all.  To interrupt the target
in async/non-stop modes, we use "interrupt", not C-c.

> and then gdb is in a weird
> state (I can't "continue" any inferior and if I try "run" I get an
> internal error).
> 
> I am planning to debug these next; but if you know what is going on
> already...

Can't say I do.

-- 
Pedro Alves

Re: RFC: lazily call save_current_space_and_thread

[Pedro Alves]

On Tuesday 08 March 2011 18:02:35, Pedro Alves wrote:
> I suspect it's related to spawning iconv (or rather, that's
> triggering some bug).

Err, I must have blown a fuse or something.  That example
was me running gdb under gdb, and doing "run", with the
options you posted.  Naturaly, the inferior gdb is spawning
iconv, and the superior gdb catches that.  That is triggering
some bug, which may not even be what you're seeing, but
it's most likely irrevelant _what_ does the inferior spawn,
as long as it spawns something.  Errors accessing memory
like in my previous post are usually caused by gdb
reading/writing to/from the wrong process..

-- 
Pedro Alves

Re: RFC: lazily call save_current_space_and_thread

[Pedro Alves]

On Tuesday 08 March 2011 19:49:57, Tom Tromey wrote:
> Pedro> I think the case at hand is while we're handling internal
> Pedro> events, before giving the prompt to the user.  Is that so?
> 
> Tom> Yes, that is what is happening.
> Tom> I will try to reword the comment to be more clear.
> 
> What do you think of this?
> 
> 	  /* When processing internal events, there might not be a
> 	     selected frame.  If we naively call get_selected_frame
> 	     here, then we can end up reading debuginfo for the
> 	     current frame, but we don't generally need the debuginfo
> 	     at this point.  */

Perfect.  Thanks.

-- 
Pedro Alves

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

Pedro> I think the case at hand is while we're handling internal
Pedro> events, before giving the prompt to the user.  Is that so?

Tom> Yes, that is what is happening.
Tom> I will try to reword the comment to be more clear.

What do you think of this?

	  /* When processing internal events, there might not be a
	     selected frame.  If we naively call get_selected_frame
	     here, then we can end up reading debuginfo for the
	     current frame, but we don't generally need the debuginfo
	     at this point.  */

Tom

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro> Okay.  In any case, what I was aluding to, is that
Pedro> in non-stop mode gdb is making sure the user selected
Pedro> frame is constant between events (fetch_inferior_event),
Pedro> so in that case, there's a selected frame to restore back
Pedro> to.  But thinking a bit more, that obviously doesn't matter
Pedro> because if the user had a frame selected, then the debug info
Pedro> for that frame has already been read anyway, and there's nothing
Pedro> to optimize.

Also, this code will still do the right thing if there actually is a
selected frame.

Moving the selected frame to be per-thread would also help here.
But I am going to guess that this is tricky, for the same reasons that
making the regcache per-thread is tricky.

Pedro> Error in re-setting breakpoint 1: Cannot access memory at address 0x45e22a
Pedro> Error in re-setting breakpoint 2: Cannot access memory at address 0x4e2e5a
Pedro> warning: Error removing breakpoint 1
Pedro> warning: Error removing breakpoint 2

I didn't run into this, but I have seen other weird things, like errors
from libpthread_db.  I haven't investigated yet.

Tom

Re: RFC: lazily call save_current_space_and_thread

[Pedro Alves]

On Tuesday 08 March 2011 19:54:31, Tom Tromey wrote:
> Also, this code will still do the right thing if there actually is a
> selected frame.

Yeah, no question.  I was just wondering whether in non-stop
this particular optimization would be enough or if we
had to do something else at a higher layer, but it turns
out it is.  Thanks, BTW.

-- 
Pedro Alves

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This patch makes make_cleanup_restore_current_thread more efficient, by
Tom> having it not compute the current frame when there is no selected frame.
Tom> This approach let me remove another hack I had in my tree.

I am checking in this revised version.  I made the changes Jan and Pedro
suggested.

Built and regtested on x86-64 (compile farm).

Tom

2011-03-09  Tom Tromey  <tromey@redhat.com>

	* thread.c (restore_selected_frame): Handle frame_level == -1.
	(make_cleanup_restore_current_thread): Use
	get_selected_frame_if_set.
	* frame.h (get_selected_frame_if_set): Declare.
	* frame.c (get_selected_frame_if_set): New function.

Index: frame.c
===================================================================
RCS file: /cvs/src/src/gdb/frame.c,v
retrieving revision 1.291
diff -u -r1.291 frame.c
--- frame.c	7 Jan 2011 19:36:17 -0000	1.291
+++ frame.c	9 Mar 2011 14:21:33 -0000
@@ -1247,6 +1247,14 @@
   return selected_frame;
 }
 
+/* If there is a selected frame, return it.  Otherwise, return NULL.  */
+
+struct frame_info *
+get_selected_frame_if_set (void)
+{
+  return selected_frame;
+}
+
 /* This is a variant of get_selected_frame() which can be called when
    the inferior does not have a frame; in that case it will return
    NULL instead of calling error().  */
Index: frame.h
===================================================================
RCS file: /cvs/src/src/gdb/frame.h,v
retrieving revision 1.188
diff -u -r1.188 frame.h
--- frame.h	27 Feb 2011 16:25:37 -0000	1.188
+++ frame.h	9 Mar 2011 14:21:33 -0000
@@ -260,6 +260,9 @@
    and then return that thread's previously selected frame.  */
 extern struct frame_info *get_selected_frame (const char *message);
 
+/* If there is a selected frame, return it.  Otherwise, return NULL.  */
+extern struct frame_info *get_selected_frame_if_set (void);
+
 /* Select a specific frame.  NULL, apparently implies re-select the
    inner most frame.  */
 extern void select_frame (struct frame_info *);
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.136
diff -u -r1.136 thread.c
--- thread.c	21 Feb 2011 23:40:46 -0000	1.136
+++ thread.c	9 Mar 2011 14:21:33 -0000
@@ -1019,6 +1019,13 @@
   struct frame_info *frame = NULL;
   int count;
 
+  /* This means there was no selected frame.  */
+  if (frame_level == -1)
+    {
+      select_frame (NULL);
+      return;
+    }
+
   gdb_assert (frame_level >= 0);
 
   /* Restore by level first, check if the frame id is the same as
@@ -1137,7 +1144,14 @@
 	  && target_has_registers
 	  && target_has_stack
 	  && target_has_memory)
-	frame = get_selected_frame (NULL);
+	{
+	  /* When processing internal events, there might not be a
+	     selected frame.  If we naively call get_selected_frame
+	     here, then we can end up reading debuginfo for the
+	     current frame, but we don't generally need the debuginfo
+	     at this point.  */
+	  frame = get_selected_frame_if_set ();
+	}
       else
 	frame = NULL;
 

Re: RFC: lazily call save_current_space_and_thread

[Tom Tromey]

Tom> "run" appears to work in the background, which is mysterious to me.

Pedro> Hmm.  It shouldn't.  Well, behind the scenes it does run
Pedro> asynchronously, but only "run&" should work in the background,
Pedro> from the user's perpective.

Pedro> Boo, something's seriously broken :-/ :

I looked into this.

What happens is that fetch_inferior_event calls normal_stop whenever a
subprocess exits.  E.g., I see this:

Starting program: /usr/bin/make clean
[...]
[New process 13249]
[...]
[Inferior 2 (process 13249) exited normally]

... at which point I get the (gdb) prompt.

The initial `make' process and the other things it invokes continue
running.  I thought this was due to non-stop, but disabling that didn't
seem to change anything.


What I expected to happen is that after my `run', I should not get a
prompt back until either (1) some event occurs (breakpoint, SEGV,
whatever), or (2) the inferior and all its subprocesses have exited.

This would require some extra logic when handling exit events, since
AFAICT there is nothing to support this in there right now.

I would appreciate your comments on this.


Once I stop the inferiors, it is hard to restart them nicely.  I guess
this is where the I/T set idea comes in -- you need a way to be able to
say "continue all inferiors".

Tom