Re: [PATCH] interpreter-exec error path

Re: [PATCH] interpreter-exec error path

[Nick Roberts]

> If any of the commands executed through "interpreter-exec" fails, the
> "quiet" value is not restored for the interpreter.  This may result in
> something like the following.


> (gdb) interpreter-exec console rubbish
> Undefined command: "rubbish".  Try "help".
> error in command: "rubbish".
> ...

It's a somewhat circular example but I get your point.

> The attached patch will handle the error more correctly.

It's more convenient to post such a small patch just as text

> 2006-09-09  Andrzej Zaborowski  <balrog@zabor.org>
> 	* interps.c (interpreter_exec_cmd): Restore interpreter properties.

> --- gdb-orig/gdb/interps.c	2006-09-08 06:10:15.000000000 +0000
> +++ gdb/gdb/interps.c	2006-09-08 06:04:20.000000000 +0000
> @@ -402,9 +402,9 @@ interpreter_exec_cmd (char *args, int fr
>        if (e.reason < 0)
>  	{
>  	  interp_set (old_interp);
> -	  interp_set_quiet (interp_to_use, old_quiet);
> +	  interp_set_quiet (interp_to_use, use_quiet);
> +	  interp_set_quiet (old_interp, old_quiet);
>  	  error (_("error in command: \"%s\"."), prules[i]);
> -	  break;
>  	}
>      }

Yes, I think this does what Andrew Cagney intended but the underlying
interpreter has already signalled the exception so I think it could be
handled normally:

*** interps.c	13 Jul 2006 21:03:38 +1200	1.17
--- interps.c	12 Sep 2006 09:25:17 +1200	
***************
*** 399,411 ****
    for (i = 1; i < nrules; i++)
      {
        struct gdb_exception e = interp_exec (interp_to_use, prules[i]);
!       if (e.reason < 0)
! 	{
! 	  interp_set (old_interp);
! 	  interp_set_quiet (interp_to_use, old_quiet);
! 	  error (_("error in command: \"%s\"."), prules[i]);
! 	  break;
! 	}
      }
  
    interp_set (old_interp);
--- 399,405 ----
    for (i = 1; i < nrules; i++)
      {
        struct gdb_exception e = interp_exec (interp_to_use, prules[i]);
!       if (e.reason < 0) break;
      }
  
    interp_set (old_interp);

Taking things a step further, I see that mi_interpreter_exec always returns
exception_none so cli_interpreter_exec could do the same (patch below).  The
command interpreter-exec can handle a list of commands, this would mean if the
first fails, GDB will still handle the subsequent commands.  This is currently
true for mi e.g
 
(gdb) i interpreter-exec mi -ttd -environment-pwd
^error,msg="Undefined MI command: ttd"
(gdb)
^done,cwd="/home/nickrob"
(gdb)
(gdb)

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


*** interps.c	13 Jul 2006 21:03:38 +1200	1.17
--- interps.c	12 Sep 2006 10:02:34 +1200	
***************
*** 397,412 ****
      error (_("Could not switch to interpreter \"%s\"."), prules[0]);
  
    for (i = 1; i < nrules; i++)
!     {
!       struct gdb_exception e = interp_exec (interp_to_use, prules[i]);
!       if (e.reason < 0)
! 	{
! 	  interp_set (old_interp);
! 	  interp_set_quiet (interp_to_use, old_quiet);
! 	  error (_("error in command: \"%s\"."), prules[i]);
! 	  break;
! 	}
!     }
  
    interp_set (old_interp);
    interp_set_quiet (interp_to_use, use_quiet);
--- 397,403 ----
      error (_("Could not switch to interpreter \"%s\"."), prules[0]);
  
    for (i = 1; i < nrules; i++)
!       interp_exec (interp_to_use, prules[i]);
  
    interp_set (old_interp);
    interp_set_quiet (interp_to_use, use_quiet);


*** cli-interp.c	18 Dec 2005 11:40:17 +1300	1.11
--- cli-interp.c	12 Sep 2006 10:03:35 +1200	
***************
*** 96,102 ****
  cli_interpreter_exec (void *data, const char *command_str)
  {
    struct ui_file *old_stream;
-   struct gdb_exception result;
  
    /* FIXME: cagney/2003-02-01: Need to const char *propogate
       safe_execute_command.  */
--- 96,101 ----
***************
*** 109,117 ****
       It is important that it gets reset everytime, since the user could
       set gdb to use a different interpreter.  */
    old_stream = cli_out_set_stream (cli_uiout, gdb_stdout);
!   result = safe_execute_command (cli_uiout, str, 1);
    cli_out_set_stream (cli_uiout, old_stream);
!   return result;
  }
  
  static void
--- 108,116 ----
       It is important that it gets reset everytime, since the user could
       set gdb to use a different interpreter.  */
    old_stream = cli_out_set_stream (cli_uiout, gdb_stdout);
!   safe_execute_command (cli_uiout, str, 1);
    cli_out_set_stream (cli_uiout, old_stream);
!   return exception_none;
  }
  
  static void

Re: [PATCH] interpreter-exec error path

[Daniel Jacobowitz]

On Tue, Sep 12, 2006 at 10:07:46AM +1200, Nick Roberts wrote:
> Yes, I think this does what Andrew Cagney intended but the underlying
> interpreter has already signalled the exception so I think it could be
> handled normally:

There's a FIXME saying that the underlying interpreter shouldn't do
this, if I understand your suggestion properly:

  /* FIXME: cagney/2005-01-13: This shouldn't be needed.  Instead the
     caller should print the exception.  */
  exception_print (gdb_stderr, e);

> Taking things a step further, I see that mi_interpreter_exec always returns
> exception_none so cli_interpreter_exec could do the same (patch below).  The
> command interpreter-exec can handle a list of commands, this would mean if the
> first fails, GDB will still handle the subsequent commands.  This is currently
> true for mi e.g

And indeed, this makes me ask why this would be a desirable feature. 
We stop executing a CLI script if one command fails; I think the same
should apply here?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] interpreter-exec error path

[Nick Roberts]

 > > Yes, I think this does what Andrew Cagney intended but the underlying
 > > interpreter has already signalled the exception so I think it could be
 > > handled normally:
 > 
 > There's a FIXME saying that the underlying interpreter shouldn't do
 > this, if I understand your suggestion properly:
 > 
 >   /* FIXME: cagney/2005-01-13: This shouldn't be needed.  Instead the
 >      caller should print the exception.  */
 >   exception_print (gdb_stderr, e);
 >
 > > Taking things a step further, I see that mi_interpreter_exec always
 > > returns exception_none so cli_interpreter_exec could do the same (patch
 > > below).  The command interpreter-exec can handle a list of commands, this
 > > would mean if the first fails, GDB will still handle the subsequent
 > > commands.  This is currently true for mi e.g
 > 
 > And indeed, this makes me ask why this would be a desirable feature. 

It's like make and "make -k" but I guess the former is the preferred/default
behaviour.

 > We stop executing a CLI script if one command fails; I think the same
 > should apply here?

OK, I'll do that if you're agreeable and remove exception_print so each error
only gets reported once.

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

Re: [PATCH] interpreter-exec error path

[Daniel Jacobowitz]

On Sat, Sep 16, 2006 at 09:34:02PM +1200, Nick Roberts wrote:
>  > > Yes, I think this does what Andrew Cagney intended but the underlying
>  > > interpreter has already signalled the exception so I think it could be
>  > > handled normally:
>  > 
>  > There's a FIXME saying that the underlying interpreter shouldn't do
>  > this, if I understand your suggestion properly:
>  > 
>  >   /* FIXME: cagney/2005-01-13: This shouldn't be needed.  Instead the
>  >      caller should print the exception.  */
>  >   exception_print (gdb_stderr, e);
>  >
>  > > Taking things a step further, I see that mi_interpreter_exec always
>  > > returns exception_none so cli_interpreter_exec could do the same (patch
>  > > below).  The command interpreter-exec can handle a list of commands, this
>  > > would mean if the first fails, GDB will still handle the subsequent
>  > > commands.  This is currently true for mi e.g
>  > 
>  > And indeed, this makes me ask why this would be a desirable feature. 
> 
> It's like make and "make -k" but I guess the former is the preferred/default
> behaviour.
> 
>  > We stop executing a CLI script if one command fails; I think the same
>  > should apply here?
> 
> OK, I'll do that if you're agreeable and remove exception_print so each error
> only gets reported once.

Hi Nick,

I had this message flagged in my inbox, but reading it, I can't
remember why.  Did you need anything from me in this thread?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] interpreter-exec error path

[Nick Roberts]

 > >  > We stop executing a CLI script if one command fails; I think the same
 > >  > should apply here?
 > > 
 > > OK, I'll do that if you're agreeable and remove exception_print so each
 > > error only gets reported once.
 > 
 > Hi Nick,
 > 
 > I had this message flagged in my inbox, but reading it, I can't
 > remember why.  Did you need anything from me in this thread?

Just that you agree to the corresponding change to MI before I submit a patch.

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

Re: [PATCH] interpreter-exec error path

[Daniel Jacobowitz]

On Sat, Nov 18, 2006 at 11:35:23AM +1300, Nick Roberts wrote:
>  > >  > We stop executing a CLI script if one command fails; I think the same
>  > >  > should apply here?
>  > > 
>  > > OK, I'll do that if you're agreeable and remove exception_print so each
>  > > error only gets reported once.
>  > 
>  > Hi Nick,
>  > 
>  > I had this message flagged in my inbox, but reading it, I can't
>  > remember why.  Did you need anything from me in this thread?
> 
> Just that you agree to the corresponding change to MI before I submit a patch.

Is it just the behavior of a second command on the interpreter-exec
line after the first errors?  If so, I thought I had already agreed;
I don't think continuing there is useful.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] interpreter-exec error path

[Nick Roberts]

 > > Just that you agree to the corresponding change to MI before I submit a
 > > patch.
 > 
 > Is it just the behavior of a second command on the interpreter-exec
 > line after the first errors?  If so, I thought I had already agreed;
 > I don't think continuing there is useful.

OK, I'll submit something.

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

Re: [PATCH] interpreter-exec error path

[Daniel Jacobowitz]

On Mon, Sep 11, 2006 at 02:48:33AM +0200, andrzej zaborowski wrote:
> If any of the commands executed through "interpreter-exec" fails, the
> "quiet" value is not restored for the interpreter.  This may result in
> something like the following.
> 
> (gdb) interpreter-exec console rubbish
> Undefined command: "rubbish".  Try "help".
> error in command: "rubbish".
> 
> readline: readline_callback_read_char() called with no handler!
> Aborted

Thanks for the patch!  Regardless of the bigger picture, I think it's
correct for the current code, so I've applied it.

-- 
Daniel Jacobowitz
CodeSourcery

[PATCH] interpreter-exec error path

[andrzej zaborowski]

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

If any of the commands executed through "interpreter-exec" fails, the
"quiet" value is not restored for the interpreter.  This may result in
something like the following.

(gdb) interpreter-exec console rubbish
Undefined command: "rubbish".  Try "help".
error in command: "rubbish".

readline: readline_callback_read_char() called with no handler!
Aborted

The attached patch will handle the error more correctly.
-- 
balrog 2oo6

[-- Attachment #2: gdb-interpreter-exec.patch --]
[-- Type: application/octet-stream, Size: 584 bytes --]

2006-09-09  Andrzej Zaborowski  <balrog@zabor.org>
	* interps.c (interpreter_exec_cmd): Restore interpreter properties.

--- gdb-orig/gdb/interps.c	2006-09-08 06:10:15.000000000 +0000
+++ gdb/gdb/interps.c	2006-09-08 06:04:20.000000000 +0000
@@ -402,9 +402,9 @@ interpreter_exec_cmd (char *args, int fr
       if (e.reason < 0)
 	{
 	  interp_set (old_interp);
-	  interp_set_quiet (interp_to_use, old_quiet);
+	  interp_set_quiet (interp_to_use, use_quiet);
+	  interp_set_quiet (old_interp, old_quiet);
 	  error (_("error in command: \"%s\"."), prules[i]);
-	  break;
 	}
     }