[PATCH] PR mi/2086 -break-insert missing error diagnostic

[PATCH] PR mi/2086 -break-insert missing error diagnostic

[Nick Roberts]

This patch fixes PR mi/2086.  There may be similar bugs arising from using
catch_exceptions_with_msg inappropriately.

I could slowly work my way through the mi PRs in the bug database.  Can I close
a bug or is that another level? (I don't know my password, if I have one).

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


*** gdb.h	18 Dec 2005 11:33:59 +1300	1.5
--- gdb.h	18 Oct 2006 02:57:25 +1300	
***************
*** 53,60 ****
  /* 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,
! 			    char **error_message);
  
  /* Switch thread and print notification. */
  enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
--- 53,59 ----
  /* 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);
  
  /* Switch thread and print notification. */
  enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,



*** breakpoint.c	09 Aug 2006 09:32:37 +1200	1.229
--- breakpoint.c	18 Oct 2006 03:04:02 +1300	
*************** do_captured_breakpoint (struct ui_out *u
*** 5466,5473 ****
  enum gdb_rc
  gdb_breakpoint (char *address, char *condition,
  		int hardwareflag, int tempflag,
! 		int thread, int ignore_count,
! 		char **error_message)
  {
    struct captured_breakpoint_args args;
    args.address = address;
--- 5466,5472 ----
  enum gdb_rc
  gdb_breakpoint (char *address, char *condition,
  		int hardwareflag, int tempflag,
! 		int thread, int ignore_count)
  {
    struct captured_breakpoint_args args;
    args.address = address;
*************** gdb_breakpoint (char *address, char *con
*** 5476,5483 ****
    args.tempflag = tempflag;
    args.thread = thread;
    args.ignore_count = ignore_count;
!   return catch_exceptions_with_msg (uiout, do_captured_breakpoint, &args,
! 				    error_message, RETURN_MASK_ALL);
  }
  
  
--- 5475,5481 ----
    args.tempflag = tempflag;
    args.thread = thread;
    args.ignore_count = ignore_count;
!   return do_captured_breakpoint (uiout, &args);
  }
  
  

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Daniel Jacobowitz]

On Wed, Oct 18, 2006 at 10:07:14AM +1300, Nick Roberts wrote:
> 
> This patch fixes PR mi/2086.  There may be similar bugs arising from using
> catch_exceptions_with_msg inappropriately.

Is this the whole patch?  I guess you updated the caller too.

I think removing catch_exceptions_with_msg isn't enough - if you look
at the call site, it's not prepared for an error() to escape there.
It would have to use a cleanup for deprecated_set_gdb_event_hooks.

Ulrich posted a detailed analysis of this to gdb@ on Oct. 4th.  I think
it was just a buggy conversion to catch_exceptions_with_msg; we should
check everywhere touched by whatever patch that was.  Looks like it was
from 2005-01-13.

> I could slowly work my way through the mi PRs in the bug database.  Can I close
> a bug or is that another level? (I don't know my password, if I have one).

I'll send it offlist.  Feel free to close bugs.


-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Nick Roberts]

 > > This patch fixes PR mi/2086.  There may be similar bugs arising from using
 > > catch_exceptions_with_msg inappropriately.
 > 
 > Is this the whole patch?  I guess you updated the caller too.

I have other changes but they're all older.  I think that's it but I'll
check more carefully.

 > I think removing catch_exceptions_with_msg isn't enough - if you look
 > at the call site, it's not prepared for an error() to escape there.
 > It would have to use a cleanup for deprecated_set_gdb_event_hooks.

Heres my take:

I think the problem arises from changes Andrew Cagney made on 2005-01-13.
It worked before then but was probably a bit of a hack.  By removing
catch_exceptions_with_msg the error propagates back to catch_exception
with the function argument captured_mi_execute_command:

#14 0x0812fdf8 in catch_exception (uiout=0x8823918, func=0x80ca1e7 <captured_mi_execute_command>, func_args=0xbfc229a4, mask=6) at exceptions.c:469
#15 0x080ca5de in mi_execute_command (cmd=0x8855220 "-break-insert *", from_tty=

(mi_execute_command was also changed on 2005-01-13).

 > Ulrich posted a detailed analysis of this to gdb@ on Oct. 4th.  I think
 > it was just a buggy conversion to catch_exceptions_with_msg; we should
 > check everywhere touched by whatever patch that was.  Looks like it was
 > from 2005-01-13.

I'll take a look but I'm guessing 2086 might be less general.

 > > I could slowly work my way through the mi PRs in the bug database.  Can I
 > > close a bug or is that another level? (I don't know my password, if I have
 > > one).
 > 
 > I'll send it offlist.  Feel free to close bugs.

Thanks.

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

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Daniel Jacobowitz]

On Wed, Oct 18, 2006 at 10:48:29AM +1300, Nick Roberts wrote:
>  > > This patch fixes PR mi/2086.  There may be similar bugs arising from using
>  > > catch_exceptions_with_msg inappropriately.
>  > 
>  > Is this the whole patch?  I guess you updated the caller too.
> 
> I have other changes but they're all older.  I think that's it but I'll
> check more carefully.

Well, it won't compile as-is, so obviously you haven't tested this
patch on its own :-)

>  > I think removing catch_exceptions_with_msg isn't enough - if you look
>  > at the call site, it's not prepared for an error() to escape there.
>  > It would have to use a cleanup for deprecated_set_gdb_event_hooks.
> 
> Heres my take:
> 
> I think the problem arises from changes Andrew Cagney made on 2005-01-13.
> It worked before then but was probably a bit of a hack.  By removing
> catch_exceptions_with_msg the error propagates back to catch_exception
> with the function argument captured_mi_execute_command:
> 
> #14 0x0812fdf8 in catch_exception (uiout=0x8823918, func=0x80ca1e7 <captured_mi_execute_command>, func_args=0xbfc229a4, mask=6) at exceptions.c:469
> #15 0x080ca5de in mi_execute_command (cmd=0x8855220 "-break-insert *", from_tty=
> 
> (mi_execute_command was also changed on 2005-01-13).

But we've messed up whatever those saved/restored hooks were.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Nick Roberts]

 > > I have other changes but they're all older.  I think that's it but I'll
 > > check more carefully.
 > 
 > Well, it won't compile as-is, so obviously you haven't tested this
 > patch on its own :-)

Oops, I forgot to look in the MI directory.

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


*** mi-cmd-break.c	24 Dec 2005 07:57:46 +1300	1.13
--- mi-cmd-break.c	18 Oct 2006 02:54:07 +1300	
*************** mi_cmd_break_insert (char *command, char
*** 140,153 ****
      case REG_BP:
        rc = gdb_breakpoint (address, condition,
  			   0 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count,
! 			   &mi_error_message);
        break;
      case HW_BP:
        rc = gdb_breakpoint (address, condition,
  			   1 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count,
! 			   &mi_error_message);
        break;
  #if 0
      case REGEXP_BP:
--- 140,151 ----
      case REG_BP:
        rc = gdb_breakpoint (address, condition,
  			   0 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count);
        break;
      case HW_BP:
        rc = gdb_breakpoint (address, condition,
  			   1 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count);
        break;
  #if 0
      case REGEXP_BP:

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Nick Roberts]

 > >  > I think removing catch_exceptions_with_msg isn't enough - if you look
 > >  > at the call site, it's not prepared for an error() to escape there.
 > >  > It would have to use a cleanup for deprecated_set_gdb_event_hooks.
 > > 
 > > Heres my take:
 > > 
 > > I think the problem arises from changes Andrew Cagney made on 2005-01-13.
 > > It worked before then but was probably a bit of a hack.  By removing
 > > catch_exceptions_with_msg the error propagates back to catch_exception
 > > with the function argument captured_mi_execute_command:
 > > 
 > > #14 0x0812fdf8 in catch_exception (uiout=0x8823918, func=0x80ca1e7 <captured_mi_execute_command>, func_args=0xbfc229a4, mask=6) at exceptions.c:469
 > > #15 0x080ca5de in mi_execute_command (cmd=0x8855220 "-break-insert *", from_tty=
 > > 
 > > (mi_execute_command was also changed on 2005-01-13).
 > 
 > But we've messed up whatever those saved/restored hooks were.

The name deprecated_set_gdb_event_hooks suggests that they're earmarked for
removal.  These hooks only seem to get get in one place so presumably they
could be set up once at initialisation and left with that value.

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

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Daniel Jacobowitz]

On Wed, Oct 18, 2006 at 12:44:20PM +1300, Nick Roberts wrote:
> The name deprecated_set_gdb_event_hooks suggests that they're earmarked for
> removal.  These hooks only seem to get get in one place so presumably they
> could be set up once at initialisation and left with that value.

They're also used by insight.  This is one of the sucky things about
having insight directly linked to GDB.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Nick Roberts]

Daniel Jacobowitz writes:
 > On Wed, Oct 18, 2006 at 12:44:20PM +1300, Nick Roberts wrote:
 > > The name deprecated_set_gdb_event_hooks suggests that they're earmarked for
 > > removal.  These hooks only seem to get get in one place so presumably they
 > > could be set up once at initialisation and left with that value.
 > 
 > They're also used by insight.  This is one of the sucky things about
 > having insight directly linked to GDB.

OK, how about just using the hack in mi_cmd_thread_select for
mi_cmd_break_insert for the moment.

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


*** mi-cmd-break.c	24 Dec 2005 07:57:46 +1300	1.13
--- mi-cmd-break.c	19 Oct 2006 04:46:35 +1300	
***************
*** 25,30 ****
--- 25,31 ----
  #include "mi-out.h"
  #include "breakpoint.h"
  #include "gdb_string.h"
+ #include "exceptions.h"
  #include "mi-getopt.h"
  #include "gdb-events.h"
  #include "gdb.h"
*************** mi_cmd_break_insert (char *command, char
*** 164,170 ****
      }
    deprecated_set_gdb_event_hooks (old_hooks);
  
!   if (rc == GDB_RC_FAIL)
      return MI_CMD_ERROR;
    else
      return MI_CMD_DONE;
--- 165,175 ----
      }
    deprecated_set_gdb_event_hooks (old_hooks);
  
!   /* RC is enum gdb_rc if it is successful (>=0)
!      enum return_reason if not (<0). */
!   if ((int) rc < 0 && (enum return_reason) rc == RETURN_ERROR)
!     return MI_CMD_ERROR;
!   else if ((int) rc >= 0 && rc == GDB_RC_FAIL)
      return MI_CMD_ERROR;
    else
      return MI_CMD_DONE;

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Daniel Jacobowitz]

On Thu, Oct 19, 2006 at 04:52:29AM +1300, Nick Roberts wrote:
> Daniel Jacobowitz writes:
>  > On Wed, Oct 18, 2006 at 12:44:20PM +1300, Nick Roberts wrote:
>  > > The name deprecated_set_gdb_event_hooks suggests that they're earmarked for
>  > > removal.  These hooks only seem to get get in one place so presumably they
>  > > could be set up once at initialisation and left with that value.
>  > 
>  > They're also used by insight.  This is one of the sucky things about
>  > having insight directly linked to GDB.
> 
> OK, how about just using the hack in mi_cmd_thread_select for
> mi_cmd_break_insert for the moment.

This is just nasty.  I believe I pointed you at Ulrich's analysis of
this problem upthread:

  http://sources.redhat.com/ml/gdb/2006-10/msg00021.html

It seems pretty clear to me that the patch which switched things to
return the result of catch_exceptions_with_msg was wrong.  The
functions are defined to return an enum gdb_rc.  Can't we make
them do that again?  Simple, obviously correct.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic

[Nick Roberts]

 > > OK, how about just using the hack in mi_cmd_thread_select for
 > > mi_cmd_break_insert for the moment.
 > 
 > This is just nasty.  I believe I pointed you at Ulrich's analysis of
 > this problem upthread:
 > 
 >   http://sources.redhat.com/ml/gdb/2006-10/msg00021.html
 > 
 > It seems pretty clear to me that the patch which switched things to
 > return the result of catch_exceptions_with_msg was wrong.  The
 > functions are defined to return an enum gdb_rc.  Can't we make
 > them do that again?  Simple, obviously correct.

Well we need to propagate the error message back to
captured_mi_execute_command.  The best I can do is create a new function
catch_errors_with_msg (gdb_breakpoint used catch_errors previously).  We can
also change mi_cmd_thread_list_ids accordingly.  Also do_captured_breakpoint,
do_captured_breakpoint_query, do_captured_thread_select and
do_captured_list_thread_ids should really be type enum gdb_rc.

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


*** exceptions.h	22 Sep 2006 10:00:57 +1200	1.19
--- exceptions.h	19 Nov 2006 17:09:58 +1300	
***************
*** 227,236 ****
     indication of the exact exception that it caught - quit_flag might
     help.
  
!    This function is superseeded by catch_exceptions().  */
  
  typedef int (catch_errors_ftype) (void *);
  extern int catch_errors (catch_errors_ftype *, void *, char *, return_mask);
  
  /* Template to catch_errors() that wraps calls to command
     functions. */
--- 227,238 ----
     indication of the exact exception that it caught - quit_flag might
     help.
  
!    This function is superseded by catch_exceptions().  */
  
  typedef int (catch_errors_ftype) (void *);
  extern int catch_errors (catch_errors_ftype *, void *, char *, return_mask);
+ extern int catch_errors_with_msg (catch_errors_ftype *, void *, char *,
+ 				  char **, return_mask);
  
  /* Template to catch_errors() that wraps calls to command
     functions. */


*** exceptions.c	04 Feb 2006 10:50:25 +1300	1.24
--- exceptions.c	19 Nov 2006 17:00:36 +1300	
***************
*** 508,513 ****
--- 508,520 ----
  catch_errors (catch_errors_ftype *func, void *func_args, char *errstring,
  	      return_mask mask)
  {
+   return catch_errors_with_msg (func, func_args, errstring, NULL, mask);
+ }
+ 
+ int
+ catch_errors_with_msg (catch_errors_ftype *func, void *func_args, char *errstring,
+ 		       char **gdberrmsg, return_mask mask)
+ {
    volatile int val = 0;
    volatile struct gdb_exception exception;
    TRY_CATCH (exception, mask)
***************
*** 516,522 ****
      }
    print_any_exception (gdb_stderr, errstring, exception);
    if (exception.reason != 0)
!     return 0;
    return val;
  }
  
--- 523,541 ----
      }
    print_any_exception (gdb_stderr, errstring, exception);
    if (exception.reason != 0)
!     {
!       /* If caller wants a copy of the low-level error message, make
! 	 one.  This is used in the case of a silent error whereby the
! 	 caller may optionally want to issue the message.  */
!       if (gdberrmsg != NULL)
! 	{
! 	  if (exception.message != NULL)
! 	    *gdberrmsg = xstrdup (exception.message);
! 	  else
! 	    *gdberrmsg = NULL;
! 	}
!       return 0;
!     }
    return val;
  }
  

*** breakpoint.c	23 Oct 2006 08:48:48 +1300	1.231
--- breakpoint.c	19 Nov 2006 17:19:52 +1300	
***************
*** 3615,3621 ****
    };
  
  static int
! do_captured_breakpoint_query (struct ui_out *uiout, void *data)
  {
    struct captured_breakpoint_query_args *args = data;
    struct breakpoint *b;
--- 3615,3621 ----
    };
  
  static int
! do_captured_breakpoint_query (void *data)
  {
    struct captured_breakpoint_query_args *args = data;
    struct breakpoint *b;
***************
*** 3632,3645 ****
  }
  
  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. */
!   return catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
! 				    error_message, RETURN_MASK_ALL);
  }
  
  /* Return non-zero if B is user settable (breakpoints, watchpoints,
--- 3632,3645 ----
  }
  
  enum gdb_rc
! gdb_breakpoint_query (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. */
!   return catch_errors_with_msg (do_captured_breakpoint_query, &args, NULL,
! 				error_message, RETURN_MASK_ALL);
  }
  
  /* Return non-zero if B is user settable (breakpoints, watchpoints,
***************
*** 5373,5379 ****
    };
  
  static int
! do_captured_breakpoint (struct ui_out *uiout, void *data)
  {
    struct captured_breakpoint_args *args = data;
    struct symtabs_and_lines sals;
--- 5373,5379 ----
    };
  
  static int
! do_captured_breakpoint (void *data)
  {
    struct captured_breakpoint_args *args = data;
    struct symtabs_and_lines sals;
***************
*** 5481,5488 ****
    args.tempflag = tempflag;
    args.thread = thread;
    args.ignore_count = ignore_count;
!   return catch_exceptions_with_msg (uiout, do_captured_breakpoint, &args,
! 				    error_message, RETURN_MASK_ALL);
  }
  
  
--- 5481,5488 ----
    args.tempflag = tempflag;
    args.thread = thread;
    args.ignore_count = ignore_count;
!   return catch_errors_with_msg (do_captured_breakpoint, &args,
! 				NULL, error_message, RETURN_MASK_ALL);
  }


*** mi-cmd-break.c	24 Dec 2005 07:57:46 +1300	1.13
--- mi-cmd-break.c	19 Nov 2006 17:31:58 +1300	
***************
*** 39,45 ****
  static void
  breakpoint_notify (int b)
  {
!   gdb_breakpoint_query (uiout, b, NULL);
  }
  
  
--- 39,45 ----
  static void
  breakpoint_notify (int b)
  {
!   gdb_breakpoint_query (b, NULL);
  }


*** mi-main.c	18 Nov 2006 19:13:33 +1300	1.86
--- mi-main.c	19 Nov 2006 14:17:16 +1300	
***************
*** 234,244 ****
    else
      rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
  
!   /* RC is enum gdb_rc if it is successful (>=0)
!      enum return_reason if not (<0). */
!   if ((int) rc < 0 && (enum return_reason) rc == RETURN_ERROR)
!     return MI_CMD_ERROR;
!   else if ((int) rc >= 0 && rc == GDB_RC_FAIL)
      return MI_CMD_ERROR;
    else
      return MI_CMD_DONE;
--- 234,240 ----
    else
      rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
  
!   if (rc == GDB_RC_FAIL)
      return MI_CMD_ERROR;
    else
      return MI_CMD_DONE;