Re: [PATCH] PR cli/21688: Fix multi-line/inline command differentiation

Sergio Durigan Junior
2017-06-29T19:48:54Z

On Thursday, June 29 2017, Simon Marchi wrote:

> On 2017-06-29 04:05, Sergio Durigan Junior wrote:
>> This bug is a regression caused by the following commit:
>>
>> 604c4576fdcfc4e7c28f569b3748a1b6b4e0dbd4 is the first bad commit
>> commit 604c4576fdcfc4e7c28f569b3748a1b6b4e0dbd4
>> Author: Jerome Guitton <guitton@adacore.com>
>> Date: Tue Jan 10 15:15:53 2017 +0100
>>
>> The problem happens because, on cli/cli-script.c:process_next_line,
>> GDB is not using the command line string to identify which command to
>> run, but it instead using the 'struct cmd_list_element *' that is
>> obtained by using the mentioned string. The problem with that is that
>> the 'struct cmd_list_element *' doesn't have any information on
>> whether the command issued by the user is a multi-line or inline one.
>>
>> A multi-line command is a command that will necessarily be composed of
>> more than 1 line. For example:
>>
>> (gdb) if 1
>> >python
>> >print ('hello')
>> >end
>> >end
>>
>> As can be seen in the example above, the 'python' command actually
>> "opens" a new command line (represented by the change in the
>> indentation) that will then be used to enter Python code. OTOH, an
>> inline command is a command that is "self-contained" in a single line,
>> for example:
>>
>> (gdb) if 1
>> >python print ('hello')
>> >end
>>
>> This Python command is a one-liner, and therefore there is no other
>> Python code that can be entered for this same block. There is also no
>> change in the indentation.
>>
>> So, the fix is somewhat simple: we have to revert the change and use
>> the full command line string passed to process_next_line in order to
>> identify whether we're dealing with a multi-line or an inline command.
>> This commit does just that. As can be seen, this regression also
>> affects other languages, like guile or the compile framework. To make
>> things clearer, I decided to create a new helper function responsible
>> for identifying a non-inline command.
>>
>> Testcase is attached.
>
> Hi Sergio,
>
> Thanks for the fix and the test! I have a few comments below.

Hey Simon,

Thanks for the review!

>> gdb/ChangeLog:
>> 2017-06-29 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> PR cli/21688
>> * cli/cli-script.c (command_name_equals_not_inline): New function.
>> (process_next_line): Adjust 'if' clauses for "python", "compile"
>> and "guile" to use command_name_equals_not_inline.
>>
>> gdb/testsuite/ChangeLog:
>> 2017-06-29 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> PR cli/21688
>> * gdb.python/py-cmd.exp (test_pr21688): New procedure. Call it.
>> ---
>> gdb/ChangeLog | 7 +++++++
>> gdb/cli/cli-script.c | 21 +++++++++++++++++----
>> gdb/testsuite/ChangeLog | 5 +++++
>> gdb/testsuite/gdb.python/py-cmd.exp | 30
>> ++++++++++++++++++++++++++++++
>> 4 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index a82026f..194cda8 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,10 @@
>> +2017-06-29 Sergio Durigan Junior <sergiodj@redhat.com>
>> +
>> + PR cli/21688
>> + * cli/cli-script.c (command_name_equals_not_inline): New function.
>> + (process_next_line): Adjust 'if' clauses for "python", "compile"
>> + and "guile" to use command_name_equals_not_inline.
>> +
>> 2017-06-28 Pedro Alves <palves@redhat.com>
>>
>> * command.h: Include "common/scoped_restore.h".
>> diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
>> index e0e27ef..72f316f 100644
>> --- a/gdb/cli/cli-script.c
>> +++ b/gdb/cli/cli-script.c
>> @@ -900,6 +900,20 @@ command_name_equals (struct cmd_list_element
>> *cmd, const char *name)
>> && strcmp (cmd->name, name) == 0);
>> }
>>
>> +/* Return true if NAME is the only command between COMMAND_START and
>> + COMMAND_END. This is useful when we want to know whether the
>> + command is inline (i.e., has arguments like 'python command1') or
>> + is the start of a multi-line command block. */
>> +
>> +static bool
>> +command_name_equals_not_inline (const char *command_start,
>> + const char *command_end,
>> + const char *name)
>> +{
>> + return (command_end - command_start == strlen (name)
>> + && startswith (command_start, name));
>> +}
>> +
>> /* Given an input line P, skip the command and return a pointer to the
>> first argument. */
>>
>> @@ -997,21 +1011,20 @@ process_next_line (char *p, struct command_line
>> **command, int parse_commands,
>> {
>> *command = build_command_line (commands_control,
>> line_first_arg (p));
>> }
>> - else if (command_name_equals (cmd, "python"))
>> + else if (command_name_equals_not_inline (p_start, p_end,
>> "python"))
>
> Another (maybe simpler) way would be to check
>
> else if (command_name_equals (cmd, "python") && *cmd_name == '\0')
>
> It's not clear when expressed like this though because cmd_name is not
> well named at this point (it points just after the command name).

Hm, right. Would you prefer this way instead? I don't have a strong
opinion on this.

>
>> {
>> /* Note that we ignore the inline "python command" form
>> here. */
>> *command = build_command_line (python_control, "");
>> }
>> - else if (command_name_equals (cmd, "compile"))
>> + else if (command_name_equals_not_inline (p_start, p_end,
>> "compile"))
>> {
>> /* Note that we ignore the inline "compile command" form
>> here. */
>> *command = build_command_line (compile_control, "");
>> (*command)->control_u.compile.scope = COMPILE_I_INVALID_SCOPE;
>> }
>> -
>> - else if (command_name_equals (cmd, "guile"))
>> + else if (command_name_equals_not_inline (p_start, p_end,
>> "guile"))
>> {
>> /* Note that we ignore the inline "guile command" form here. */
>> *command = build_command_line (guile_control, "");
>> diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
>> index b7462a5..ef46a5d 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2017-06-29 Sergio Durigan Junior <sergiodj@redhat.com>
>> +
>> + PR cli/21688
>> + * gdb.python/py-cmd.exp (test_pr21688): New procedure. Call it.
>> +
>> 2017-06-28 Doug Gilmore <Doug.Gilmore@imgtec.com>
>>
>> PR gdb/21337
>> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp
>> b/gdb/testsuite/gdb.python/py-cmd.exp
>> index 2dbf23ce..052afa4 100644
>> --- a/gdb/testsuite/gdb.python/py-cmd.exp
>> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
>> @@ -181,6 +181,36 @@ gdb_test "complete expr_test bar\." \
>> "expr_test bar\.bc.*expr_test bar\.ij.*" \
>> "test completion through complete command"
>>
>> +# Testing PR cli/21688. This is not language-specific, but the
>> +# easiest way is just to test with Python.
>> +proc test_pr21688 { } {
>
> I am not a fan of naming procs and documenting things solely based on
> PR numbers, it's cryptic and requires to go check the web page to know
> what this is for. I'd prefer a short description (e.g. Test that the
> "python" command is correctly recognized as an inline or multi-line
> command when entering a sequence of commands, something like that) and
> an appropriate name. Mentioning the PR in the comment is still good
> though, if the reader wants to know the context in which this was
> added.

Sure thing, I'll change the proc name and make sure to mention the PR in
the comments.

>
>> + set define_cmd_not_inline {
>> + { "if 1" " >$" }
>> + { "python" " >$" }
>> + { "print ('hello')" " >$" }
>> + { "end" " >$" }
>> + { "end" "hello\r\n" } }
>> +
>> + set define_cmd_inline {
>> + { "if 1" " >$" }
>> + { "python print ('hello')" " >$" }
>> + { "end" "hello\r\n" } }
>> +
>> + foreach t [list $define_cmd_not_inline $define_cmd_inline] {
>> + foreach l $t {
>> + foreach { command regex } $l {
>
> I didn't understand this last foreach at first, but IIUC it's for
> unpacking $l? An alternative that might be clearer is lassign:
>
> lassign $l command regex

Yeah, it is for unpacking $l. Indeed, lassign makes it clearer, I'll
use that.

>
>> + gdb_test_multiple "$command" "$command" {
>
> Watch out, this will make some test names that end with parenthesis,
> and a few of them will be non-unique.

Hm, good point. I'll review the test messages.

I'll send a v2 soon.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible