[RFA] Make mi_cmd_break_insert exception-safe.

[Vladimir Prus <vladimir@codesourcery.com>]

KDevelop user has reported a case where GDB produces really 
confusing output, as follows:

   (gdb) -var-update *
   ^done,changelist=[bkpt={number="0",type="call dummy",disp="del",
                     enabled="y",addr="0x00000000004068b0",at="<_start>",
                     thread="1",thread="1",times="1"}]


The explanation is as follows:

1. My previous breakpoint refactoring make gdb_breakpoint throw exceptions.
2. The mi_cmd_break_insert is not exception-safe, so if gdb_breakpoint throws,
it fails to reset various hooks that report all new breakpoint.
3. It's possible to create variable object for an expression with function calls.
Evaluating function call requires inserting a temporary breakpoint.

So, in the event that -break-insert fails, all future evaluations of function
call will print information about that temporary breakpoint. The -var-update
case above happens exactly when one variable object involves function call expression.

The gdb_breakpoint function, as I understand it, was supposed to be part of
libgdb interface, defined in gdb.h header. However, libgdb is not even close to
being usable, and when I've asked about using gdb as a library some time ago, the
response was that it's too hard to do, and it's no longer a goal. Therefore,
I think it makes no sense to keep gdb_breakpoint non-throwing.

The attached patch make mi_cmd_break_insert exception-safe, so that it always
resets the hook it installs. There's new testcase that fails before the
patch, and works after. OK?

- Volodya

	[gdb]
	* breakpoint.c (break_command_1): Return void.
	(break_command_really): Return void.  Rethrow
	exceptions instead of returning.
	(gdb_breakpoint): Remove the error_message parameter.
	Return void.
	* gdb.h (gdb_breakpoint): Adjust prototype.
	* mi/mi-cmb-break.c (mi_cmd_break_insert): Restore
	event hooks even if exception is thrown.  Adjust to
	gdb_breakpoint interface changes.

	[gdb/testsuite]
	* gdb.mi/basic.c (return_1): New function.
	* gdb.mi/mi-break.exp: Make sure that failed -break-insert
	don't cause future evaluations of function to report
	creation of internal breakpoints.
---
 gdb/breakpoint.c                  |   49 +++++++++++++----------------
 gdb/gdb.h                         |    9 ++---
 gdb/mi/mi-cmd-break.c             |   62 +++++++++++++++++++------------------
 gdb/testsuite/gdb.mi/basics.c     |    5 +++
 gdb/testsuite/gdb.mi/mi-break.exp |   12 +++++++
 5 files changed, 75 insertions(+), 62 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index d5de3e7..cd9f198 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -90,7 +90,7 @@ static void watch_command (char *, int);
 
 static int can_use_hardware_watchpoint (struct value *);
 
-static int break_command_1 (char *, int, int);
+static void break_command_1 (char *, int, int);
 
 static void mention (struct breakpoint *);
 
@@ -5235,7 +5235,7 @@ find_condition_and_thread (char *tok, CORE_ADDR pc,
    and thread specified by the COND_STRING and THREAD
    parameters.  */
 
-static int
+static void
 break_command_really (char *arg, char *cond_string, int thread,
 		      int parse_condition_and_thread,
 		      int tempflag, int hardwareflag, 
@@ -5273,8 +5273,7 @@ break_command_really (char *arg, char *cond_string, int thread,
   switch (e.reason)
     {
     case RETURN_QUIT:
-      exception_print (gdb_stderr, e);
-      return e.reason;
+      throw_exception (e);
     case RETURN_ERROR:
       switch (e.error)
 	{
@@ -5292,7 +5291,7 @@ break_command_really (char *arg, char *cond_string, int thread,
 	     selects no, then simply return the error code.  */
 	  if (pending_break_support == AUTO_BOOLEAN_AUTO && 
 	      !nquery ("Make breakpoint pending on future shared library load? "))
-	    return e.reason;
+	    return;
 
 	  /* At this point, either the user was queried about setting
 	     a pending breakpoint and selected yes, or pending
@@ -5306,12 +5305,11 @@ break_command_really (char *arg, char *cond_string, int thread,
 	  pending = 1;
 	  break;
 	default:
-	  exception_print (gdb_stderr, e);
-	  return e.reason;
+	  throw_exception (e);
 	}
     default:
       if (!sals.nelts)
-	return GDB_RC_FAIL;
+	return;
     }
 
   /* Create a chain of things that always need to be cleaned up. */
@@ -5407,8 +5405,6 @@ break_command_really (char *arg, char *cond_string, int thread,
   discard_cleanups (breakpoint_chain);
   /* But cleanup everything else. */
   do_cleanups (old_chain);
-
-  return GDB_RC_OK;
 }
 
 /* Set a breakpoint. 
@@ -5418,34 +5414,33 @@ break_command_really (char *arg, char *cond_string, int thread,
    and if breakpoint is temporary, using BP_HARDWARE_FLAG
    and BP_TEMPFLAG.  */
    
-static int
+static void
 break_command_1 (char *arg, int flag, int from_tty)
 {
   int hardwareflag = flag & BP_HARDWAREFLAG;
   int tempflag = flag & BP_TEMPFLAG;
 
-  return break_command_really (arg, 
-			       NULL, 0, 1 /* parse arg */,
-			       tempflag, hardwareflag,
-			       0 /* Ignore count */,
-			       pending_break_support, from_tty);
+  break_command_really (arg, 
+			NULL, 0, 1 /* parse arg */,
+			tempflag, hardwareflag,
+			0 /* Ignore count */,
+			pending_break_support, from_tty);
 }
 
 
-enum gdb_rc
+void
 gdb_breakpoint (char *address, char *condition,
 		int hardwareflag, int tempflag,
 		int thread, int ignore_count,
-		int pending,
-		char **error_message)
-{
-  return break_command_really (address, condition, thread,
-			       0 /* condition and thread are valid.  */,
-			       tempflag, hardwareflag,
-			       ignore_count,
-			       pending 
-			       ? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE,
-			       0);
+		int pending)
+{
+  break_command_really (address, condition, thread,
+			0 /* condition and thread are valid.  */,
+			tempflag, hardwareflag,
+			ignore_count,
+			pending 
+			? AUTO_BOOLEAN_TRUE : AUTO_BOOLEAN_FALSE,
+			0);
 }
 
 
diff --git a/gdb/gdb.h b/gdb/gdb.h
index c7c405c..9029e3b 100644
--- a/gdb/gdb.h
+++ b/gdb/gdb.h
@@ -48,11 +48,10 @@ enum gdb_rc gdb_breakpoint_query (struct ui_out *uiout, int bnum,
 				  char **error_message);
 
 /* Create a breakpoint at ADDRESS (a GDB source and line). */
-enum gdb_rc gdb_breakpoint (char *address, char *condition,
-			    int hardwareflag, int tempflag,
-			    int thread, int ignore_count,
-			    int pending,
-			    char **error_message);
+void gdb_breakpoint (char *address, char *condition,
+		     int hardwareflag, int tempflag,
+		     int thread, int ignore_count,
+		     int pending);
 
 /* Switch thread and print notification. */
 enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
diff --git a/gdb/mi/mi-cmd-break.c b/gdb/mi/mi-cmd-break.c
index c77b0dc..d838363 100644
--- a/gdb/mi/mi-cmd-break.c
+++ b/gdb/mi/mi-cmd-break.c
@@ -26,6 +26,7 @@
 #include "mi-getopt.h"
 #include "gdb-events.h"
 #include "gdb.h"
+#include "exceptions.h"
 
 enum
   {
@@ -69,7 +70,7 @@ mi_cmd_break_insert (char *command, char **argv, int argc)
   int ignore_count = 0;
   char *condition = NULL;
   int pending = 0;
-  enum gdb_rc rc;
+  struct gdb_exception e;
   struct gdb_events *old_hooks;
   enum opt
     {
@@ -132,41 +133,42 @@ mi_cmd_break_insert (char *command, char **argv, int argc)
 
   /* Now we have what we need, let's insert the breakpoint! */
   old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
-  switch (type)
+  /* Make sure we restore hooks even if exception is thrown.  */
+  TRY_CATCH (e, RETURN_MASK_ALL)
     {
-    case REG_BP:
-      rc = gdb_breakpoint (address, condition,
-			   0 /*hardwareflag */ , temp_p,
-			   thread, ignore_count,
-			   pending,
-			   &mi_error_message);
-      break;
-    case HW_BP:
-      rc = gdb_breakpoint (address, condition,
-			   1 /*hardwareflag */ , temp_p,
-			   thread, ignore_count,
-			   pending,
-			   &mi_error_message);
-      break;
+      switch (type)
+	{
+	case REG_BP:
+	  gdb_breakpoint (address, condition,
+			  0 /*hardwareflag */ , temp_p,
+			  thread, ignore_count,
+			  pending);
+	  break;
+	case HW_BP:
+	  gdb_breakpoint (address, condition,
+			  1 /*hardwareflag */ , temp_p,
+			  thread, ignore_count,
+			  pending);
+	  break;
 #if 0
-    case REGEXP_BP:
-      if (temp_p)
-	error (_("mi_cmd_break_insert: Unsupported tempoary regexp breakpoint"));
-      else
-	rbreak_command_wrapper (address, FROM_TTY);
-      return MI_CMD_DONE;
-      break;
+	case REGEXP_BP:
+	  if (temp_p)
+	    error (_("mi_cmd_break_insert: Unsupported tempoary regexp breakpoint"));
+	  else
+	    rbreak_command_wrapper (address, FROM_TTY);
+	  return MI_CMD_DONE;
+	  break;
 #endif
-    default:
-      internal_error (__FILE__, __LINE__,
-		      _("mi_cmd_break_insert: Bad switch."));
+	default:
+	  internal_error (__FILE__, __LINE__,
+			  _("mi_cmd_break_insert: Bad switch."));
+	}
     }
   deprecated_set_gdb_event_hooks (old_hooks);
+  if (e.reason < 0)
+    throw_exception (e);
 
-  if (rc == GDB_RC_FAIL)
-    return MI_CMD_ERROR;
-  else
-    return MI_CMD_DONE;
+  return MI_CMD_DONE;
 }
 
 enum wp_type
diff --git a/gdb/testsuite/gdb.mi/basics.c b/gdb/testsuite/gdb.mi/basics.c
index 5b52112..e4be7b2 100644
--- a/gdb/testsuite/gdb.mi/basics.c
+++ b/gdb/testsuite/gdb.mi/basics.c
@@ -51,6 +51,11 @@ void callme (int i)
   printf ("callme\n");
 }
 
+int return_1 ()
+{
+  return 1;
+}
+
 main ()
 {
   callee1 (2, "A string argument.", 3.5);
diff --git a/gdb/testsuite/gdb.mi/mi-break.exp b/gdb/testsuite/gdb.mi/mi-break.exp
index ca59348..b952f29 100644
--- a/gdb/testsuite/gdb.mi/mi-break.exp
+++ b/gdb/testsuite/gdb.mi/mi-break.exp
@@ -181,6 +181,18 @@ proc test_error {} {
     mi_gdb_test "-break-insert function_that_does_not_exit" \
         ".*\\^error,msg=\"Function \\\\\"function_that_does_not_exit\\\\\" not defined.\"" \
         "breakpoint at nonexistent function"
+
+    # We used to have a bug whereby -break-insert that failed would not
+    # clear some event hooks.  As result, whenever we evaluate expression
+    # containing function call, the internal breakpoint created to handle
+    # function call would be reported, messing up MI output.
+    mi_gdb_test "-var-create V * return_1()" \
+        "\\^done,name=\"V\",numchild=\"0\",value=\"1\",type=\"int\"" \
+        "create varobj for function call"
+
+    mi_gdb_test "-var-update *" \
+        "\\^done,changelist=\\\[\\\]" \
+        "update varobj for function call"    
 }
 
 test_tbreak_creation_and_listing
-- 
1.5.3.5