From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30963 invoked by alias); 25 Aug 2018 22:43:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30944 invoked by uid 89); 25 Aug 2018 22:43:17 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=grace, legacy, literally, maintained X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 25 Aug 2018 22:43:14 +0000 Received: by mail-wr1-f65.google.com with SMTP id o37-v6so10332967wrf.6 for ; Sat, 25 Aug 2018 15:43:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=6a6JX7eV9krMd39OiD8E/UKvRBXSJAhuFxWsTd+dsf0=; b=fEBiCD7YJKOmhheUv5DuH4cOtxTuLm4JGCNWkJWaXRQKaruEIoiFJ0dL8TncNQc1uC jg2MNx83xjeMK036Cd7V7S0TFdF4i8X1DmjmuVTcCcKKr7Q2EZP9gPtHmhDkMek++Rx9 49eBkTVtdaDjiXg24uSAyqxnX9qcqba7Hd+cpYoiNzzM0wk0WveAKesgJaCiVT13rCuX zqykKlPDX2apj5B+WK80/lAvvDzEoAQPBuP30I9o7AxNUHBG2fPmjKJ0DdjmYaePVsBz l+z+/6Lwz0JcFg0zbHsSHDviNpWG0WTZGAgDeE6iT2laRWfKAV0jmV6D4ITb2WyeJHBz qZfg== Return-Path: Received: from localhost (host86-134-20-86.range86-134.btcentralplus.com. [86.134.20.86]) by smtp.gmail.com with ESMTPSA id j133-v6sm7733054wmd.12.2018.08.25.15.43.10 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 25 Aug 2018 15:43:10 -0700 (PDT) Date: Sat, 25 Aug 2018 22:43:00 -0000 From: Andrew Burgess To: Philippe Waroquiers Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] gdb: Allow parenthesis to group arguments to user-defined commands Message-ID: <20180825224310.GA32506@embecosm.com> References: <1535225533.1438.5.camel@skynet.be> <1535230403.1438.10.camel@skynet.be> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1535230403.1438.10.camel@skynet.be> X-Fortune: Phone call for chucky-pooh. X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00620.txt.bz2 * Philippe Waroquiers [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