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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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
http://sergiodj.net/