RFC: lazily call save_current_space_and_thread

[Tom Tromey <tromey at redhat dot com>]

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