Re: [PATCH] Simplify interp::exec / interp_exec - let exceptions propagate

[Kevin Buettner <kevinb@redhat.com>]

On Fri, 27 Jan 2023 23:05:45 +0000
Pedro Alves <pedro@palves.net> wrote:

> This patch implements a simplication that I suggested here:
> 
>   https://sourceware.org/pipermail/gdb-patches/2022-March/186320.html
> 
> Currently, the interp::exec virtual method interface is such that
> subclass implementations must catch exceptions and then return them
> via normal function return.
> 
> However, higher up the in chain, for the CLI we get to
> interpreter_exec_cmd, which does:
> 
>   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, 0);
> 	  error (_("error in command: \"%s\"."), prules[i]);
> 	}
>     }
> 
> and for MI we get to mi_cmd_interpreter_exec, which has:
> 
>   void
>   mi_cmd_interpreter_exec (const char *command, char **argv, int argc)
>   {
>   ...
>     for (i = 1; i < argc; i++)
>       {
> 	struct gdb_exception e = interp_exec (interp_to_use, argv[i]);
> 
> 	if (e.reason < 0)
> 	  error ("%s", e.what ());
>       }
>   }
> 
> Note that if those errors are reached, we lose the original
> exception's error code.  I can't see why we'd want that.
> 
> And, I can't see why we need to have interp_exec catch the exception
> and return it via the normal return path.  That's normally needed when
> we need to handle propagating exceptions across C code, like across
> readline or ncurses, but that's not the case here.
> 
> It seems to me that we can simplify things by removing some
> try/catch-ing and just letting exceptions propagate normally.
> 
> Note, the "error in command" error shown above, which only exists in
> the CLI interpreter-exec command, is only ever printed AFAICS if you
> run "interpreter-exec console" when the top level interpreter is
> already the console/tui.  Like:
> 
>  (gdb) interpreter-exec console "foobar"
>  Undefined command: "foobar".  Try "help".
>  error in command: "foobar".
> 
> You won't see it with MI's "-interpreter-exec console" from a top
> level MI interpreter:
> 
>  (gdb)
>  -interpreter-exec console "foobar"
>  &"Undefined command: \"foobar\".  Try \"help\".\n"
>  ^error,msg="Undefined command: \"foobar\".  Try \"help\"."
>  (gdb)
> 
> nor with MI's "-interpreter-exec mi" from a top level MI interpreter:
> 
>  (gdb)
>  -interpreter-exec mi "-foobar"
>  ^error,msg="Undefined MI command: foobar",code="undefined-command"
>  ^done
>  (gdb)
> 
> in both these cases because MI's -interpreter-exec just does:
> 
>   error ("%s", e.what ());
> 
> You won't see it either when running an MI command with the CLI's
> "interpreter-exec mi":
> 
>  (gdb) interpreter-exec mi "-foobar"
>  ^error,msg="Undefined MI command: foobar",code="undefined-command"
>  (gdb)
> 
> This last case is because MI's interp::exec implementation never
> returns an error:
> 
>  gdb_exception
>  mi_interp::exec (const char *command)
>  {
>    mi_execute_command_wrapper (command);
>    return gdb_exception ();
>  }
> 
> Thus I think that "error in command" error is pretty pointless, and
> since it simplifies things to not have it, the patch just removes it.
> 
> The patch also ends up addressing an old FIXME.

Thanks for doing this as it'll simplify part of my "Fix
gdb.base/gdb-sigterm.exp failure/error" series.

LGTM.

Kevin