[RFA] Make mi_cmd_break_insert exception-safe.

[RFA] Make mi_cmd_break_insert exception-safe.

[Vladimir Prus]

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

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Nick Roberts]

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

Is this patch is on top of your earlier one?  If so, I think that has to be
approved before we can review this one.  In fact there appear to be many
changes in the pipeline that need to be resolved before the next release.

 > - Volodya
 > 
 > 	[gdb]
 > 	* breakpoint.c (break_command_1): Return void.
                                          ^^^^^^^^^^^
That's a contradiction in terms, isn't it?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Vladimir Prus]

On Monday 28 January 2008 00:10:21 Nick Roberts wrote:
>  > 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?
> 
> Is this patch is on top of your earlier one?  

Yes.

> If so, I think that has to be 
> approved before we can review this one.  In fact there appear to be many
> changes in the pipeline that need to be resolved before the next release.
> 
>  > - Volodya
>  > 
>  > 	[gdb]
>  > 	* breakpoint.c (break_command_1): Return void.
>                                           ^^^^^^^^^^^
> That's a contradiction in terms, isn't it?

Not sure. C++ allows 

	return <expression-of-type-void>;

I suspect modern C allows as well, so terminologically,
this is fine.

- Volodya

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Nick Roberts]

 > >  > 	[gdb]
 > >  > 	* breakpoint.c (break_command_1): Return void.
 > >                                           ^^^^^^^^^^^
 > > That's a contradiction in terms, isn't it?
 > 
 > Not sure. C++ allows 
 > 
 > 	return <expression-of-type-void>;
 > 
 > I suspect modern C allows as well, so terminologically,
 > this is fine.

Yeah, a bit like a negative increase maybe.  Anyway in your patch
break_command_1 doesn't return anything, void or otherwise.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Daniel Jacobowitz]

On Sun, Jan 27, 2008 at 05:15:07PM +0300, Vladimir Prus wrote:
> 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.

There is a convention that the gdb_* functions don't throw, though.
It's very confusing what does and does not throw in GDB.  Before this
patch, did gdb_breakpoint actually throw?  If so, would fixing it
by using try/catch inside gdb_breakpoint fix this bug too?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Vladimir Prus]

On Friday 01 February 2008 01:16:53 Daniel Jacobowitz wrote:
> On Sun, Jan 27, 2008 at 05:15:07PM +0300, Vladimir Prus wrote:
> > 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.
> 
> There is a convention that the gdb_* functions don't throw, though.
> It's very confusing what does and does not throw in GDB.  Before this
> patch, did gdb_breakpoint actually throw? If so, would fixing it 
> by using try/catch inside gdb_breakpoint fix this bug too?

Putting try/catch inside gdb_breakpoint can possibly fix this bug.
However, right now gdb_breakpoint is used in a single place -- 
in mi_cmd_break_insert. If gdb_breakpoint throws, mi_cmd_break_insert is
capable of reporting this error property -- because top-level MI code
will handle the exception already.

So, why bother trying to make gdb_breakpoint non-throwing? I believe
any such change will be at least as complex as making mi_cmd_break_insert
exception-safe? If your concern is about gdb_ prefix, how about renaming
gdb_exception into 'set_breakpoint'?

- Volodya

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Daniel Jacobowitz]

On Fri, Feb 01, 2008 at 09:53:46AM +0300, Vladimir Prus wrote:
> So, why bother trying to make gdb_breakpoint non-throwing? I believe
> any such change will be at least as complex as making mi_cmd_break_insert
> exception-safe? If your concern is about gdb_ prefix, how about renaming
> gdb_exception into 'set_breakpoint'?

That's fine too.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Vladimir Prus]

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

On Friday 01 February 2008 17:24:39 Daniel Jacobowitz wrote:
> On Fri, Feb 01, 2008 at 09:53:46AM +0300, Vladimir Prus wrote:
> > So, why bother trying to make gdb_breakpoint non-throwing? I believe
> > any such change will be at least as complex as making mi_cmd_break_insert
> > exception-safe? If your concern is about gdb_ prefix, how about renaming
> > gdb_exception into 'set_breakpoint'?
> 
> That's fine too.

What about the following, then?

- Volodya


[-- Attachment #2: 0001-Make-mi_cmd_break_insert-exception-safe.patch --]
[-- Type: text/x-diff, Size: 10164 bytes --]

From d2a24e0c16437cbad9a8ad79ab22899b3749f5ca Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Sun, 27 Jan 2008 17:06:05 +0300
Subject: [RFA] Make mi_cmd_break_insert exception-safe.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

	[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.  Rename to set_breakpoint.
	* gdb.h (gdb_breakpoint): Rename and move to...
        * breakpoint.h (set_breakpoint): ...here.
	* 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                  |   51 +++++++++++++----------------
 gdb/breakpoint.h                  |    5 +++
 gdb/gdb.h                         |    7 ----
 gdb/mi/mi-cmd-break.c             |   62 +++++++++++++++++++------------------
 gdb/testsuite/gdb.mi/basics.c     |    5 +++
 gdb/testsuite/gdb.mi/mi-break.exp |   12 +++++++
 6 files changed, 77 insertions(+), 65 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f69002c..c2bf50a 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 *);
 
@@ -5285,7 +5285,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, 
@@ -5323,8 +5323,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)
 	{
@@ -5342,7 +5341,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
@@ -5356,12 +5355,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. */
@@ -5457,8 +5455,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. 
@@ -5468,34 +5464,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
-gdb_breakpoint (char *address, char *condition,
+void
+set_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/breakpoint.h b/gdb/breakpoint.h
index 7b72e19..41730c0 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -710,6 +710,11 @@ extern void awatch_command_wrapper (char *, int);
 extern void rwatch_command_wrapper (char *, int);
 extern void tbreak_command (char *, int);
 
+extern void set_breakpoint (char *address, char *condition,
+			    int hardwareflag, int tempflag,
+			    int thread, int ignore_count,
+			    int pending);
+
 extern void insert_breakpoints (void);
 
 extern int remove_breakpoints (void);
diff --git a/gdb/gdb.h b/gdb/gdb.h
index c7c405c..fcd3e3b 100644
--- a/gdb/gdb.h
+++ b/gdb/gdb.h
@@ -47,13 +47,6 @@ enum gdb_rc {
 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);
-
 /* 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 c77b0dc..3a5faf3 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:
+	  set_breakpoint (address, condition,
+			  0 /*hardwareflag */ , temp_p,
+			  thread, ignore_count,
+			  pending);
+	  break;
+	case HW_BP:
+	  set_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 6933a34..9f10364 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_exist" \
         ".*\\^error,msg=\"Function \\\\\"function_that_does_not_exist\\\\\" 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

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Daniel Jacobowitz]

On Fri, Feb 01, 2008 at 05:49:41PM +0300, Vladimir Prus wrote:
> On Friday 01 February 2008 17:24:39 Daniel Jacobowitz wrote:
> > On Fri, Feb 01, 2008 at 09:53:46AM +0300, Vladimir Prus wrote:
> > > So, why bother trying to make gdb_breakpoint non-throwing? I believe
> > > any such change will be at least as complex as making mi_cmd_break_insert
> > > exception-safe? If your concern is about gdb_ prefix, how about renaming
> > > gdb_exception into 'set_breakpoint'?
> > 
> > That's fine too.
> 
> What about the following, then?

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Doug Evans]

On Feb 1, 2008 6:49 AM, Vladimir Prus <vladimir@codesourcery.com> wrote:
>
> On Friday 01 February 2008 17:24:39 Daniel Jacobowitz wrote:
> > On Fri, Feb 01, 2008 at 09:53:46AM +0300, Vladimir Prus wrote:
> > > So, why bother trying to make gdb_breakpoint non-throwing? I believe
> > > any such change will be at least as complex as making mi_cmd_break_insert
> > > exception-safe? If your concern is about gdb_ prefix, how about renaming
> > > gdb_exception into 'set_breakpoint'?
> >
> > That's fine too.
>
> What about the following, then?

Heh.  Or rename the function. :-)

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Doug Evans]

On Jan 31, 2008 2:16 PM, Daniel Jacobowitz <drow@false.org> wrote:
> On Sun, Jan 27, 2008 at 05:15:07PM +0300, Vladimir Prus wrote:
> > 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.
>
> There is a convention that the gdb_* functions don't throw, though.
> It's very confusing what does and does not throw in GDB.  Before this
> patch, did gdb_breakpoint actually throw?  If so, would fixing it
> by using try/catch inside gdb_breakpoint fix this bug too?

For future generations I wonder if we need to add something to
gdbint.texinfo.  Maybe something along the lines of "gdb_* functions
in general do not throw, but that is an artifact of the original
libgdb design which is no longer important.  That does not mean go out
and make anything throw willy-nilly, but if the preferred solution
involves making a gdb_* function throw where it did not before, then
go ahead."  Or some such.

Re: [RFA] Make mi_cmd_break_insert exception-safe.

[Vladimir Prus]

On Friday 01 February 2008 18:45:39 you wrote:
> On Feb 1, 2008 6:49 AM, Vladimir Prus <vladimir@codesourcery.com> wrote:
> >
> > On Friday 01 February 2008 17:24:39 Daniel Jacobowitz wrote:
> > > On Fri, Feb 01, 2008 at 09:53:46AM +0300, Vladimir Prus wrote:
> > > > So, why bother trying to make gdb_breakpoint non-throwing? I believe
> > > > any such change will be at least as complex as making mi_cmd_break_insert
> > > > exception-safe? If your concern is about gdb_ prefix, how about renaming
> > > > gdb_exception into 'set_breakpoint'?
> > >
> > > That's fine too.
> >
> > What about the following, then?
> 
> Heh.  Or rename the function. :-)

Which the patch does, in fact.

- Volodya