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

Joel Brobecker
2013-12-02T03:26:30Z

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