PR cli/21688: Fix multi-line/inline command differentiation
Commit Message
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(-)
Comments
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!
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
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,
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
@@ -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".
@@ -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, "");
@@ -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
@@ -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"