Re: [PATCH 2/2] gdb: Allow parenthesis to group arguments to user-defined commands

Andrew Burgess
2018-08-25T22:43:18Z

* Philippe Waroquiers <philippe.waroquiers@skynet.be> [2018-08-25 22:53:23 +0200]:

> On Sat, 2018-08-25 at 21:32 +0200, Philippe Waroquiers wrote:
> > Woudn't it be more 'usual' (and maybe simpler) to use a \ for this ?
> > e.g. like the shell:
> > ls ab\ cd
> > ls: cannot access 'ab cd': No such file or directory
> >
> > Philippe
> See also the RFA (in 'review needed status')
> https://sourceware.org/ml/gdb-patches/2018-07/msg00131.html
> where I (inconsistently with my comment above but still consistent with the
> shell) have used single quotes for such purposes.
>
> Like for the level versus number, we should aim at a single approach
> in gdb for such things ...

The problem here is that user defined commands already have (IMHO)
weird handling of single/double quotes. So,

my_command 'ab cd'

will replace $arg0 with literally, 'ac cd', including the quotes. The
same for double quotes. Next, there is some strange handling of
backslashes already in place, so,

my_command ac\ cd

Will replace $arg0 with literally, ac\ cd, including the backslash.

Honestly, if there is a logic behind the argument processing, I'm not
seeing it, and the docs don't explain it. It feels like an attempt
was made to do something like you suggest, except it all went horribly
wrong...

However, as far as I can tell, the code has been this way for years, a
quick look back to revision d318976c46b92e4d in year 2000, showed the
same argument processing code unchanged, so my concern is, if we
change things too much now we might break existing scripts....

So, my suggestion deliberately avoids using quotes or backslashes as
these are bogged down in the existing code. And using (...) is fairly
intuitive given GDBs C like expression handling, personally I'd rather
write:

my_command ({unsigned long long} &global_var)

than:

my_command {unsigned\ long\ long}\ &global_var

though, I think you also suggested quotes as a possibility, so this
isn't so bad:

my_command '{unsigned long long} &global_var'

but the C like expression does feel more natural to me. But I could
live with the quotes to get the functionality I need in.

Additionally, using (...) for grouping didn't feel like it was likely
to break much existing code, I convinced myself that it was unlikely
that someone would write:

my_command (ab cd)

and actually want '(ab' and 'cd)' to be separate arguments...

The only saving grace here is that the argument processing for user
defined commands is completely separate, and not used anywhere else.
We could make the choice that this is just a legacy corner of GDB that
has its own rules.

In summary I think that the choices are:

1. Break backward compatibility for use of quote and backslashes,
and rework argument processing for user defined commands.

2. Stick with (...)

3. Define a new scheme that allows most backward compatibility to be
maintained, but extends use backslash and quote to allow for
argument grouping.

Happy to hear more thoughts on this topic,

Thanks,
Andrew

>
> Philippe
>
> >
> >
> > On Wed, 2018-08-15 at 15:39 +0100, Andrew Burgess wrote:
> > > When calling a user-defined command then currently, arguments are
> > > whitespace separated. This means that it is impossible to pass a
> > > single argument that contains a whitespace.
> > >
> > > The exception to the no whitespace rule is strings, a string argument,
> > > enclosed in double, or single quotes, can contain whitespace.
> > >
> > > However, if a user wants to reference, for example, a type name that
> > > contains a space, as in these examples:
> > >
> > > user_command *((unsigned long long *) some_pointer)
> > >
> > > user_command {unsigned long long}some_pointer
> > >
> > > then this will not work, as the whitespace between 'unsigned' and
> > > 'long', as well as the whitespace between 'long' and 'long', will mean
> > > GDB interprets this as many arguments.
> > >
> > > The solution proposed in this patch is to allow parenthesis to be used
> > > to group arguments, so the use could now write:
> > >
> > > user_command (*((unsigned long long *) some_pointer))
> > >
> > > user_command ({unsigned long long}some_pointer)
> > >
> > > And GDB will interpret these as a single argument.
> > >
> > > gdb/ChangeLog:
> > >
> > > * cli/cli-script.c (user_args::user_args): Allow parenthesis to
> > > group arguments.
> > >
> > > gdb/testsuite/ChangeLog:
> > >
> > > * gdb.base/commands.exp (args_with_whitespace): New proc, which is
> > > added to the list of procs to call.
> > > * gdb.base/run.c (global_var): Defined global.
> > >
> > > gdb/doc/ChangeLog:
> > >
> > > * gdb.texinfo (Define): Additional documentation about argument
> > > syntax.
> > > ---
> > > gdb/ChangeLog | 5 ++++
> > > gdb/cli/cli-script.c | 8 +++++-
> > > gdb/doc/ChangeLog | 5 ++++
> > > gdb/doc/gdb.texinfo | 54 +++++++++++++++++++++++++++++++++----
> > > gdb/testsuite/ChangeLog | 6 +++++
> > > gdb/testsuite/gdb.base/commands.exp | 36 +++++++++++++++++++++++++
> > > gdb/testsuite/gdb.base/run.c | 3 +++
> > > 7 files changed, 111 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
> > > index 6f31a400197..fd80ab9fbcf 100644
> > > --- a/gdb/cli/cli-script.c
> > > +++ b/gdb/cli/cli-script.c
> > > @@ -758,6 +758,7 @@ user_args::user_args (const char *command_line)
> > > int squote = 0;
> > > int dquote = 0;
> > > int bsquote = 0;
> > > + int pdepth = 0;
> > >
> > > /* Strip whitespace. */
> > > while (*p == ' ' || *p == '\t')
> > > @@ -769,7 +770,8 @@ user_args::user_args (const char *command_line)
> > > /* Get to the end of this argument. */
> > > while (*p)
> > > {
> > > - if (((*p == ' ' || *p == '\t')) && !squote && !dquote && !bsquote)
> > > + if (((*p == ' ' || *p == '\t'))
> > > + && !squote && !dquote && !bsquote && pdepth == 0)
> > > break;
> > > else
> > > {
> > > @@ -793,6 +795,10 @@ user_args::user_args (const char *command_line)
> > > squote = 1;
> > > else if (*p == '"')
> > > dquote = 1;
> > > + else if (*p == '(')
> > > + pdepth++;
> > > + else if (*p == ')')
> > > + pdepth--;
> > > }
> > > p++;
> > > }
> > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> > > index 433a2698a92..11cbef97b8f 100644
> > > --- a/gdb/doc/gdb.texinfo
> > > +++ b/gdb/doc/gdb.texinfo
> > > @@ -25219,14 +25219,58 @@
> > >
> > > @noindent
> > > This defines the command @code{adder}, which prints the sum of
> > > -its three arguments. Note the arguments are text substitutions, so they may
> > > -reference variables, use complex expressions, or even perform inferior
> > > -functions calls.
> > > +its three arguments.
> > > +
> > > +The arguments to user-defined commands are text substitutions, so they
> > > +may reference variables, use complex expressions, or even perform
> > > +inferior functions calls. Each argument is separated with whitespace,
> > > +so in the previous example three arguments were passed. The following
> > > +example also passes three arguments, though the arguments are more
> > > +complex:
> > > +
> > > +@smallexample
> > > +adder 10+1 10+2 10+3
> > > +@end smallexample
> > > +
> > > +@noindent
> > > +However, if whitespace were added around the @code{+} characters, then
> > > +9 arguments would be passed, @code{adder} only uses the first 3 of
> > > +these arguments, and the others would be silently ignored:
> > > +
> > > +@smallexample
> > > +adder 10 + 1 10 + 2 10 + 3
> > > +@end smallexample
> > > +
> > > +@noindent
> > > +Parenthesis can be uses to group complex expressions that include
> > > +whitespace into a single argument, so the previous example can be
> > > +modified to pass just 3 arguments again, like this:
> > > +
> > > +@smallexample
> > > +adder (10 + 1) (10 + 2) (10 + 3)
> > > +@end smallexample
> > > +
> > > +@noindent
> > > +The parenthesis are passed through as part of the argument, so the
> > > +previous example causes @value{GDBN} to evaluate:
> > > +
> > > +@smallexample
> > > +print (10 + 1) + (10 + 2) + (10 + 3)
> > > +@end smallexample
> > > +
> > > +@noindent
> > > +Nested parenthesis are also allowed within an argument, in the
> > > +following example 3 arguments are still passed:
> > > +
> > > +@smallexample
> > > +adder (10 + 1) (10 + (1 + 1)) (10 + (1 + (1 + 1)))
> > > +@end smallexample
> > >
> > > @cindex argument count in user-defined commands
> > > @cindex how many arguments (user-defined commands)
> > > -In addition, @code{$argc} may be used to find out how many arguments have
> > > -been passed.
> > > +@noindent
> > > +Within a user-defined command @code{$argc} may be used to find out how
> > > +many arguments have been passed.
> > >
> > > @smallexample
> > > define adder
> > > diff --git a/gdb/testsuite/gdb.base/commands.exp b/gdb/testsuite/gdb.base/commands.exp
> > > index 7ce33fdefa9..42ffd6fa0af 100644
> > > --- a/gdb/testsuite/gdb.base/commands.exp
> > > +++ b/gdb/testsuite/gdb.base/commands.exp
> > > @@ -1125,6 +1125,41 @@ proc_with_prefix backslash_in_multi_line_command_test {} {
> > > gdb_test "print 1" "" "run command"
> > > }
> > >
> > > +proc_with_prefix args_with_whitespace {} {
> > > + gdb_test_multiple "define show_args" "define show_args" {
> > > + -re "End with" {
> > > + pass "define show_args"
> > > + }
> > > + }
> > > +
> > > + # This test should alternate between 0xdeadbeef and 0xfeedface two times.
> > > + gdb_test \
> > > + [multi_line_input \
> > > + {printf "nargs=%d:", $argc} \
> > > + {set $i = 0} \
> > > + {while $i < $argc} \
> > > + {printf " "} \
> > > + {eval "echo '$arg%d'", $i} \
> > > + {set $i = $i + 1} \
> > > + {end} \
> > > + {printf "\n"} \
> > > + {end}] \
> > > + "" \
> > > + "enter commands"
> > > +
> > > + gdb_test "show_args 1 2 3" \
> > > + "nargs=3: '1' '2' '3'"
> > > +
> > > + gdb_test "show_args 1 (1 + 1) (1 + (1 + 1))" \
> > > + "nargs=3: '1' '\\(1 \\+ 1\\)' '\\(1 \\+ \\(1 \\+ 1\\)\\)'"
> > > +
> > > + gdb_test "show_args ({unsigned long long} &global_var)" \
> > > + "nargs=1: '\\({unsigned long long} &global_var\\)'"
> > > +
> > > + gdb_test "show_args (*((unsigned long long *) &global_var))" \
> > > + "nargs=1: '\\(\\*\\(\\(unsigned long long \\*\\) &global_var\\)\\)'"
> > > +}
> > > +
> > > gdbvar_simple_if_test
> > > gdbvar_simple_while_test
> > > gdbvar_complex_if_while_test
> > > @@ -1154,5 +1189,6 @@ backslash_in_multi_line_command_test
> > > define_if_without_arg_test
> > > loop_break_test
> > > loop_continue_test
> > > +args_with_whitespace
> > > # This one should come last, as it redefines "backtrace".
> > > redefine_backtrace_test
> > > diff --git a/gdb/testsuite/gdb.base/run.c b/gdb/testsuite/gdb.base/run.c
> > > index 614b018260d..d89bad78bb4 100644
> > > --- a/gdb/testsuite/gdb.base/run.c
> > > +++ b/gdb/testsuite/gdb.base/run.c
> > > @@ -8,6 +8,9 @@
> > >
> > > #include "../lib/unbuffer_output.c"
> > >
> > > +/* Used by commands.exp test script. */
> > > +volatile unsigned long long global_var = 34;
> > > +
> > > int factorial (int);
> > >
> > > int