[RFA] [1/4] Remove libgdb API (gdb_breakpoint_query)

[RFA] [1/4] Remove libgdb API (gdb_breakpoint_query)

[Keith Seitz]

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

Hi,

This is the first patch in a series of four which aims to remove the 
libgdb API (aka gdb.h) from gdb. As it is, there are only three 
functions defined in this API and just about all of it is only used by MI.

This patch replaces the function gdb_breakpoint_query with a function of 
similar name and functionality which can be used by MI. I've repeated 
this methodology for the remaining functions, too.

At the same time, it also necessitated/facilitated a related cleanup of 
various breakpoint.c functions which did something like:

void
foo (blah, blah, blah)
{
    struct ui_out *uiout = current_uiout;
    /* ... */

instead of simply passing in uiout as an argument.

No regressions on x86_64-linux.

Keith

ChangeLog
2012-01-12  Keith Seitz  <keiths@redhat.com>

	* breakpoint.c: Do not include gdb.h.
	(print_breakpoint_location): Add UIOUT parameter.
	(print_one_breakpoint_location): Likewise.
	(print_one_breakpoint): Likewise.
	(struct captured_breakpoint_query_args): Remove.
	(do_captured_breakpoint_query): Remove.
	(gdb_breakpoint_query): Remove.
	(breakpoint_query): New function based on above functions.
	(breakpoint_1): Pass uiout to print_one_breakpoint.
	(print_one_ranged_breakpoint): Likewise.
	* breakpoint.h (breakpoint_query): Declare.
	* gdb.h (gdb_breakpoint_query): Remove declaration.
	* mi/mi-cmd-break.c: Do not include gdb.h.
	(breakpoint_notify): Call breakpoint_query instead of
	gdb_breakpoint_query.
	* mi/mi-interp.c: Include breakpoint.h instead of gdb.h.
	(mi_breakpoint_created): Call breakpoint_query instead of
	gdb_breakpoint_query.
	(mi_breakpoint_modified): Likewise.

[-- Attachment #2: remove-gdb_breakpoint_query.patch --]
[-- Type: text/x-patch, Size: 8786 bytes --]

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 88fc176..f3c3cfd 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -44,7 +44,6 @@
 #include "source.h"
 #include "linespec.h"
 #include "completer.h"
-#include "gdb.h"
 #include "ui-out.h"
 #include "cli/cli-script.h"
 #include "gdb_assert.h"
@@ -4574,10 +4573,9 @@ wrap_indent_at_field (struct ui_out *uiout, const char *col_name)
 /* Print the LOC location out of the list of B->LOC locations.  */
 
 static void
-print_breakpoint_location (struct breakpoint *b,
+print_breakpoint_location (struct ui_out *uiout, struct breakpoint *b,
 			   struct bp_location *loc)
 {
-  struct ui_out *uiout = current_uiout;
   struct cleanup *old_chain = save_current_program_space ();
 
   if (loc != NULL && loc->shlib_disabled)
@@ -4684,10 +4682,11 @@ bptype_string (enum bptype type)
   return bptypes[(int) type].description;
 }
 
-/* Print B to gdb_stdout.  */
+/* Print B to UIOUT.  */
 
 static void
-print_one_breakpoint_location (struct breakpoint *b,
+print_one_breakpoint_location (struct ui_out *uiout,
+			       struct breakpoint *b,
 			       struct bp_location *loc,
 			       int loc_number,
 			       struct bp_location **last_loc,
@@ -4696,7 +4695,6 @@ print_one_breakpoint_location (struct breakpoint *b,
   struct command_line *l;
   static char bpenables[] = "nynny";
 
-  struct ui_out *uiout = current_uiout;
   int header_of_multiple = 0;
   int part_of_multiple = (loc != NULL);
   struct value_print_options opts;
@@ -4826,7 +4824,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 	  }
 	annotate_field (5);
 	if (!header_of_multiple)
-	  print_breakpoint_location (b, loc);
+	  print_breakpoint_location (uiout, b, loc);
 	if (b->loc)
 	  *last_loc = b->loc;
 	break;
@@ -4996,16 +4994,16 @@ print_one_breakpoint_location (struct breakpoint *b,
 }
 
 static void
-print_one_breakpoint (struct breakpoint *b,
+print_one_breakpoint (struct ui_out *uiout,
+		      struct breakpoint *b,
 		      struct bp_location **last_loc, 
 		      int allflag)
 {
   struct cleanup *bkpt_chain;
-  struct ui_out *uiout = current_uiout;
 
   bkpt_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "bkpt");
 
-  print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
+  print_one_breakpoint_location (uiout, b, NULL, 0, last_loc, allflag);
   do_cleanups (bkpt_chain);
 
   /* If this breakpoint has custom print function,
@@ -5031,13 +5029,27 @@ print_one_breakpoint (struct breakpoint *b,
 	    {
 	      struct cleanup *inner2 =
 		make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
-	      print_one_breakpoint_location (b, loc, n, last_loc, allflag);
+	      print_one_breakpoint_location (uiout, b, loc, n, last_loc,
+					     allflag);
 	      do_cleanups (inner2);
 	    }
 	}
     }
 }
 
+/* Print information about breakpoint B to UIOUT.  */
+
+void
+breakpoint_query (struct ui_out *uiout, struct breakpoint *b)
+{
+  if (b != NULL)
+    {
+      struct bp_location *dummy_loc = NULL;
+
+      print_one_breakpoint (uiout, b, &dummy_loc, 0);
+    }
+}
+
 static int
 breakpoint_address_bits (struct breakpoint *b)
 {
@@ -5061,45 +5073,6 @@ breakpoint_address_bits (struct breakpoint *b)
   return print_address_bits;
 }
 
-struct captured_breakpoint_query_args
-  {
-    int bnum;
-  };
-
-static int
-do_captured_breakpoint_query (struct ui_out *uiout, void *data)
-{
-  struct captured_breakpoint_query_args *args = data;
-  struct breakpoint *b;
-  struct bp_location *dummy_loc = NULL;
-
-  ALL_BREAKPOINTS (b)
-    {
-      if (args->bnum == b->number)
-	{
-	  print_one_breakpoint (b, &dummy_loc, 0);
-	  return GDB_RC_OK;
-	}
-    }
-  return GDB_RC_NONE;
-}
-
-enum gdb_rc
-gdb_breakpoint_query (struct ui_out *uiout, int bnum, 
-		      char **error_message)
-{
-  struct captured_breakpoint_query_args args;
-
-  args.bnum = bnum;
-  /* For the moment we don't trust print_one_breakpoint() to not throw
-     an error.  */
-  if (catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
-				 error_message, RETURN_MASK_ALL) < 0)
-    return GDB_RC_FAIL;
-  else
-    return GDB_RC_OK;
-}
-
 /* Return true if this breakpoint was set by the user, false if it is
    internal or momentary.  */
 
@@ -5236,7 +5209,7 @@ breakpoint_1 (char *args, int allflag,
       /* We only print out user settable breakpoints unless the
 	 allflag is set.  */
       if (allflag || user_breakpoint_p (b))
-	print_one_breakpoint (b, &last_loc, allflag);
+	print_one_breakpoint (uiout, b, &last_loc, allflag);
     }
 
   do_cleanups (bkpttbl_chain);
@@ -8212,7 +8185,7 @@ print_one_ranged_breakpoint (struct breakpoint *b,
        by print_one_detail_ranged_breakpoint.  */
     ui_out_field_skip (uiout, "addr");
   annotate_field (5);
-  print_breakpoint_location (b, bl);
+  print_breakpoint_location (uiout, b, bl);
   *last_loc = bl;
 }
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index c1d3be9..c9503da 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1379,4 +1379,5 @@ extern int user_breakpoint_p (struct breakpoint *);
 /* Attempt to determine architecture of location identified by SAL.  */
 extern struct gdbarch *get_sal_arch (struct symtab_and_line sal);
 
+extern void breakpoint_query (struct ui_out *uiout, struct breakpoint *b);
 #endif /* !defined (BREAKPOINT_H) */
diff --git a/gdb/gdb.h b/gdb/gdb.h
index b0ef8b9..dcd00c1 100644
--- a/gdb/gdb.h
+++ b/gdb/gdb.h
@@ -42,11 +42,6 @@ enum gdb_rc {
 };
 
 
-/* Print the specified breakpoint on GDB_STDOUT.  (Eventually this
-   function will ``print'' the object on ``output'').  */
-enum gdb_rc gdb_breakpoint_query (struct ui_out *uiout, int bnum,
-				  char **error_message);
-
 /* Switch thread and print notification.  */
 enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
 			       char **error_message);
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index 7c89a3f..76ea3b9 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -25,7 +25,6 @@
 #include "breakpoint.h"
 #include "gdb_string.h"
 #include "mi-getopt.h"
-#include "gdb.h"
 #include "exceptions.h"
 #include "observer.h"
 #include "mi-main.h"
@@ -48,8 +47,13 @@ static int mi_can_breakpoint_notify;
 static void
 breakpoint_notify (struct breakpoint *b)
 {
+  volatile struct gdb_exception except;
+
   if (mi_can_breakpoint_notify)
-    gdb_breakpoint_query (current_uiout, b->number, NULL);
+    {
+      TRY_CATCH (except, RETURN_MASK_ERROR)
+	breakpoint_query (current_uiout, b);
+    }
 }
 
 enum bp_type
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index cb22ce2..b1f580e 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -34,7 +34,7 @@
 #include "observer.h"
 #include "gdbthread.h"
 #include "solist.h"
-#include "gdb.h"
+#include "breakpoint.h"
 
 /* These are the interpreter setup, etc. functions for the MI interpreter */
 static void mi_execute_command_wrapper (char *cmd);
@@ -502,16 +502,16 @@ mi_breakpoint_created (struct breakpoint *b)
   target_terminal_ours ();
   fprintf_unfiltered (mi->event_channel,
 		      "breakpoint-created");
-  /* We want the output from gdb_breakpoint_query to go to
+  /* We want the output from breakpoint_query to go to
      mi->event_channel.  One approach would be to just
-     call gdb_breakpoint_query, and then use mi_out_put to
+     call breakpoint_query, and then use mi_out_put to
      send the current content of mi_outout into mi->event_channel.
      However, that will break if anything is output to mi_uiout
      prior the calling the breakpoint_created notifications.
      So, we use ui_out_redirect.  */
   ui_out_redirect (mi_uiout, mi->event_channel);
   TRY_CATCH (e, RETURN_MASK_ERROR)
-    gdb_breakpoint_query (mi_uiout, b->number, NULL);
+    breakpoint_query (mi_uiout, b);
   ui_out_redirect (mi_uiout, NULL);
 
   gdb_flush (mi->event_channel);
@@ -554,16 +554,16 @@ mi_breakpoint_modified (struct breakpoint *b)
   target_terminal_ours ();
   fprintf_unfiltered (mi->event_channel,
 		      "breakpoint-modified");
-  /* We want the output from gdb_breakpoint_query to go to
+  /* We want the output from breakpoint_query to go to
      mi->event_channel.  One approach would be to just
-     call gdb_breakpoint_query, and then use mi_out_put to
+     call breakpoint_query, and then use mi_out_put to
      send the current content of mi_outout into mi->event_channel.
      However, that will break if anything is output to mi_uiout
      prior the calling the breakpoint_created notifications.
      So, we use ui_out_redirect.  */
   ui_out_redirect (mi_uiout, mi->event_channel);
   TRY_CATCH (e, RETURN_MASK_ERROR)
-    gdb_breakpoint_query (mi_uiout, b->number, NULL);
+    breakpoint_query (mi_uiout, b);
   ui_out_redirect (mi_uiout, NULL);
 
   gdb_flush (mi->event_channel);

Re: [RFA] [1/4] Remove libgdb API (gdb_breakpoint_query)

[Jan Kratochvil]

Hi Keith,

On Fri, 13 Jan 2012 20:59:10 +0100, Keith Seitz wrote:
> This is the first patch in a series of four which aims to remove the
> libgdb API (aka gdb.h) from gdb.

looks great.


> At the same time, it also necessitated/facilitated a related cleanup
> of various breakpoint.c functions which did something like:
> 
> void
> foo (blah, blah, blah)
> {
>    struct ui_out *uiout = current_uiout;
>    /* ... */
> 
> instead of simply passing in uiout as an argument.

This is sure a great idea but it is done only a half way.

Former wrapper catch_exceptions_with_msg replaces CURRENT_UIOUT temporarily,
therefore it applies even to all the callees not patched in this patchset.

With this change those callees access unchanged CURRENT_UIOUT which is
a regression.

Maybe could you more thoroughly review all the uses of CURRENT_UIOUT?

Used this patch on top of your whole patchset and it causes regressions for:
	gdb.ada/mi_catch_ex.exp gdb.mi/mi-nonstop.exp gdb.mi/mi-nsmoribund.exp
	gdb.mi/mi-pthreads.exp gdb.mi/mi2-pthreads.exp
	gdb.python/py-evthreads.exp gdb.threads/hand-call-in-threads.exp
	gdb.threads/linux-dp.exp gdb.threads/no-unwaited-for-left.exp
	gdb.threads/tls.exp

Such as:

Program terminated with signal 11, Segmentation fault.
#0  0x00000000007bc12f in ui_out_is_mi_like_p (uiout=0x0) at ui-out.c:736
736	  return uiout->impl->is_mi_like_p;
(gdb) up
#1  0x00000000006fa93d in print_stack_frame (frame=0x41cfb80, print_level=1, print_what=SRC_AND_LOC) at stack.c:158
158	  if (ui_out_is_mi_like_p (current_uiout))
(gdb) bt
#0  in ui_out_is_mi_like_p (uiout=0x0) at ui-out.c:736
#1  in print_stack_frame (frame=0x41cfb80, print_level=1, print_what=SRC_AND_LOC) at stack.c:158
#2  in thread_select (uiout=0x3c47bd0, tidstr=0x407faa0 "5") at thread.c:1403
#3  in mi_cmd_thread_select (command=0x4231030 "thread-select", argv=0x41af000, argc=1) at ./mi/mi-main.c:470
#4  in mi_cmd_execute (parse=0x422c3c0) at ./mi/mi-main.c:2090
#5  in captured_mi_execute_command (uiout=0x3c47bd0, context=0x422c3c0) at ./mi/mi-main.c:1834
#6  in mi_execute_command (cmd=0x41d4f40 "-thread-select 5", from_tty=1) at ./mi/mi-main.c:1956
#7  in mi_execute_command_wrapper (cmd=0x41d4f40 "-thread-select 5") at ./mi/mi-interp.c:290
#8  in gdb_readline2 (client_data=0x0) at event-top.c:717
#9  in stdin_event_handler (error=0, client_data=0x0) at event-top.c:375
#10 in handle_file_event (data=...) at event-loop.c:827


Thanks,
Jan


--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5037,17 +5037,30 @@ print_one_breakpoint (struct ui_out *uiout,
     }
 }
 
+void
+restore_current_uiout (void *arg)
+{
+  gdb_assert (current_uiout == NULL);
+  current_uiout = arg;
+}
+
 /* Print information about breakpoint B to UIOUT.  */
 
 void
 breakpoint_query (struct ui_out *uiout, struct breakpoint *b)
 {
+extern void restore_current_uiout (void *arg);
+  struct cleanup *back_to = make_cleanup (restore_current_uiout, current_uiout);
+  current_uiout = NULL;
+
   if (b != NULL)
     {
       struct bp_location *dummy_loc = NULL;
 
       print_one_breakpoint (uiout, b, &dummy_loc, 0);
     }
+
+  do_cleanups (back_to);
 }
 
 static int
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -486,6 +486,10 @@ list_thread_ids (struct ui_out *uiout)
   struct cleanup *cleanup_chain;
   int current_thread = -1;
 
+extern void restore_current_uiout (void *arg);
+  struct cleanup *back_to = make_cleanup (restore_current_uiout, current_uiout);
+  current_uiout = NULL;
+
   update_thread_list ();
 
   cleanup_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "thread-ids");
@@ -507,6 +511,8 @@ list_thread_ids (struct ui_out *uiout)
   if (current_thread != -1)
     ui_out_field_int (uiout, "current-thread-id", current_thread);
   ui_out_field_int (uiout, "number-of-threads", num);
+
+  do_cleanups (back_to);
 }
 
 /* Return true if TP is an active thread.  */
@@ -1363,6 +1369,10 @@ thread_select (struct ui_out *uiout, char *tidstr)
   int num;
   struct thread_info *tp;
 
+extern void restore_current_uiout (void *arg);
+  struct cleanup *back_to = make_cleanup (restore_current_uiout, current_uiout);
+  current_uiout = NULL;
+
   num = value_as_long (parse_and_eval (tidstr));
 
   tp = find_thread_id (num);
@@ -1396,6 +1406,8 @@ thread_select (struct ui_out *uiout, char *tidstr)
   /* Since the current thread may have changed, see if there is any
      exited thread we can now delete.  */
   prune_threads ();
+
+  do_cleanups (back_to);
 }
 
 void