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

[Simon Marchi <simon.marchi@polymtl.ca>]

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.

> 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).

>  	{
>  	  /* 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.

> +    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

> +		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.

> +		    -re "$regex" {
> +			pass "$command"
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +}
> +
> +test_pr21688
> +
>  if { [readline_is_used] } {
>      set test "complete 'expr_test bar.i'"
>      send_gdb "expr_test bar\.i\t\t"

Thanks,

Simon