Re: [RFA 1/2] New GDB/MI command "-info-gdb-mi-command"

[Joel Brobecker <brobecker at adacore dot com>]

Kind request for review. The code itself is fairly straightforward,
I think. It's more about making sure that we're defining the right
command...

Thank you!

On Tue, Nov 19, 2013 at 08:10:22AM +0400, Joel Brobecker wrote:
> > > I now think that it was indeed the correct choice.  Not only does it
> > > facilitate implementation (but only marginally), it also is consistent
> > > with the current output.  For instance, notice how GDB names the command
> > > in the following error message:
> > > 
> > >     -unsupported
> > >     ^error,msg="Undefined MI command: unsupported"
> > >                                       ^^^^^^^^^^^
> > >                                     (no leading dash)
> > 
> > Your example shows _output_ from MI.  By contrast, we are talking
> > about _input_.  When I send commands to MI, I cannot omit the leading
> > dash, so it can be very natural to consider it part of the command.
> > 
> > We don't have to advertise that we support the dash, 
> > 
> > > Also, looking at the grammar, the leading dash isn't listed
> > > as part of what they call the "operation"
> > 
> > IMO, this line of reasoning makes little sense to users.  Grammars are
> > for programs, not for people.
> 
> To me, documentation is not an issue.  I confess that I remain
> unconvinced in this case, especially since this is a command meant
> for programs rather than humans, so the risk of using it improperly
> is low, given the clear documentation.  However, I don't have a strong
> opinion, and supporting both forms is pretty easy, so unless someone
> strongly objects to allowing the second form, I've just gone ahead and
> added it.
> 
> Updated patch attached. And for review convenience, I am also attaching
> a diff of the changes I made on top of path #1 (to get to the updated
> patch).
> 
> gdb/ChangeLog:
> 
>         * mi/mi-cmds.h (mi_cmd_info_gdb_mi_command): Declare.
>         * mi/mi-cmd-info.c (mi_cmd_info_gdb_mi_command): New function.
>         * mi/mi-cmds.c (mi_cmds): Add -info-gdb-mi-command command.
>         * mi/mi-main.c (mi_cmd_list_features): Add "info-gdb-mi-command"
>         field to output of "-list-features".
> 
>         * NEWS: Add entry for new -info-gdb-mi-command.
> 
> gdb/doc/ChangeLog:
> 
>         * gdb.texinfo (GDB/MI Miscellaneous Commands): Document
>         the new -info-gdb-mi-command GDB/MI command.  Document
>         the meaning of "-info-gdb-mi-command" in the output of
>         -list-features.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.mi/mi-i-cmd.exp: New file.
> 
> Re-tested on x86_64-linux.  OK to commit?
> 
> Thank you,
> -- 
> Joel

> From 5a8ff8bf85f7d455c3f75312e30ec5c1e077ae01 Mon Sep 17 00:00:00 2001
> From: Joel Brobecker <brobecker@adacore.com>
> Date: Tue, 12 Nov 2013 14:51:30 +0400
> Subject: [PATCH] New GDB/MI command "-info-gdb-mi-command"
> 
> This patch adds a new GDB/MI command meant for graphical frontends
> trying to determine whether a given GDB/MI command exists or not.
> 
> Examples:
> 
>     -info-gdb-mi-command unsupported-command
>     ^done,command={exists="false"}
>     (gdb)
>     -info-gdb-mi-command symbol-list-lines
>     ^done,command={exists="true"}
>     (gdb)
> 
> At the moment, this is the only piece of information that this
> command returns.
> 
> Eventually, and if needed, we can extend it to provide
> command-specific pieces of information, such as updates to
> the command's syntax since inception.  This could become,
> for instance:
> 
>     -info-gdb-mi-command symbol-list-lines
>     ^done,command={exists="true",features=[]}
>     (gdb)
>     -info-gdb-mi-command catch-assert
>     ^done,command={exists="true",features=["conditions"]}
> 
> In the first case, it would mean that no extra features,
> while in the second, it announces that the -catch-assert
> command in this version of the debugger supports a feature
> called "condition" - exact semantics to be documented with
> combined with the rest of the queried command's documentation.
> 
> But for now, we start small, and only worry about existance.
> And to bootstrap the process, I have added an entry in the
> output of the -list-features command as well ("info-gdb-mi-command"),
> allowing the graphical frontends to go through the following process:
> 
>   1. Send -list-features, collect info from there as before;
>   2. Check if the output contains "info-gdb-mi-command".
>      If it does, then support for various commands can be
>      queried though -info-gdb-mi-command. Newer commands
>      will be expected to always be checked via this new
>      -info-gdb-mi-command.
> 
> gdb/ChangeLog:
> 
>         * mi/mi-cmds.h (mi_cmd_info_gdb_mi_command): Declare.
>         * mi/mi-cmd-info.c (mi_cmd_info_gdb_mi_command): New function.
>         * mi/mi-cmds.c (mi_cmds): Add -info-gdb-mi-command command.
>         * mi/mi-main.c (mi_cmd_list_features): Add "info-gdb-mi-command"
>         field to output of "-list-features".
> 
>         * NEWS: Add entry for new -info-gdb-mi-command.
> 
> gdb/doc/ChangeLog:
> 
>         * gdb.texinfo (GDB/MI Miscellaneous Commands): Document
>         the new -info-gdb-mi-command GDB/MI command.  Document
>         the meaning of "-info-gdb-mi-command" in the output of
>         -list-features.
> 
> gdb/testsuite/ChangeLog:
> 
>         * gdb.mi/mi-i-cmd.exp: New file.
> ---
>  gdb/NEWS                          |  3 +++
>  gdb/doc/gdb.texinfo               | 53 +++++++++++++++++++++++++++++++++++++++
>  gdb/mi/mi-cmd-info.c              | 29 +++++++++++++++++++++
>  gdb/mi/mi-cmds.c                  |  1 +
>  gdb/mi/mi-cmds.h                  |  1 +
>  gdb/mi/mi-main.c                  |  1 +
>  gdb/testsuite/gdb.mi/mi-i-cmd.exp | 46 +++++++++++++++++++++++++++++++++
>  7 files changed, 134 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-i-cmd.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 9fc3638..e61c79f 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -153,6 +153,9 @@ show startup-with-shell
>  
>    ** All MI commands now accept an optional "--language" option.
>  
> +  ** The new command -info-gdb-mi-command allows the user to determine
> +     whether a GDB/MI command is supported or not.
> +
>    ** The -trace-save MI command can optionally save trace buffer in Common
>       Trace Format now.
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 19e9aa5..41856b4 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -35069,6 +35069,57 @@ default shows this information when you start an interactive session.
>  (gdb)
>  @end smallexample
>  
> +@subheading The @code{-info-gdb-mi-command} Command
> +@cindex @code{-info-gdb-mi-command}
> +@findex -info-gdb-mi-command
> +
> +@subsubheading Synopsis
> +
> +@smallexample
> + -info-gdb-mi-command @var{cmd_name}
> +@end smallexample
> +
> +Query support for the @sc{gdb/mi} command named @var{cmd_name}.
> +
> +Note that the dash (@code{-}) starting all @sc{gdb/mi} commands
> +is technically not part of the command name (@pxref{GDB/MI Input
> +Syntax}), and thus should be omitted in @var{cmd_name}.  However,
> +for ease of use, this command also accepts the form with the leading
> +dash.
> +
> +@subsubheading @value{GDBN} Command
> +
> +There is no corresponding @value{GDBN} command.
> +
> +@subsubheading Result
> +
> +The result is a tuple.  There is currently only one field:
> +
> +@table @samp
> +@item exists
> +This field is equal to @code{"true"} if the @sc{gdb/mi} command exists,
> +@code{"false"} otherwise.
> +
> +@end table
> +
> +@subsubheading Example
> +
> +Here is an example where the @sc{gdb/mi} command does not exist:
> +
> +@smallexample
> +-info-gdb-mi-command unsupported-command
> +^done,command=@{exists="false"@}
> +@end smallexample
> +
> +@noindent
> +And here is an example where the @sc{gdb/mi} command is known
> +to the debugger:
> +
> +@smallexample
> +-info-gdb-mi-command symbol-list-lines
> +^done,command=@{exists="true"@}
> +@end smallexample
> +
>  @subheading The @code{-list-features} Command
>  @findex -list-features
>  
> @@ -35122,6 +35173,8 @@ exceptions: @code{-info-ada-exceptions}, @code{-catch-assert} and
>  @item language-option
>  Indicates that all @sc{gdb/mi} commands accept the @option{--language}
>  option (@pxref{Context management}).
> +@item info-gdb-mi-command
> +Indicates support for the @code{-info-gdb-mi-command} command.
>  @end table
>  
>  @subheading The @code{-list-target-features} Command
> diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
> index aa4d210..0fce25a 100644
> --- a/gdb/mi/mi-cmd-info.c
> +++ b/gdb/mi/mi-cmd-info.c
> @@ -71,6 +71,35 @@ mi_cmd_info_ada_exceptions (char *command, char **argv, int argc)
>    do_cleanups (old_chain);
>  }
>  
> +/* Implement the "-info-gdb-mi-command" GDB/MI command.  */
> +
> +void
> +mi_cmd_info_gdb_mi_command (char *command, char **argv, int argc)
> +{
> +  const char *cmd_name;
> +  struct mi_cmd *cmd;
> +  struct ui_out *uiout = current_uiout;
> +  struct cleanup *old_chain;
> +
> +  /* This command takes exactly one argument.  */
> +  if (argc != 1)
> +    error (_("Usage: -info-gdb-mi-command MI_COMMAND_NAME"));
> +  cmd_name = argv[0];
> +
> +  /* Normally, the command name (aka the "operation" in the GDB/MI
> +     grammar), does not include the leading '-' (dash).  But for
> +     the user's convenience, allow the user to specify the command
> +     name to be with or without that leading dash.  */
> +  if (cmd_name[0] == '-')
> +    cmd_name++;
> +
> +  cmd = mi_lookup (cmd_name);
> +
> +  old_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "command");
> +  ui_out_field_string (uiout, "exists", cmd != NULL ? "true" : "false");
> +  do_cleanups (old_chain);
> +}
> +
>  void
>  mi_cmd_info_os (char *command, char **argv, int argc)
>  {
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index c536d8a..58a8b89 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -125,6 +125,7 @@ static struct mi_cmd mi_cmds[] =
>    DEF_MI_CMD_MI ("inferior-tty-set", mi_cmd_inferior_tty_set),
>    DEF_MI_CMD_MI ("inferior-tty-show", mi_cmd_inferior_tty_show),
>    DEF_MI_CMD_MI ("info-ada-exceptions", mi_cmd_info_ada_exceptions),
> +  DEF_MI_CMD_MI ("info-gdb-mi-command", mi_cmd_info_gdb_mi_command),
>    DEF_MI_CMD_MI ("info-os", mi_cmd_info_os),
>    DEF_MI_CMD_MI ("interpreter-exec", mi_cmd_interpreter_exec),
>    DEF_MI_CMD_MI ("list-features", mi_cmd_list_features),
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index cb8aac1..4ea95fa 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -74,6 +74,7 @@ extern mi_cmd_argv_ftype mi_cmd_gdb_exit;
>  extern mi_cmd_argv_ftype mi_cmd_inferior_tty_set;
>  extern mi_cmd_argv_ftype mi_cmd_inferior_tty_show;
>  extern mi_cmd_argv_ftype mi_cmd_info_ada_exceptions;
> +extern mi_cmd_argv_ftype mi_cmd_info_gdb_mi_command;
>  extern mi_cmd_argv_ftype mi_cmd_info_os;
>  extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
>  extern mi_cmd_argv_ftype mi_cmd_list_features;
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index 793204d..48c8d09 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -1817,6 +1817,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
>        ui_out_field_string (uiout, NULL, "ada-task-info");
>        ui_out_field_string (uiout, NULL, "ada-exceptions");
>        ui_out_field_string (uiout, NULL, "language-option");
> +      ui_out_field_string (uiout, NULL, "info-gdb-mi-command");
>        
>  #if HAVE_PYTHON
>        if (gdb_python_initialized)
> diff --git a/gdb/testsuite/gdb.mi/mi-i-cmd.exp b/gdb/testsuite/gdb.mi/mi-i-cmd.exp
> new file mode 100644
> index 0000000..d460559
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-i-cmd.exp
> @@ -0,0 +1,46 @@
> +# Copyright 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +load_lib mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +gdb_exit
> +if [mi_gdb_start] {
> +    continue
> +}
> +
> +# First, verify that the debugger correctly advertises support
> +# for the -info-gdb-mi-command command.
> +mi_gdb_test "-list-features" \
> +            "\\^done,features=\\\[.*\"info-gdb-mi-command\".*\\\]" \
> +            "-list-features should include \"info-gdb-mi-command\""
> +
> +mi_gdb_test "-info-gdb-mi-command unsupported-command" \
> +            "\\^done,command=\\\{exists=\"false\"\\\}" \
> +            "-info-gdb-mi-command unsupported-command"
> +
> +# Same test as above, but including the leading '-' in the command name.
> +mi_gdb_test "-info-gdb-mi-command -unsupported-command" \
> +            "\\^done,command=\\\{exists=\"false\"\\\}" \
> +            "-info-gdb-mi-command -unsupported-command"
> +
> +mi_gdb_test "-info-gdb-mi-command symbol-list-lines" \
> +            "\\^done,command=\\\{exists=\"true\"\\\}" \
> +            "-info-gdb-mi-command symbol-list-lines"
> +
> +# Same test as above, but including the leading '-' in the command name.
> +mi_gdb_test "-info-gdb-mi-command -symbol-list-lines" \
> +            "\\^done,command=\\\{exists=\"true\"\\\}" \
> +            "-info-gdb-mi-command -symbol-list-lines"
> -- 
> 1.8.1.2
> 

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f0662ff..4227f31 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -35088,8 +35088,13 @@ default shows this information when you start an interactive session.
>   -info-gdb-mi-command @var{cmd_name}
>  @end smallexample
>  
> -Query support for the @sc{gdb/mi} command named @var{cmd_name}
> -(the leading dash (@code{-}) in the command name should be omitted).
> +Query support for the @sc{gdb/mi} command named @var{cmd_name}.
> +
> +Note that the dash (@code{-}) starting all @sc{gdb/mi} commands
> +is technically not part of the command name (@pxref{GDB/MI Input
> +Syntax}), and thus should be omitted in @var{cmd_name}.  However,
> +for ease of use, this command also accepts the form with the leading
> +dash.
>  
>  @subsubheading @value{GDBN} Command
>  
> @@ -35120,6 +35120,7 @@ Here is an example where the @sc{gdb/mi} command does not exist:
>  ^done,command=@{exists="false"@}
>  @end smallexample
>  
> +@noindent
>  And here is an example where the @sc{gdb/mi} command is known
>  to the debugger:
>  
> diff --git a/gdb/mi/mi-cmd-info.c b/gdb/mi/mi-cmd-info.c
> index 18f4927..0fce25a 100644
> --- a/gdb/mi/mi-cmd-info.c
> +++ b/gdb/mi/mi-cmd-info.c
> @@ -85,6 +85,14 @@ mi_cmd_info_gdb_mi_command (char *command, char **argv, int argc)
>    if (argc != 1)
>      error (_("Usage: -info-gdb-mi-command MI_COMMAND_NAME"));
>    cmd_name = argv[0];
> +
> +  /* Normally, the command name (aka the "operation" in the GDB/MI
> +     grammar), does not include the leading '-' (dash).  But for
> +     the user's convenience, allow the user to specify the command
> +     name to be with or without that leading dash.  */
> +  if (cmd_name[0] == '-')
> +    cmd_name++;
> +
>    cmd = mi_lookup (cmd_name);
>  
>    old_chain = make_cleanup_ui_out_tuple_begin_end (uiout, "command");
> diff --git a/gdb/testsuite/gdb.mi/mi-i-cmd.exp b/gdb/testsuite/gdb.mi/mi-i-cmd.exp
> index 5285d31..d460559 100644
> --- a/gdb/testsuite/gdb.mi/mi-i-cmd.exp
> +++ b/gdb/testsuite/gdb.mi/mi-i-cmd.exp
> @@ -31,7 +31,16 @@ mi_gdb_test "-info-gdb-mi-command unsupported-command" \
>              "\\^done,command=\\\{exists=\"false\"\\\}" \
>              "-info-gdb-mi-command unsupported-command"
>  
> +# Same test as above, but including the leading '-' in the command name.
> +mi_gdb_test "-info-gdb-mi-command -unsupported-command" \
> +            "\\^done,command=\\\{exists=\"false\"\\\}" \
> +            "-info-gdb-mi-command -unsupported-command"
> +
>  mi_gdb_test "-info-gdb-mi-command symbol-list-lines" \
>              "\\^done,command=\\\{exists=\"true\"\\\}" \
>              "-info-gdb-mi-command symbol-list-lines"
>  
> +# Same test as above, but including the leading '-' in the command name.
> +mi_gdb_test "-info-gdb-mi-command -symbol-list-lines" \
> +            "\\^done,command=\\\{exists=\"true\"\\\}" \
> +            "-info-gdb-mi-command -symbol-list-lines"


-- 
Joel