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

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

[Sergio Durigan Junior]

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.

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"))
 	{
 	  /* 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 { } {
+    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 {
+		gdb_test_multiple "$command" "$command" {
+		    -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"
-- 
2.9.3

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

[Jerome Guitton]

Sergio Durigan Junior (sergiodj@redhat.com):

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

Indeed. Thank you for catching this bug!

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

[Simon Marchi]

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

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

[Sergio Durigan Junior]

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/

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

[Simon Marchi]

On 2017-06-29 21:48, Sergio Durigan Junior wrote:
> On Thursday, June 29 2017, Simon Marchi wrote:
>> 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.

My opinion is the solution with the least code is probably best, if they 
are equivalent otherwise, but I don't really mind.  It's just a 
suggestion.

Simon

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

[Sergio Durigan Junior]

On Thursday, June 29 2017, Simon Marchi wrote:

> On 2017-06-29 21:48, Sergio Durigan Junior wrote:
>> On Thursday, June 29 2017, Simon Marchi wrote:
>>> 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.
>
> My opinion is the solution with the least code is probably best, if
> they are equivalent otherwise, but I don't really mind.  It's just a
> suggestion.

Right.  I did some more tests here, and unfortunately your solution
doesn't work for all cases.  For example, if the user puts trailing
whitespace on the command name (like "python "), *cmd_name will point to
a whitespace after the call to lookup_cmd_1.

So here's second version of the patch, with the fixes you requested
except the one above.  WDYT?

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

From 5c80c97c4a11ec954cc5d426e77cf209dc43f10d Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Wed, 28 Jun 2017 21:55:03 -0400
Subject: [PATCH] PR cli/21688: Fix multi-line/inline command differentiation

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.

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_python_inline_or_multiline): New
	procedure.  Call it.
---
 gdb/ChangeLog                       |  7 +++++++
 gdb/cli/cli-script.c                | 21 +++++++++++++++++----
 gdb/testsuite/ChangeLog             |  6 ++++++
 gdb/testsuite/gdb.python/py-cmd.exp | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b47226bc..7d0f5cf 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-29  Pedro Alves  <palves@redhat.com>
 
 	* completer.c (expression_completer): Call
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"))
 	{
 	  /* 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 41c5434..51dd86d 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2017-06-29  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR cli/21688
+	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): New
+	procedure.  Call it.
+
 2017-06-29  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/printcmds.exp: Add tests.
diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index 2dbf23ce..39bb785 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -181,6 +181,38 @@ gdb_test "complete expr_test bar\." \
     "expr_test bar\.bc.*expr_test bar\.ij.*" \
     "test completion through complete command"
 
+# Test that the "python" command is correctly recognized as
+# inline/multi-line when entering a sequence of commands.
+#
+# This proc tests PR cli/21688.  The PR is not language-specific, but
+# the easiest way is just to test with Python.
+proc test_python_inline_or_multiline { } {
+    set define_cmd_not_inline {
+	{ "if 1"                 " >$"            "multi-line if 1" }
+	{ "python"               " >$"            "multi-line python command" }
+	{ "print ('hello')"      "  >$"           "multi-line print" }
+	{ "end"                  " >$"            "multi-line first end" }
+	{ "end"                  "hello\r\n"      "multi-line last end" } }
+
+    set define_cmd_inline {
+	{ "if 1"                      " >$"          "inline if 1" }
+	{ "python print ('hello')"    " >$"          "inline python command" }
+	{ "end"                       "hello\r\n"    "inline end" } }
+
+    foreach t [list $define_cmd_not_inline $define_cmd_inline] {
+	foreach l $t {
+	    lassign $l command regex testmsg
+	    gdb_test_multiple "$command" "$testmsg" {
+		-re "$regex" {
+		    pass "$testmsg"
+		}
+	    }
+	}
+    }
+}
+
+test_python_inline_or_multiline
+
 if { [readline_is_used] } {
     set test "complete 'expr_test bar.i'"
     send_gdb "expr_test bar\.i\t\t"
-- 
2.9.3

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

[Simon Marchi]

On 2017-06-30 00:21, Sergio Durigan Junior wrote:
> On Thursday, June 29 2017, Simon Marchi wrote:
> 
>> On 2017-06-29 21:48, Sergio Durigan Junior wrote:
>>> On Thursday, June 29 2017, Simon Marchi wrote:
>>>> 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.
>> 
>> My opinion is the solution with the least code is probably best, if
>> they are equivalent otherwise, but I don't really mind.  It's just a
>> suggestion.
> 
> Right.  I did some more tests here, and unfortunately your solution
> doesn't work for all cases.  For example, if the user puts trailing
> whitespace on the command name (like "python "), *cmd_name will point 
> to
> a whitespace after the call to lookup_cmd_1.

Ah, I got confused because there's some code that strips trailing 
whitespaces, but it only set p_end, it doesn't modify the string.

> So here's second version of the patch, with the fixes you requested
> except the one above.  WDYT?

That LGTM.

Simon

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

[Pedro Alves]

On 06/29/2017 11:21 PM, Sergio Durigan Junior wrote:
> +/* 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));
> +}

...

> -      else if (command_name_equals (cmd, "python"))
> +      else if (command_name_equals_not_inline (p_start, p_end, "python"))
>  	{

Does this handle command aliases?  It doesn't look like it.


> +    set define_cmd_inline {
> +	{ "if 1"                      " >$"          "inline if 1" }
> +	{ "python print ('hello')"    " >$"          "inline python command" }

For example, what if you write instead:

	{ "py print ('hello')"    " >$"          "inline python command" }

and/or you do:

 (gdb) alias foo=python

and then:

	{ "foo print ('hello')"    " >$"          "inline python command" }

Thanks,
Pedro Alves

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

[Sergio Durigan Junior]

On Friday, June 30 2017, Simon Marchi wrote:

> On 2017-06-30 00:21, Sergio Durigan Junior wrote:
>> On Thursday, June 29 2017, Simon Marchi wrote:
>>
>>> On 2017-06-29 21:48, Sergio Durigan Junior wrote:
>>>> On Thursday, June 29 2017, Simon Marchi wrote:
>>>>> 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.
>>>
>>> My opinion is the solution with the least code is probably best, if
>>> they are equivalent otherwise, but I don't really mind.  It's just a
>>> suggestion.
>>
>> Right.  I did some more tests here, and unfortunately your solution
>> doesn't work for all cases.  For example, if the user puts trailing
>> whitespace on the command name (like "python "), *cmd_name will
>> point to
>> a whitespace after the call to lookup_cmd_1.
>
> Ah, I got confused because there's some code that strips trailing
> whitespaces, but it only set p_end, it doesn't modify the string.

Yeah.  Another option would be to advance cmd_name until there is no
more whitespace-like char.  Anyway...

>> So here's second version of the patch, with the fixes you requested
>> except the one above.  WDYT?
>
> That LGTM.

Thanks, pushed.

51ed89aa0dce3db46561235efdc4bbc0661bcf37

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

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

[Sergio Durigan Junior]

On Friday, June 30 2017, Pedro Alves wrote:

> On 06/29/2017 11:21 PM, Sergio Durigan Junior wrote:
>> +/* 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));
>> +}
>
> ...
>
>> -      else if (command_name_equals (cmd, "python"))
>> +      else if (command_name_equals_not_inline (p_start, p_end, "python"))
>>  	{
>
> Does this handle command aliases?  It doesn't look like it.

Hm, no, it doesn't.  I guess that the best approach would be to make
sure that lookup_cmd_1 advances the **text pointer past all the
whitespace chars after it matches a command, and then we could use
Simon's idea and check for *cmd_name != '\0'.

I'll prepare a patch here and do some testings.

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

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

[Pedro Alves]

On 06/30/2017 12:24 PM, Sergio Durigan Junior wrote:

> Hm, no, it doesn't.  I guess that the best approach would be to make
> sure that lookup_cmd_1 advances the **text pointer past all the
> whitespace chars after it matches a command, and then we could use
> Simon's idea and check for *cmd_name != '\0'.

I don't see the point of touching lookup_cmd_1, and then
handling fallout of that.

Simply do this after the lookup_cmd_1 call:

  lookup_cmd = skip_spaces_const (cmd_name);
  bool inline_cmd = *cmd_name != '\0';

and then you can do:

      else if (command_name_equals (cmd, "python") && !inline_cmd)

?

> 
> I'll prepare a patch here and do some testings.
> 


-- 
Thanks,
Pedro Alves

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

[Sergio Durigan Junior]

On Friday, June 30 2017, Pedro Alves wrote:

> On 06/30/2017 12:24 PM, Sergio Durigan Junior wrote:
>
>> Hm, no, it doesn't.  I guess that the best approach would be to make
>> sure that lookup_cmd_1 advances the **text pointer past all the
>> whitespace chars after it matches a command, and then we could use
>> Simon's idea and check for *cmd_name != '\0'.
>
> I don't see the point of touching lookup_cmd_1, and then
> handling fallout of that.
>
> Simply do this after the lookup_cmd_1 call:
>
>   lookup_cmd = skip_spaces_const (cmd_name);
>   bool inline_cmd = *cmd_name != '\0';
>
> and then you can do:
>
>       else if (command_name_equals (cmd, "python") && !inline_cmd)
>
> ?

Indeed, that is much easier.  I'll send a patch soon.

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

[PATCH] PR cli/21688: Detect aliases when issuing python/compile/guile commands (and fix last commit)

[Sergio Durigan Junior]

My last commit fixed a regression that happened when using
inline/multi-line commands for Python/Compile/Guile, but introduced
another regression: it is now not possible to use aliases for the
commands mentioned above.  The fix is to almost revert the change I've
made and go back to using the 'struct cmd_list_element *', but at the
same time make sure that we advance the 'cmd_name' variable past all
the whitespace characters after the command name.  If, after skipping
the whitespace, we encounter a '\0', it means that the command is not
inline.  Otherwise, it is.

This patch also expands the testcase in order to check for aliases and
for trailing whitespace after the command name.

gdb/ChangeLog:
2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	PR cli/21688
	* cli/cli-script.c (command_name_equals_not_inline): Remove function.
	(process_next_line): New variables 'lookup_cmd' and 'inline'.
	Adjust 'if' clauses for "python", "compile" and "guile" to use
	'command_name_equals' and check for '!inline'.

gdb/testsuite/ChangeLog:
2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR cli/21688
	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): Add new
	tests for alias commands and trailing whitespace.
---
 gdb/ChangeLog                       |  9 +++++++++
 gdb/cli/cli-script.c                | 22 +++++-----------------
 gdb/testsuite/ChangeLog             |  6 ++++++
 gdb/testsuite/gdb.python/py-cmd.exp | 15 ++++++++++++++-
 4 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4cd7aad..b103438 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,13 @@
 2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
+	    Pedro Alves  <palves@redhat.com>
+
+	PR cli/21688
+	* cli/cli-script.c (command_name_equals_not_inline): Remove function.
+	(process_next_line): New variables 'lookup_cmd' and 'inline'.
+	Adjust 'if' clauses for "python", "compile" and "guile" to use
+	'command_name_equals' and check for '!inline'.
+
+2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	PR cli/21688
 	* cli/cli-script.c (command_name_equals_not_inline): New function.
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 72f316f..7d5731c 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -900,20 +900,6 @@ 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.  */
 
@@ -966,6 +952,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
       const char *cmd_name = p;
       struct cmd_list_element *cmd
 	= lookup_cmd_1 (&cmd_name, cmdlist, NULL, 1);
+      const char *lookup_cmd = skip_spaces_const (cmd_name);
+      bool inline_cmd = *lookup_cmd != '\0';
 
       /* If commands are parsed, we skip initial spaces.  Otherwise,
 	 which is the case for Python commands and documentation
@@ -1011,20 +999,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_not_inline (p_start, p_end, "python"))
+      else if (command_name_equals (cmd, "python") && !inline_cmd)
 	{
 	  /* Note that we ignore the inline "python command" form
 	     here.  */
 	  *command = build_command_line (python_control, "");
 	}
-      else if (command_name_equals_not_inline (p_start, p_end, "compile"))
+      else if (command_name_equals (cmd, "compile") && !inline_cmd)
 	{
 	  /* 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_not_inline (p_start, p_end, "guile"))
+      else if (command_name_equals (cmd, "guile") && !inline_cmd)
 	{
 	  /* 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 06bf5a4..6160c4b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,6 +1,12 @@
 2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	PR cli/21688
+	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): Add new
+	tests for alias commands and trailing whitespace.
+
+2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR cli/21688
 	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): New
 	procedure.  Call it.
 
diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index 39bb785..287ecda 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -194,12 +194,25 @@ proc test_python_inline_or_multiline { } {
 	{ "end"                  " >$"            "multi-line first end" }
 	{ "end"                  "hello\r\n"      "multi-line last end" } }
 
+    # This also tests trailing whitespace on the command.
+    set define_cmd_alias_not_inline {
+	{ "if 1"                 " >$"            "multi-line if 1 alias" }
+	{ "py    "               " >$"            "multi-line python command alias" }
+	{ "print ('hello')"      "  >$"           "multi-line print alias" }
+	{ "end"                  " >$"            "multi-line first end alias" }
+	{ "end"                  "hello\r\n"      "multi-line last end alias" } }
+
     set define_cmd_inline {
 	{ "if 1"                      " >$"          "inline if 1" }
 	{ "python print ('hello')"    " >$"          "inline python command" }
 	{ "end"                       "hello\r\n"    "inline end" } }
 
-    foreach t [list $define_cmd_not_inline $define_cmd_inline] {
+    set define_cmd_alias_inline {
+	{ "if 1"                      " >$"          "inline if 1 alias" }
+	{ "py print ('hello')"    " >$"          "inline python command alias" }
+	{ "end"                       "hello\r\n"    "inline end alias" } }
+
+    foreach t [list $define_cmd_not_inline $define_cmd_alias_not_inline $define_cmd_inline $define_cmd_alias_inline] {
 	foreach l $t {
 	    lassign $l command regex testmsg
 	    gdb_test_multiple "$command" "$testmsg" {
-- 
2.9.3

Re: [PATCH] PR cli/21688: Detect aliases when issuing python/compile/guile commands (and fix last commit)

[Pedro Alves]

On 06/30/2017 01:34 PM, Sergio Durigan Junior wrote:

> @@ -966,6 +952,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>        const char *cmd_name = p;
>        struct cmd_list_element *cmd
>  	= lookup_cmd_1 (&cmd_name, cmdlist, NULL, 1);
> +      const char *lookup_cmd = skip_spaces_const (cmd_name);
> +      bool inline_cmd = *lookup_cmd != '\0';

The "lookup_cmd" in my suggestion:

~~~~~
 lookup_cmd = skip_spaces_const (cmd_name);
~~~~~

was a typo/pasto from "lookup_cmd_1"...  I meant:

 cmd_name = skip_spaces_const (cmd_name);

Fine with me to use a new variable like you had, but
it should have a name that actually means something
related to the task at hand.  "cmd_arg" or something.

>  	  /* 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 06bf5a4..6160c4b 100644
> --- a/gdb/testsuite/ChangeLog
> +++ b/gdb/testsuite/ChangeLog
> @@ -1,6 +1,12 @@
>  2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
>  
>  	PR cli/21688
> +	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): Add new
> +	tests for alias commands and trailing whitespace.
> +
> +2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
> +
> +	PR cli/21688
>  	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): New
>  	procedure.  Call it.
>  
> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
> index 39bb785..287ecda 100644
> --- a/gdb/testsuite/gdb.python/py-cmd.exp
> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
> @@ -194,12 +194,25 @@ proc test_python_inline_or_multiline { } {
>  	{ "end"                  " >$"            "multi-line first end" }
>  	{ "end"                  "hello\r\n"      "multi-line last end" } }
>  
> +    # This also tests trailing whitespace on the command.
> +    set define_cmd_alias_not_inline {
> +	{ "if 1"                 " >$"            "multi-line if 1 alias" }
> +	{ "py    "               " >$"            "multi-line python command alias" }
> +	{ "print ('hello')"      "  >$"           "multi-line print alias" }
> +	{ "end"                  " >$"            "multi-line first end alias" }
> +	{ "end"                  "hello\r\n"      "multi-line last end alias" } }
> +
>      set define_cmd_inline {
>  	{ "if 1"                      " >$"          "inline if 1" }
>  	{ "python print ('hello')"    " >$"          "inline python command" }
>  	{ "end"                       "hello\r\n"    "inline end" } }
>  
> -    foreach t [list $define_cmd_not_inline $define_cmd_inline] {
> +    set define_cmd_alias_inline {
> +	{ "if 1"                      " >$"          "inline if 1 alias" }
> +	{ "py print ('hello')"    " >$"          "inline python command alias" }
> +	{ "end"                       "hello\r\n"    "inline end alias" } }
> +

Any reason you didn't add a test for the "alias foo=python" case?
We want to be sure that aliases that are not abbreviations are
handled too.  "py" is probably really implemented as a
disambiguating alias, due to "python-interactive", but it could be an 
abbreviation too [py, pyt, pyth], etc.  I think an explicit test for
a non-abbreviation alias would be good, to be sure the code isn't just
doing a "startswith"-like check.  Otherwise, I'm going to ask for a
test that exercises abbreviations, like for example "compil", but you
don't want that. :-)

Thanks,
Pedro Alves

Re: [PATCH] PR cli/21688: Detect aliases when issuing python/compile/guile commands (and fix last commit)

[Sergio Durigan Junior]

On Friday, June 30 2017, Pedro Alves wrote:

> On 06/30/2017 01:34 PM, Sergio Durigan Junior wrote:
>
>> @@ -966,6 +952,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
>>        const char *cmd_name = p;
>>        struct cmd_list_element *cmd
>>  	= lookup_cmd_1 (&cmd_name, cmdlist, NULL, 1);
>> +      const char *lookup_cmd = skip_spaces_const (cmd_name);
>> +      bool inline_cmd = *lookup_cmd != '\0';
>
> The "lookup_cmd" in my suggestion:
>
> ~~~~~
>  lookup_cmd = skip_spaces_const (cmd_name);
> ~~~~~
>
> was a typo/pasto from "lookup_cmd_1"...  I meant:
>
>  cmd_name = skip_spaces_const (cmd_name);
>
> Fine with me to use a new variable like you had, but
> it should have a name that actually means something
> related to the task at hand.  "cmd_arg" or something.

Right.  I'll use cmd_name then, since we're not using it anywhere else
after that point.

>>  	  /* 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 06bf5a4..6160c4b 100644
>> --- a/gdb/testsuite/ChangeLog
>> +++ b/gdb/testsuite/ChangeLog
>> @@ -1,6 +1,12 @@
>>  2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
>>  
>>  	PR cli/21688
>> +	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): Add new
>> +	tests for alias commands and trailing whitespace.
>> +
>> +2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
>> +
>> +	PR cli/21688
>>  	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): New
>>  	procedure.  Call it.
>>  
>> diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
>> index 39bb785..287ecda 100644
>> --- a/gdb/testsuite/gdb.python/py-cmd.exp
>> +++ b/gdb/testsuite/gdb.python/py-cmd.exp
>> @@ -194,12 +194,25 @@ proc test_python_inline_or_multiline { } {
>>  	{ "end"                  " >$"            "multi-line first end" }
>>  	{ "end"                  "hello\r\n"      "multi-line last end" } }
>>  
>> +    # This also tests trailing whitespace on the command.
>> +    set define_cmd_alias_not_inline {
>> +	{ "if 1"                 " >$"            "multi-line if 1 alias" }
>> +	{ "py    "               " >$"            "multi-line python command alias" }
>> +	{ "print ('hello')"      "  >$"           "multi-line print alias" }
>> +	{ "end"                  " >$"            "multi-line first end alias" }
>> +	{ "end"                  "hello\r\n"      "multi-line last end alias" } }
>> +
>>      set define_cmd_inline {
>>  	{ "if 1"                      " >$"          "inline if 1" }
>>  	{ "python print ('hello')"    " >$"          "inline python command" }
>>  	{ "end"                       "hello\r\n"    "inline end" } }
>>  
>> -    foreach t [list $define_cmd_not_inline $define_cmd_inline] {
>> +    set define_cmd_alias_inline {
>> +	{ "if 1"                      " >$"          "inline if 1 alias" }
>> +	{ "py print ('hello')"    " >$"          "inline python command alias" }
>> +	{ "end"                       "hello\r\n"    "inline end alias" } }
>> +
>
> Any reason you didn't add a test for the "alias foo=python" case?
> We want to be sure that aliases that are not abbreviations are
> handled too.  "py" is probably really implemented as a
> disambiguating alias, due to "python-interactive", but it could be an 
> abbreviation too [py, pyt, pyth], etc.  I think an explicit test for
> a non-abbreviation alias would be good, to be sure the code isn't just
> doing a "startswith"-like check.  Otherwise, I'm going to ask for a
> test that exercises abbreviations, like for example "compil", but you
> don't want that. :-)

I didn't think about including a test for explicitly set aliases at
first, but thanks for pointing that out.  It's now included.

Here's the final version of the patch.

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

From dc4bde35d16df749e529229657b3468417937cfc Mon Sep 17 00:00:00 2001
From: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Fri, 30 Jun 2017 08:27:29 -0400
Subject: [PATCH] PR cli/21688: Detect aliases when issuing
 python/compile/guile commands (and fix last commit)

My last commit fixed a regression that happened when using
inline/multi-line commands for Python/Compile/Guile, but introduced
another regression: it is now not possible to use aliases for the
commands mentioned above.  The fix is to almost revert the change I've
made and go back to using the 'struct cmd_list_element *', but at the
same time make sure that we advance the 'cmd_name' variable past all
the whitespace characters after the command name.  If, after skipping
the whitespace, we encounter a '\0', it means that the command is not
inline.  Otherwise, it is.

This patch also expands the testcase in order to check for aliases and
for trailing whitespace after the command name.

gdb/ChangeLog:
2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	PR cli/21688
	* cli/cli-script.c (command_name_equals_not_inline): Remove function.
	(process_next_line): New variable 'inline_cmd'.
	Adjust 'if' clauses for "python", "compile" and "guile" to use
	'command_name_equals' and check for '!inline_cmd'.

gdb/testsuite/ChangeLog:
2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR cli/21688
	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): Add new
	tests for alias commands and trailing whitespace.
---
 gdb/ChangeLog                       |  9 +++++++++
 gdb/cli/cli-script.c                | 22 +++++-----------------
 gdb/testsuite/ChangeLog             |  6 ++++++
 gdb/testsuite/gdb.python/py-cmd.exp | 35 ++++++++++++++++++++++++++++++++++-
 4 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4cd7aad..7080256 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,13 @@
 2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
+	    Pedro Alves  <palves@redhat.com>
+
+	PR cli/21688
+	* cli/cli-script.c (command_name_equals_not_inline): Remove function.
+	(process_next_line): New variable 'inline_cmd'.
+	Adjust 'if' clauses for "python", "compile" and "guile" to use
+	'command_name_equals' and check for '!inline_cmd'.
+
+2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	PR cli/21688
 	* cli/cli-script.c (command_name_equals_not_inline): New function.
diff --git a/gdb/cli/cli-script.c b/gdb/cli/cli-script.c
index 72f316f..5674404 100644
--- a/gdb/cli/cli-script.c
+++ b/gdb/cli/cli-script.c
@@ -900,20 +900,6 @@ 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.  */
 
@@ -966,6 +952,8 @@ process_next_line (char *p, struct command_line **command, int parse_commands,
       const char *cmd_name = p;
       struct cmd_list_element *cmd
 	= lookup_cmd_1 (&cmd_name, cmdlist, NULL, 1);
+      cmd_name = skip_spaces_const (cmd_name);
+      bool inline_cmd = *cmd_name != '\0';
 
       /* If commands are parsed, we skip initial spaces.  Otherwise,
 	 which is the case for Python commands and documentation
@@ -1011,20 +999,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_not_inline (p_start, p_end, "python"))
+      else if (command_name_equals (cmd, "python") && !inline_cmd)
 	{
 	  /* Note that we ignore the inline "python command" form
 	     here.  */
 	  *command = build_command_line (python_control, "");
 	}
-      else if (command_name_equals_not_inline (p_start, p_end, "compile"))
+      else if (command_name_equals (cmd, "compile") && !inline_cmd)
 	{
 	  /* 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_not_inline (p_start, p_end, "guile"))
+      else if (command_name_equals (cmd, "guile") && !inline_cmd)
 	{
 	  /* 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 06bf5a4..6160c4b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,6 +1,12 @@
 2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	PR cli/21688
+	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): Add new
+	tests for alias commands and trailing whitespace.
+
+2017-06-30  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR cli/21688
 	* gdb.python/py-cmd.exp (test_python_inline_or_multiline): New
 	procedure.  Call it.
 
diff --git a/gdb/testsuite/gdb.python/py-cmd.exp b/gdb/testsuite/gdb.python/py-cmd.exp
index 39bb785..953d52a 100644
--- a/gdb/testsuite/gdb.python/py-cmd.exp
+++ b/gdb/testsuite/gdb.python/py-cmd.exp
@@ -187,6 +187,8 @@ gdb_test "complete expr_test bar\." \
 # This proc tests PR cli/21688.  The PR is not language-specific, but
 # the easiest way is just to test with Python.
 proc test_python_inline_or_multiline { } {
+    global gdb_prompt
+
     set define_cmd_not_inline {
 	{ "if 1"                 " >$"            "multi-line if 1" }
 	{ "python"               " >$"            "multi-line python command" }
@@ -194,12 +196,43 @@ proc test_python_inline_or_multiline { } {
 	{ "end"                  " >$"            "multi-line first end" }
 	{ "end"                  "hello\r\n"      "multi-line last end" } }
 
+    # This also tests trailing whitespace on the command.
+    set define_cmd_alias_not_inline {
+	{ "if 1"                 " >$"            "multi-line if 1 alias" }
+	{ "py    "               " >$"            "multi-line python command alias" }
+	{ "print ('hello')"      "  >$"           "multi-line print alias" }
+	{ "end"                  " >$"            "multi-line first end alias" }
+	{ "end"                  "hello\r\n"      "multi-line last end alias" } }
+
+    set define_cmd_alias_foo_not_inline {
+	{ "alias foo=python"     "\r\n"           "multi-line alias foo" }
+	{ "if 1"                 " >$"            "multi-line if 1 alias foo" }
+	{ "foo    "              " >$"            "multi-line python command alias foo" }
+	{ "print ('hello')"      "  >$"           "multi-line print alias foo" }
+	{ "end"                  " >$"            "multi-line first end alias foo" }
+	{ "end"                  "hello\r\n"      "multi-line last end alias foo" } }
+
     set define_cmd_inline {
 	{ "if 1"                      " >$"          "inline if 1" }
 	{ "python print ('hello')"    " >$"          "inline python command" }
 	{ "end"                       "hello\r\n"    "inline end" } }
 
-    foreach t [list $define_cmd_not_inline $define_cmd_inline] {
+    set define_cmd_alias_inline {
+	{ "if 1"                      " >$"          "inline if 1 alias" }
+	{ "py print ('hello')"        " >$"          "inline python command alias" }
+	{ "end"                       "hello\r\n"    "inline end alias" } }
+
+    set define_cmd_alias_foo_inline {
+	{ "if 1"                      " >$"          "inline if 1 alias foo" }
+	{ "foo print ('hello')"       " >$"          "inline python command alias foo" }
+	{ "end"                       "hello\r\n"    "inline end alias foo" } }
+
+    foreach t [list $define_cmd_not_inline \
+	       $define_cmd_alias_not_inline \
+	       $define_cmd_alias_foo_not_inline \
+	       $define_cmd_inline \
+	       $define_cmd_alias_inline \
+	       $define_cmd_alias_foo_inline] {
 	foreach l $t {
 	    lassign $l command regex testmsg
 	    gdb_test_multiple "$command" "$testmsg" {
-- 
2.9.3

Re: [PATCH] PR cli/21688: Detect aliases when issuing python/compile/guile commands (and fix last commit)

[Pedro Alves]

On 06/30/2017 02:33 PM, Sergio Durigan Junior wrote:

> Here's the final version of the patch.

OK.

Thanks,
Pedro Alves

Re: [PATCH] PR cli/21688: Detect aliases when issuing python/compile/guile commands (and fix last commit)

[Sergio Durigan Junior]

On Friday, June 30 2017, Pedro Alves wrote:

> On 06/30/2017 02:33 PM, Sergio Durigan Junior wrote:
>
>> Here's the final version of the patch.
>
> OK.

Thanks, pushed.

dc4bde35d16df749e529229657b3468417937cfc

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